Give ContentAction classes an action attribute#24785
Merged
diox merged 3 commits intomozilla:masterfrom Apr 21, 2026
Merged
Conversation
diox
force-pushed
the
abuse-actions-class-action-attribute
branch
from
07ee7c0 to
754dd51
Compare
diox
commented
Apr 20, 2026
| def test_notify_reporters_reporters_provided(self): | ||
| action = self.ActionClass(self.decision) | ||
| action.notify_reporters(reporter_abuse_reports=[self.abuse_report_no_auth]) | ||
| action_helper = self.ActionClass(self.decision) |
Member
Author
There was a problem hiding this comment.
I tried to make the tests in this file more consistent by:
- Naming the content action class variable
ActionClasseverywhere - Naming the variable with the instance of the action class
action_helper - Naming the variable with the actual action
action
That's why I ended up with +261-252 in this test file.
eviljeff
reviewed
Apr 20, 2026
Member
eviljeff
left a comment
eviljeff
left a comment
There was a problem hiding this comment.
The .action change is sensible; the .target_versions change seems to be introducing footguns with different values for target_versions on self and self.decision :/
This will allow us to have some logic inside the content action classes that depend on the action being executed by that class, e.g. we could have ContentActionDisableAddon look at past AMO_DISABLE_ADDON actions when processing and deciding not to do it again. Similarly having an actual target_versions attribute rather than a property depending on the decision will allow us to have logic that depends on the target versions inside the action class but still allow the target versions considered for that logic to be set by the caller before a decision has been made.
diox
force-pushed
the
abuse-actions-class-action-attribute
branch
from
754dd51 to
19d7979
Compare
diox
force-pushed
the
abuse-actions-class-action-attribute
branch
from
19d7979 to
3a5e6fe
Compare
diox
changed the title
eviljeff
reviewed
Apr 21, 2026
| ) | ||
|
|
||
| # There should be at least as many DECISION_ACTIONS as there are classes, | ||
| # except for AMO_ESCALATE_ADDON (which is obsolete) and AMO_REQUEUE (which |
Member
There was a problem hiding this comment.
just thinking out loud at this point, but we should probably create an empty class for AMO_REQUEUE (that possibly raises in it's process_action), and delete AMO_ESCALATE_ADDON.
Member
Author
There was a problem hiding this comment.
I almost did but decided it was not worth spending the time on that right now...
eviljeff
approved these changes
Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This will allow us to have some logic inside the content action classes that depend on the action being executed by that class, e.g. we could have
ContentActionDisableAddonlook at pastAMO_DISABLE_ADDONactions when processing and deciding not to do it again.Pre-requisite for mozilla/addons#16123