Move mutes to new cog, add role-based and temporary mutes

[TrustyJAID]

Type

• Bugfix
• Enhancement
• New feature

Description of the changes

This moves mutes outside of the mod cog into its own separate cog.

• Allow mutes to be role based
• [p]mute defaults to serverwide muting and [p]mutechannel mutes in the current channel
• Mutes all have an optional time in the reason field to automatically remove the mute
• Mutes allow for more than 1 user to be muted at a time utilizing commands.Greedy

[kennnyshiwa]

Couple small things i found when testing

1. Muting someone when you don't have a mute role defined and are just using channel overrides works, however it doesn't actually send the mod-log entry and instead appears to error (side note on this: found that without the modlog enabled it won't say someone has been muted and instead just says they can't talk in any channels which looking back it, does imply it worked, but might be nice to say user has been muted, as it says it when the modlog is enabled, but not when its not)

https://media.kstj.us/DarkorangeImaginativeDunlin.png

though i can confirm it adds the person to each channel and revokes perms properly,

2. After muting them and getting that error going to unmute them results in this

Exception in command 'unmute'
Traceback (most recent call last):
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 83, in wrapped
    ret = await coro(*args, **kwargs)
  File "c:\users\kenneth\redenv\lib\site-packages\redbot\cogs\mutes\mutes.py", line 506, in unmute
    await self.config.guild(ctx.guild).muted_users.set(self._server_mutes[ctx.guild.id])
KeyError: 706554165194653718

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\bot.py", line 892, in invoke
    await ctx.command.invoke(ctx)
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 797, in invoke
    await injected(*ctx.args, **ctx.kwargs)
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 92, in wrapped
    raise CommandInvokeError(exc) from exc
discord.ext.commands.errors.CommandInvokeError: Command raised an exception: KeyError: 706554165194653718

though it does remove the overrides properly it just throws an ugly error, i also get a similar traceback when trying it with role mutes too, though the key error changes their ID versus the guild id as it was before

3. Seems that tempmutes with channel overrides doesn't actually ever unmuted, tested with [p]mute @user 5m and [p]mute @user 1 minutes and both mute but never seem to unmute, no apparent errors in console either just no unmute

4. [p]channelunmute has the same issue as unmute does

Exception` in command 'channelunmute'
Traceback (most recent call last):
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 83, in wrapped
    ret = await coro(*args, **kwargs)
  File "c:\users\kenneth\redenv\lib\site-packages\redbot\cogs\mutes\mutes.py", line 555, in unmute_channel
    await self.config.channel(channel).muted_users.set(self._channel_mutes[channel.id])
KeyError: 706554165194653721

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\bot.py", line 892, in invoke
    await ctx.command.invoke(ctx)
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 797, in invoke
    await injected(*ctx.args, **ctx.kwargs)
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 92, in wrapped
    raise CommandInvokeError(exc) from exc
discord.ext.commands.errors.CommandInvokeError: Command raised an exception: KeyError: 706554165194653721

traceback just for consistency

5. sense we have mutechannel, i feel unmutechannel works better than channelunmute, but thats just my preference

6. voicemute and voiceunmute throw the following traceback when trying to mute or unmute someone in voice

Exception in command 'voicemute'
Traceback (most recent call last):
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 83, in wrapped
    ret = await coro(*args, **kwargs)
  File "c:\users\kenneth\redenv\lib\site-packages\redbot\cogs\mutes\voicemutes.py", line 167, in voice_mute
    success, issue = await self.mute_user(guild, channel, author, user, audit_reason)
TypeError: mute_user() takes 5 positional arguments but 6 were given

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\bot.py", line 892, in invoke
    await ctx.command.invoke(ctx)
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 797, in invoke
    await injected(*ctx.args, **ctx.kwargs)
  File "c:\users\kenneth\redenv\lib\site-packages\discord\ext\commands\core.py", line 92, in wrapped
    raise CommandInvokeError(exc) from exc
discord.ext.commands.errors.CommandInvokeError: Command raised an exception: TypeError: mute_user() takes 5 positional arguments but 6 were given

If any of this doesn't make sense let me know and i can explain further

[mikeshardmind]

I'm not sure what the reasoning behind abandoning the work done on V3/feature/mutes, but there are issues with this code here which were actually considered in that branch even in an unfinished state

[mikeshardmind]

This isn't a safe way to determine if someone was muted, data about mutes should be stored upon muting, it also only works if they were role muted instead of overwrites being applied.

[TrustyJAID]

This was intentional when I started on this as I did not want to support permission based muting be applied when a user left and rejoined encouraging people use the role based model instead if they wanted to prevent people leaving and rejoining to avoid a mute.

[mikeshardmind]

This also prevents re-applying channel permission based mutes.

[mikeshardmind]

Storing the old overwrites is an error that could change other permissions upon unmute, this should only store the difference in permissions caused by the mute.

[TrustyJAID]

This only stores the old overwrites for specific permissions on that user. overwrites above is the channel perms for that user only and the state they were in before they were muted. This was straight from the old mutes in mod.

[DevilXD]

It would be great if this would expose some kind of an API that'd allow 3rd party cogs to apply timed / permanent mutes as well. I have a cog that currently has to do this manually, via creating it's own Muted role and tracking the unmute time on each muted member. It'd be much easier if I could just hook-up to this via an API.

[TrustyJAID]

@DevilXD that was outside the scope of this PR. I wanted to get the new features in people's hands and get them stable before things are re-arranged as an API which also moves a lot of this outside of a cog and into something core always has available.

[Dav-Git]

This is just a code review, I still have to test any of this.

I marked any instances of datetime usage (I'm sure I missed some) so that someone who better understood #4017 can more easily take a look at them.

Most part of this looks fine to me

[TrustyJAID]

MuteTime converter is quite lax and detects time in the middle of text as well, is that intentional?

I can't imagine a lot of situations where a time delta would be used inside a reason field that isn't meant to be the time itself. It's more lax this way so that users can start with the time or end with it and not have to really think "which goes first again?" when they're trying to just get the mute done. If it's a problem you can specify time as well with t=2 hours but the idea was to be more user friendly.

[Jackenmen]

• It seems that you only did the reason grouping for the notification channel, but not for any of the mute commands which is what my previous review comment was presenting.
• Would it possible to check these before trying the channel mutes so that it only shows once for the whole user?
  
• I consider this a nit, but now that we send information when the user is already muted in the channel, it looks a bit weird for the server unmute (and mute too I guess):
  

[Jackenmen]

It's more lax this way so that users can start with the time or end with it and not have to really think "which goes first again?" when they're trying to just get the mute done. If it's a problem you can specify time as well with t=2 hours but the idea was to be more user friendly.

And I really like the idea of being more user friendly here, and I think it's really cool that you can start or end with the time. I was just wondering, if allowing to put it in the middle was also intended here or if it's just a bit unfortunate side effect.

[Jackenmen]

Once all changes that were requested so far (including the review comments in this review) are done, this should be in a mergeable state. I know it's easier said than done though, but I just wanted to let you know that I won't add anything else to my reviews than I already have (unless some new issues get introduced of course). And if you will need any help, feel free to ask :)

Now onto this review, aside from code comments, there's one thing that hasn't been included in them as it wasn't referring to one specific code part:

• Channel overrides not being reapplied on guild join should be documented in some help docstrings and probably also sent instructions.

[Jackenmen]

Aside from the code comments, it looks like there's still one thing that wasn't addressed and/or answered to (quote rephrased to better explain what I meant):

• "Multi" channel unmutes in modlog don't mention the channel(s) they refer to. Maybe it should be at least put somewhere in the reason since we can't set multiple channels on a modlog entry?

---

This might be the last review before approval - I still need to do some testing with the newly introduced changes so there is a chance I'll find some bug, but if I don't, it will be time for approval.

[Jackenmen]

• Channel mutes are only saved to cache, they don't persist on bot restart (applies to both overrides-based [p]mute and to [p]channelmute and [p]voicemute commands)
• Case for [p]vocemute doesn't contain the duration/until fields
• It seems that Administrator check is done before channel mutes and is shown nicely, but hierarchy check is done during channel mutes and lists all channels it failed for:
  
  What can also be seen on the screenshot below is that there's a nice empty line between AlmostSlime and Jackenmen that improves readability, but such an empty line doesn't exist between Jackenmen and KslibLivxud.
  Besides what I mentioned, I think we could do one more thing improving readability - make the different reasons form a list by preceding each reason with -:
  
• not very critical, but the check for manage roles permission could additionally be checked before proceeding to loop through users so that this doesn't happen:
  
• the error for missing Manage Permissions in the channel is inaccurate (it's talking about role hierarchy that doesn't apply to channel permissions at all + the bot doesn't need the Manage Roles permission but rather the Manage Permissions permission in that specific channel):
  

[Jackenmen]

It's that time! LGTM, let's ship it! 😏
Thanks for all the work on this @TrustyJAID and thanks for all the help with testing to our contributors ❤️