Leaderboard: Add limit/offset params #256
Conversation
arinze19
changed the title
| if (user) { | ||
| // on user leave guild: remove user from server? |
There was a problem hiding this comment.
Not sure what the purpose of this comment is.
| ); | ||
| const data = users.data.filter((user) => guild.members.cache.get(user.discord_id)); |
There was a problem hiding this comment.
I get why you've removed this line where we validate if the user exists in the guild. But without this filtering, we'll end up displaying "UNDEFINED" in our leaderboard. This is what I mean:
Even if we keep this filter, it won't have a huge effect because we're always fetching a subset of the leaderboard. One way to tackle this would be to still display the points and rank on the leaderboard with "User left the server" instead of username. Or maybe not do anything at all? I'll also discuss with the team. Do share your thoughts, thanks!
There was a problem hiding this comment.
Yeah that was very much the purpose of this comment earlier.
Perhaps we could trigger an API call that removes the user details from the API once they leave or are banned from the guild?
There was a problem hiding this comment.
After discussing with the team, we feel that the leaderboard would lose its value if it includes users who have left the server. Hence, ideally we would like to avoid that.
Perhaps we could trigger an API call that removes the user details from the API once they leave or are banned from the guild?
This needs more thought as well, since we'll have to consider scenarios where a user leaves the server or gets banned, and then comes back.
The issue needs more work put into it (both in the bot and the API) than initially thought. Fortunately, the leaderboard command works as intended right now and it's unlikely we'll run into performance issues anytime soon.
Putting this PR and the issue on hold now. Thanks for all the work you put in @arinze19 🚀
01zulfi
added
the
Status: On Hold
Because
Current bot implementation fetches ALL users from API on command
!leaderboardand this may cause performance issuesThis PR
nandstartaslimitandoffsetparameters to API call respectivelyn=5andstart=0if no argument providedIssue
Closes #175
Additional Information
Pull Request Requirements
location of change: brief description of changeformat, e.g.Callbacks command: Update verbiageBecausesection summarizes the reason for this PRThis PRsection has a bullet point list describing the changes in this PRIssuesection