otter-computer / GaymersBot

The bot that helps run the Gaymers Discord.
GNU General Public License v2.0
24 stars 17 forks source link

MVP for Starboards #377

Closed Ed-boye closed 3 years ago

Ed-boye commented 4 years ago

The presentation of the message within the starboard

Ready to review Pretty self-explanatory: whatever formatting or visual treatment we want to give it, maybe adding links to the message posted

Star threshold and duplicate messages handling

I think this isn't too bad for now. Bumped fetch to the default of 50. Currently I'm just going off of as soon as you hit the threshold to avoid re-running checks. I guard for someone hitting the threshold, removing the star, and re-adding by checking into the last 20 messages in the starboard channel and looking for a dupe. Not the most elegant but the immediate other solution that comes to mind is tracking in a DB (or writing to a file?) the message ID's of starboard messages and instead looking for those (saving the computation of fetching 20+ messages and doing string comparisons).

onMessageReactionAdd event not triggering sometimes

Resolved; the USER partial was missing. This might also be affecting existing handling reactions for roles I ran into some issues where old messages being reacted to weren't working and also where the first message posted after launching the bot would trigger on the first reaction but not the second (didn't test 3rd +). I see we're using the REACTION partial as well following guidance from the discordjs guide (https://discordjs.guide/popular-topics/reactions.html#listening-for-reactions-on-old-messages) but still running into issues.

Updating star-config.json

Need to update IDs for the starboard channels in Gaymers and review the channel blacklist

Ed-boye commented 4 years ago

I'll be updating the message posted into the starboard to be embedded so it has a nicer treatment, I think. Perhaps I'll also link to the original comment and utilize the message ID (or some part of the URL) for the duplicate check.

Ed-boye commented 4 years ago

Gave the starboard message a nicer treatment: image Note: bot avatar in the footer will be Kirby's once deployed

otter-computer commented 4 years ago

This is looking great! Gonna need to look at the channel setup first before I can merge it though. The server is having a lot of lag issues related to permissions and the amount of roles, so category permission overrides are a bit of a pain to manage, and having an 18+ starboard in that category might cause more issues. Perhaps that starboard can sit outside the category itself. Will need to test it!

otter-computer commented 3 years ago

@Ed-boye Finally got around to testing this after our channel reshuffle and chats with staff 🎉 I refactored it a lot to make it more readable in certain areas like the embed, but otherwise it was super great. Thanks for putting this together so long ago 😂

Ed-boye commented 3 years ago

I took a peek at a the refactoring, I agree with it :)

The only suggestion I have is to rely on channel/section IDs over the names to make refactoring changes in code in the future less likely to break things.

On the flip side, it does allow flexibility when administering the server itself where you just have to make sure the name is right. Perhaps that was the main thinking--cycles saved using Id vs strings is minimal I'd imagine.

otter-computer commented 3 years ago

Yeah, I swapped to names IDs are much worse to work with in testing environments. Could also be done by passing in the config as an env var, but I felt just hardcoding the names was better for now.

As for the other comment you left on the commit itself about about removing the already on starboard festure (I can't find it for some reason on the mobile app lol) , it was done for performance, yeah. It was a choice between the more expensive call or leaving it up to human moderation, and I think abusing the starboard in this way is something that we can just leave the actual server staff to deal with. Were there an easier way to look it up, like having a database record that linked the two together, then I'd be up for bringing it back, but from experience working with fetching messages without a specific ID it can get really painful the longer a channel's history gets.

Ed-boye commented 3 years ago

OK that makes sense, iirc it's only fetching the last 50 messages (and could be configured to be lower) so it wouldn't be perfect anyway since someone could go back and abuse it after the message drops out of range of the fetch.

Yeah, the "correct" approach would be to utilize some DB-backed approach. We would start getting into more trickiness with deployment, testing, cost, etc. so keeping it state-less for now is still probably ideal.

otter-computer commented 3 years ago

Yah I guess that's a fair point, the last 50 might be worth it to stick it back in 🤔 My memories are also of a time before the message cache existed in a friendly way too so these days it's probably nicer and more performant than way back when.

But there's always a database calling for one reason or another it might make sense to add sooner rather than later. Do you have any recommendations in that area? Mercy currently uses Firebase because it's super simple but I'm open to anything—especially things that are provider independent.

Ed-boye commented 3 years ago

Mmm, yeah it does seem to come up frequently. Let me see what I can dig up 😃