Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/application commands #455

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

LilyBergonzat
Copy link

I started from Obliie's pull request and started implementing slash commands.

Break down of the commits

I think it makes sense to review this per commit. The first few ones are Obliie's, and the merge commit, so I will omit those and only explain mine.

First commit: WIP: Note Slash Command

I needed a proof of concept. I took the Note command because it seemed simple enough, but not too simple.

  • Started with a command I am familiar with, the ban command, started moving code parts around so it would be ready to extract the business part to an actualBanCmd function, realized it was probably too big for a proof of concept so picked another command instead (but what I had moved in the ban command made sense so I kept it)
  • Extracted the business code to an "actualNoteCmd" function so that the slash command file and the message command file could call the same code
  • Had to rename the type of message commands, to specify they are message commands since they are not anymore the only type of command in the plugin, which modified every single command file
  • First modification of the sendSuccessMessage and sendErrorMessage functions
  • Introduction of a "context" concept, where a lot of functions now don't know whether they are called by a message command or a slash command, so they can take either a text channel or an interaction (because at that point the context was used to reply only)
  • We decided with Dragory that since Zeppelin definitely has more than a hundred commands, and since Discord's maximum amount of slash commands for one bot is 100, we are going to make each Zeppelin plugin a slash command group (at least when it's relevant) and each Zeppelin command inside of that plugin is going to be a slash command in that group.

Second commit: feat: first batch of emojis 🎉

I can't believe I am noticing just now that I had a brainfart while writing the name of this commit... I was so glad I wanted to name it "First batch of slash commands", but then I felt like it needed an emoji. I guess somewhere along the line I typed "emojis" instead of "slash commands" :') . But yeah, this is supposed to be the first batch of slash commands.

Following the note command's proof of concept, I did the same thing on all the other commands of the ModActions plugin. It was a huge amount of work and I had to modify a bunch of util functions!

  • New util functions isContextInteraction and sendContextResponse. A lot of functions need to reply to a context, and writing the same "ifs" everywhere doesn't make sense when I can just make a function, so that's how sendContextResponse was born. And in order to know what I should do with a context to send a response, I mainly needed to know if it was an interaction or not, hence the other function. Context also got the "User" type because there is a util function somewhere that can take a User as well as a TextBasedChannel, don't ask me which one I don't remember, but it made sense since User.send() is a thing, so I added it. Edit: I found which one. It was createPaginatedMessage().
  • Every command got transferred to its own neat folder. Most command folders have only two files, a SmthMsgCmd.ts one and a SmthSlashCmd.ts one. Some have more than two.
  • "actualCommand"s really started popping out in the functions folder, so I gave them their own cozy subfolder
  • When mods use Zeppelin, they only see a single "cases" command, even though there are technically two in the code. For message commands, knub makes it work to have two commands with the same name because their signature is different. For slash commands, that was impossible. I had to somehow merge those two. And that required changing GuildCases.ts a bit to offer new features to the end result!
  • Did you know that a slash command group has a character limit??? Well if you did, you're a nerd. I didn't. Discord adds up the amount of characters in the slash command group's name and description and all of its slash command children's names, descriptions, option names and descriptions, and if all of that is more than 4000 characters, it is denied. For that reason, I had to find out how to cut down on the amount of characters. I noticed that a lot of it was probably the three attachment options a lot of commands had, so I made a constants.ts file that would hold two constants: one for the amount of attachments each case creating command (like ban, warn, mute, etc) has, and one for the amount of attachments each case updating command (there's only one, update) has. With that, I could easily switch it for every single command and make sure to not hit the limit.
  • Slash command's attachment type options can only take one file. So I made helper functions. One of them generated as many attachment slash command options as you want, the other one retrieves as many attachment slash command option values as you want. They are used throughout the slash commands that need attachments.

Third commit: Added Discord attachment link reaction, fixed emoji configuration and moved util functions

This one felt like it would get the best of me, it seemed so easy but ended up being so much work. Discord attachment links are only valid for a limited amount of time (I thought it was a month, but turns out it's like two weeks? Or maybe a week only? But anyways they said it could change so yeah), so basically using those links for case archives is no bueno.

  • Added a "getContextChannel" util function because a lot of functions need to get a TextBasedChannel from a context.
  • Context had to accept messages too, for attachment handling reasons
  • Added a function to append the message link to a case reason, so that way the message link is stored in the case archives, and that one will last forever, unless someone deletes the message.
    • If this function receives a message, coolio, just adds the message link, ezpz lemon squeezie.
    • If this function receives an interaction, oof. There is no message link for interactions, so we have to do something about that.
      • Added a configuration so that server admins can decide where Zeppelin stores attachments
      • This function will use that configuration and repost the attachments to that channel, if it does not exist it will repost them to the current channel (the interaction.channel channel)
      • Reposting the attachments means a message now exists, so the function uses that message link and adds it to the case reason
  • For each function that touches a case reason, the reason sent to the user has the old fashioned attachment links so they receive the images, and reason stored in the archives has the message link
  • Added a configuration to decide what to do if Zeppelin detects that a Discord attachment link was manually added to a reason (nothing, warn or restrict)
  • For each function that touches a case reason, if that reason contains a manually added Discord attachment link,
    • If the configuration is "none", the bot keeps it in the reason and stores it as if nothing was detected
    • If the configuration is "warn" or null (because that's the default), the bot will send a message saying that there is a temporary attachment link in that reason and that it's better to upload attachments with the command, but will still proceed with the command
    • If the configuration is "restrict", the bot will send the same warning as previously but will also inform the user that the command was canceled, and will just stop there.
  • Fixed success and error emoji configuration
    • Added an error message if they are still configured in the root of the yaml, explaining how to properly configure them instead
    • Added a "common" plugin that can receive the configuration for those emojis
    • Moved "sendErrorMessage" and "sendSuccessMessage" to that new plugin to avoid a circular reference
    • Making that move basically had me modify every single typescript file in the universe, but don't worry about my mental health it's fine I'll just send you my therapy bill

Conclusion

This is a huge pull request. I was not able to test everything and make sure there are no bugs. I tested what I could, obviously, but I wouldn't be surprised if we were to find a lot of bugs in this, even I am not enough people to test this many cases.

I think what I will do is merge this on VANGUARD, and let our mods do the testing. They will probably find bugs, I will fix them in this PR and merge again on VANGUARD. Eventually, we should have a pretty stable version!

Copy link

cla-bot bot commented Feb 16, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 16, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

1 similar comment
Copy link

cla-bot bot commented Feb 17, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 17, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 17, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 17, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

@TheKodeToad
Copy link

Not sure if feedback is wanted but perhaps ephemeral commands should be optional in some way? Otherwise you don't really have parity with the message commands where they were publicly visible (which I think was quite nice for transparency or even for moderators to find out who is spamming reminders :P)

Copy link

cla-bot bot commented Feb 17, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 18, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 18, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 20, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 22, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 22, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 24, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 24, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 24, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

Copy link

cla-bot bot commented Feb 24, 2024

Thank you for contributing to Zeppelin! We require contributors to sign our Contributor License Agreement (CLA). To let us review and merge your code, please visit https://github.com/ZeppelinBot/CLA to sign the CLA!

@cla-bot cla-bot bot added the cla-signed label Feb 26, 2024
Copy link
Collaborator

@Dragory Dragory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks amazing! I need to run it myself and do some testing as well, but I did an initial review pass.

In addition to the code comments:

  1. Let's use the guildPluginSlashCommand helper function from Knub for creating slash commands. That way we get proper option type autocomplete and don't need to do { type: "slash", ...cmd }.

  2. Since we're now using plugin public functions a lot more than before, it might make sense to store the common plugin's public interface in pluginData.state rather than calling getPlugin every time. E.g. in the beforeInit hook:

    pluginData.state.common = pluginData.getPlugin(CommonPlugin);
    

    This works because beforeInit, unlike beforeLoad, allows access to getPlugin.

  3. Now that mod action commands are neatly organized, let's move the "actualCommand" function files to those subfolders as well (e.g. actualBanCmd in commands/ban)

.then(async (submitted) => {
await submitted
.deferReply({ ephemeral: true })
.catch((err) => logger.error(`Clean interaction defer failed: ${err}`));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to continue on error here?

async run({ pluginData, interaction }) {
await interaction
.deferReply({ ephemeral: true })
.catch((err) => logger.error(`Mod menu interaction defer failed: ${err}`));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to continue on error here?

const utility = pluginData.getPlugin(UtilityPlugin);
if (
!userCfg.can_use ||
(await !utility.hasPermission(executingMember, interaction.channelId, "can_open_mod_menu"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The utility plugin's hasPermission function only checks against the utility plugin's permissions. It was used before to re-use the can_clean permission from there. Here, just checking userCfg.can_open_mod_menu should be enough.

That said, maybe we should also add a check for mod_actions perms for each action you can take from the mod menu? Similar to the old can_clean check. This would require a similar hasPermission public function in the mod actions plugin.

return reason;
}

const attachmentChannelId = pluginData.config.get().attachment_storing_channel;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should move attachment storing to a generic public function in the new common plugin? That way other plugins could make use of it as well, and the attachment storage channel wouldn't have to be configured separately for each of them.

Comment on lines 37 to 43
return contextIsInteraction
? context.replied
? context.editReply.bind(context)
: context.reply.bind(context)
: "send" in context
? context.send.bind(context)
: context.channel.send.bind(context.channel);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use ifs and early returns here for clarity!

@Pukimaa
Copy link

Pukimaa commented Mar 5, 2024

Also I noticed that even though knub was updated in the package lock, it wasn't in the package.json itself, thus the newest version was not installed and there were some type errors. You might wanna fix that too.

return actualCasesCmd(
pluginData,
interaction,
options.mod,
Copy link

@Pukimaa Pukimaa Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an user object, but the resolver ( resolveUserId) is looking for an ID, mention or username.
Because the TypeError is that the value doesn't have a .match() function.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue might be in multiple files, but this is the one I recently discovered.

@Dragory
Copy link
Collaborator

Dragory commented Aug 11, 2024

I've just merged this to the "next" branch, though GitHub won't recognize the PR as merged until that is also merged into master.

Thank you again for the absolutely amazing work on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants