-
Notifications
You must be signed in to change notification settings - Fork 195
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
base: master
Are you sure you want to change the base?
Feat/application commands #455
Conversation
… moved util functions
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! |
…plication-commands
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
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! |
b3239e1
to
99f1368
Compare
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! |
99f1368
to
cafcc28
Compare
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! |
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! |
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) |
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! |
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! |
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! |
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! |
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! |
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! |
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! |
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! |
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! |
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! |
There was a problem hiding this 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:
-
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 }
. -
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 inpluginData.state
rather than callinggetPlugin
every time. E.g. in thebeforeInit
hook:pluginData.state.common = pluginData.getPlugin(CommonPlugin);
This works because
beforeInit
, unlikebeforeLoad
, allows access togetPlugin
. -
Now that mod action commands are neatly organized, let's move the "actualCommand" function files to those subfolders as well (e.g.
actualBanCmd
incommands/ban
)
.then(async (submitted) => { | ||
await submitted | ||
.deferReply({ ephemeral: true }) | ||
.catch((err) => logger.error(`Clean interaction defer failed: ${err}`)); |
There was a problem hiding this comment.
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}`)); |
There was a problem hiding this comment.
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")) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use if
s and early returns here for clarity!
Also I noticed that even though knub was updated in the package lock, it wasn't in the |
return actualCasesCmd( | ||
pluginData, | ||
interaction, | ||
options.mod, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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! |
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.
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!
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.
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!