search

flybywiresim / discord-bot-utils

Utilities Discord Bot created for the FlyByWire Discord server.
https://flybywiresim.com/
GNU Affero General Public License v3.0
5 stars 7 forks source link

feat: Prefix Command Management #85

Closed pdellaert closed 2 weeks ago

pdellaert commented 1 month ago

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

benw202 commented 1 month ago

Some further comments as discussed:

benw202 commented 1 month ago

Moderator in Prefix Command Cache Update is not a clickable tag. Could make this clickable if we want it to match other mod logs?

pdellaert commented 4 weeks ago

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)

pdellaert commented 4 weeks ago
  • On timeout of modal it says "You did not provide the necessary content information and the change was not made.". The user may have provided info, but not in time. Is it worth saying that there is a two minute timeout somewhere in the modal description?

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?

benw202 commented 4 weeks ago
  • On timeout of modal it says "You did not provide the necessary content information and the change was not made.". The user may have provided info, but not in time. Is it worth saying that there is a two minute timeout somewhere in the modal description?

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.

ExampleWasTaken commented 3 weeks ago

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 a32nx and a380x being custom versions of the .bg command:

a380x is disabled but set as default version for channel A:

Running .bg results in nothing being shown. IMO this should result in the GENERIC version being shown. The presence of the a380x is not leaked as showing the GENERIC version is the default behavior anyway. Additionally, the way it currently works, there is not way to get a list of all available versions.

Commands without content?

Is there any reason why commands can be "empty" i.e. have no content set? IMO there should at least be a GENERIC version of every command. This will

  1. ensure that users are able to retrieve a list of all available versions
  2. prevents "ghost" commands that exist but have no content and therefore no use

I might be missing something here but I didn't find an explanation for this and thought I'd raise it. :)

Consistent slash command

As it stands right now there are three different slash-commands for this feature:

I think this is pretty inconsistent and confusing to remember (was it /prefix-commands-permissions, /prefix-command-permission or /prefix-command-permissions?). Instead, I suggest we use a common slash-command /prefix and have everything else be sub commands.

Example:

This way, the user can explore all available commands in the Discord client by just typing /prefix. It also reduces the overall slash-commands used and provides a cleaner command structure imo.

I think overall this is looking pretty awesome! Really nice work!

ExampleWasTaken commented 3 weeks ago

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.

pdellaert commented 3 weeks ago

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) https://github.com/pdellaert/fbw-discord-bot-utils/compare/e95eae2ab3ceef2e7048a88f53b9433d011d613e...dynamic-command-management

ExampleWasTaken commented 3 weeks ago

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.

benw202 commented 3 weeks ago

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.