Merge pull request #108 from eviljeff/15151-store-override-decisions

Store override decisions as seperate ContentDecision instances

src/olympia/abuse/actions.py

@@ -328,7 +328,7 @@ class ContentActionEscalateAddon(ContentAction):
     def process_action(self):
         from olympia.abuse.tasks import handle_escalate_action
 
-        handle_escalate_action.delay(job_pk=self.decision.cinder_job.pk)
+        handle_escalate_action.delay(job_pk=self.decision.originating_job.pk)
 
 
 class ContentActionDeleteCollection(ContentAction):

src/olympia/abuse/migrations/0046_contentdecision_override_of.py

@@ -0,0 +1,19 @@
+# Generated by Django 4.2.16 on 2024-12-03 12:47
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('abuse', '0045_auto_20241120_1503'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='contentdecision',
+            name='override_of',
+            field=models.OneToOneField(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='overridden_by', to='abuse.contentdecision'),
+        ),
+    ]

src/olympia/abuse/models.py

@@ -120,12 +120,23 @@ def target(self):
                 return initial_report.target
             return None
 
+    @property
+    def final_decision(self):
+        return (
+            self.decision
+            if not self.decision or not hasattr(self.decision, 'overridden_by')
+            else self.decision.overridden_by
+        )
+
     @property
     def all_abuse_reports(self):
         return [
             *chain.from_iterable(
-                decision.cinder_job.all_abuse_reports
-                for decision in self.appealed_decisions.filter(cinder_job__isnull=False)
+                decision.originating_job.all_abuse_reports
+                for decision in self.appealed_decisions.filter(
+                    Q(cinder_job__isnull=False)
+                    | Q(override_of__cinder_job__isnull=False)
+                )
             ),
             *self.abusereport_set.all(),
         ]
@@ -292,35 +303,33 @@ def process_decision(
     ):
         """This is called for cinder originated decisions.
         See resolve_job for reviewer tools originated decisions."""
-        overridden_action = getattr(self.decision, 'action', None)
         # We need either an AbuseReport or ContentDecision for the target props
         abuse_report_or_decision = (
             self.appealed_decisions.first() or self.abusereport_set.first()
         )
-        cinder_decision, _ = ContentDecision.objects.update_or_create(
-            cinder_job=self,
-            defaults={
-                'addon': (
-                    self.target_addon
-                    if self.target_addon_id
-                    else abuse_report_or_decision.addon
-                ),
-                'rating': abuse_report_or_decision.rating,
-                'collection': abuse_report_or_decision.collection,
-                'user': abuse_report_or_decision.user,
-                'cinder_id': decision_cinder_id,
-                'action': decision_action,
-                'notes': decision_notes[
-                    : ContentDecision._meta.get_field('notes').max_length
-                ],
-            },
+        decision = ContentDecision.objects.create(
+            addon=(
+                self.target_addon
+                if self.target_addon_id
+                else abuse_report_or_decision.addon
+            ),
+            rating=abuse_report_or_decision.rating,
+            collection=abuse_report_or_decision.collection,
+            user=abuse_report_or_decision.user,
+            cinder_id=decision_cinder_id,
+            action=decision_action,
+            notes=decision_notes[: ContentDecision._meta.get_field('notes').max_length],
+            override_of=self.decision,
         )
-        self.update(decision=cinder_decision)
         policies = CinderPolicy.objects.filter(
             uuid__in=policy_ids
         ).without_parents_if_their_children_are_present()
-        cinder_decision.policies.add(*policies)
-        cinder_decision.process_action(overridden_action=overridden_action)
+        decision.policies.add(*policies)
+
+        if not self.decision:
+            self.update(decision=decision)
+
+        decision.process_action()
 
     def process_queue_move(self, *, new_queue, notes):
         CinderQueueMove.objects.create(cinder_job=self, notes=notes, to_queue=new_queue)
@@ -357,27 +366,32 @@ def resolve_job(self, *, log_entry):
             resolved_in_reviewer_tools=self.resolvable_in_reviewer_tools,
         )
 
-        cinder_decision = self.decision or ContentDecision(
+        decision = ContentDecision(
             addon=abuse_report_or_decision.addon,
             rating=abuse_report_or_decision.rating,
             collection=abuse_report_or_decision.collection,
             user=abuse_report_or_decision.user,
+            override_of=self.decision,
         )
-        cinder_decision.cinder_job = self
-        cinder_decision.notify_reviewer_decision(
+        if is_first_decision := self.decision is None:
+            decision.cinder_job = self
+
+        decision.notify_reviewer_decision(
             log_entry=log_entry,
             entity_helper=entity_helper,
             appealed_action=getattr(self.appealed_decisions.first(), 'action', None),
         )
-        self.update(decision=cinder_decision)
-        if cinder_decision.is_delayed:
+        if is_first_decision:
+            self.update(decision=decision)
+
+        if decision.is_delayed:
             version_list = log_entry.versionlog_set.values_list('version', flat=True)
             self.pending_rejections.add(
                 *VersionReviewerFlags.objects.filter(version__in=version_list)
             )
         else:
             self.pending_rejections.clear()
-        if cinder_decision.addon_id:
+        if decision.addon_id:
             self.clear_needs_human_review_flags()
 
     def clear_needs_human_review_flags(self):
@@ -953,6 +967,12 @@ class ContentDecision(ModelBase):
         # appeal for multiple previous decisions (jobs).
         related_name='appealed_decisions',
     )
+    override_of = models.OneToOneField(
+        to='self',
+        null=True,
+        on_delete=models.SET_NULL,
+        related_name='overridden_by',
+    )
     addon = models.ForeignKey(to=Addon, null=True, on_delete=models.deletion.SET_NULL)
     user = models.ForeignKey(UserProfile, null=True, on_delete=models.SET_NULL)
     rating = models.ForeignKey(Rating, null=True, on_delete=models.SET_NULL)
@@ -995,6 +1015,14 @@ class Meta:
             ),
         ]
 
+    @property
+    def originating_job(self):
+        return (
+            self.override_of.originating_job
+            if self.override_of
+            else getattr(self, 'cinder_job', None)
+        )
+
     def get_reference_id(self, short=True):
         if short and self.cinder_id:
             return self.cinder_id
@@ -1033,7 +1061,7 @@ def get_target_display(self):
 
     @property
     def is_third_party_initiated(self):
-        return hasattr(self, 'cinder_job') and bool(self.cinder_job.all_abuse_reports)
+        return bool((job := self.originating_job) and job.all_abuse_reports)
 
     @classmethod
     def get_action_helper_class(cls, decision_action):
@@ -1097,6 +1125,8 @@ def can_be_appealed(self, *, is_reporter, abuse_report=None):
             # appealed decision (new decision id) can be appealed by the author
             # though (see below).
             and not self.appealed_decision_already_made()
+            # if a decision has been overriden, the appeal must be on the overide
+            and not hasattr(self, 'overridden_by')
         )
         user_criteria = (
             # Reporters can appeal decisions if they have a report and that
@@ -1108,7 +1138,7 @@ def can_be_appealed(self, *, is_reporter, abuse_report=None):
                 is_reporter
                 and abuse_report
                 and self.is_third_party_initiated
-                and abuse_report.cinder_job == self.cinder_job
+                and abuse_report.cinder_job == self.originating_job
                 and not hasattr(abuse_report, 'cinderappeal')
                 and self.action in DECISION_ACTIONS.APPEALABLE_BY_REPORTER
             )
@@ -1170,8 +1200,7 @@ def appeal(self, *, abuse_report, appeal_text, user, is_reporter):
             appealer_entity = CinderUser(user)
 
         resolvable_in_reviewer_tools = (
-            not hasattr(self, 'cinder_job')
-            or self.cinder_job.resolvable_in_reviewer_tools
+            not (job := self.originating_job) or job.resolvable_in_reviewer_tools
         )
         if not self.can_be_appealed(is_reporter=is_reporter, abuse_report=abuse_report):
             raise CantBeAppealed
@@ -1221,25 +1250,29 @@ def notify_reviewer_decision(
                 'Missing or invalid cinder_action in activity log details passed to '
                 'notify_reviewer_decision'
             )
-        overridden_action = self.action
+        overridden_action = (
+            self.override_of
+            and self.override_of.action_date
+            and self.override_of.action
+        )
         self.action = DECISION_ACTIONS.for_constant(
             log_entry.details['cinder_action']
         ).value
         self.notes = log_entry.details.get('comments', '')
         policies = {cpl.cinder_policy for cpl in log_entry.cinderpolicylog_set.all()}
 
-        if self.action not in DECISION_ACTIONS.APPROVING or hasattr(self, 'cinder_job'):
+        any_cinder_job = self.originating_job
+        current_cinder_job = getattr(self, 'cinder_job', None)
+        if self.action not in DECISION_ACTIONS.APPROVING or any_cinder_job:
             # we don't create cinder decisions for approvals that aren't resolving a job
             create_decision_kw = {
                 'action': self.action.api_value,
                 'reasoning': self.notes,
                 'policy_uuids': [policy.uuid for policy in policies],
             }
-            if not overridden_action and (
-                cinder_job := getattr(self, 'cinder_job', None)
-            ):
+            if current_cinder_job:
                 decision_cinder_id = entity_helper.create_job_decision(
-                    job_id=cinder_job.job_id, **create_decision_kw
+                    job_id=current_cinder_job.job_id, **create_decision_kw
                 )
             else:
                 decision_cinder_id = entity_helper.create_decision(**create_decision_kw)
@@ -1252,8 +1285,8 @@ def notify_reviewer_decision(
         action_helper = self.get_action_helper(
             overridden_action=overridden_action, appealed_action=appealed_action
         )
-        if cinder_job := getattr(self, 'cinder_job', None):
-            cinder_job.notify_reporters(action_helper)
+        if any_cinder_job:
+            any_cinder_job.notify_reporters(action_helper)
         version_numbers = log_entry.versionlog_set.values_list(
             'version__version', flat=True
         )
@@ -1284,25 +1317,31 @@ def notify_reviewer_decision(
             },
         )
 
-    def process_action(self, *, overridden_action=None, release_hold=False):
+    def process_action(self, *, release_hold=False):
         """currently only called by decisions from cinder.
         see https://mozilla-hub.atlassian.net/browse/AMOENG-1125
         """
         assert not self.action_date  # we should not be attempting to process twice
+        cinder_job = self.originating_job
         appealed_action = (
-            getattr(self.cinder_job.appealed_decisions.first(), 'action', None)
-            if hasattr(self, 'cinder_job')
+            getattr(cinder_job.appealed_decisions.first(), 'action', None)
+            if cinder_job
             else None
         )
 
+        overridden_action = (
+            self.override_of
+            and self.override_of.action_date
+            and self.override_of.action
+        )
         action_helper = self.get_action_helper(
             overridden_action=overridden_action,
             appealed_action=appealed_action,
         )
         if release_hold or not action_helper.should_hold_action():
             self.action_date = datetime.now()
             log_entry = action_helper.process_action()
-            if cinder_job := getattr(self, 'cinder_job', None):
+            if cinder_job:
                 cinder_job.notify_reporters(action_helper)
             action_helper.notify_owners(log_entry_id=getattr(log_entry, 'id', None))
             self.save(update_fields=('action_date',))

src/olympia/abuse/templates/abuse/appeal.html

@@ -37,6 +37,9 @@ <h3>{{ _('Decision {0}')|format_html(decision_cinder_id) }}</h3>
                         <p> {{ _("We have already reviewed a similar appeal from another reporter, and have reversed our prior decision. We have taken action against the content and/or account holder in accordance with our policies.") }}</p>
                         <p> {{ _("Because the decision you are appealing has already been overturned, your appeal will not be processed.") }} </p>
                     {% endif %}
+                {% elif appealed_decision_overridden %}
+                    <p> {{ _("Thank you for your report.") }} </p>
+                    <p> {{ _("The decision you are appealing has already been overridden by a new decision, so this decision can't be appealed.") }} </p>
                 {% else %}
                     <p> {{ _("This decision can't be appealed.") }} </p>
                 {% endif %}