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

feat(silence): allow to silence threads #3122

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Snipy7374
Copy link

@Snipy7374 Snipy7374 commented Jul 14, 2024

The code might not be that clean, also i think that the annotations were wrong before, now they are surely wrong, do you care about them?

Also, since this is my first contribution to this repository, let me know if i missed anything out.

  • I tested the code changes locally
  • I documented new functions

@Snipy7374 Snipy7374 requested a review from mbaruh as a code owner July 14, 2024 23:02
Copy link
Contributor

@n0Oo0Oo0b n0Oo0Oo0b left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I haven't tested the code yet, but here are some comments.

A few more things I'd like to point out:

  • I'm not sure what you mean by "the annotations were wrong before". Could you elaborate a bit on that?
  • Thread isn't a subclass of TextChannel so TextOrVoiceChannel should be updated accordingly. Renaming it to something like SilencableChannel also seems reasonable.
  • The existing method names sound pretty inconsistent. This isn't your fault, but maybe they could be updated while we're at it?

bot/exts/moderation/silence.py Outdated Show resolved Hide resolved
bot/exts/moderation/silence.py Outdated Show resolved Hide resolved
bot/exts/moderation/silence.py Outdated Show resolved Hide resolved
bot/exts/moderation/silence.py Outdated Show resolved Hide resolved
@Snipy7374
Copy link
Author

Sorry for the delay, i was busy doing the code jam. I tried to fix the typing issues but a lot of other errors persist from previous code, for example see this, a lot of things are unknown, also the Context annotation always doesn't have the bot type var. I'm using pyright in strict mode so maybe that's why but yeah, there are a lot of typing errors across all the codebase.

@Snipy7374 Snipy7374 requested a review from n0Oo0Oo0b July 28, 2024 22:02
Copy link
Contributor

@n0Oo0Oo0b n0Oo0Oo0b left a comment

Choose a reason for hiding this comment

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

Almost there! Just a few more nitpicks to go.

Also: discussion about naming in #dev-contrib

bot/exts/moderation/silence.py Show resolved Hide resolved
bot/exts/moderation/silence.py Outdated Show resolved Hide resolved
bot/exts/moderation/silence.py Outdated Show resolved Hide resolved
@Snipy7374 Snipy7374 requested a review from n0Oo0Oo0b July 29, 2024 09:05
Copy link
Contributor

@n0Oo0Oo0b n0Oo0Oo0b left a comment

Choose a reason for hiding this comment

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

I finally got around to testing this change and found an edge case: (un)silencing when the thread is already (un)locked.

@@ -242,6 +242,11 @@ async def _set_silence_overwrites(self, channel: TextOrVoiceChannel, *, kick: bo
send_messages_in_threads=overwrite.send_messages_in_threads
)

elif isinstance(channel, Thread):
log.info(f"Silencing thread #{channel.parent}/{channel} ({channel.parent.id}/{channel.id}).")
Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, logging this is handled in _silence() instead so this wasn't necessary.

Suggested change
log.info(f"Silencing thread #{channel.parent}/{channel} ({channel.parent.id}/{channel.id}).")

@@ -242,6 +242,11 @@ async def _set_silence_overwrites(self, channel: TextOrVoiceChannel, *, kick: bo
send_messages_in_threads=overwrite.send_messages_in_threads
)

elif isinstance(channel, Thread):
log.info(f"Silencing thread #{channel.parent}/{channel} ({channel.parent.id}/{channel.id}).")
await channel.edit(locked=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the thread is already locked, this will just schedule an unlock in the set amount of time. Failing is probably better to keep consistent with how it behaves with text channels.

@@ -331,6 +340,12 @@ async def _unsilence(self, channel: TextOrVoiceChannel) -> bool:
role = self._everyone_role
overwrite = channel.overwrites_for(role)
permissions = "`Send Messages` and `Add Reactions`"
elif isinstance(channel, Thread):
await channel.edit(locked=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: if the thread was manually unlocked before the scheduled time, there won't be any indication of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow silencing of threads
2 participants