Chore: Use Limit and Offset Params when Fetching the Points from The API in the Leaderboard command

[KevinMulhern]

Description

Currently we fetch all the points records from the API and filter them using JS. This makes the leaderboard command code complicated and will eventually become a performance problem.

Letting the database handle this filtering on the api side would be much more efficient and it would simplify the leaderboard bot code significantly. We already have the parameters to do this set up on the api, we just need to use them in the bot.

Acceptance Criteria

1. When fetching points and no n argument is provided, the limit parameter should default to 5 when making the points api call.
2. When fetching points and no start argument is provided, the offset parameter should default to 0 when making the points api call.
3. Otherwise pass the provided n and start arguments through as the limit and offset parameter values respectively.

Example:
using the command: leaderboard n=10 start=5
will translate to the following api call: https://www.theodinproject.com/api/points?limit=10&offset=5

Additional Information

Just to make it clear:

• The limit param does the the same as the n argument we pass to the leaderboard command in discord
• The offset param does the the same as the start argument we pass to the leaderboard command in discord

Important
Let me know when this is ready to be merged as there is one more requirement implemented on the bot size to limit the n argument to 25 results returned. This will need to be implemented on the api side and go out at the same time this story goes out.

[vishant-nambiar]

Hello! I would love to tackle this issue!

[zachmmeyer]

@marvingay All yours!

[arinze19]

Hey is anyone still working on this? Buzzing to take a go at it @zachmmeyer @marvingay

[zachmmeyer]

@arinze19 Have fun!

[arinze19]

Hey @KevinMulhern made some changes and it's ready to be implemented, you can review the PR here

[Mclilzee]

@01zulfi beside the status clearly stating On Hold I was wondering if there is more discussions regarding this, or is it still open for feedback.

[01zulfi]

@Mclilzee Discussions and ideas are always welcome. @KevinMulhern and I discussed this issue and the corresponding PR and found that this would require more work (in the API and the bot) than outlined in the issue. However, the leaderboard command works as intended for now. We don't see any problems with the current implementation of the command for the foreseeable future, hence we put the issue on hold.

[Mclilzee]

My approach would be to keep the changes in the Bot to a minimal, nothing needs to changes beside the GET request to include the headers offset and limit this will even simplify the bot testing to only test output formating and leave the testing suite of fetching data to the Api.

As for the API, I would let it handle getting the proper data ready for the bot, since all users still have entries in the database and no user gets deleted when they leave or get banned, I would add a state flag to each user active and inactive. This will even simplify the API logic and let the Database handle a proper Query with active users included which is faster since a database is optimized to fetch data. This is even more flexible and we can expand upon it with more states if necessary such as persistant, for example someone special that the team decides to keep their ranking in the system even after them leaving the server.

Flagging users with active, and inactive needs more thought tho, for example a user state changes if they were previously inactive and they get a point added to their name, a user can be set to inactive upon leaving the server although i'm not sure how many frequent requests will that be I'm not sure if we get many users leaving the server frequently. It can also be done through a weekly maintnance, if someone leaves the server their name in Ranking can persist for a week until the maintant with PUT request to change the state of all the users that is currently not in the server and make them inactive.

[KevinMulhern]

@Mclilzee thanks for the great write up 💪 we landed somewhere similar when this was put on hold.

I do like their idea of triggering api calls when people leave. I probably prefer setting point records to active/inactive instead of removing them for the scenario where people leave the server and come back.

Theres a bunch of unknowns around what will happen when we do the periodical purge of inactive accounts on discord - will that trigger a leave event at all? and if it does, we could be overwhelmed with too many api calls for the server to handle all at once.

We should be able to easily handle the amount of users leaving daily. On average, we get about 50 a day. We have the odd outlier day where we get nearly 200 users leaving, those usually coincide with when we make full server announcements lol.

We periodical purge inactive users every few months to reduce hacked accounts and spam risk. This presents the biggest unknown for us. We'll need to update the points data in bulk when that happens. But we're not sure if Discord emits any events when that happens - if we had that figured out I think this would be good to open again.

[Mclilzee]

It is true there is limitation to that, one of which also that if people left while the bot was down, their state will not change to inactive, which why a trigger to clean up event manually with an API call from an admin can help when we need it after a downtime of the bot we can trigger it once by sending all current active users and let the API handle filtering them out if necessary.

A way also can be done with the bot triggering an event on someone leaving, but instead of sending that user ID directly, it gets added to an Array with a callback function and triggers a callback timer event. While the time is ticking that array can still be gathering more IDs, After a certain amount of time has passed, the array data get sent to the server with all the user IDs to be set to inactive.

For example if we set the timer to 10 min and the periodical purge is done under 10 min then the bot will only send the request to the server once with all the users inside the array, then emptying out the array to be ready for the next request.

[Mclilzee]

I made an example usage of the backend handling situation, mind my lack of Ruby knowledge there could be many bugs in there I haven't worked with ruby before, this is just an example. A review can be found here TheOdinProject/theodinproject@main...Mclilzee:theodinproject:main

New endpoint /api/points/update will handle triggering updating the database, I don't think there is Admin Authentication requirement as this option can be trigger through the bot itself with a command such as !update-database with someone that have role of admin, maintainer

A trigger bot on leave event as mentioned above with a timer, can also suffice by limiting the frequent requests to the server and only send them in bulk.

[Mclilzee]

@KevinMulhern @01zulfi
I have been working on prototypical events of handling the situation to solve both setting inactive members on leave without overwhelming the backend with too many requests at the same time, and manually cleaning up the server on downtime.

Here is a prototype of the code that would handle first situation: https://github.com/Mclilzee/odin-bot-v2/blob/leaderboard-limit/events/leave.js
The interval was set to 20 sec between requests if frequent people leave all at the same time (in the event of purging)

Here is a prototype code of a command that would handle cleaning up the database manually, on the occasion where the bot was down for a period of time, or when it was turned off for the purge: https://github.com/Mclilzee/odin-bot-v2/blob/leaderboard-limit/botCommands/databaseCleanup.js

And here is the backend reflection of the changes: https://github.com/Mclilzee/theodinproject/blob/leaderboard-limit/app/controllers/api/points_controller.rb

[01zulfi]

Apologies @Mclilzee, didn't get the time to look at this thoroughly. On a cursory glance I have a couple of pointers:

1. The listener for the member leave event: I'm sure we will lose all of the inactive_discord_ids for that 20 second time frame if a new push to the bot is made and it redeploys. This is not a huge deal, just something to keep in mind.
2. I'm not sure if we'd need a command because if we can automate this action entirely, we should.

Another idea, we can stay away from discord events/command entirely. Run a cron job once a month/week that sends the active discord users to the server, and let server handle the rest. One problem with this would be that the leaderboard will not be the "latest" as it would have inactive users for the last month/week. But I'm sure this approach is worth considering.

What do you think @Mclilzee?

[Mclilzee]

Hey @01zulfi Thanks for considering the points i have made and yes i know you are busy and would have replied after you got time.

Your Idea is exactly what the command does, the command is not tied to members leaving at all. It tells the Bot to gather all current members IDs and send them to the server, and when the server recieves them, it does the cron up by setting people that is not on the list as inactive, We could also make it so it sets active users to active again but i'm not sure how is that necessary, because the changes in the backedn also reflects that when people get new points their activity becomes true immediatly.

I wasn't aware that we could gather all members IDs through other means than the bot, and even if we use something like Postman to send them it would have been a hassle which why I figured automating gathering members IDs and sending them to the server with a bot that is alreayd authenticated would be a good approach. Once an admin trigger the command the cron happens automatically, unless you have a better automated ways that i'm not aware of.

As for the ids being lost that was ok as long as we got the command or the other ways of cron that you mentioned. If members that left were at the top of the list, then we can trigger the event to clean it up, if they were at the bottom they can be ignored as probably no one will see them and they will get cleaned up with the next cron / purge event.

[KevinMulhern]

Sorry for the radio silence @Mclilzee, I've had a full on week 😅. This is seriously impressive discovery work, thank you so much for digging into this so deep 💪

I've been asking around to find out the numbers we're dealing with during purging. It can range anywhere from 100 to over 10k depending on a number of variables. Time of year, time between prunes etc. The most recent prune we ran removed 13k users. We're not sure how long it takes, once we press the button it goes off and does its thing with no feedback other than the user count going down the next day.

We could be talking about payloads with thousands of id's being sent between the bot and the site. It'll likely be too much for us to handle with our current restricted server resources.

[Mclilzee]

Ty for the nice words. Unfortunetly i'm not expert when it comes to performance and payloads handling at the moment, that i will leave up to you both to decide on. As the bot is working properly atm those points can be considered for the future.

What concerns me for the time being, is every single GET request will send back an array with 8.9k+ objects in it, while requiring no authorization, I or anyone else can simply call it from a browser for example. If i may make a suggestion for the time being to atleast bump up authorization privlidges so only the bot can call such a big data calls from the server until changes made in place for it to handle many requests even from other API calls.

[KevinMulhern]

Sorry for the delay @Mclilzee, the site repo has been keeping me busy this week lol. Yeah it can get tricky with large payloads, we should have the resources to do all this in the future. But for the time-being we're limited. Thankfully the endpoint is still performing ok for now.

Thats a really good point about the points GET endpoints being open. We definitely should stick those behind auth with how big they are getting. Should be easy though, the except just needs removed from this line on the site. On the bot side the post here needs to be changed to common I believe. Two line change all in. Since you pointed it out @Mclilzee, would you like to PR it? I'll make an issue otherwise.

[Mclilzee]

@KevinMulhern Thank you for replying back about the open Endpoint request, I will be making a PR shortly.