Skip to content

Add the modmail extension#884

Merged
ajax146 merged 35 commits intomainfrom
modmail_poc
Apr 13, 2024
Merged

Add the modmail extension#884
ajax146 merged 35 commits intomainfrom
modmail_poc

Conversation

@Cpt-Dingus
Copy link
Contributor

Also rewrites how the main bots asyncio thread works, uses a completely separate bot token for modmail.

@Cpt-Dingus Cpt-Dingus changed the title Adds the modmail extension Add the modmail extension Feb 17, 2024
Copy link
Contributor

@dkay0670 dkay0670 left a comment

Choose a reason for hiding this comment

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

A few possible bugs and typos

@Cpt-Dingus Cpt-Dingus added the New module Issues relating to building a new module label Mar 1, 2024
Copy link
Contributor

@ajax146 ajax146 left a comment

Choose a reason for hiding this comment

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

Bugs (or potential bugs):

  • Initial thread creation embed is missing the header containing name and icon
  • Role IDs in config must be stored as strings, as rounding issues can occur in some cases, causing all ints to become invalid
  • Do not allow an empty message to be sent (..areply/..reply shouldn't do anything without content OR just an attachment)
  • Attachments from user should be sent in the same message as the content (main embed)
  • Mods cannot send an attachment to a user
  • Typo: "To restrict spam, you can not open a new thread within 24 hours ofa thread being closed. Please try again later."
  • A user who was contacted using .contact should be able to send messages, even within 24 hours of last thread.
  • Sending a message user->staff doesn't cancel a timed close
  • The database entry for modmail ban is just user_id, but the bot also uses ban_date, making ban not work at all
  • The past threads counter always says no past threads, even when there are past threads.
  • A user can open multiple threads by sending multiple messages without hitting confirm, then clicking confirm on all of them. A check should be done after hitting confirm to make sure people are still allowed to create a thread.

Feature requests/changes:

  • "Please confirm! Create a Modmail thread?" - Message should be configurable in the guild config patch
  • Timeout on thread creation should explicitly say timeout, and explicitly say you are welcome to create another thread.
  • Make a ..help to show modmail commands, or move all modmail commands to use the standard command registration to allow use of the standard help
  • Track message edits (don't care for deletes right now).
  • If a user sends only an attachment but no content, the embed should say no content/only attachment
  • The closing log should state if the close was done silently or not

@TheKrol
Copy link
Contributor

TheKrol commented Apr 1, 2024

Fixes #400

@TheKrol TheKrol linked an issue Apr 1, 2024 that may be closed by this pull request
@Cpt-Dingus Cpt-Dingus requested a review from ajax146 April 7, 2024 13:48
ajax146
ajax146 previously requested changes Apr 7, 2024
Copy link
Contributor

@ajax146 ajax146 left a comment

Choose a reason for hiding this comment

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

Bugs/requests that will hold up the PR:

  • Reboot no longer works
  • Thread 24h timeout no longer works
  • Aliases do not work, stating that Member has no attributes attachments (see stacktrace below)
  • On this message "Thread confirmation prompt timed out, please hit Confirm or Deny when creating a new thread. You are welcome to send another message.", the buttons are confim or cancel, not confirm or deny.
  • Sending attachments user->staff with no message should show that it is only an attachment
  • Sending attachments always show <Image> even if the attachment isn't an image
  • You can still open multiple threads if you push the confirm button multiple times quickly. Perhaps a better solution is to track who has a confirm prompt open and not allow another confirm prompt to be sent
  • Role IDs in config must be stored as strings, as rounding issues can occur in some cases, causing all ints to become invalid
  • Typo: @USER-zsZWAgPNHL was successfully banned from creating future modmailthreads.
  • Do not send message edits if the person editing the message has been banned from modmail
  • For all traditionally registered commands, replace brief with description. I'm not sure what brief is used for, but description is needed for the help menu.
  • Sending factoids should respect the restricted and disabled flags. Unless the modmail forum channel is listed in the restricted channels, don't send the factoid
  • Sending a factoid alias does not work, just saying "You need to include message contents!"
  • Don't allow the same role to be pinged multiple times on thread creation. The list should be deduplicated
  • In the event that there are so many roles selected to be mentioned (AFTER deduplication) that message length > 2000, don't ping any roles. Otherwise the thread won't be created
  • The disable_thread_creation should only prevent new threads from being created via DM, should not prevent any already open thread, or threads opened via contact, from being used on both sides

Wishful thinking:

  • Many config items are incapable of being refreshed when config patch is run. While this doesn't prevent anything from working, it is fairly inconvenient to have to reboot the entire bot only for changing some of the modmail config.
  • The closing log should state if the close was done silently or not (for this, I mean in the dedicated logging channel, not the threads)
  • Attempt to only send the user the thread creation message if and only if the thread was actually created successfully. In the event of an unexpected error, the user should ideally be notified that their thread wasn't created and that it isn't their fault.
  • Add a global config item (in config yaml) to completely disable modmail, this should prevent all modmail commands from loading, and prevent the modmail bot from logging in.

Errors/stacktraces:

I was able to get the following error on DMing modmail, but I am unsure how to repeat it. A reboot solved the issue:

2024-04-07 16:09:51.167 ERROR: Ignoring exception in on_message
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/discord/client.py", line 441, in _run_event
    await coro(*args, **kwargs)
  File "/var/TechSupportBot/techsupport_bot/commands/modmail.py", line 109, in on_message
    await handle_dm(message)
  File "/var/TechSupportBot/techsupport_bot/commands/modmail.py", line 257, in handle_dm
    if thread.id in closure_jobs:
       ^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'id'

Aliases not working stacktrace:

2024-04-07 16:26:21.732 ERROR: Ignoring exception in on_message
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/discord/client.py", line 441, in _run_event
    await coro(*args, **kwargs)
  File "/var/TechSupportBot/techsupport_bot/commands/modmail.py", line 961, in on_message
    await reply_to_thread(aliases[alias], message.author, message.channel, True)
  File "/var/TechSupportBot/techsupport_bot/commands/modmail.py", line 512, in reply_to_thread
    if message.attachments:
       ^^^^^^^^^^^^^^^^^^^
AttributeError: 'Member' object has no attribute 'attachments'

Cannot be tested due to new bugs:

  • Typo: "To restrict spam, you can not open a new thread within 24 hours ofa thread being closed. Please try again later."
  • A user who was contacted using .contact should be able to send messages, even within 24 hours of last thread.

@Cpt-Dingus Cpt-Dingus dismissed ajax146’s stale review April 12, 2024 13:56

review stuff done

@Cpt-Dingus Cpt-Dingus requested a review from ajax146 April 12, 2024 13:56
Copy link
Contributor

@ajax146 ajax146 left a comment

Choose a reason for hiding this comment

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

Hopefully this is it:

MAJOR BUGS:
Someone hitting cancel gets locked out of thread creation, just saying "Please respond to the existing prompt before trying to open a new modmail thread!". Same thing happens with timeout.
The factoids restricted list should compare the parent forum channel, not each individual thread when deciding to send or not.
Closing a thread timed then instant will close the thread twice, potentially sending the user 2 messages, creating 2 close logs, and having the thread be named [CLOSED] D]
Silently closing a thread does not remove it from the active_threads list, thus preventing the user from ever opening another thread until reboot and causing an error. Looking at the code the same thing would happen if the user has left the guild.

MINOR BUGS:
Edits only work on messages 1024 characters and fewer (both before and after counted separately).
Don't allow the same role to be pinged multiple times on thread creation. The list should be deduplicated (This doesn't appear to have been fixed)

MULTIPLE THREAD ISSUES:
Multiple threads can be created if contact and user both attempt to open threads at the same time
Contact has no checks after hitting confirm to see if threads can still be created, allowing the same person to run contact unlimited times and create all the threads. Or two mods accidently both doing it (which has happened before)

INCONSISTENCY/FORMATTING:
Send a message in the channel when a member joins as well as when they leave
This "rTechSupport Moderator" shouldn't be hardcoded, but rather use the actual set name of the guild
React envelope on the very first message sent by the user, for consistency
The thread created should be sent first, above any auto responses.
I think that "Modmail isn't accepting messages right now. Please try again later." should take precedence over "To restrict spam, you are timed out from creating new threads. You are welcome to create a new thread after 24 hours since your previous thread's closing."
Replying takes characters on new lines, but if no characters are on the first line, matching with ..reply/..areply, it won't send.
Example:
works:
```
..areply a

    b
    
    c
    ```
    doesn't work: ```
    ..areply
    
    a
    
    b
    
    c
    ```

@ajax146
Copy link
Contributor

ajax146 commented Apr 12, 2024

For the multiple thread issues, I am not overly worried about the combined contact and DM, but the multiple contacts do need to be addressed, as that has been a real-world case before

Copy link
Contributor

@ajax146 ajax146 left a comment

Choose a reason for hiding this comment

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

Thanks for all the hard work on this

@ajax146 ajax146 merged commit 4b69502 into main Apr 13, 2024
@Cpt-Dingus Cpt-Dingus deleted the modmail_poc branch April 13, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New module Issues relating to building a new module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add modmail features to the bot

5 participants