Don't record parent CinderPolicy on top of a child policy (#21839)

* Don't record parent CinderPolicy on top of a child policy

We don't want to include the parent when the child policy has already
been selected, it's redundant and would mess up the notification.

* Return a list, preventing chaining, which would have made result ambiguous

src/olympia/abuse/models.py

@@ -340,7 +340,10 @@ def process_decision(
                 : self._meta.get_field('decision_notes').max_length
             ],
         )
-        self.policies.add(*CinderPolicy.objects.filter(uuid__in=policy_ids))
+        policies = CinderPolicy.objects.filter(
+            uuid__in=policy_ids
+        ).without_parents_if_their_children_are_present()
+        self.policies.add(*policies)
         action_helper = self.get_action_helper(existing_decision, override=override)
         if action_helper.process_action():
             action_helper.process_notifications()
@@ -392,13 +395,11 @@ def resolve_job(self, *, decision, log_entry):
         """This is called for reviewer tools originated decisions.
         See process_decision for cinder originated decisions."""
         entity_helper = self.get_entity_helper(self.abuse_reports[0])
-        policies = list(
-            {
-                review_action.reason.cinder_policy
-                for review_action in log_entry.reviewactionreasonlog_set.all()
-                if review_action.reason.cinder_policy_id
-            }
-        )
+        policies = CinderPolicy.objects.filter(
+            pk__in=log_entry.reviewactionreasonlog_set.all()
+            .filter(reason__cinder_policy__isnull=False)
+            .values_list('reason__cinder_policy', flat=True)
+        ).without_parents_if_their_children_are_present()
         decision_id = entity_helper.create_decision(
             reasoning=log_entry.details.get('comments', ''),
             policy_uuids=[policy.uuid for policy in policies],
@@ -840,6 +841,17 @@ class CantBeAppealed(Exception):
     pass
 
 
+class CinderPolicyQuerySet(models.QuerySet):
+    def without_parents_if_their_children_are_present(self):
+        """Evaluates the queryset into a list, excluding parents of any child
+        policy if present.
+        """
+        parents_ids = set(
+            self.filter(parent__isnull=False).values_list('parent', flat=True)
+        )
+        return list(self.exclude(pk__in=parents_ids))
+
+
 class CinderPolicy(ModelBase):
     uuid = models.CharField(max_length=36, unique=True)
     name = models.CharField(max_length=50)
@@ -852,5 +864,7 @@ class CinderPolicy(ModelBase):
         related_name='children',
     )
 
+    objects = CinderPolicyQuerySet.as_manager()
+
     def __str__(self):
         return self.name

src/olympia/abuse/tests/test_models.py

@@ -668,6 +668,37 @@ def test_process_decision(self):
         assert notify_mock.call_count == 1
         assert list(cinder_job.policies.all()) == [policy_a, policy_b]
 
+    def test_process_decision_with_duplicate_parent(self):
+        cinder_job = CinderJob.objects.create(job_id='1234')
+        new_date = datetime(2023, 1, 1)
+        parent_policy = CinderPolicy.objects.create(
+            uuid='678-90', name='bbb', text='BBB'
+        )
+        policy = CinderPolicy.objects.create(
+            uuid='123-45', name='aaa', text='AAA', parent=parent_policy
+        )
+
+        with mock.patch.object(
+            CinderActionBanUser, 'process_action'
+        ) as action_mock, mock.patch.object(
+            CinderActionBanUser, 'process_notifications'
+        ) as notify_mock:
+            action_mock.return_value = True
+            cinder_job.process_decision(
+                decision_id='12345',
+                decision_date=new_date,
+                decision_action=CinderJob.DECISION_ACTIONS.AMO_BAN_USER.value,
+                decision_notes='teh notes',
+                policy_ids=['123-45', '678-90'],
+            )
+        assert cinder_job.decision_id == '12345'
+        assert cinder_job.decision_date == new_date
+        assert cinder_job.decision_action == CinderJob.DECISION_ACTIONS.AMO_BAN_USER
+        assert cinder_job.decision_notes == 'teh notes'
+        assert action_mock.call_count == 1
+        assert notify_mock.call_count == 1
+        assert list(cinder_job.policies.all()) == [policy]
+
     def test_appeal_as_target(self):
         abuse_report = AbuseReport.objects.create(
             guid=addon_factory().guid,
@@ -828,6 +859,74 @@ def test_resolve_job(self):
         assert 'some review text' in mail.outbox[1].body
         assert str(abuse_report.target.current_version.version) in mail.outbox[1].body
 
+    def test_resolve_job_duplicate_policy(self):
+        cinder_job = CinderJob.objects.create(job_id='999')
+        addon_developer = user_factory()
+        abuse_report = AbuseReport.objects.create(
+            guid=addon_factory(users=[addon_developer]).guid,
+            reason=AbuseReport.REASONS.POLICY_VIOLATION,
+            location=AbuseReport.LOCATION.ADDON,
+            cinder_job=cinder_job,
+            reporter=user_factory(),
+        )
+        responses.add(
+            responses.POST,
+            f'{settings.CINDER_SERVER_URL}create_decision',
+            json={'uuid': '123'},
+            status=201,
+        )
+        responses.add(
+            responses.POST,
+            f'{settings.CINDER_SERVER_URL}jobs/{cinder_job.job_id}/cancel',
+            json={'external_id': cinder_job.job_id},
+            status=200,
+        )
+        parent_policy = CinderPolicy.objects.create(
+            name='parent policy', uuid='12345678'
+        )
+        policy = CinderPolicy.objects.create(
+            name='policy', uuid='4815162342', parent=parent_policy
+        )
+        review_action_reason1 = ReviewActionReason.objects.create(cinder_policy=policy)
+        review_action_reason2 = ReviewActionReason.objects.create(
+            cinder_policy=parent_policy
+        )
+
+        log_entry = ActivityLog.objects.create(
+            amo.LOG.REJECT_VERSION,
+            abuse_report.target,
+            abuse_report.target.current_version,
+            review_action_reason1,
+            review_action_reason2,
+            details={'comments': 'some review text'},
+            user=user_factory(),
+        )
+
+        cinder_job.resolve_job(
+            decision=CinderJob.DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON,
+            log_entry=log_entry,
+        )
+
+        request = responses.calls[0].request
+        request_body = json.loads(request.body)
+        assert request_body['policy_uuids'] == [policy.uuid]
+        assert request_body['reasoning'] == 'some review text'
+        assert request_body['entity']['id'] == str(abuse_report.target.id)
+        cinder_job.reload()
+        assert cinder_job.decision_action == (
+            CinderJob.DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON
+        )
+        self.assertCloseToNow(cinder_job.decision_date)
+        # Parent policy was a duplicate since we already have its child, and
+        # has been ignored.
+        assert list(cinder_job.policies.all()) == [policy]
+        assert len(mail.outbox) == 2
+        assert mail.outbox[0].to == [abuse_report.reporter.email]
+        assert mail.outbox[1].to == [addon_developer.email]
+        assert str(log_entry.id) in mail.outbox[1].extra_headers['Message-ID']
+        assert 'some review text' in mail.outbox[1].body
+        assert str(abuse_report.target.current_version.version) in mail.outbox[1].body
+
     def test_abuse_reports(self):
         job = CinderJob.objects.create(job_id='fake_job_id')
         assert list(job.abuse_reports) == []
@@ -1153,3 +1252,36 @@ def test_create_cinder_policy_with_duplicate_uuid(self):
                 text='Policy 2 Description',
                 uuid=existing_policy.uuid,
             )
+
+    def test_without_parents_if_their_children_are_present(self):
+        parent_policy = CinderPolicy.objects.create(
+            name='Parent of Policy 1',
+            text='Policy Parent 1 Description',
+            uuid='some-parent-uuid',
+        )
+        child_policy = CinderPolicy.objects.create(
+            name='Policy 1',
+            text='Policy 1 Description',
+            uuid='some-child-uuid',
+            parent=parent_policy,
+        )
+        lone_policy = CinderPolicy.objects.create(
+            name='Policy 2',
+            text='Policy 2 Description',
+            uuid='some-uuid',
+        )
+        qs = CinderPolicy.objects.all()
+        assert set(qs) == {parent_policy, child_policy, lone_policy}
+        assert isinstance(qs.without_parents_if_their_children_are_present(), list)
+        assert set(qs.without_parents_if_their_children_are_present()) == {
+            child_policy,
+            lone_policy,
+        }
+        assert set(
+            qs.exclude(
+                pk=child_policy.pk
+            ).without_parents_if_their_children_are_present()
+        ) == {
+            parent_policy,
+            lone_policy,
+        }