search

enteryournamehere / wilson

A custom bot for the Wintergatan Discord server
GNU Affero General Public License v3.0
16 stars 8 forks source link

Redesign the Idea Vault #10

Closed Bluenix2 closed 3 years ago

Bluenix2 commented 3 years ago

The scope of this pull request is to improve the starboard implementation, as of opening this as a draft it only moves the starboard events into the utils/models/starboard file and sets some partial events, so we don't have to use "raw". Further testing will have to be done, as I am not sure if the only partial we actually want is "REACTION" ( and not "MESSAGE" too).

I did contemplate if I wanted to refactor the code, though issues would be with the database. But if you would agree to it, then we could rename the tables and continue with a full refactoring. This is a draft, not really tested as I have to get everything set up. Which I haven't done yet

enteryournamehere commented 3 years ago

Hey, thanks a lot for your interest and the PR! I've actually been working one some upgrades for the starboard locally as well, adding support for comments from e-team members.

Regarding the partial events / raw: at least when I implemented this, reactions to old (not cached, iirc) messages weren't received by listening in discord.js for the messageReactionAdd event, only by listening for raw. If it does work by listening for the specific events now, great, but it's something to test for.

I'll try to finish up the comment stuff within a few days so you can build on that. Though if you want to refactor the code and add comment support, that'd be cool too, but it might be more work. Let me know what you think, I can also throw my current code in a second branch just to look at and copy the general structure from.

Bluenix2 commented 3 years ago

Regarding the partial events / raw: at least when I implemented this, reactions to old (not cached, iirc) messages weren't received by listening in discord.js for the messageReactionAdd event, only by listening for raw. If it does work by listening for the specific events now, great, but it's something to test for.

Yes, normally the events for reactions to older messages that no longer is in cache were not propagated. This is because if we look at the official Discord documentation we can see that the bot basically only get the IDs. Discord.js solves this using partials. This means that we subscribe to reaction events and Discord.js will push out those events even if it couldn't get all the cached properties. There is a known bug though where it doesn't propagate when it should, see this merged pull request. So it will be in next release soon

I'll try to finish up the comment stuff within a few days so you can build on that. Though if you want to refactor the code and add comment support, that'd be cool too, but it might be more work. Let me know what you think, I can also throw my current code in a second branch just to look at and copy the general structure from.

If you could do that, then that would be great so that we don't get too many collisions and conflicts. Additionally, depending on how you want to do it, I could give you full access to my fork here and we can collectively work in the branch for this pull request. When we're done we can do some rebase-magic and squash together commits and polish the commit history a bit, and finally merge into the real repository. Especially if we're both modifying the same files.

enteryournamehere commented 3 years ago

Thanks for the info about the events! Didn't know about that partials feature, sounds like it fits our needs here.

I've pushed all of my current work to the new branch starboard-comments. It works almost completely. Ideas get an ID assigned when posted, which shows up at the bottom of the embed. People with a certain role as defined in the config file can use the comment <id> <text> command. This makes a comment appear as a separate field in the existing idea vault embed.

The only thing that doesn't work yet is editing comments. In the database model is defined that the combination of author and idea-id must be unique, so using sequelize's upsert should edit the existing row. It currently creates a new one. I can't look into this tonight, will have time tomorrow or the day after.

Additionally, existing ideas will need to have their embed updated to show the ID so they can be commented on. (I think it already will automatically happen when they reach a new tier, but I'm not entirely sure, can't test atm.)

Bluenix2 commented 3 years ago

Alright, that would be the last of my testing. I would say everything is working perfectly now.

If you want to make some testing and review this then the time would be now, I am gonna open this as ready for review so it can be merged once you find that this is in an acceptable state. I'll be happy to help you with any migrations of the MySQL database.

For the merging itself, I guess squashing would make sense to get a uniform "Redesign Idea Vault" commit name in the git blame and other scenarios. Though I am not sure what would actually be the best strategy. Saying this here for historical reasons, we've been talking on Discord

Bluenix2 commented 3 years ago

I notice now that Github believes this has merge conflicts with master, sadly it's not something I can resolve for you. I cannot even see what is conflicting, only the file names.

enteryournamehere commented 3 years ago

Resolved merge conflicts & (finally) merged! As discussed on Discord - I've migrated the old database, written a little script to add the ID to all old idea embeds, & deployed the new code. I also made a small adjustment to the embed generator code as it failed for messages with attachments (had a misplaced parenthesis), and added the restriction for the category from which it accepts new ideas. With those changes it now all works flawlessly. Thank you very much for your contribution!