Skip to content
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

Moderation cog cleanup #462

Merged
merged 58 commits into from Oct 8, 2019

Conversation

@MarkKoz
Copy link
Member

commented Sep 27, 2019

This is not extensively tested. I am hoping for others (i.e. the reviewers) to help out in this regard considering the importance of these cogs.

This aims to reduce redundancy and code density in the moderation cog.

The moderation cog was split in two: Infractions in infractions.py and ModManagement in management.py (contains infraction search and edit commands). They were then moved to a moderation sub-package along with ModLog and Superstarify. The sub-package now acts as the extension for all four of these cogs. utils/moderation.py was also moved inside the sub-package as moderation.utils.py.

To facilitate this move, custom category support was added to the help command. This allows the Infractions and ModManagement cogs to appear under the Moderation category in the help command's output.

Behaviour

Just to define some terms:

  • confirmation message - A message sent to the invocation context of the command. It shows if the command was successful.
  • appyling infractions - Doing a ban, a mute, a kick, or a warning and any shadow/temporary variations.
  • pardoning infractions - Doing an unmute or unban.

Error Handling

  • When applying infractions, most errors are deferred to the regular error handler
  • The Forbidden exception is currently the only caught exception when applying infractions
  • When pardoning infractions, the errors will be displayed in the confirmation message and the mod log
    • Displaying them in the mod log allows for errors which occur during automatic expiration to also be seen easily in Discord
  • If multiple active infractions exist when pardoning and an error occurs when trying to remove them, the error will simply say to check the bot's logs (i.e. stdout in the console)
  • If an automatic expiration fails, the moderator role will be mentioned in the mod log. Failures for not finding the user or the ban on Discord are excluded as the intention of the mention is to prevent infractions from being stuck.

Confirmation Messages

  • A message is sent to the invoking context specifying if the action failed or suceeded
  • For applying infractions, the failure message will be generic
  • When pardoning, the error messages will be appended to the confirmation message
  • Expiration date is shown in the confirmation
    • If the infraction is permanent, "permnanetly" is shown instead
    • No expiration is shown if the infraction fails to apply

DM Notifications

  • A mail emoji is shown in the confirmation message when DM is sent
  • The mod log shows a DM status (sent/failed)
  • The actor is mentioned in the mod log when a DM fails to send
  • If it's a shadow infraction, no emoji is shown and the DM status is absent from the mod log

Mod Log

  • A mod log is always sent for applying and pardoning infractions, even when done automatically for expired infractions
  • The events are always ignored in the mod log to prevent the default messages from showing
  • The title will show the infraction type and whether it failed or succeeded
  • Manual pardons and automatic expirations are distinguished in the title
  • Automatic expirations will show the bot as the actor

Miscellaneous

  • Pardoning/expiring infractions will now have reasons in the audit log which mention the infraction ID
  • When pardoning/expiring an infraction, if one step fails, the others will still be executed. This means that if, for example, the mute role fails to be removed, the PATCH request will still be sent to the API and the expiration task will still be cancelled.

Todo

  • Find a nice way to generalise pardoning infractions (unmute and unban)
  • Move extensions to a moderation subpackage.
    • Moderation cog will be in infractions.py and Infractions cog will be in management.py
    • [x ] Create a new group attribute for cogs so that the help command shows all grouped modules under a single name
      • Consider how this affects the reload command. Should they even be separate extensions or part of a single moderation extension? Part of a single moderation extension.
    • Possibly move other extensions here (watchchannels, superstarify, and modlog)
    • Move utils/moderation.py to cogs/moderation/utils.py
      • Should only contain utility functions that are used by several modules or that could be in the future. Module-specific utilities should remain in their corresponding modules.
  • Write unit tests? May be better to save for a future PR.

Manual tests for apply_infraction()

  • DM not sending
    • Confirmation message doesn't have mail emoji
    • Actor is mentioned in mod log
    • DM status shows as failed in mod log
  • DM sending
    • Confirmation message has mail emoji
    • Actor is not mentioned in mod log
    • DM status shows as sent in mod log
    • User recieves DM
      • Expiry correctly formatted
      • Icon correctly corresponds to the infraction type
  • Shadow infractions
    • No DM status in mod log
    • User does not recieve a DM
  • Mod log
    • Mod log is sent
    • Title appropriately indicates when the infraction failed to apply
    • Icon correctly corresponds to the infraction type
    • Mentions the actor when applying the infraction fails
  • Confirmation message
    • Success
      • Shows OK hand emoji and a message of affirmation
      • Mentions the user
      • Shows "permanently" for expiry if duration unspecified
    • Failure
      • Shows X emoji and an appropriate message
      • Does not show expiry
  • Test temporary infractions
    • Expiry is shown in mod log w/ correct format
    • Infraction is scheduled and does indeed expire
    • Expiry is shown in confirmation message w/ correct format
@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2019

The infraction search and edit commands should be separated from the moderation cog and put into a new cog. Should both the moderation and the new cog be moved into a moderation-related subpackage (moglog could go in there too)? If so, what would the package be named and what would the modules (yes, I mean the modules, not the cog classes) be named? Would the contents of utils/moderation.py be moved to the subpackages __init__?

@scragly

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

I can understand the logic behind splitting the moderation cog up to make it more manageable in the files, but having to put infr search and edit into an entirely different command category (user-facing) doesn't make much sense to me. Infraction management fits perfectly under the category of "Moderation", so I'm not sure I'm a fan.

@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2019

OK, that's fine. I don't really care for splitting the cogs. Really I just want to split the code - the modules. It could be implemented in such a way that it's one cog but two modules.

@scragly

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

This was discussed further in-server and it was decided splitting the cogs is fine under the condition that we support setting a category for a cog explicitly for help output, which should be a simple thing to support.

@MarkKoz MarkKoz force-pushed the moderation-cleanup branch from 9e9745e to 1e3b67b Oct 1, 2019
@sco1 sco1 added this to Needs review in Bot Tracking Oct 1, 2019
MarkKoz added 24 commits Sep 26, 2019
* Move respect_role_hierarchy to the decorators modules
* Get the command name from the context instead of an argument
* Add some logging
* Always return None from inner function
* Change annotation of self parameter to Cog
It may also be an Asset because when converted to a string the URL is
returned.
These adjustments make it easier to call the function using values
directly from the infraction object as arguments.

* Set actual default values inside the function if values are None
* Accept only a string for expires_at
* Rename the UserTypes alias to UserConverter
* Create a new non-converter alias similar to UserConverter which has
  Object instead of the proxy_user converter in the Union.
* Use the new alias in the utility functions instead of just a Union of
  a Member and User.
* Add warning & note icons to the infraction icons dictionary
* Rename to apply_infraction
* Make messages more generic to simplify implementation
* Send the confirmation message inside the function; return nothing
* Rename UserConverter to MemberConverter
* Rename UserObject to MemberObject
* Move MemberObject to moderation utils module
* Move proxy_user to moderation utils module
Commands defer to these functions, configuring them to be temporary
and/or shadow infractions by passing some kwargs. This reduces code
redundancy.
* Rename Infractions cog to ModManagement
* Rename Moderation cog to Infractions
* Rename infractions.py to management.py
* Rename moderation.py to infractions.py
* Move moderation utils to sub-package and rename to utils.py
* Move Modlog, Infractions, and ModManagement to sub-package
* Use sub-package as an extension that loads aforementioned cogs
* Read names from JSON instead of a module
* Move get_nick function inside the Superstarify cog
* Load Superstarify cog through the moderation extension
* Define __all__ for moderation module
The sub-package is now the extension instead of each module being a
separate extension. Thus, the setup methods are now useless.
@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

I'm working on resolving the conflict caused by #486. I want to just replace all the redundant infraction deactivation code it brings with a call to self.deactivate_infraction(). However, using this method has two problems, both caused by the role failing to be removed (which could be solved by re-adding the role before calling the method but that seems redundant):

  1. The mod log will show as "failed". There is already an parameter to disable sending the mod log. However, some mod log probably should be sent and writing separate code for that is easy but just feels redundant and thus not ideal.
  2. Because the DM is sent after the role is removed, the role removal failing means there won't be an attempt to send a DM. Generally, I think this is desirable behaviour. However, this specific case would be an exception.

Does anyone have ideas for clean and non-redundant way to handle deactivating the infraction?

Copy link
Member

left a comment

For the four infractions types below, the active status does not really make sense, but we never had any pressing needs to change it. Now we're cleaning up the moderation cog and planning to add validation for multiple active infractions of the same type to the server, it makes sense to change it.

bot/cogs/moderation/infractions.py Outdated Show resolved Hide resolved
bot/cogs/moderation/infractions.py Outdated Show resolved Hide resolved
bot/cogs/moderation/infractions.py Outdated Show resolved Hide resolved
bot/cogs/moderation/infractions.py Outdated Show resolved Hide resolved
@commands.Cog.listener()
async def on_ready(self) -> None:
"""Schedule expiration for previous infractions."""
infractions = await self.bot.api_client.get(
'bot/infractions',
params={'active': 'true'}
)
for infraction in infractions:
if infraction["expires_at"] is not None:
self.schedule_task(self.bot.loop, infraction["id"], infraction)
Comment for lines 42  – 51

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 3, 2019

Member

@scragly made a good point yesterday in #dev-core and I think it needs to be addressed. Because we're using the on_ready event to reschedule the expiration of all active infractions with an expiration date and the on_ready event does not fire when you reload the cog, reloading the moderation extension will erase all of the scheduled tasks:

2019-10-03_08-48

This is probably a problem that has always existed and has always gone unnoticed, but now is good time to fix it, I think. The solution to this would be to move this rescheduling logic to a regular coroutine that's added to the event loop as a Task at the end of the Infraction cog's __init__. I think we only need to add an await bot.wait_until_ready() as the first line, but I don't think the rest of the code needs to be changed.

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 3, 2019

Author Member

Is efd8a30 what you had in mind? Tried it out and it solves the issue. I looked at watchchannels and that assigns the task to a private attribute which is never referenced anywhere. Is that necessary to do here too?

@SebastiaanZ

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

I'm working on resolving the conflict caused by #486. I want to just replace all the redundant infraction deactivation code it brings with a call to self.deactivate_infraction(). However, using this method has two problems, both caused by the role failing to be removed (which could be solved by re-adding the role before calling the method but that seems redundant):

  1. The mod log will show as "failed". There is already an parameter to disable sending the mod log. However, some mod log probably should be sent and writing separate code for that is easy but just feels redundant and thus not ideal.

  2. Because the DM is sent after the role is removed, the role removal failing means there won't be an attempt to send a DM. Generally, I think this is desirable behaviour. However, this specific case would be an exception.

Does anyone have ideas for clean and non-redundant way to handle deactivating the infraction?

One option is to split this method that does two things, removing the role from the member and requesting the API to mark an entry in the database as inactive, into two methods that do one thing, optionally having the remove_role method call the mark_inactive (just filler names; I'd say you're activity is just fine) if we think that's a combination we always want to have. Another option would be to add a parameter for remove_role and an if check, but that feels messier to me and achieves basically the same (but with a greater cyclomatic complexity).

We would get the most flexibility if we were to add a third method, a wrapper that calls both, and don't have the remove_role call the mark_inactive, but I honestly don't see a case in which we want to attempt to remove a mute role/ban, but not make sure it's also marked as inactive in the database.

@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

@SebastiaanZ

One option is to split this method that does two things, removing the role from the member and requesting the API to mark an entry in the database as inactive, into two methods that do one thing, optionally having the remove_role method call the mark_inactive (just filler names; I'd say you're activity is just fine) if we think that's a combination we always want to have.

If the mute was split, it'd be something like this:

async def pardon_mute(self, ctx: Context, user: Member):
    self.mod_log.ignore(Event.member_update, user.id)
    await user.remove_roles(self._muted_role, reason=reason)

    # DM the user about the expiration.
    notified = await utils.notify_pardon(
        user=user,
        title="You have been unmuted.",
        content="You may now send messages in the server.",
        icon_url=utils.INFRACTION_ICONS["mute"][1]
    )

    return notified

However, splitting the function won't solve the issue of the DM not being sent on failure. As I said, normally we'd want the DM to not send if removing the role failed. Therefore, in this case, a try-except would need to be around the remove_roles call. How would it know when to ignore the exception and when to propagate it? Well, it'd need a parameter for that or a separate function definition which always exhibits such behaviour (this is redundant and not ideal).

Another option would be to add a parameter for remove_role and an if check, but that feels messier to me and achieves basically the same (but with a greater cyclomatic complexity).

I would prefer to not have such a parameter but it seems like it may be the better option given how convoluted and intraconnected this function already is.

@SebastiaanZ

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

I don't think I understand what you mean. From the fix you were referencing, I was under the impression we were talking about calling deactivate_infraction in the on_member_join event after finding an active mute infraction for the joining user in the database that has already expired.

If so, that means the user does not have the mute role, since they just joined the server. By splitting up the deactivate_infraction method into two methods, remove_role (related to the user's state on Discord) and mark_inactive (related to the infraction state in the database), we could then call mark_inactive and skip the remove_role function entirely. That way, we don't have to worry about a role and/or a DM, since they are not relevant for people (re)joining the server without any infractions that should be active.

@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

So you don't think in that situation a user should be DM'd about being unmuted?

@SebastiaanZ

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

I don't really think it's relevant. The mute expired while the user was not a member, so they don't get a DM. It's just an implementation detail that we're only marking it as inactive when they rejoin in some circumstances.

@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

I think you mean was not a member, but that make sense so I agree. Now I am wondering if we even want to show the expiration as failed in the mod log when the user already doesn't have the muted role or they don't have an active ban. Thinking about it now, it's actually technically still a success in the sense that they indeed are no longer affected by the infraction, which was the goal of the function.

If that is changed to not show as a failure along with not caring about DMing in this situation, then I can once again simply use deactivate_infraction in on_member_update.

@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

I'm just going to go ahead and do that. It makes sense.

MarkKoz added 2 commits Oct 3, 2019
@MarkKoz

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2019

Turns out remove_roles does not fail if the user already doesn't have the role...

# If multiple active infractions were found, mark them as inactive in the database
# and cancel their expiration tasks.
if len(response) > 1:
log.warning(f"Found more than one active {infr_type} infraction for user {user.id}")

footer = f"Infraction IDs: {', '.join(str(infr['id']) for infr in response)}"

log_note = f"Found multiple **active** {infr_type} infractions in the database."
if "Note" in log_text:
log_text["Note"] = f" {log_note}"
else:
log_text["Note"] = log_note

# deactivate_infraction() is not called again because:
# 1. Discord cannot store multiple active bans or assign multiples of the same role
# 2. It would send a pardon DM for each active infraction, which is redundant
for infraction in response[1:]:
_id = infraction['id']
try:
# Mark infraction as inactive in the database.
await self.bot.api_client.patch(
f"bot/infractions/{_id}",
json={"active": False}
)
except ResponseCodeError:
log.exception(f"Failed to deactivate infraction #{_id} ({infr_type})")
# This is simpler and cleaner than trying to concatenate all the errors.
log_text["Failure"] = "See bot's logs for details."

# Cancel pending expiration task.
if infraction["expires_at"] is not None:
self.cancel_task(infraction["id"])
Comment for lines +483  – +514

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 3, 2019

Author Member

This code handles multiple active infractions. Does it still need to be here for the time being or can it already be removed given what you are working on? I'm not caught up on the status of that. @SebastiaanZ

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 3, 2019

Member

Double active infractions may still occur for now. The code for the PR is done, the migrations file as well, but it will be stalled until python-discord/site#269 is resolved, since that PR also adds a migration file (and is actually far more critical). (We can't merge them independently because migration files have a single, explicit parent file mentioned in their body.)

I was debating with myself not to include the data migration and open a separate PR for it later, to prevent the conflict and not stall the validation, but that would still leave users with multiple active infractions in the database (17 users have double bans; no users have double mutes), it only guarantees we won't add more using the API.

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 3, 2019

Author Member

Thanks. Maybe everything will get merged in time but that seems unlikely. I'll leave this unresolved so I don't forget.

MarkKoz and others added 4 commits Oct 3, 2019
It doesn't make sense for these types of infractions to be "active".

Co-Authored-By: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
* Document support for custom categories.
@SebastiaanZ SebastiaanZ self-requested a review Oct 7, 2019
Copy link
Member

left a comment

One small URL change, the rest looks good to me.

bot/cogs/moderation/superstarify.py Outdated Show resolved Hide resolved
MarkKoz and others added 2 commits Oct 7, 2019
Co-Authored-By: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
Bot Tracking automation moved this from Needs review to Reviewer approved Oct 8, 2019
Copy link
Member

left a comment

Nice job, that's one massive PR.

@SebastiaanZ SebastiaanZ merged commit 3764950 into master Oct 8, 2019
2 checks passed
2 checks passed
Bot Build #20191008.3 succeeded
Details
Bot (Lint & Test) Lint & Test succeeded
Details
Bot Tracking automation moved this from Reviewer approved to Done Oct 8, 2019
@SebastiaanZ SebastiaanZ deleted the moderation-cleanup branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bot Tracking
  
Done
3 participants
You can’t perform that action at this time.