Skip to content

Fix antispam looking at wrong messages#1777

Merged
ChrisLovering merged 1 commit into
mainfrom
mbaruh-patch-1
Aug 24, 2021
Merged

Fix antispam looking at wrong messages#1777
ChrisLovering merged 1 commit into
mainfrom
mbaruh-patch-1

Conversation

@mbaruh
Copy link
Copy Markdown
Member

@mbaruh mbaruh commented Aug 24, 2021

No description provided.

@mbaruh mbaruh requested a review from jb3 as a code owner August 24, 2021 20:47
@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Aug 24, 2021

This seems rushed. Can you explain the differences between the sets of messages and the reason for this change?

@MarkKoz MarkKoz added a: filters Related to message filters: (antimalware, antispam, filtering, token_remover) t: bug Something isn't working labels Aug 24, 2021
@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Aug 24, 2021

@MarkKoz I erroneously used the wrong set of messages in the cross-channel antispam PR. The one I used is all messages that the predicate looks at to see if there was a violation. The one that needs to be used is the one I use here which is the actual messages that are part of the offense.

@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Aug 24, 2021

To put it simply, currently if there's a violation it deletes unrelated messages.

Copy link
Copy Markdown
Contributor

@janine9vn janine9vn left a comment

Choose a reason for hiding this comment

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

Didn't do a full test but after looking at this and discussing with you this should now have the intended behavior.

@ChrisLovering ChrisLovering merged commit 09b45e6 into main Aug 24, 2021
@ChrisLovering ChrisLovering deleted the mbaruh-patch-1 branch August 24, 2021 23:05
@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Aug 24, 2021

Okay this code is confusing because it reassigned relevant_messages to be a different set of messages... I was staring at it for 5 minute and this change seemed wrong until I caught that important detail.

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Aug 24, 2021

It doesn't help that this comment is inaccurate since it actually returns 3 items, not 2

If it doesn't, it returns a tuple in the form (str, Iterable[discord.Member])

@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Aug 24, 2021

Yeah it took me a bit to understand what's going on there. But the key point is that the messages that are logged were already relevant_messages (line 211), and those were the correct messages, which is probably why it wasn't caught in testing the original PR.

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) t: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants