Enable policy->action selection in reviewer tools, for negative policies

src/olympia/abuse/models.py

@@ -47,6 +47,7 @@
     CinderUnauthenticatedReporter,
     CinderUser,
 )
+from .utils import split_enforcement_actions
 
 
 log = olympia.core.logger.getLogger('z.abuse')
@@ -961,28 +962,17 @@ def full_text(self, *, values=None):
         )
         return f'{self.full_name()}: {text}'
 
+    @cached_property
+    def split_enforcement_actions(self):
+        """Returns enums for this policy's enforcement action slugs, split into primary
+        and follow-up actions. Cached."""
+        return split_enforcement_actions(self.enforcement_actions or [])
+
     @classmethod
-    def get_full_texts(self, policies, *, values=None):
+    def get_full_texts(cls, policies, *, values=None):
         values = values or {}
         return [policy.full_text(values=values.get(policy.uuid)) for policy in policies]
 
-    @classmethod
-    def get_decision_actions_from_policies(cls, policies, *, for_entity=None):
-        actions = {
-            action
-            for policy in policies
-            for api_value in policy.enforcement_actions
-            if policy.enforcement_actions
-            and api_value in DECISION_ACTIONS.api_values
-            and (action := DECISION_ACTIONS.from_api_value(api_value))
-            and (
-                not for_entity
-                or for_entity
-                in CONTENT_ACTION_FROM_DECISION_ACTION[action.value].valid_targets
-            )
-        }
-        return list(actions)
-
     def get_text_formatter_pairs(self):
         return [(text, key) for text, key, _, _ in Formatter().parse(self.text)]
 

src/olympia/abuse/tests/test_models.py

@@ -39,7 +39,6 @@
 from olympia.files.models import File
 from olympia.ratings.models import Rating
 from olympia.reviewers.models import NeedsHumanReview
-from olympia.users.models import UserProfile
 from olympia.versions.models import Version, VersionReviewerFlags
 
 from ..actions import (
@@ -2315,46 +2314,6 @@ def test_without_parents_if_their_children_are_present(self):
             lone_policy,
         }
 
-    def test_get_decision_actions_from_policies(self):
-        policies = (
-            # no actions, ignored
-            CinderPolicy.objects.create(uuid='1', enforcement_actions=[]),
-            # multiple actions
-            CinderPolicy.objects.create(
-                uuid='2',
-                enforcement_actions=[
-                    'amo-disable-addon',
-                    'amo-approve',
-                    'amo-ban-user',
-                ],
-            ),
-            # some duplicates, and unsupported actions
-            CinderPolicy.objects.create(
-                uuid='3', enforcement_actions=['amo-disable-addon', 'not-amo-action']
-            ),
-        )
-        assert sorted(CinderPolicy.get_decision_actions_from_policies(policies)) == [
-            DECISION_ACTIONS.AMO_BAN_USER,
-            DECISION_ACTIONS.AMO_DISABLE_ADDON,
-            DECISION_ACTIONS.AMO_APPROVE,
-        ]
-
-        assert sorted(
-            CinderPolicy.get_decision_actions_from_policies(policies, for_entity=Addon)
-        ) == [
-            DECISION_ACTIONS.AMO_DISABLE_ADDON,
-            DECISION_ACTIONS.AMO_APPROVE,
-        ]
-
-        assert sorted(
-            CinderPolicy.get_decision_actions_from_policies(
-                policies, for_entity=UserProfile
-            )
-        ) == [
-            DECISION_ACTIONS.AMO_BAN_USER,
-            DECISION_ACTIONS.AMO_APPROVE,
-        ]
-
     def test_get_text_formatter_pairs(self):
         policy = CinderPolicy.objects.create(
             name='Child Policy',

src/olympia/abuse/tests/test_utils.py

@@ -10,21 +10,26 @@
 
 from olympia import amo
 from olympia.activity.models import ActivityLog
+from olympia.addons.models import Addon
 from olympia.amo.tests import TestCase, addon_factory, user_factory, version_factory
 from olympia.bandwagon.models import Collection
 from olympia.blocklist.models import Block, BlockType, BlockVersion
 from olympia.constants.abuse import DECISION_ACTIONS
 from olympia.constants.promoted import PROMOTED_GROUP_CHOICES
 from olympia.promoted.models import PromotedGroup
 from olympia.ratings.models import Rating
+from olympia.users.models import UserProfile
 
 from ..actions import ContentActionBlockAddon
 from ..models import CinderJob, CinderPolicy, ContentDecision
 from ..utils import (
+    SplitEnforcementActions,
+    filter_enforcement_actions,
     find_automated_enforcement_actions_from_policies,
     get_instance_from_entity,
     is_same_time,
     reject_and_block_addons,
+    split_enforcement_actions,
 )
 
 
@@ -161,8 +166,8 @@ def test_find_nothing(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == []
-        assert followup_actions == []
+        assert actions == ()
+        assert followup_actions == ()
 
     def test_basic(self):
         bad_things_policy = CinderPolicy.objects.create(
@@ -180,8 +185,8 @@ def test_basic(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == [DECISION_ACTIONS.AMO_DISABLE_ADDON]
-        assert followup_actions == []
+        assert actions == (DECISION_ACTIONS.AMO_DISABLE_ADDON,)
+        assert followup_actions == ()
 
     def test_block_wins(self):
         bad_things_policy = CinderPolicy.objects.create(
@@ -199,8 +204,8 @@ def test_block_wins(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == [DECISION_ACTIONS.AMO_BLOCK_ADDON]
-        assert followup_actions == []
+        assert actions == (DECISION_ACTIONS.AMO_BLOCK_ADDON,)
+        assert followup_actions == ()
 
     def test_ordering_with_follow_up_action_against_not(self):
         bad_things_policy = CinderPolicy.objects.create(
@@ -224,10 +229,10 @@ def test_ordering_with_follow_up_action_against_not(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == [DECISION_ACTIONS.AMO_DISABLE_ADDON]
-        assert followup_actions == [
-            DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON
-        ]
+        assert actions == (DECISION_ACTIONS.AMO_DISABLE_ADDON,)
+        assert followup_actions == (
+            DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON,
+        )
 
     def test_ordering_with_two_follow_up_actions_compared(self):
         bad_things_policy = CinderPolicy.objects.create(
@@ -254,10 +259,10 @@ def test_ordering_with_two_follow_up_actions_compared(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == [DECISION_ACTIONS.AMO_DISABLE_ADDON]
-        assert followup_actions == [
-            DECISION_ACTIONS.AMO_FU_DELAY_SHORT_HARD_BLOCK_ADDON
-        ]
+        assert actions == (DECISION_ACTIONS.AMO_DISABLE_ADDON,)
+        assert followup_actions == (
+            DECISION_ACTIONS.AMO_FU_DELAY_SHORT_HARD_BLOCK_ADDON,
+        )
 
     def test_ordering_with_two_follow_up_actions_compared_different_delay(self):
         bad_things_policy = CinderPolicy.objects.create(
@@ -281,10 +286,10 @@ def test_ordering_with_two_follow_up_actions_compared_different_delay(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == [DECISION_ACTIONS.AMO_DISABLE_ADDON]
-        assert followup_actions == [
-            DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON
-        ]
+        assert actions == (DECISION_ACTIONS.AMO_DISABLE_ADDON,)
+        assert followup_actions == (
+            DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON,
+        )
 
     def test_ordering_with_multiple_followup_actions(self):
         bad_things_policy = CinderPolicy.objects.create(
@@ -309,11 +314,11 @@ def test_ordering_with_multiple_followup_actions(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == [DECISION_ACTIONS.AMO_DISABLE_ADDON]
-        assert followup_actions == [
+        assert actions == (DECISION_ACTIONS.AMO_DISABLE_ADDON,)
+        assert followup_actions == (
             DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON,
             DECISION_ACTIONS.AMO_FU_DELAY_MID_HARD_BLOCK_ADDON,
-        ]
+        )
 
     @mock.patch.object(
         ContentActionBlockAddon,
@@ -342,8 +347,8 @@ def test_skip_action_that_should_be_skipped_by_automation(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == [DECISION_ACTIONS.AMO_DISABLE_ADDON]
-        assert followup_actions == []
+        assert actions == (DECISION_ACTIONS.AMO_DISABLE_ADDON,)
+        assert followup_actions == ()
 
     def test_skip_policy_with_multiple_primary_actions(self):
         broken_policy = CinderPolicy.objects.create(
@@ -359,8 +364,8 @@ def test_skip_policy_with_multiple_primary_actions(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == []
-        assert followup_actions == []
+        assert actions == ()
+        assert followup_actions == ()
 
     def test_skip_policy_if_successful_appeal_for_it_in_the_past(self):
         appealed_policy = CinderPolicy.objects.create(
@@ -386,5 +391,69 @@ def test_skip_policy_if_successful_appeal_for_it_in_the_past(self):
             addon=self.addon,
             version=self.version,
         )
-        assert actions == []
-        assert followup_actions == []
+        assert actions == ()
+        assert followup_actions == ()
+
+
+def test_split_enforcement_actions():
+    slugs = [
+        'amo-disable-addon',
+        'amo-ban-user',
+        'amo-approve',
+        'not-amo-action',  # not a valid action at all
+        # valid, but not a primary action
+        'amo-fu-delay-mid-soft-block-addon',
+    ]
+    split = split_enforcement_actions(slugs)
+    assert split.primary == (
+        DECISION_ACTIONS.AMO_DISABLE_ADDON,
+        DECISION_ACTIONS.AMO_BAN_USER,
+        DECISION_ACTIONS.AMO_APPROVE,
+    )
+    assert split.followup == (
+        # AMO_FU_DELAY_MID_SOFT_BLOCK_ADDON action is returned in the
+        # second list since it's a follow-up action
+        DECISION_ACTIONS.AMO_FU_DELAY_MID_SOFT_BLOCK_ADDON,
+    )
+
+
+def test_filter_enforcement_actions():
+    assert filter_enforcement_actions([], Addon) == ()
+    assert filter_enforcement_actions(
+        SplitEnforcementActions([], []), Addon
+    ) == SplitEnforcementActions((), ())
+
+    primary_actions = (
+        DECISION_ACTIONS.AMO_DISABLE_ADDON,
+        DECISION_ACTIONS.AMO_BAN_USER,
+        DECISION_ACTIONS.AMO_APPROVE,
+    )
+    followup_actions = (DECISION_ACTIONS.AMO_FU_DELAY_MID_SOFT_BLOCK_ADDON,)
+    split_actions = SplitEnforcementActions(
+        primary_actions,
+        followup_actions,
+    )
+
+    # filter_enforcement_actions works on a flat list/tuple of actions
+    assert filter_enforcement_actions(primary_actions, Addon) == (
+        DECISION_ACTIONS.AMO_DISABLE_ADDON,
+        # no AMO_BAN_USER action because not a user target
+        DECISION_ACTIONS.AMO_APPROVE,
+    )
+    assert filter_enforcement_actions(followup_actions, Addon) == followup_actions
+    # check with another content type too
+    assert filter_enforcement_actions(primary_actions, UserProfile) == (
+        # no AMO_DISABLE_ADDON action because not an add-on target
+        DECISION_ACTIONS.AMO_BAN_USER,
+        DECISION_ACTIONS.AMO_APPROVE,
+    )
+
+    # and also when it's a SplitEnforcementActions of lists/tuples of actions
+    assert filter_enforcement_actions(split_actions, Addon) == SplitEnforcementActions(
+        (
+            DECISION_ACTIONS.AMO_DISABLE_ADDON,
+            # no AMO_BAN_USER action because not a user target
+            DECISION_ACTIONS.AMO_APPROVE,
+        ),
+        (DECISION_ACTIONS.AMO_FU_DELAY_MID_SOFT_BLOCK_ADDON,),
+    )

src/olympia/abuse/tests/test_views.py

@@ -39,12 +39,7 @@
     ContentActionTargetAppealRemovalAffirmation,
 )
 from ..models import AbuseReport, CinderAppeal, CinderJob, ContentDecision
-from ..views import (
-    CinderInboundPermission,
-    cinder_webhook,
-    filter_enforcement_actions,
-    to_enforcement_actions,
-)
+from ..views import CinderInboundPermission, cinder_webhook
 
 
 TESTS_DIR = os.path.dirname(os.path.abspath(__file__))
@@ -1240,47 +1235,6 @@ def test_cinder_inbound_permission(self):
             None,
         )
 
-    def test_to_enforcement_actions_and_filter_enforcement_actions(self):
-        addon = addon_factory()
-        assert filter_enforcement_actions([], addon) == ([], [])
-
-        actions_from_json = to_enforcement_actions(
-            [
-                'amo-disable-addon',
-                'amo-ban-user',
-                'amo-approve',
-                'not-amo-action',  # not a valid action at all
-                # valid, but not a primary action
-                'amo-fu-delay-mid-soft-block-addon',
-            ]
-        )
-        assert actions_from_json == [
-            DECISION_ACTIONS.AMO_DISABLE_ADDON,
-            DECISION_ACTIONS.AMO_BAN_USER,
-            DECISION_ACTIONS.AMO_APPROVE,
-            DECISION_ACTIONS.AMO_FU_DELAY_MID_SOFT_BLOCK_ADDON,
-        ]
-        assert filter_enforcement_actions(actions_from_json, addon) == (
-            [
-                DECISION_ACTIONS.AMO_DISABLE_ADDON,
-                # no AMO_BAN_USER action because not a user target
-                DECISION_ACTIONS.AMO_APPROVE,
-            ],
-            # AMO_FU_DELAY_MID_SOFT_BLOCK_ADDON action is returned in the
-            # second list since it's a follow-up action
-            [DECISION_ACTIONS.AMO_FU_DELAY_MID_SOFT_BLOCK_ADDON],
-        )
-
-        # check with another content type too
-        assert filter_enforcement_actions(actions_from_json, user_factory()) == (
-            [
-                # no AMO_DISABLE_ADDON action because not an add-on target
-                DECISION_ACTIONS.AMO_BAN_USER,
-                DECISION_ACTIONS.AMO_APPROVE,
-            ],
-            [],
-        )
-
     def test_create_and_execute_decision_called(
         self, data=None, *, slug='amo-content-infringement'
     ):