djdeveloperr / ban_appeal_plugin Goto Github PK

All the command responses are sent as normal message content, rather than embeds.

Ideally, they would respond as embeds with the configured error and ok colors for ModMail, as appropriate. This would be more consistent with the core bot.

None of the commands under the "banappeal" group show a permission level in the [p]help command, because they don't have one explicitly set with a decorator. They still respect the inherited permissions of the group, as expected, but ideally, like core ModMail, they'd all have decorators so the actual required permission level shows in the help command response.

https://github.com/kyb3r/modmail/wiki/Plugins#database-interfacing
Says:

Do not interact with bot.api directly. Fetch a partition and use it:

Yet, in https://github.com/DjDeveloperr/ban_appeal_plugin/blob/main/ban_appeal/ban_appeal.py#L299 self.bot.api is directly called for the post_log method. Is this not possible to due through the partition?

Additionally, that's under the section "Database Interfacing", but I notice bot.db is being used directly? Are the interactions there not possible through a partition?

I'm not very experienced with Python, nor have I done a deep dive of Modmail's codebase, so if there's something I'm missing here, just let me know.

On the "Can't re-appeal." error page, for an accepted appeal, it would be nice to add some additional context.

So, instead of this:
"Your previous appeal was accepted."

This:
"Your previous appeal was accepted.
You have since been banned again, and are ineligible to re-appeal."

(I know this contradicts what I said during initial discussion a bit, but I've thought it through a bit more since.)

Core ModMail, when creating a new thread, creates with the default permissions for the category, (d.py standard) then adds a user specific override to allow the bot too see the channel, in case it's not already included in the category permissions. This ensures that threads are more likely to work as intended.

It would be ideal to do the same for ban appeal threads, to help prevent any permissions issues.

Core ModMail doesn't show any valid commands in the logviewer. It would be nice if this behaved in the same way, but isn't strictly necessary. Feel free to mark "won't fix" if this is a high effort ask.

After updating to the latest commits, the embed method seems to have broken things. No commands give any response, and this is what I see in the bot logs.

If there are no questions set through the bot, there's a default set of questions that show on the site. But, when you do [p]banappeal questions list it shows "No questions have been set!" which would seem to indicate to me that no questions will show on the site. (Leading to the site either being non-functional or allowing users to submit an empty appeal)

This message should say something like:

No questions have been set. Default questions will be used:
1. Who banned you?
2. Why do you think you were banned?
3. Are you sorry?

To connect to the Atlas Mongo instance I set up following the ModMail wiki I needed to add &authMechanism=SCRAM-SHA-1 to my mongo connection string.

Probably a good idea to add that to the readme. Maybe in a "Troubleshooting" section.

It's possible to fill out an appeal and hit the character limit for an embed/message in Discord with just 6 questions (which isn't an unreasonable amount for an appeal form)

In this case, the bot opens an appeal thread, but the message fails to send in the channel and questions/answers don't show in the logs.

This is something that's unlikely to be a problem in a real appeal, as people won't realistically be intentionally maxing out the character limit, so feel free to mark as "wont fix" if resolving this is too much of a hassle. But, it'd be nice to support this edge case if possible.

When we discussed on Discord, I asked if you'd be willing to release under MIT, but I see the repo is under the the Apache 2.0 License.

Is there a particular reason for this change?

Docker run supports .env with the --env-file parameter, so you can fill out the .env file, and pass that rather than using the -e parameter for each environment variable. Also, the -p parameter is needed to expose the port to the web/nginx, and it's probably best to run detached with a restart policy.

So, I think the docker run command should look more like docker run -d -p 2012:2012 --env-file .env --restart unless-stopped appeal-server with instructions to fill out the .env file before running.

At least that's how I launched mine, and what I needed to do to get it running as I wanted.