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

Invite filter DM & DMed invite modlog #251

Merged
merged 8 commits into from Jan 5, 2019

Conversation

4 participants
@sco1
Copy link
Member

sco1 commented Jan 3, 2019

  • Add configurable user notifications for message filter actions
    • Booleans added to server configuration, with the exception of the token filter since notification is handled by its own cog
    • If the filter's "user_notification" is True, a message is DMed to the user on filter action. This falls back to an in-channel mention if the user has DMs disabled
    • User notification message is configurable in the filter dictionary
  • Adjust the mod log message to read "via DM" rather than "in <#deleted-channel>" if a filter is triggered in a DM

Closes: #243
Closes: #245

sco1 added some commits Jan 3, 2019

Move user notification bools to config
Intentionally hardcode token notification, this is handled by the
token remover cog

Signed-off-by: sco1 <sco1.git@gmail.com>
@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 5, 2019

I'm testing this PR and one thing I've noticed is the following:

When you enable the user notification for a filter that has no message specified, the bot will throw the obvious exception:

discord.errors.HTTPException: BAD REQUEST (status code: 400): Cannot send an empty message

This isn't wrong, I think, but as it's anticipated, we may want to handle it. Maybe we can have the bot send this to modlog or mod_alert so it's obvious to the one that decided to turn the feature on that they forgot a crucial step: Setting the actual message to be send to the user.

Except for that, everything works well here, so if we decide against the thing above, I'll approve it directly.

@sco1

This comment has been minimized.

Copy link
Member

sco1 commented Jan 5, 2019

I guess it does make sense to prepopulate them if they're going to be a config option

sco1 added some commits Jan 5, 2019

Add notification strings for remaining filters
Hardcode watch words filter notification to False since it's a watchlist. This logic should be reworked eventually so we ignore user notifications for watchlist type filters.
Marginally smarter filter notification logic
Only check for a user notification for "filter" type filters
@SebastiaanZ
Copy link
Member

SebastiaanZ left a comment

Tested and works for me!

Bot Tracking automation moved this from Needs review to Reviewer approved Jan 5, 2019

@heavysaturn
Copy link
Member

heavysaturn left a comment

Elegant solution to an annoying problem. Thanks.

@sco1 sco1 dismissed stale reviews from heavysaturn and SebastiaanZ via 1c965ac Jan 5, 2019

Bot Tracking automation moved this from Reviewer approved to Needs review Jan 5, 2019

@heavysaturn
Copy link
Member

heavysaturn left a comment

re-approve

@SebastiaanZ
Copy link
Member

SebastiaanZ left a comment

Stills works! Looks good to me.

Bot Tracking automation moved this from Needs review to Reviewer approved Jan 5, 2019

@sco1 sco1 merged commit a7bd2ed into master Jan 5, 2019

Bot Tracking automation moved this from Reviewer approved to Done Jan 5, 2019

@sco1 sco1 deleted the invite-filter-dm branch Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment