Implemented optional duration parameter in slowmode command #3331
Conversation
| @mock.patch("bot.exts.moderation.slowmode.datetime", wraps=datetime.datetime) | ||
| async def test_reschedules_slowmodes(self, mock_datetime) -> None: | ||
| """Slowmodes are loaded from cache at cog reload and scheduled to be reverted.""" | ||
| mock_datetime.now.return_value = datetime.datetime(2025, 6, 2, 12, 0, 0, tzinfo=datetime.UTC) | ||
| mock_now = datetime.datetime(2025, 6, 2, 12, 0, 0, tzinfo=datetime.UTC) | ||
| self.cog._reschedule = mock.AsyncMock(wraps=self.cog._reschedule) | ||
|
|
||
| channels = [] | ||
| slowmodes = ( | ||
| (123, (mock_now - datetime.timedelta(10)).timestamp(), 2), # expiration in the past | ||
| (456, (mock_now + datetime.timedelta(10)).timestamp(), 4), # expiration in the future | ||
| ) | ||
|
|
||
| for channel_id, expiration_datetime, delay in slowmodes: | ||
| channel = MockTextChannel(slowmode_delay=delay, id=channel_id) | ||
| channels.append(channel) | ||
|
|
||
| await self.cog.slowmode_expiration_cache.set(channel_id, expiration_datetime) | ||
| await self.cog.original_slowmode_cache.set(channel_id, delay) | ||
|
|
||
| await self.cog.cog_unload() | ||
| await self.cog.cog_load() | ||
|
|
||
| # check that _reschedule function was called upon cog reload. | ||
| self.cog._reschedule.assert_called() | ||
|
|
||
| # check that a task was created for every cached slowmode. | ||
| for channel in channels: | ||
| self.assertIn(channel.id, self.cog.scheduler) | ||
|
|
||
| # check that one channel with slowmode expiration in the past was edited immediately. | ||
| channels[0].edit.assert_awaited_once_with(slowmode_delay=channels[0].slowmode_delay) |
There was a problem hiding this comment.
This test checks if temporary slowmodes in cache are rescheduled after the cog is reloaded but I'm having trouble with getting the assertion on line 203 to pass. I mock two channels, one where the slowmode is already expired by the time the cog is loaded, and one where it is not expired. The task for the channel where it is expired should immediately callback the func that removes the slowmode. On line 200 I check that two tasks are created, and they are. On line 203 I check if the slowmode was changed and the assertion fails, indicating that the channel was not edited.
I'm not sure if it's something small that I'm missing, or if it's the way that i've mocked everything, or if it's just a consequence of me not really understanding how these events and tasks are handled. I'd appreciate if someone could have a look 🥺
There was a problem hiding this comment.
You are creating MockTextChannels, but _revert_slowmode will create it's own mocks from calling self.bot.get_channel(id). Those are the mocks that will have edit called on them, not your ones.
Not sure what the easiest fix would be.
| # Stores the expiration timestamp in POSIX format for active slowmodes, keyed by channel ID. | ||
| slowmode_expiration_cache = RedisCache() | ||
|
|
||
| # Stores the original slowmode interval by channel ID, allowing its restoration after temporary slowmode expires. | ||
| original_slowmode_cache = RedisCache() |
There was a problem hiding this comment.
Given these are both structured and known values, I would maybe consider serializing them together into one store?
This is a nit though and others may disagree, it just appears that we use them more or less in the same place so whilst if we were just working in dict-land this would be preferred when working in a place with an underlying store I prefer it being the same.
| if await self.slowmode_expiration_cache.contains(channel.id): | ||
| expiration_time = await self.slowmode_expiration_cache.get(channel.id) |
There was a problem hiding this comment.
| if await self.slowmode_expiration_cache.contains(channel.id): | |
| expiration_time = await self.slowmode_expiration_cache.get(channel.id) | |
| expiration_time = await self.slowmode_expiration_cache.get(channel.id, None) | |
| if expiration_time is not None: |
| expiration_time = await self.slowmode_expiration_cache.get(channel.id) | ||
| expiration_timestamp = discord_timestamp(expiration_time, TimestampFormats.RELATIVE) | ||
| await ctx.send( | ||
| f"The slowmode delay for {channel.mention} is {humanized_delay} and expires in {expiration_timestamp}." |
There was a problem hiding this comment.
| f"The slowmode delay for {channel.mention} is {humanized_delay} and expires in {expiration_timestamp}." | |
| f"The slowmode delay for {channel.mention} is {humanized_delay} and expires {expiration_timestamp}." |
The "in" should be already included in a relative timestamp if I'm not mistaken
Also, should this specify what happens when the slowmode expires? e.g "and will revert to {humanized_original_delay} {expiration_timestamp}"
| if channel.id in COMMONLY_SLOWMODED_CHANNELS: | ||
| log.info(f"Recording slowmode change in stats for {channel.name}.") | ||
| self.bot.stats.gauge(f"slowmode.{COMMONLY_SLOWMODED_CHANNELS[channel.id]}", slowmode_delay) | ||
| if not slowmode_delay <= SLOWMODE_MAX_DELAY: |
There was a problem hiding this comment.
| if not slowmode_delay <= SLOWMODE_MAX_DELAY: | |
| if slowmode_delay > SLOWMODE_MAX_DELAY: |
|
|
||
| @slowmode_group.command(name="set", aliases=["s"]) | ||
| async def set_slowmode( | ||
| self, | ||
| ctx: Context, | ||
| channel: MessageHolder, | ||
| delay: DurationDelta | Literal["0s", "0seconds"], | ||
| duration: DurationDelta | None = None |
There was a problem hiding this comment.
Use Duration instead of DurationDelta, which handles the conversion to datetime for you (and change the variable name to something like expiry to make it less confusing with delay).
| slowmode_duration = time.relativedelta_to_timedelta(duration).total_seconds() | ||
| humanized_duration = time.humanize_delta(duration) | ||
|
|
||
| expiration_time = datetime.now(tz=UTC) + timedelta(seconds=slowmode_duration) |
There was a problem hiding this comment.
This section is then simplified with the usage of Duration.
| f"{ctx.author} tried to set the slowmode delay of #{channel} to {humanized_delay}, " | ||
| "which is not between 0 and 6 hours." | ||
| f"{ctx.author} set the slowmode delay for #{channel} to" | ||
| f"{humanized_delay} which expires in {humanized_duration}." |
There was a problem hiding this comment.
| f"{humanized_delay} which expires in {humanized_duration}." | |
| f"{humanized_delay} which expires {humanized_duration}." |
Same here, can specify what it's going to revert to.
| await channel.edit(slowmode_delay=slowmode_delay) | ||
| await ctx.send( | ||
| f"{Emojis.check_mark} The slowmode delay for {channel.mention}" | ||
| f" is now {humanized_delay} and expires in {expiration_timestamp}." |
| await channel.send( | ||
| f"{Emojis.check_mark} A previously applied slowmode has expired and has been reverted to {slowmode_delay}." | ||
| ) |
There was a problem hiding this comment.
Slowmode changes are usually silent. Unless the mods agreed that this is what they want I would send this message to #mods and/or #mod-log
| async def _revert_slowmode(self, channel_id: int) -> None: | ||
| original_slowmode = await self.original_slowmode_cache.get(channel_id) | ||
| slowmode_delay = time.humanize_delta(seconds=original_slowmode) | ||
| channel = self.bot.get_channel(channel_id) |
There was a problem hiding this comment.
I would use get_or_fetch_channel here
| if channel is None: | ||
| channel = ctx.channel | ||
| if await self.slowmode_expiration_cache.contains(channel.id): | ||
| await self.slowmode_expiration_cache.delete(channel.id) | ||
| await self.original_slowmode_cache.delete(channel.id) | ||
| self.scheduler.cancel(channel.id) |
There was a problem hiding this comment.
Shouldn't it all be already handled in set_slowmode?
Closes #3327.
Implemented an optional
durationargument for the slowmode command. If the duration argument is present, the intended expiration datetime and the original slowmode interval are saved in redis cache. After the duration is elapsed, the original slowmode interval is grabbed from redis and restored to the channel.The expiration datetime is cached so that it can persist bot restart.