Improve performance of CinderJob.resolvable_in_reviewer_tools() / `…

…CinderJob.for_addon()` (#21882)

* Improve performance of CinderJob.resolvable_in_reviewer_tools() / CinderJob.for_addon()

Add denormalized fields to help figure out whether a CinderJob is for a particular
add-on and whether it's meant for AMO reviewers, which greatly simplifies the
corresponding queries, improving their performance.

- `target_addon` is set at reporting time and never changes after
- `resolvable_in_reviewer_tools` is also set at reporting time but can be
   flipped when processing escalations from Cinder.

src/olympia/abuse/management/commands/fill_cinderjobs_denormalized_fields.py

@@ -0,0 +1,30 @@
+from django.core.management.base import BaseCommand
+
+from olympia.abuse.models import CinderJob
+
+
+class Command(BaseCommand):
+    help = (
+        'One-off command to fill CinderJob.target_addon and '
+        'CinderJob.resolvable_in_reviewer_tools fields'
+    )
+
+    def handle(self, *args, **options):
+        jobs = CinderJob.objects.all()
+
+        for job in jobs:
+            if abuse_report := job.initial_abuse_report:
+                job.target_addon = (
+                    # If the abuse report is against a guid, set target_addon
+                    # on the job accordingly.
+                    abuse_report.guid and abuse_report.target
+                )
+                job.resolvable_in_reviewer_tools = (
+                    # If the abuse report was meant to be handled by reviewers
+                    # from the start or it's been escalated, set
+                    # resolvable_in_reviewer_tools accordingly.
+                    abuse_report.is_handled_by_reviewers
+                    or job.decision_action
+                    == CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON
+                )
+                job.save()

src/olympia/abuse/migrations/0025_cinderjob_resolvable_in_reviewer_tools_and_more.py

@@ -0,0 +1,29 @@
+# Generated by Django 4.2.9 on 2024-02-19 10:53
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("addons", "0049_clear_bad_url_data"),
+        ("abuse", "0024_cinderpolicy_parent_alter_cinderpolicy_uuid"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="cinderjob",
+            name="resolvable_in_reviewer_tools",
+            field=models.BooleanField(default=None, null=True),
+        ),
+        migrations.AddField(
+            model_name="cinderjob",
+            name="target_addon",
+            field=models.ForeignKey(
+                blank=True,
+                null=True,
+                on_delete=django.db.models.deletion.SET_NULL,
+                to="addons.addon",
+            ),
+        ),
+    ]

src/olympia/abuse/models.py

@@ -3,7 +3,6 @@
 
 from django.core.exceptions import ImproperlyConfigured
 from django.db import models
-from django.db.models import Q
 from django.db.transaction import atomic
 
 from olympia import amo
@@ -41,38 +40,15 @@
 
 class CinderJobQuerySet(BaseQuerySet):
     def for_addon(self, addon):
-        return (
-            self.filter(
-                Q(abusereport__guid=addon.addonguid_guid)
-                | Q(appealed_jobs__abusereport__guid=addon.addonguid_guid)
-            )
-            .order_by('-created')
-            .distinct()
-        )
+        return self.filter(target_addon=addon).order_by('-pk')
 
     def unresolved(self):
         return self.filter(
             decision_action__in=tuple(CinderJob.DECISION_ACTIONS.UNRESOLVED.values)
         )
 
     def resolvable_in_reviewer_tools(self):
-        # Note this isn't quite the same as AbuseReport.is_handled_by_reviewers
-        # - it doesn't verify the guids are valid add-ons,
-        # - and includes escalations too.
-        def get_filter_fields(prefix):
-            filter_fields = (
-                ('__reason__in', AbuseReport.REASONS.REVIEWER_HANDLED.values),
-                ('__location__in', AbuseReport.LOCATION.REVIEWER_HANDLED.values),
-            )
-            return {prefix + key: tuple(val) for key, val in filter_fields}
-
-        return self.exclude(
-            abusereport__guid=None, appealed_jobs__abusereport__guid=None
-        ).filter(
-            Q(**get_filter_fields('abusereport'))
-            | Q(**get_filter_fields('appealed_jobs__abusereport'))
-            | Q(decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON)
-        )
+        return self.filter(resolvable_in_reviewer_tools=True)
 
 
 class CinderJobManager(ManagerBase):
@@ -146,11 +122,17 @@ class CinderJob(ModelBase):
         # appeal for multiple previous decisions (jobs).
         related_name='appealed_jobs',
     )
+    target_addon = models.ForeignKey(
+        to=Addon, blank=True, null=True, on_delete=models.deletion.SET_NULL
+    )
+    resolvable_in_reviewer_tools = models.BooleanField(default=None, null=True)
 
     objects = CinderJobManager()
 
     @property
     def target(self):
+        if self.target_addon_id:
+            return self.target_addon
         # this works because all abuse reports for a single job and all appeals
         # for a single job are for the same target.
         if initial_abuse_report := self.initial_abuse_report:
@@ -309,8 +291,13 @@ def report(cls, abuse_report):
         reporter_entity = cls.get_cinder_reporter(abuse_report)
         entity_helper = cls.get_entity_helper(abuse_report)
         job_id = entity_helper.report(report=report_entity, reporter=reporter_entity)
+        target = abuse_report.target
+        defaults = {
+            'target_addon': target if isinstance(target, Addon) else None,
+            'resolvable_in_reviewer_tools': abuse_report.is_handled_by_reviewers,
+        }
         with atomic():
-            cinder_job, _ = cls.objects.get_or_create(job_id=job_id)
+            cinder_job, _ = cls.objects.get_or_create(job_id=job_id, defaults=defaults)
             abuse_report.update(cinder_job=cinder_job)
         # Additional context can take a while, so it is reported outside the
         # atomic() block so that the transaction can be committed quickly,
@@ -339,6 +326,8 @@ def process_decision(
             decision_notes=decision_notes[
                 : self._meta.get_field('decision_notes').max_length
             ],
+            resolvable_in_reviewer_tools=self.resolvable_in_reviewer_tools
+            or decision_action == self.DECISION_ACTIONS.AMO_ESCALATE_ADDON,
         )
         policies = CinderPolicy.objects.filter(
             uuid__in=policy_ids
@@ -390,7 +379,13 @@ def appeal(self, *, abuse_report, appeal_text, user, is_reporter):
             appealer=appealer_entity,
         )
         with atomic():
-            appeal_job, _ = self.__class__.objects.get_or_create(job_id=appeal_id)
+            appeal_job, _ = self.__class__.objects.get_or_create(
+                job_id=appeal_id,
+                defaults={
+                    'target_addon': self.target_addon,
+                    'resolvable_in_reviewer_tools': self.resolvable_in_reviewer_tools,
+                },
+            )
             self.update(appeal_job=appeal_job)
             if is_reporter:
                 abuse_report.update(
@@ -428,7 +423,7 @@ def resolve_job(self, *, decision, log_entry):
         action_helper.notify_reporters()
         if not getattr(log_entry.log, 'hide_developer', False):
             action_helper.notify_owners(policy_text=log_entry.details.get('comments'))
-        if (report := self.initial_abuse_report) and report.is_handled_by_reviewers:
+        if self.resolvable_in_reviewer_tools:
             entity_helper.close_job(job_id=self.job_id)
 
 

src/olympia/abuse/tests/test_commands.py

@@ -0,0 +1,106 @@
+from django.core.management import call_command
+
+from olympia.abuse.models import AbuseReport, CinderJob
+from olympia.amo.tests import TestCase, addon_factory, user_factory
+
+
+class TestFillCinderJob(TestCase):
+    def test_fill_somehow_no_abuse_reports(self):
+        job = CinderJob.objects.create(job_id='job123')
+
+        call_command('fill_cinderjobs_denormalized_fields')
+
+        job.reload()
+        assert not job.target_addon
+        assert not job.resolvable_in_reviewer_tools
+
+    def test_fill_addon(self):
+        addon_factory()  # Extra add-on, shouldn't matter.
+        addon = addon_factory()
+        job = CinderJob.objects.create(job_id='job123')
+        report = AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
+
+        call_command('fill_cinderjobs_denormalized_fields')
+
+        job.reload()
+        assert job.target_addon == report.target == addon
+        assert not job.resolvable_in_reviewer_tools
+
+    def test_fill_appealed_job(self):
+        addon_factory()  # Extra add-on, shouldn't matter.
+        addon = addon_factory()
+        job = CinderJob.objects.create(
+            job_id='job123', appeal_job=CinderJob.objects.create(job_id='appeal123')
+        )
+        report = AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
+
+        call_command('fill_cinderjobs_denormalized_fields')
+
+        job.reload()
+        assert job.target_addon == report.target == addon
+        assert not job.resolvable_in_reviewer_tools
+        job.appeal_job.reload()
+        assert job.appeal_job.target_addon == report.target == addon
+        assert not job.appeal_job.resolvable_in_reviewer_tools
+
+    def test_fill_non_addon(self):
+        user = user_factory()
+        job = CinderJob.objects.create(job_id='job123')
+        AbuseReport.objects.create(user=user, cinder_job=job)
+
+        call_command('fill_cinderjobs_denormalized_fields')
+
+        job.reload()
+        assert job.target_addon is None
+        assert not job.resolvable_in_reviewer_tools
+
+    def test_fill_resolvable_in_reviewer_tools(self):
+        addon_factory()  # Extra add-on, shouldn't matter.
+        addon = addon_factory()
+        job = CinderJob.objects.create(job_id='job123')
+        report = AbuseReport.objects.create(
+            guid=addon.guid,
+            cinder_job=job,
+            location=AbuseReport.LOCATION.BOTH,
+            reason=AbuseReport.REASONS.POLICY_VIOLATION,
+        )
+
+        call_command('fill_cinderjobs_denormalized_fields')
+
+        job.reload()
+        assert job.target_addon == report.target == addon
+        assert job.resolvable_in_reviewer_tools
+
+    def test_fill_not_resolvable_in_reviewer_tools(self):
+        addon_factory()  # Extra add-on, shouldn't matter.
+        addon = addon_factory()
+        job = CinderJob.objects.create(job_id='job123')
+        # Location makes it not resolvable in reviewer tools unless escalated
+        # even though the reason is policy violation.
+        report = AbuseReport.objects.create(
+            guid=addon.guid,
+            cinder_job=job,
+            location=AbuseReport.LOCATION.AMO,
+            reason=AbuseReport.REASONS.POLICY_VIOLATION,
+        )
+
+        call_command('fill_cinderjobs_denormalized_fields')
+
+        job.reload()
+        assert job.target_addon == report.target == addon
+        assert not job.resolvable_in_reviewer_tools
+
+    def test_fill_escalated_addon(self):
+        addon_factory()  # Extra add-on, shouldn't matter.
+        addon = addon_factory()
+        job = CinderJob.objects.create(
+            job_id='job123',
+            decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON,
+        )
+        report = AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
+
+        call_command('fill_cinderjobs_denormalized_fields')
+
+        job.reload()
+        assert job.target_addon == report.target == addon
+        assert job.resolvable_in_reviewer_tools