Skip to content

Improve Filter Rerun Logic#1937

Merged
HassanAbouelela merged 4 commits into
mainfrom
filter-improvement
Nov 6, 2021
Merged

Improve Filter Rerun Logic#1937
HassanAbouelela merged 4 commits into
mainfrom
filter-improvement

Conversation

@TizzySaurus
Copy link
Copy Markdown
Contributor

@TizzySaurus TizzySaurus commented Nov 5, 2021

Closes #1934.

Will now only rerun filters in on_message_edit if the content or attachments of a message changed. This stops reruns for removal of embeds, change in pin status, etc.

@TizzySaurus TizzySaurus added t: bug Something isn't working a: filters Related to message filters: (antimalware, antispam, filtering, token_remover) p: 2 - normal Normal Priority s: needs review Author is waiting for someone to review and approve labels Nov 5, 2021
@TizzySaurus TizzySaurus requested a review from mbaruh November 5, 2021 11:38
@TizzySaurus TizzySaurus requested a review from jb3 as a code owner November 5, 2021 11:38
@HassanAbouelela HassanAbouelela enabled auto-merge (squash) November 5, 2021 12:47
@Qwerty-133
Copy link
Copy Markdown
Contributor

Qwerty-133 commented Nov 5, 2021

This would not alert us when an embed has been added to a message, which is why we should only really be skipping the filters if the message was pinned or unpinned.

@TizzySaurus
Copy link
Copy Markdown
Contributor Author

TizzySaurus commented Nov 5, 2021 •
edited
Loading

This would not alert us when an embed has been added to a message, which is why we should only really be skipping the filters if the message was pinned or unpinned.

As the comment says, we don't want to be alerted of that. If we were, then we'd get double-pinged when a filter has a link in it (as is the case for the most recent mod-alert)

@Qwerty-133
Copy link
Copy Markdown
Contributor

Qwerty-133 commented Nov 5, 2021

We might miss self-bots though.

@HassanAbouelela
Copy link
Copy Markdown
Contributor

HassanAbouelela commented Nov 5, 2021

This is only for message edits, can you actually edit an embed in?

@Qwerty-133
Copy link
Copy Markdown
Contributor

Qwerty-133 commented Nov 5, 2021

Yep, you can

@TizzySaurus
Copy link
Copy Markdown
Contributor Author

TizzySaurus commented Nov 5, 2021

We might miss self-bots though.

I believe we have other filters for that

@Qwerty-133
Copy link
Copy Markdown
Contributor

Qwerty-133 commented Nov 5, 2021 •
edited
Loading

The reason we got two pings for that mod-alert is because of an oversight in the embed filter. So we should be running our filters on embed additions. I can PR a fix for the embed filter.

#1938 relevant issue.

@TizzySaurus
Copy link
Copy Markdown
Contributor Author

TizzySaurus commented Nov 6, 2021 •
edited
Loading

We should be running our filters on embed additions.

@Qwerty-133 This has been addressed 👍

Qwerty-133
Qwerty-133 approved these changes Nov 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Qwerty-133 Qwerty-133 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, thanks!

@HassanAbouelela HassanAbouelela merged commit 1302632 into main Nov 6, 2021
@HassanAbouelela HassanAbouelela deleted the filter-improvement branch November 6, 2021 10:56
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label Nov 10, 2021
@onerandomusername onerandomusername mentioned this pull request Dec 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbaruh mbaruh Awaiting requested review from mbaruh

@jb3 jb3 Awaiting requested review from jb3

2 more reviewers

@HassanAbouelela HassanAbouelela HassanAbouelela approved these changes

@Qwerty-133 Qwerty-133 Qwerty-133 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: filters Related to message filters: (antimalware, antispam, filtering, token_remover) p: 2 - normal Normal Priority t: bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Don't trigger filters on pin

4 participants

@TizzySaurus @Qwerty-133 @HassanAbouelela @Xithrius