Skip to content

Migrate temporary ban/mute #267

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

Merged
merged 14 commits into from
Dec 29, 2021
Merged

Migrate temporary ban/mute #267

merged 14 commits into from
Dec 29, 2021

Conversation

Zabuzard
Copy link
Member

@Zabuzard Zabuzard commented Nov 17, 2021

Overview

Dyno supports temporary bans and mutes, so should we. This adds support for such a feature. Part of #63 and #64 .

Therefore, a duration option is added to the existing ban and mute commands, which allows selection from a menu, such as:

choice menu

response

user DM

Details

The feature is implemented by using the existing ModerationActionsStore, which already supports a field expires_at.

A new class TemporaryModerationRoutine is scheduled every 5 minutes. The routine basically selects all actions from the store where expires_at is behind now (minus edge cases, like already revoked, overwriten by another action, ...) and then revokes those actions by unbanning/unmuting the user.

The system is designed modular enough to easily be expanded with new temporary moderation actions (i.e. other than temp ban and temp mute). Therefore, the slim interface RevocableModerationAction is implemented and an implementation is added to a list in TemporaryModerationRoutine, the rest is automatic.

The system also gracefully supports long downtimes. Hence, it is not a problem if the actual revocation date of an action is missed, the system will revoke it on the next execution when the bot is online again. This also holds true if the bot is temporarily removed from a guild or, more importantly if an user is temporarily not a member of the guild anymore (think about a temporary mute, then user leaves and wants to rejoin after the expiration date, mute should not be applied anymore).

Notes

Error handling, such as users who got deleted in the meantime, left the guild or the even the bot leaving the guild and similar, are handled solely by logger messages. There is no direct feedback in any guild channel (on purpose).

logger success

Revocation is picked up by the existing mod-audit-log feature:

audit log

As well as by the /audit command (ignore the timestamps, I didnt have the bot running for some time):

audit command

Checklist

TODO

Testing

Base cases:

  • Perm ban
  • Perm mute
  • Temp ban
  • Temp unmute

Bot leaves guild

  • Temp ban/mute, then remove bot from guild and wait until revoke kicks in (can be mocked easily by editing the DB guildId value)

User does not exist

  • Temp ban/mute, then delete user account from Discord and wait until revoke kicks in (can be mocked easily by editing the DB userId value)

User is not a member

  • Temp mute user, kick user, wait until revoke kicks in

Multiple overlapping actions

  • Temp ban user, then unban, then temp ban again with different time (all within less than 5 minutes (the routine schedule))

Stale event

  • Temp ban user, then unban, temp unban should not be executed anymore
  • Temp mute user, then unmute, temp unmute should not be executed anymore

@Zabuzard Zabuzard added the enhance command Modify or improve an existing command or group of commands of the bot label Nov 17, 2021
@Zabuzard Zabuzard added this to the Migrate existing commands milestone Nov 17, 2021
@Zabuzard Zabuzard self-assigned this Nov 17, 2021
@Zabuzard Zabuzard added the blocked This issue is currently blocked by another issue (see comments) label Nov 17, 2021
@Zabuzard
Copy link
Member Author

Blocked by #252

@RealYusufIsmail
Copy link
Contributor

Would it not be better waiting for the audit command?

@Zabuzard
Copy link
Member Author

Would it not be better waiting for the audit command?

Why? I could ask you the same. I do not see any actual blocking dependency between the two. Only inconvenience and extra work introduced for both of them.

@Zabuzard
Copy link
Member Author

Zabuzard commented Dec 3, 2021

Blocked by #297

@Zabuzard Zabuzard mentioned this pull request Dec 3, 2021
6 tasks
@Zabuzard Zabuzard removed the blocked This issue is currently blocked by another issue (see comments) label Dec 10, 2021
@Zabuzard Zabuzard mentioned this pull request Dec 10, 2021
This was referenced Dec 14, 2021
@Zabuzard Zabuzard force-pushed the feature/add_temp_ban_mute branch 2 times, most recently from 8d2d6ee to 69ab638 Compare December 16, 2021 09:08
@Zabuzard Zabuzard marked this pull request as ready for review December 16, 2021 09:09
@Zabuzard Zabuzard requested review from a team as code owners December 16, 2021 09:09
@Zabuzard Zabuzard enabled auto-merge (rebase) December 16, 2021 09:09
@Zabuzard Zabuzard mentioned this pull request Dec 16, 2021
@Zabuzard Zabuzard removed the request for review from RealYusufIsmail December 17, 2021 09:35
@Zabuzard
Copy link
Member Author

Yikes, got no time to resolve the conflict before holiday anymore. Will do it afterwards 🤷

* It was used quite a lot in the package but also outside, it made no sense anymore to keep it locked down in the utils class.
it will get more classes soon
* temporary moderation actions can now be added easily via interface instead of being all mushed into the main logic class
* database returns multiple results, forgot limit(1)
* logic mistake of using last expired action instead of last temp action (implicitly given by last action that passes the "not permanent" check)
* can use `fetchOptional` with database
* added note regarding startup race condition issue
* cant Instant#plus weeks and months
* also some improvements to the option order (UX)
* added fixme for code duplication
* moved temporary data computation into util
* also, simplification of the queries in the rejoin listener
* added /warn action that was added in the meantime
@Zabuzard Zabuzard force-pushed the feature/add_temp_ban_mute branch from f333deb to d9ac5d9 Compare December 29, 2021 17:08
@Zabuzard
Copy link
Member Author

Conflict resolved (no real code change)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Zabuzard Zabuzard disabled auto-merge December 29, 2021 17:12
@Zabuzard Zabuzard merged commit 126c40e into develop Dec 29, 2021
@Zabuzard
Copy link
Member Author

Merging since 2 weeks without actual code changes and everything looks fine.

@Zabuzard Zabuzard deleted the feature/add_temp_ban_mute branch December 29, 2021 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhance command Modify or improve an existing command or group of commands of the bot priority: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants