Skip to content

Prevent role hierarchy issues with infractions#1798

Merged
jchristgit merged 6 commits into
mainfrom
ignore-infra-mods-errors
Oct 5, 2021
Merged

Prevent role hierarchy issues with infractions#1798
jchristgit merged 6 commits into
mainfrom
ignore-infra-mods-errors

Conversation

@TizzySaurus
Copy link
Copy Markdown
Contributor

Closes #1780.

log.warning is no longer called when an infraction/pardon fails due to the target being a Moderator+

I believe this is what @MarkKoz was after when they created their issue.

Not sure whether we want to still ping @Moderators if the pardon fails for this reason or not, but have left the ping there for now (can remove if requested).

MarkKoz
MarkKoz previously requested changes Sep 1, 2021
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

Did you test this first? It won't work because you're comparing role objects against IDs.

Also, do you think it would be scope creep to create a utility function for this role checking, since a similar pattern is being used elsewhere in the codebase?

@MarkKoz MarkKoz added a: moderation Related to community moderation functionality: (moderation, defcon, verification) p: 3 - low Low Priority t: enhancement Changes or improvements to existing features labels Sep 1, 2021
@TizzySaurus TizzySaurus force-pushed the ignore-infra-mods-errors branch from 15b4ab7 to df7b23b Compare September 1, 2021 10:53
@TizzySaurus
Copy link
Copy Markdown
Contributor Author

TizzySaurus commented Sep 1, 2021

After a long discussion on discord it was decided to simply prevent the error raised from using !starify on members above the bot in the role hierarchy by not allowing invocation on such members.

A summary of reason is basically that in the other cases we want to know something went wrong, because there shouldn't be an error in any other infractions due to the user being staff (we can successfully mute etc. staff, including Moderators+, it's only starify we can't as it changes nickname which is hierarchy dependant.)

Edit: just realised kicks and bans are also hierarchy dependant, so have added the same check to them.

Now explicitly states that the bot is unable to starify/kick/ban someone who's higher in the role hierarchy
@TizzySaurus TizzySaurus force-pushed the ignore-infra-mods-errors branch from df7b23b to 8e25ed1 Compare September 1, 2021 11:03
@TizzySaurus TizzySaurus requested a review from MarkKoz September 1, 2021 11:03
Now uses `>=` instead of `>`, as is meant to happen.
@TizzySaurus TizzySaurus changed the title Suppress logs caused by infraction target being a Moderator+ Prevent role hierarchy issues with infractions Sep 1, 2021
@TizzySaurus
Copy link
Copy Markdown
Contributor Author

After many hours dabbling with the tests I figured out how to fix them. Hopefully this should all now be ready to merge, just needs the reviews 👍

@Xithrius Xithrius added the s: needs review Author is waiting for someone to review and approve label Sep 8, 2021
Copy link
Copy Markdown
Contributor

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

I like this. One minor nitpick. Feel free to ignore it completely.

Comment thread bot/exts/moderation/infraction/infractions.py Outdated
@jchristgit jchristgit enabled auto-merge October 5, 2021 21:29
@jchristgit jchristgit merged commit 8ec4593 into main Oct 5, 2021
@jchristgit jchristgit deleted the ignore-infra-mods-errors branch October 5, 2021 21:59
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: moderation Related to community moderation functionality: (moderation, defcon, verification) p: 3 - low Low Priority t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suppress exceptions for infraction attempts on mods+

6 participants