From 6724fffb23c2b770b85e5b6be27c0682d6fb6df1 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Fri, 6 Dec 2024 16:17:35 +0100 Subject: [PATCH] Record review queue history: when a version enters the queue, when they leave it --- .../0027_alter_activitylog_action.py | 18 +++++++++ src/olympia/activity/utils.py | 2 +- src/olympia/addons/models.py | 29 +++++++------- src/olympia/devhub/views.py | 4 +- .../0027_backfill_needshumanreview.py | 8 +--- .../migrations/0041_reviewqueuehistory.py | 36 ++++++++++++++++++ src/olympia/reviewers/models.py | 25 ++++++++++++ src/olympia/reviewers/utils.py | 38 +++++++++++++------ src/olympia/reviewers/views.py | 2 +- src/olympia/versions/models.py | 27 ++++++++++++- 10 files changed, 150 insertions(+), 39 deletions(-) create mode 100644 src/olympia/activity/migrations/0027_alter_activitylog_action.py create mode 100644 src/olympia/reviewers/migrations/0041_reviewqueuehistory.py diff --git a/src/olympia/activity/migrations/0027_alter_activitylog_action.py b/src/olympia/activity/migrations/0027_alter_activitylog_action.py new file mode 100644 index 000000000000..50ccde4bfc09 --- /dev/null +++ b/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)]), + ), + ] diff --git a/src/olympia/activity/utils.py b/src/olympia/activity/utils.py index 1820b35dc006..7d60f5f5bba5 100644 --- a/src/olympia/activity/utils.py +++ b/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) ) diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py index 8125e9ce7678..d82c049f983f 100644 --- a/src/olympia/addons/models.py +++ b/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') diff --git a/src/olympia/devhub/views.py b/src/olympia/devhub/views.py index 7d71c94b55b6..8bbb50d063de 100644 --- a/src/olympia/devhub/views.py +++ b/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.')) diff --git a/src/olympia/reviewers/migrations/0027_backfill_needshumanreview.py b/src/olympia/reviewers/migrations/0027_backfill_needshumanreview.py index 182a4646ba02..a332e15a158f 100644 --- a/src/olympia/reviewers/migrations/0027_backfill_needshumanreview.py +++ b/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): diff --git a/src/olympia/reviewers/migrations/0041_reviewqueuehistory.py b/src/olympia/reviewers/migrations/0041_reviewqueuehistory.py new file mode 100644 index 000000000000..7687c177600d --- /dev/null +++ b/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), + ), + ] diff --git a/src/olympia/reviewers/models.py b/src/olympia/reviewers/models.py index 3f11ebcda555..745bd0fba2d4 100644 --- a/src/olympia/reviewers/models.py +++ b/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, diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 1f8fb714bb4c..558fa31f9de6 100644 --- a/src/olympia/reviewers/utils.py +++ b/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.""" diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py index 47656ba54931..05dd4dbcd959 100644 --- a/src/olympia/reviewers/views.py +++ b/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) diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 107abe00c55a..c42402a81b4b 100644 --- a/src/olympia/versions/models.py +++ b/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):