[Discord] Add more badges (members/online/boosts) #9098
Conversation
|
|
👋 Long time no PRs Looks like you've got a few failing tests.
@Av32000 is right that one of the issues is duplicate class names, but it looks like you've also got a failure on the discord auth tests. |
chris48s
added
the
service-badge
|
Yeah, it's been a minute 😅 I'm not exactly sure what we are using the Discord auth for, as I don't think any of these endpoints require any auth AFAICT. |
This reverts commit b02753f.
|
It is not required, but calling the discord API as an anonymous user has a low rate limit. Setting shields/services/discord/discord.service.js Lines 33 to 37 in c2998a2 We have DISCORD_BOT_TOKEN token set in production and this may also be used by some self-hosting users.
|
|
Having had a quick look over this, a few more comments for when you revisit to look at the auth:
|
| static category = 'chat' | ||
|
|
||
| static route = { | ||
| base: 'discord/online-count', |
There was a problem hiding this comment.
Today on "naming things is hard".. "online count" could equally describe either variant of this badge (chat | 123 online or chat | 123/456 online). Really the difference here is like "online" vs "online with total" - they're both a count. At the same time, I can't think of any better suggestion for nice way to succinctly express "online out of total" in a single url-friendly slug.
There was a problem hiding this comment.
Only thoughts on possible friendly names all relay the same sort of meaning
online-total - same as online
online-count - same as online
online-with-total - is probably fine, it's not too long IMO?
There was a problem hiding this comment.
Recognize this option might add some internal complexity within the code, but from an end user perspective I wonder if it would be preferable to have the "with total" option provided as a boolean query parameter?
Not sure if that would be too inconsistent with existing badges, but seems like it would be an easier way for a user to build the badge from the frontend site.
Otherwise, I'd vote online-with-total
| async fetch({ inviteId }) { | ||
| const url = `https://discord.com/api/v9/invites/${inviteId}?with_counts=true` | ||
| return this._requestJson( | ||
| this.authHelper.withBearerAuthHeader( | ||
| { | ||
| url, | ||
| schema, | ||
| errorMessages: { | ||
| 404: 'invalid server invite', | ||
| }, | ||
| }, | ||
| 'Bot' | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
It seems like there's some commonality/duplication across the new classes especially in these fetch functions. Probably worth hoisting that out into a Discord base class or a common helper function IMO
|
@RedSparr0w it would be great to get this landed, do you think you can spend some time to address the latest review comments? |
|
Given that there hasn't been any activity for a while, I'm going to close this pull request. Feel free to ping us or reopen it if you get a chance to look into it again. 😉 If others think that repository level projects would be an option and want to pick this up, you can simply fetch the commits by running git fetch origin pull/9098/head:pr-9098. Change the implementation to target repository level projects and open a new pull request with a co-authored trailer included in the commit message to give attribution to the original author. |
|
One other thing that has happened since this PR was opened is we've started logging 429s to sentry. We are seeing a lot of While we are still sometimes hitting the rate limit with just the 'members online now' badge, we probably don't want to add more badges and increase our traffic to the discord api. |
|
We do have a point of contact and a special token for Discord if I recall correctly, we could ask for a rate limit increase. |
|
Interesting. I had not really thought about where our discord token came from. I assumed we just had a free account. Lets move this discussion to #9862 rather than the comments of a closed PR. |
|
To further reassure whoever would want to pick this up, it's worth noting that the new badges in this PR uses a separate API endpoint, |
closes #4500
I've done some testing with the "approximate" values
Seems like the values are cached for ~5 minutes or so, most are pretty close to the actual value, I think the online count within Discord may be off for the
Discord Developersserver due to some people not having completed the verification/screening process but still being in the server, so they will not show in the online list.