[Discourse] Update schema keys to use plural form (topic_count -> topics_count)
#9778
Conversation
(e.g. topic_count -> topics_countN)
|
joshuacwnewton
changed the title
topic_count -> topics_count)topic_count -> topics_count)
|
Hi. Thanks for submitting a PR to fix this ❤️ I agree it would be nice to maintain compatibility with both versions. I think the way to implement this would be to define 2 schemas, one using the plurals and one using the singulars and then use There's a service that uses this technique in https://github.com/badges/shields/blob/master/services/polymart/polymart-base.js that you could use as an example. Are you up for updating to follow that approach? |
chris48s
added
bug
Thank you for the guidance! I'll give it a shot. I'm not entirely sure how to set up the testing harness locally, so I'm going to (naively) push and hope the CI will catch any errors I make. Apologies in advance for any noise on this PR as I work things out. :) |
joshuacwnewton
force-pushed
the
jn/9776-fix-discourse-js-schema
branch
3 times, most recently
from
786524c
to
954866c
Compare
Previously, for e.g. 'topic', the call to the `DiscourseMetricIntegrationFactory` function supplied both 'topics' and 'topic_count'. And, we now need to check for 'topics_count' as well. To cover all three string variations, why not supply only 'topic' to the function, then selectively add the 's' on a case-by-case basis. ((Note: I've preserved the old metricName variable as to minimize the diff here and make my changes clearer.))
joshuacwnewton
force-pushed
the
jn/9776-fix-discourse-js-schema
branch
from
954866c
to
09c7271
Compare
|
I'm not entirely sure if I've used the correct JS patterns/idioms, so please feel free to correct me if I've done something silly. 😅 |
|
🚀 Updated review app: https://pr-9778-badges-shields.fly.dev |
This PR does a simple find and replace to update the Discourse schema to use the current keys found in https://meta.discourse.org/site/statistics.json, as the older keys now return "Invalid response".
Note
I was wondering, though, if the old keys should still be preserved, as to not break backwards compatibility with servers using the old names.
(I tried finding the source of the change in Discourse's release notes/changelog, but there's surprisingly little information when querying e.g. "statistics.json" or "topic_count"/"topics_count". So, I'm not sure when this change actually took place, and thus it's hard to estimate the likelihood of there being servers out there using the old keys.)
If we do want to support both sets of keys, then I'm not 100% sure what to do, as I've never written JavaScript before. (I tried reading the Joi docs, and got as far as using
.or()in the schema definition for each pair of keys + using atry/catchwhen creating themetricIntegrationsobject, but I wasn't confident enough to commit that solution before discussing it first.)Fixes #9776.