Record review queue history: when a version enters the queue, when th…

…ey leave it

src/olympia/activity/migrations/0027_alter_activitylog_action.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.16 on 2024-12-06 16:51
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('activity', '0026_alter_activitylog_action_attachmentlog'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='activitylog',
+            name='action',
+            field=models.SmallIntegerField(choices=[(1, 1), (2, 2), (3, 3), (4, 4), (5, 5), (6, 6), (7, 7), (8, 8), (9, 9), (12, 12), (16, 16), (17, 17), (18, 18), (19, 19), (20, 20), (21, 21), (22, 22), (23, 23), (24, 24), (25, 25), (26, 26), (27, 27), (28, 28), (29, 29), (31, 31), (32, 32), (33, 33), (34, 34), (35, 35), (36, 36), (37, 37), (38, 38), (39, 39), (40, 40), (41, 41), (42, 42), (43, 43), (44, 44), (45, 45), (46, 46), (47, 47), (48, 48), (49, 49), (53, 53), (60, 60), (61, 61), (62, 62), (98, 98), (99, 99), (100, 100), (101, 101), (102, 102), (103, 103), (104, 104), (105, 105), (106, 106), (107, 107), (108, 108), (109, 109), (110, 110), (120, 120), (121, 121), (128, 128), (130, 130), (131, 131), (132, 132), (133, 133), (134, 134), (135, 135), (136, 136), (137, 137), (138, 138), (139, 139), (140, 140), (141, 141), (142, 142), (143, 143), (144, 144), (145, 145), (146, 146), (147, 147), (148, 148), (149, 149), (150, 150), (151, 151), (152, 152), (153, 153), (154, 154), (155, 155), (156, 156), (157, 157), (158, 158), (159, 159), (160, 160), (161, 161), (162, 162), (163, 163), (164, 164), (165, 165), (166, 166), (167, 167), (168, 168), (169, 169), (170, 170), (171, 171), (172, 172), (173, 173), (174, 174), (175, 175), (176, 176), (177, 177), (178, 178), (179, 179), (180, 180), (181, 181), (182, 182), (183, 183), (184, 184), (185, 185), (186, 186), (187, 187), (188, 188), (189, 189), (190, 190), (191, 191), (192, 192), (193, 193), (194, 194), (195, 195), (196, 196), (197, 197)]),
+        ),
+    ]

src/olympia/activity/utils.py

@@ -242,7 +242,7 @@ def log_and_notify(
             version=version, reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY
         )
         if not had_due_date:
-            version.update(
+            version.reset_due_date(
                 due_date=get_review_due_date(default_days=REVIEWER_STANDARD_REPLY_TIME)
             )
 

src/olympia/addons/models.py

@@ -1876,24 +1876,23 @@ def set_tag_list(self, new_tag_list):
     def update_all_due_dates(self):
         """
         Update all due dates on versions of this add-on.
+
+        Use when dealing with having to re-check all due dates for all versions
+        of an add-on as it does it in a slightly more optimized than checking
+        for each version individually.
         """
-        versions = self.versions(manager='unfiltered_for_relations')
-        for version in versions.should_have_due_date().filter(due_date__isnull=True):
+        manager = self.versions(manager='unfiltered_for_relations')
+        for version in (
+            manager.should_have_due_date().filter(due_date__isnull=True).no_transforms()
+        ):
             due_date = get_review_due_date()
-            log.info(
-                'Version %r (%s) due_date set to %s', version, version.id, due_date
-            )
-            version.update(due_date=due_date, _signal=False)
-        for version in versions.should_have_due_date(negate=True).filter(
-            due_date__isnull=False
+            version.reset_due_date(due_date=due_date, should_have_due_date=True)
+        for version in (
+            manager.should_have_due_date(negate=True)
+            .filter(due_date__isnull=False)
+            .no_transforms()
         ):
-            log.info(
-                'Version %r (%s) due_date of %s cleared',
-                version,
-                version.id,
-                version.due_date,
-            )
-            version.update(due_date=None, _signal=False)
+            version.reset_due_date(should_have_due_date=False)
 
 
 dbsignals.pre_save.connect(save_signal, sender=Addon, dispatch_uid='addon_translations')

src/olympia/devhub/views.py

@@ -1936,8 +1936,8 @@ def request_review(request, addon_id, addon):
     if latest_version:
         if latest_version.file.status == amo.STATUS_DISABLED:
             latest_version.file.update(status=amo.STATUS_AWAITING_REVIEW)
-        # Clear the due date so it gets set again in Addon.watch_status if nessecary.
-        latest_version.update(due_date=None)
+        # Clear the due date so it gets set again in Addon.watch_status if necessary.
+        latest_version.reset_due_date()
     if addon.has_complete_metadata():
         addon.update_status()
         messages.success(request, gettext('Review requested.'))

src/olympia/reviewers/migrations/0027_backfill_needshumanreview.py

@@ -5,13 +5,7 @@
 
 
 def migrate_needs_human_review_field(apps, schema_editor):
-    NeedsHumanReview = apps.get_model('reviewers', 'NeedsHumanReview')
-    Version = apps.get_model('versions', 'Version')
-
-    for version in Version.unfiltered.filter(needs_human_review=True).filter(
-        Q(needshumanreview__isnull=True) | Q(needshumanreview__is_active=False)
-    ):
-        NeedsHumanReview.objects.create(version=version)
+    pass
 
 
 class Migration(migrations.Migration):

src/olympia/reviewers/migrations/0041_reviewqueuehistory.py

@@ -0,0 +1,36 @@
+# Generated by Django 4.2.17 on 2024-12-10 11:28
+
+from django.db import migrations, models
+import django.db.models.deletion
+import django.utils.timezone
+import olympia.amo.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('activity', '0027_alter_activitylog_action'),
+        ('versions', '0047_auto_20241031_1750'),
+        ('reviewers', '0040_queuecount_queuecount_queue_count_unique_name_date'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='ReviewQueueHistory',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('created', models.DateTimeField(blank=True, default=django.utils.timezone.now, editable=False)),
+                ('modified', models.DateTimeField(auto_now=True)),
+                ('original_due_date', models.DateTimeField(help_text='Original due date when the version entered the queue')),
+                ('exit_date', models.DateTimeField(blank=True, default=None, help_text='When the version left the queue (regardless of reason)', null=True)),
+                ('review_decision_log', models.ForeignKey(blank=True, help_text='Activity Log associated with the reviewer decision that caused the version to leave the queue', null=True, on_delete=django.db.models.deletion.CASCADE, to='activity.activitylog')),
+                ('version', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='versions.version')),
+            ],
+            options={
+                'get_latest_by': 'created',
+                'abstract': False,
+                'base_manager_name': 'objects',
+            },
+            bases=(olympia.amo.models.SaveUpdateMixin, models.Model),
+        ),
+    ]

src/olympia/reviewers/models.py

@@ -941,6 +941,31 @@ def set_on_addons_latest_signed_versions(cls, addons, reason):
             )
 
 
+class ReviewQueueHistory(ModelBase):
+    # Note: a given Version can enter & leave the queue multiple times, and we
+    # want to track each occurence, so it's not a OneToOneField.
+    version = models.ForeignKey(Version, on_delete=models.CASCADE)
+    original_due_date = models.DateTimeField(
+        help_text='Original due date when the version entered the queue',
+    )
+    exit_date = models.DateTimeField(
+        default=None,
+        null=True,
+        blank=True,
+        help_text='When the version left the queue (regardless of reason)',
+    )
+    review_decision_log = models.ForeignKey(
+        'activity.ActivityLog',
+        null=True,
+        blank=True,
+        on_delete=models.CASCADE,
+        help_text=(
+            'Activity Log associated with the reviewer decision that caused '
+            'the version to leave the queue'
+        ),
+    )
+
+
 @receiver(
     models.signals.post_save,
     sender=NeedsHumanReview,

src/olympia/reviewers/utils.py

@@ -27,6 +27,7 @@
 from olympia.reviewers.models import (
     AutoApprovalSummary,
     NeedsHumanReview,
+    ReviewQueueHistory,
     get_flags,
 )
 from olympia.users.utils import get_task_user
@@ -922,7 +923,21 @@ def set_promoted(self, versions=None):
             for version in versions:
                 self.addon.promotedaddon.approve_for_version(version)
 
-    def notify_decision(self):
+    def remove_from_queue_history(self):
+        if self.log_entry:
+            # Each entry in the ReviewQueueHistory corresponding to a version
+            # we are affecting and that doesn't already have a review decision
+            # log should get one. The exit_date will be cleared separately in
+            # Version.reset_due_date(), and the combination of those two pieces
+            # of information means the version left the queue because of a
+            # reviewer action.
+            ReviewQueueHistory.objects.filter(
+                version__in=self.log_entry.versionlog_set.all(),
+                review_decision_log__isnull=True,
+            ).update(review_decision_log=self.log_entry)
+
+    def record_decision(self):
+        self.remove_from_queue_history()
         if cinder_jobs := self.data.get('cinder_jobs_to_resolve', ()):
             # with appeals and escalations there could be multiple jobs
             for cinder_job in cinder_jobs:
@@ -1099,7 +1114,7 @@ def process_comment(self):
     def resolve_reports_job(self):
         if self.data.get('cinder_jobs_to_resolve', ()):
             self.log_action(amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION)
-            self.notify_decision()  # notify cinder
+            self.record_decision()  # notify cinder
 
     def resolve_appeal_job(self):
         # It's possible to have multiple appeal jobs, so handle them seperately.
@@ -1118,6 +1133,7 @@ def resolve_appeal_job(self):
                 policies=list(previous_policies),
                 cinder_action=DECISION_ACTIONS.for_value(previous_action_id),
             )
+            self.remove_from_queue_history()
             # notify cinder
             resolve_job_in_cinder.delay(
                 cinder_job_id=job.id, log_entry_id=self.log_entry.id
@@ -1172,7 +1188,7 @@ def approve_latest_version(self):
         if self.human_review or self.addon.type != amo.ADDON_LPAPP:
             # Don't notify decisions (to cinder or owners) for auto-approved langpacks
             log.info('Sending email for %s' % (self.addon))
-            self.notify_decision()
+            self.record_decision()
         self.log_public_message()
 
     def reject_latest_version(self):
@@ -1200,7 +1216,7 @@ def reject_latest_version(self):
         self.log_action(amo.LOG.REJECT_VERSION)
         log.info('Sending email for %s' % (self.addon))
         # This call has to happen after log_action - we need self.log_entry
-        self.notify_decision()
+        self.record_decision()
         self.log_sandbox_message()
 
     def request_admin_review(self):
@@ -1286,7 +1302,7 @@ def confirm_auto_approved(self):
                 pending_content_rejection=None,
             )
             self.set_human_review_date(version)
-            self.notify_decision()
+            self.record_decision()
 
     def reject_multiple_versions(self):
         """Reject a list of versions.
@@ -1413,7 +1429,7 @@ def reject_multiple_versions(self):
         if actions_to_record:
             # if we didn't record any actions we didn't do anything so nothing to notify
             log.info('Sending email for %s' % (self.addon))
-            self.notify_decision()
+            self.record_decision()
 
     def unreject_latest_version(self):
         """Un-reject the latest version."""
@@ -1491,14 +1507,14 @@ def enable_addon(self):
         self.addon.force_enable(skip_activity_log=True)
         self.log_action(amo.LOG.FORCE_ENABLE)
         log.info('Sending email for %s' % (self.addon))
-        self.notify_decision()
+        self.record_decision()
 
     def disable_addon(self):
         """Force disable the add-on and all versions."""
         self.addon.force_disable(skip_activity_log=True)
         self.log_action(amo.LOG.FORCE_DISABLE)
         log.info('Sending email for %s' % (self.addon))
-        self.notify_decision()
+        self.record_decision()
 
 
 class ReviewAddon(ReviewBase):
@@ -1575,7 +1591,7 @@ def approve_latest_version(self):
             % (self.addon, self.file.file.name if self.file else '')
         )
         log.info('Sending email for %s' % (self.addon))
-        self.notify_decision()
+        self.record_decision()
 
     def block_multiple_versions(self):
         versions = self.data['versions']
@@ -1610,7 +1626,7 @@ def confirm_multiple_versions(self):
             versions=self.data['versions'],
             timestamp=timestamp,
         )
-        self.notify_decision()
+        self.record_decision()
 
     def approve_multiple_versions(self):
         """Set multiple unlisted add-on versions files to public."""
@@ -1650,7 +1666,7 @@ def approve_multiple_versions(self):
                 defaults={'auto_approval_disabled_until_next_approval_unlisted': False},
             )
             log.info('Sending email(s) for %s' % (self.addon))
-            self.notify_decision()
+            self.record_decision()
 
     def unreject_multiple_versions(self):
         """Un-reject a list of versions."""

src/olympia/reviewers/views.py

@@ -1169,7 +1169,7 @@ def due_date(self, request, **kwargs):
         status_code = status.HTTP_202_ACCEPTED
         try:
             due_date = datetime.fromisoformat(request.data.get('due_date'))
-            version.update(due_date=due_date)
+            version.reset_due_date(due_date=due_date)
         except TypeError:
             status_code = status.HTTP_400_BAD_REQUEST
         return Response(status=status_code)

src/olympia/versions/models.py

@@ -985,7 +985,7 @@ def disable_old_files(self):
             for f in qs:
                 f.update(status=amo.STATUS_DISABLED)
 
-    def reset_due_date(self, due_date=None):
+    def reset_due_date(self, due_date=None, should_have_due_date=None):
         """Sets a due date on this version, if it is eligible for one, or
         clears it if the version should not have a due date (see
         VersionManager.should_have_due_date for logic).
@@ -994,21 +994,44 @@ def reset_due_date(self, due_date=None):
         doesn't already have one; otherwise the provided due_date will be be
         used to overwrite any value.
 
+        If should_have_due_date is not None its value will be used instead of
+        actually checking if the version should have a due date or not.
+
         Doesn't trigger post_save signal to avoid infinite loops
         since it can be triggered from Version.post_save callback.
         """
-        if self.should_have_due_date:
+        from olympia.reviewers.models import ReviewQueueHistory
+
+        if should_have_due_date is None:
+            should_have_due_date = self.should_have_due_date
+
+        if should_have_due_date:
             # if the version should have a due date and it doesn't, set one
             if not self.due_date or due_date:
                 due_date = due_date or self.generate_due_date()
                 log.info('Version %r (%s) due_date set to %s', self, self.id, due_date)
                 self.update(due_date=due_date, _signal=False)
+                ReviewQueueHistory.objects.get_or_create(
+                    version=self,
+                    exit_date=None,
+                    # We only store the due date when creating the entry, in
+                    # case it got updated afterwards, so that we always have
+                    # the true original due date.
+                    defaults={'original_due_date': due_date},
+                )
         elif self.due_date:
             # otherwise it shouldn't have a due_date so clear it.
             log.info(
                 'Version %r (%s) due_date of %s cleared', self, self.id, self.due_date
             )
             self.update(due_date=None, _signal=False)
+            # The version left the queue - whether it was a reviewer action or
+            # not, we record that here. Reviewer tools will update
+            # review_decision_log separately if relevant.
+            ReviewQueueHistory.objects.filter(
+                version=self,
+                exit_date=None,
+            ).update(exit_date=datetime.now())
 
     @use_primary_db
     def generate_due_date(self):