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

Reconsider FakeContext usage in infractions code #2571

Open
wookie184 opened this issue Apr 29, 2023 · 2 comments
Open

Reconsider FakeContext usage in infractions code #2571

wookie184 opened this issue Apr 29, 2023 · 2 comments
Labels
a: filters Related to message filters: (antimalware, antispam, filtering, token_remover) a: moderation Related to community moderation functionality: (moderation, defcon, verification) s: planning Discussing details t: enhancement Changes or improvements to existing features

Comments

@wookie184
Copy link
Contributor

wookie184 commented Apr 29, 2023

I think we should consider (probably not in this PR) changing the type hints for any functions that can take a FakeContext, since just Context is misleading and makes it easy to introduce bugs like this.

I think the issue now is that we have 3 context classes: Context, FakeContext and FilterContext that are being used for conversion, and post_infraction takes either Context or FakeContext.

In this particular case, I think it's an instance of FilterContext that's being converted to FakeContext at the level of the InfractionAndNotification.action method .

But I do agree that it might need to be cleaned up.

I'll try to tackle this next.

Originally posted by @shtlrs in #2556 (comment)

@wookie184
Copy link
Contributor Author

Places like this function https://github.com/shtlrs/bot/blob/6025015b471675de9f7eab9197899dd3eafcb4b0/bot/exts/moderation/infraction/_utils.py#L79 are currently type hinted as getting a Context, but they may instead get a FakeContext, which does not implement the full functionality of Context.

We should either update the type hints, or try and neaten stuff up another way that will help avoid bugs like #2555.

@mbaruh
Copy link
Member

mbaruh commented Feb 10, 2024

I'd like to note that for the linked issue, the bug would happen even if we could use Context. The bug happened because we tried to issue an infraction from a type of channel that was unaccounted for.

I'm ok with trying to clean this up. This was a hack I wrote because I didn't always have a Context to work with, such as in the case of on_voice_state_update. If you have a better idea I'm happy to discuss it, but the current solution is the least hacky thing I could come up with.

@wookie184 wookie184 added s: planning Discussing details a: moderation Related to community moderation functionality: (moderation, defcon, verification) a: filters Related to message filters: (antimalware, antispam, filtering, token_remover) t: enhancement Changes or improvements to existing features labels May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: filters Related to message filters: (antimalware, antispam, filtering, token_remover) a: moderation Related to community moderation functionality: (moderation, defcon, verification) s: planning Discussing details t: enhancement Changes or improvements to existing features
Projects
None yet
Development

No branches or pull requests

2 participants