fix: check for the modteam role before reassigning the user's moderation role#2008
fix: check for the modteam role before reassigning the user's moderation role#2008onerandomusername wants to merge 8 commits into
Conversation
onerandomusername
requested review from
Den4200,
jb3,
ks129 and
mbaruh
as code owners
Head branch was pushed to by a user without write access
Xithrius
added
a: backend
TizzySaurus
left a comment
TizzySaurus
left a comment
There was a problem hiding this comment.
A few nitpicks. Main one I'd like to see implemented is the log.warning
TizzySaurus
left a comment
TizzySaurus
left a comment
There was a problem hiding this comment.
Haven't tested anything, but aside from the backquote comment the code looks good 👍
ChrisLovering
added
s: waiting for author
onerandomusername
force-pushed
the
fix-issue-2007
branch
from
0f585f8 to
157a170
Compare
wookie184
added
s: needs review
| self.bot = bot | ||
| self._role_scheduler = Scheduler("ModPingsOnOff") | ||
| self._modpings_scheduler = Scheduler("ModPingsSchedule") | ||
| self.fetch_guild_task = self.bot.loop.create_task(self.fetch_guild()) |
There was a problem hiding this comment.
This pattern of making a task and awaiting it in functions isn't needed anymore, since d.py added async cog_load loading. As it guarantees that the cog_load function is finished before the cog accepts any command/events.
So this task, an all usages of it can be safely deleted.
| await self.fetch_guild_task | ||
| if self.guild.get_role(Roles.mod_team) not in mod.roles: |
There was a problem hiding this comment.
Why don't we just do guild = self.bot.get_guild(Guild.id) here and save all the task code?
| # or it doesn't matter since this was called in a task. | ||
| # The logging statement above is enough to ensure that this was caught | ||
| # as with a logging level of WARNING, it will create a sentry issue. | ||
| return |
There was a problem hiding this comment.
Should we delete them from pings_off_mods before returning? Does it matter?
wookie184
added
s: waiting for author
|
Is this still necessary? I think the issue was fixed in 2b2dc95 |
Without actually testing anything, it does indeed look like this issue has been fixed (which would mean this can be closed). |
|
Since it's a bit more robust to have the check in the same place that the role is applied I think we could do this anyway. I'll take this over and finish it off. |
|
Changed my mind again, I think this can just be closed :) |
7 participants
closes #2007