-
Notifications
You must be signed in to change notification settings - Fork 7
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: Prefix Command Management #85
base: staging
Are you sure you want to change the base?
feat: Prefix Command Management #85
Conversation
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.
Really good work! I like the way this has been done.
src/commands/moderation/prefixCommands/functions/addChannelPermission.ts
Outdated
Show resolved
Hide resolved
src/commands/moderation/prefixCommands/functions/modifyCommand.ts
Outdated
Show resolved
Hide resolved
src/commands/moderation/prefixCommands/functions/modifyVersion.ts
Outdated
Show resolved
Hide resolved
Some further comments as discussed:
|
Moderator in |
Addressed the review comments, will look at
Also likely to hide the choice buttons if it is a "fallback-to-GENERIC" if a different unavailable version is requested (either explicit, or by channel default) |
Can this be done? I don't see a way to add a description to the Modal? Doesn't look like we do it anywhere? |
You're right! I saw descriptions somewhere but seems it was only a concept. I guess we will just have to remember that we have a two minute limit. Make sure that people who can add and modify commands write their commands out elsewhere and just copy/paste in. |
I just gave this a test and thought I'd share what I found. I have not reviewed the code yet because I wanted to understand the functionality first. All was tested on e95eae2. To get the important stuff right out of the way:
The rest are just thoughts I had while messing about. When a custom version is set as a channel default and that version is disabled, nothing is shown.Example setup with
|
Another bug I just found: Aliases are not unique against command names and aliases of other commands. Example: These can all exist at the same time. Command 1
Command 2
Command 3
This results in undefined behavior as there are three possible commands that would all fulfil the conditions to be shown. |
Bunch of changes, most are bug fixes and minor enhancements to make sure that there's no overlap in commands, aliases & versions. Also no empty contents. Feedback on commands without content: Can't really do anything about it. I can't force setting a GENERIC content, unless we do it during the creation of the command, but would require a bit more work than we have time for right now. Feeback on different slash commands: As discussed in the channel, Discord has a max length on command definition and subgroup/commands, we would be exceeding it if we put it in one. Changes since my last comment (4 days ago) pdellaert/fbw-discord-bot-utils@e95eae2...dynamic-command-management |
That sounds awesome! Regarding the commands without content, I think that something we can introduce down the line if we want to. Definitely not a showstopper. |
Agree on this. There will be a limited amount of people who can add commands so the likelihood of us having rogue empty commands should be none. |
* AutoComplete added for categories, versions and command names where appropriate * Added Modal for Content
…l message create handler
…selection for GENERIC versions
* Allowing empty title * Fixing messageCreateHandler in case of unset permissions on command *
* Not properly checking commands and versions * Not prorperly using defer/reply/followUp
…nds. And version aliases do not overlap with commands
384855f
to
b650b38
Compare
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 is outstanding work! The way this is thought through and structured is more than impressive!
I've said a few things about it on Discord already so here just the most important stuff:
Currently, all commands that list something (version, categories, commands) will fail when there are more than 25 items to list. This is due to Discord's limitation of max 25 fields per embed. I think these commands migth need a paginated embed.
Absolutely awesome stuff!
const modLogsChannel = interaction.guild.channels.resolve(constantsConfig.channels.MOD_LOGS) as TextChannel; | ||
if (!modLogsChannel) { |
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.
const modLogsChannel = interaction.guild.channels.resolve(constantsConfig.channels.MOD_LOGS) as TextChannel; | |
if (!modLogsChannel) { | |
let modLogsChannel = interaction.guild.channels.resolve(constantsConfig.channels.MOD_LOGS); | |
if (!modLogsChannel || !modLogsChannel.isTextBased()) { | |
modLogsChannel = null; |
Might be safer to actually check if the channel is a text channel? Especially if we want to offer the codebase to other projects. Then, if it's not a text channel set it to null to prevent accidental attempts to send into a non-text channel.
|
||
const doesNotExistsEmbed = (category: string) => makeEmbed({ | ||
title: 'Prefix Commands - Modify Category - Does not exist', | ||
description: `The prefix command category ${category} does not exists. Can not modify it.`, |
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.
description: `The prefix command category ${category} does not exists. Can not modify it.`, | |
description: `The prefix command category ${category} does not exists. Cannot modify it.`, |
"Cannot" is generally more common. I suggest we use that but obviously that's personal preference and I'm happy to leave it as is!
const modLogsChannel = interaction.guild.channels.resolve(constantsConfig.channels.MOD_LOGS) as TextChannel; | ||
if (!modLogsChannel) { |
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.
const modLogsChannel = interaction.guild.channels.resolve(constantsConfig.channels.MOD_LOGS) as TextChannel; | |
if (!modLogsChannel) { | |
let modLogsChannel = interaction.guild.channels.resolve(constantsConfig.channels.MOD_LOGS); | |
if (!modLogsChannel || !modLogsChannel.isTextBased()) { | |
modLogsChannel = null; |
Might be safer to actually check if the channel is a text channel? Especially if we want to offer the codebase to other projects. Then, if it's not a text channel set it to null to prevent accidental attempts to send into a non-text channel.
|
||
const doesNotExistsEmbed = (category: string) => makeEmbed({ | ||
title: 'Prefix Commands - Delete Category - Does not exist', | ||
description: `The prefix command category ${category} does not exists. Can not delete it.`, |
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.
description: `The prefix command category ${category} does not exists. Can not delete it.`, | |
description: `The prefix command category ${category} does not exists. Cannot delete it.`, |
"Cannot" is generally more common. I suggest we use that but obviously that's personal preference and I'm happy to leave it as is!
await interaction.followUp({ embeds: [noModLogs], ephemeral: true }); | ||
return; |
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.
await interaction.followUp({ embeds: [noModLogs], ephemeral: true }); | |
return; | |
await interaction.followUp({ embeds: [noModLogs], ephemeral: true }); |
As discussed, this is just a warning and shouldn't block the creation.
for (const category of PrefixCommandCategories) { | ||
// eslint-disable-next-line no-await-in-loop | ||
await loadSinglePrefixCommandCategoryToCache(category); | ||
} |
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.
for (const category of PrefixCommandCategories) { | |
// eslint-disable-next-line no-await-in-loop | |
await loadSinglePrefixCommandCategoryToCache(category); | |
} | |
await Promise.all(PrefixCommandCategories.map((category) => loadSinglePrefixCommandCategoryToCache(category))); |
Nerd level increasing... xD
for (const dbCategory of prefixCommandCategories) { | ||
// eslint-disable-next-line no-await-in-loop | ||
await loadSinglePrefixCommandCategoryToCache(dbCategory); | ||
} |
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.
for (const dbCategory of prefixCommandCategories) { | |
// eslint-disable-next-line no-await-in-loop | |
await loadSinglePrefixCommandCategoryToCache(dbCategory); | |
} | |
await Promise.all(prefixCommandCategories.map((dbCategory) => loadSinglePrefixCommandCategoryToCache(dbCategory))); |
Awaiting all the promises...
for (const defaultVersion of PrefixCommandChannelDefaultVersions) { | ||
// eslint-disable-next-line no-await-in-loop | ||
await loadSinglePrefixCommandChannelDefaultVersionToCache(defaultVersion); | ||
} |
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.
for (const defaultVersion of PrefixCommandChannelDefaultVersions) { | |
// eslint-disable-next-line no-await-in-loop | |
await loadSinglePrefixCommandChannelDefaultVersionToCache(defaultVersion); | |
} | |
await Promise.all( | |
PrefixCommandChannelDefaultVersions | |
.map((defaultVersion) => loadSinglePrefixCommandChannelDefaultVersionToCache(defaultVersion)), | |
); |
Can't wait till prettier does the formtting for us...
for (const dbChannelDefaultVersion of prefixCommandChannelDefaultVersions) { | ||
// eslint-disable-next-line no-await-in-loop | ||
await loadSinglePrefixCommandChannelDefaultVersionToCache(dbChannelDefaultVersion); | ||
} |
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.
for (const dbChannelDefaultVersion of prefixCommandChannelDefaultVersions) { | |
// eslint-disable-next-line no-await-in-loop | |
await loadSinglePrefixCommandChannelDefaultVersionToCache(dbChannelDefaultVersion); | |
} | |
await Promise.all( | |
prefixCommandChannelDefaultVersions | |
.map( | |
(dbChannelDefaultVersion) => loadSinglePrefixCommandChannelDefaultVersionToCache(dbChannelDefaultVersion), | |
), | |
); |
Even longer lmao. But I'm always a fan of descriptive names so I rather take that than guessing what a function may do 👍.
await refreshAllPrefixCommandVersionsCache(); | ||
await refreshAllPrefixCommandCategoriesCache(); | ||
await refreshAllPrefixCommandsCache(); | ||
await refreshAllPrefixCommandChannelDefaultVersionsCache(); |
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.
These could potentially be refreshed concurrently using Promise.all()
, but it is unclear to me if, and if so, how versions and default channel version are dependent on each other. It may be simpler to avoid race conditions and update them sequentially.
Prefix Command Management
Description
Please read https://github.com/pdellaert/fbw-discord-bot-utils/blob/dynamic-command-management/docs/prefix-commands.md for all the fancy details.
NOTE: The cache refresh system will be rewritten to be safer, right now it isn't perfectly safe.
Test Results
I don't know how many hours, but probably good someone takes their own look...
Discord Username
straks