From 9a628ac3602eb033a4325480cb279f94b4a85e03 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 6 Aug 2024 15:06:58 +0100 Subject: [PATCH] Expose all the reasons why there is a due_date to the extensions queue (queryset) (#22530) * set due_date_reasons bitmask when setting due_date * add no_transforms in Version.should_have_due_date * Only annotate Version qs when nessecary * refactor to make VersionManager.should_have_due_date efficient * Add flags for newly exposed reasons for due_date * get_due_date_reason_qs -> get_due_date_reason_q_objects; add docstring * exclude has developer reply from needs_human_review_other * fix linelength that ruff format didn't autofix --- src/olympia/addons/models.py | 24 +---- src/olympia/addons/tests/test_models.py | 16 +++- src/olympia/reviewers/models.py | 46 ++++++--- src/olympia/reviewers/tests/test_models.py | 27 +++++- src/olympia/versions/models.py | 58 +++++++++-- src/olympia/versions/tests/test_models.py | 106 +++++++++++++++++++++ static/css/zamboni/reviewers.less | 5 +- 7 files changed, 229 insertions(+), 53 deletions(-) diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py index 8d3f443bff..375a2d063f 100644 --- a/src/olympia/addons/models.py +++ b/src/olympia/addons/models.py @@ -307,8 +307,6 @@ class AddonManager(ManagerBase): show_temporarily_delayed=True, show_only_upcoming=False, ): - from olympia.reviewers.models import NeedsHumanReview # circular reference - filters = { 'type__in': amo.GROUP_TYPE_THEME if theme_review else amo.GROUP_TYPE_ADDON, 'versions__due_date__isnull': False, @@ -367,24 +365,12 @@ class AddonManager(ManagerBase): first_version_id=Subquery( versions_due_qs.filter(addon=OuterRef('pk')).values('pk')[:1] ), - needs_human_review_from_cinder=Exists( - versions_due_qs.filter( - needshumanreview__is_active=True, - needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, + **{ + name: Exists(versions_due_qs.filter(q)) + for name, q in ( + Version.unfiltered.get_due_date_reason_q_objects().items() ) - ), - needs_human_review_from_abuse=Exists( - versions_due_qs.filter( - needshumanreview__is_active=True, - needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, - ) - ), - needs_human_review_from_appeal=Exists( - versions_due_qs.filter( - needshumanreview__is_active=True, - needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, - ) - ), + }, ) .filter(first_version_id__isnull=False) .transform(first_pending_version_transformer) diff --git a/src/olympia/addons/tests/test_models.py b/src/olympia/addons/tests/test_models.py index cfae53a7a4..3fad538fc3 100644 --- a/src/olympia/addons/tests/test_models.py +++ b/src/olympia/addons/tests/test_models.py @@ -3600,9 +3600,14 @@ class TestExtensionsQueues(TestCase): nhr_abuse_inactive, } assert getattr(addons[nhr_abuse.id], annotated_field) - assert not getattr(addons[nhr_other.id], annotated_field) - assert not getattr(addons[nhr_without_due_date.id], annotated_field) - assert not getattr(addons[nhr_abuse_inactive.id], annotated_field) + if annotated_field != 'needs_human_review_other': + assert not getattr(addons[nhr_other.id], annotated_field) + assert not getattr(addons[nhr_without_due_date.id], annotated_field) + assert not getattr(addons[nhr_abuse_inactive.id], annotated_field) + else: + assert getattr(addons[nhr_other.id], annotated_field) + assert getattr(addons[nhr_without_due_date.id], annotated_field) + assert getattr(addons[nhr_abuse_inactive.id], annotated_field) def test_pending_queue_needs_human_review_from_abuse(self): self._test_pending_queue_needs_human_review_from( @@ -3621,6 +3626,11 @@ class TestExtensionsQueues(TestCase): NeedsHumanReview.REASONS.CINDER_ESCALATION, 'needs_human_review_from_cinder' ) + def test_pending_queue_needs_human_review_other(self): + self._test_pending_queue_needs_human_review_from( + NeedsHumanReview.REASONS.SCANNER_ACTION, 'needs_human_review_other' + ) + def test_get_pending_rejection_queue(self): expected_addons = [ version_review_flags_factory( diff --git a/src/olympia/reviewers/models.py b/src/olympia/reviewers/models.py index e403cf770e..007d3b2c92 100644 --- a/src/olympia/reviewers/models.py +++ b/src/olympia/reviewers/models.py @@ -37,7 +37,10 @@ VIEW_QUEUE_FLAGS = ( 'needs_admin_theme_review', 'Needs Admin Static Theme Review', ), - ('sources_provided', 'Source Code Provided'), + ( + 'sources_provided', + 'Source Code Provided', + ), ( 'auto_approval_disabled', 'Auto-approval disabled', @@ -62,6 +65,32 @@ VIEW_QUEUE_FLAGS = ( 'auto_approval_delayed_indefinitely_unlisted', 'Unlisted Auto-approval delayed indefinitely', ), + # The following are annotations set by AddonManager.get_queryset_for_pending_queues + # See VersionManager.get_due_date_reason_q_objects for the names + ( + 'needs_human_review_from_cinder', + 'Abuse report forwarded from Cinder present', + ), + ( + 'needs_human_review_from_abuse', + 'Abuse report present', + ), + ( + 'needs_human_review_from_appeal', + 'Appeal on decision present', + ), + ( + 'needs_human_review_other', + 'Other NeedsHumanReview flag set', + ), + ( + 'is_pre_review_version', + 'Version(s) awaiting pre-approval review', + ), + ( + 'has_developer_reply', + 'Outstanding developer reply', + ), ) @@ -88,7 +117,7 @@ def set_reviewing_cache(addon_id, user_id): def get_flags(addon, version): """Return a list of tuples (indicating which flags should be displayed for a particular add-on.""" - flag_filters_by_channel = { + exclude_flags_by_channel = { amo.CHANNEL_UNLISTED: ( 'auto_approval_disabled', 'auto_approval_delayed_temporarily', @@ -105,22 +134,11 @@ def get_flags(addon, version): for (prop, title) in VIEW_QUEUE_FLAGS if getattr(version, prop, getattr(addon, prop, None)) and prop - not in flag_filters_by_channel.get(getattr(version, 'channel', None), ()) + not in exclude_flags_by_channel.get(getattr(version, 'channel', None), ()) ] # add in the promoted group flag and return if promoted := addon.promoted_group(currently_approved=False): flags.append((f'promoted-{promoted.api_name}', promoted.name)) - if getattr(addon, 'needs_human_review_from_cinder', False): - flags.append( - ( - 'needs-human-review-from-cinder', - 'Abuse report forwarded from Cinder present', - ) - ) - if getattr(addon, 'needs_human_review_from_abuse', False): - flags.append(('needs-human-review-from-abuse', 'Abuse report present')) - if getattr(addon, 'needs_human_review_from_appeal', False): - flags.append(('needs-human-review-from-appeal', 'Appeal on decision present')) return flags diff --git a/src/olympia/reviewers/tests/test_models.py b/src/olympia/reviewers/tests/test_models.py index dc3e7e3062..e961539308 100644 --- a/src/olympia/reviewers/tests/test_models.py +++ b/src/olympia/reviewers/tests/test_models.py @@ -32,6 +32,7 @@ from olympia.files.models import File, FileValidation, WebextPermission from olympia.promoted.models import PromotedAddon from olympia.ratings.models import Rating from olympia.reviewers.models import ( + VIEW_QUEUE_FLAGS, AutoApprovalNoValidationResultError, AutoApprovalSummary, NeedsHumanReview, @@ -1704,26 +1705,42 @@ class TestGetFlags(TestCase): def test_version_none(self): assert get_flags(self.addon, None) == [] - def test_needs_human_review_abuse_flag(self): + def test_due_date_reason_flags(self): assert get_flags(self.addon, self.addon.current_version) == [] self.addon.needs_human_review_from_abuse = False self.addon.needs_human_review_from_cinder = False self.addon.needs_human_review_from_appeal = False + self.addon.needs_human_review_other = False + self.addon.is_pre_review_version = False + self.addon.has_developer_reply = False assert get_flags(self.addon, self.addon.current_version) == [] for attribute, title in ( - ('cinder', 'Abuse report forwarded from Cinder present'), - ('abuse', 'Abuse report present'), - ('appeal', 'Appeal on decision present'), + ( + 'needs_human_review_from_cinder', + 'Abuse report forwarded from Cinder present', + ), + ('needs_human_review_from_abuse', 'Abuse report present'), + ('needs_human_review_from_appeal', 'Appeal on decision present'), + ('needs_human_review_other', 'Other NeedsHumanReview flag set'), + ('is_pre_review_version', 'Version(s) awaiting pre-approval review'), + ('has_developer_reply', 'Outstanding developer reply'), ): self.addon.needs_human_review_from_abuse = False self.addon.needs_human_review_from_cinder = False self.addon.needs_human_review_from_appeal = False - attribute = 'needs_human_review_from_' + attribute + self.addon.needs_human_review_other = False + self.addon.is_pre_review_version = False + self.addon.has_developer_reply = False setattr(self.addon, attribute, True) assert get_flags(self.addon, self.addon.current_version) == [ (attribute.replace('_', '-'), title) ] + def test_all_due_due_reasons_exposed_as_flags(self): + assert set(Version.objects.get_due_date_reason_q_objects().keys()).issubset( + {flag for flag, _ in VIEW_QUEUE_FLAGS} + ) + class TestNeedsHumanReview(TestCase): def setUp(self): diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 87f8b3578c..cd65fecd94 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -1,3 +1,5 @@ +import functools +import operator import os import re from base64 import b64encode @@ -176,17 +178,31 @@ class VersionManager(ManagerBase): """Returns a queryset filtered to versions that should have a due date set. If `negate=True` the queryset will contain versions that should not have a due date instead.""" + method = getattr(self, 'exclude' if negate else 'filter') + return ( + method( + functools.reduce( + operator.or_, self.get_due_date_reason_q_objects().values() + ) + ) + .using('default') + .distinct() + ) + + def get_due_date_reason_q_objects(cls): + """Class method that returns a dict with all the Q objects needed to determine + if a version should_have_due_date is true as values, and the annotation names + for each reason as values.""" from olympia.reviewers.models import NeedsHumanReview - method = getattr(self, 'exclude' if negate else 'filter') requires_manual_listed_approval_and_is_listed = Q( Q(addon__reviewerflags__auto_approval_disabled=True) | Q(addon__reviewerflags__auto_approval_disabled_until_next_approval=True) | Q(addon__reviewerflags__auto_approval_delayed_until__isnull=False) | Q( - addon__promotedaddon__group_id__in=( + addon__promotedaddon__group_id__in=[ g.id for g in PROMOTED_GROUPS if g.listed_pre_review - ) + ] ) | Q(addon__type__in=amo.GROUP_TYPE_THEME), addon__status__in=(amo.VALID_ADDON_STATUSES), @@ -201,9 +217,9 @@ class VersionManager(ManagerBase): addon__reviewerflags__auto_approval_delayed_until_unlisted__isnull=False ) | Q( - addon__promotedaddon__group_id__in=( + addon__promotedaddon__group_id__in=[ g.id for g in PROMOTED_GROUPS if g.unlisted_pre_review - ) + ] ) | Q(addon__type__in=amo.GROUP_TYPE_THEME), channel=amo.CHANNEL_UNLISTED, @@ -232,11 +248,33 @@ class VersionManager(ManagerBase): needshumanreview__is_active=True, needshumanreview__reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY, ) - return ( - method(is_needs_human_review | is_pre_review_version | has_developer_reply) - .using('default') - .distinct() - ) + return { + 'needs_human_review_from_cinder': Q( + is_needs_human_review, + needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, + ), + 'needs_human_review_from_abuse': Q( + is_needs_human_review, + needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, + ), + 'needs_human_review_from_appeal': Q( + is_needs_human_review, + needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, + ), + 'needs_human_review_other': Q( + is_needs_human_review, + ~Q( + needshumanreview__reason__in=( + NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION.value, + NeedsHumanReview.REASONS.CINDER_ESCALATION.value, + NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL.value, + ) + ) + & ~has_developer_reply, + ), + 'is_pre_review_version': is_pre_review_version, + 'has_developer_reply': has_developer_reply, + } class UnfilteredVersionManagerForRelations(VersionManager): diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index 8b5b8fdd5e..edefc3167a 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -393,6 +393,112 @@ class TestVersionManager(TestCase): version_preview = VersionPreview.objects.get(pk=version_preview.pk) assert version_preview.version == version + def test_should_have_due_date(self): + user_factory(pk=settings.TASK_USER_ID) + addon_kws = { + 'file_kw': {'is_signed': True, 'status': amo.STATUS_AWAITING_REVIEW} + } + + addon_factory(**addon_kws) # no due_date + + other_nhr = addon_factory(**addon_kws).current_version + # having the needs_human_review flag means a due dute is needed + NeedsHumanReview.objects.create( + version=other_nhr, reason=NeedsHumanReview.REASONS.SCANNER_ACTION + ) + + # Or if it's in a pre-review promoted group it will. + recommended = addon_factory(**addon_kws).current_version + PromotedAddon.objects.create(addon=recommended.addon, group_id=RECOMMENDED.id) + + # And not if it's a non-pre-review group + PromotedAddon.objects.create( + addon=addon_factory(**addon_kws), group_id=STRATEGIC.id + ) + + # A disabled version with a developer reply + developer_reply = addon_factory( + file_kw={'is_signed': False, 'status': amo.STATUS_DISABLED} + ).versions.all()[0] + NeedsHumanReview.objects.create( + version=developer_reply, reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY + ) + + # dsa related needs_human_review flag means due dates are needed also + abuse_nhr = addon_factory(**addon_kws).current_version + NeedsHumanReview.objects.create( + version=abuse_nhr, reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION + ) + appeal_nhr = addon_factory(**addon_kws).current_version + NeedsHumanReview.objects.create( + version=appeal_nhr, reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL + ) + # throw in an inactive NHR that should be ignored + NeedsHumanReview.objects.create( + version=appeal_nhr, + reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, + is_active=False, + ) + + # And a version with multiple reasons + multiple = addon_factory(**addon_kws).current_version + PromotedAddon.objects.create(addon=multiple.addon, group_id=LINE.id) + NeedsHumanReview.objects.create( + version=multiple, reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY + ) + NeedsHumanReview.objects.create( + version=multiple, reason=NeedsHumanReview.REASONS.CINDER_ESCALATION + ) + + qs = Version.objects.should_have_due_date().order_by('id') + assert list(qs) == [ + # absent addon with nothing special set + other_nhr, + recommended, + # absent promoted but not prereview addon + developer_reply, + abuse_nhr, + appeal_nhr, + multiple, + ] + + def test_get_due_date_reason_q_objects(self): + self.test_should_have_due_date() # to set up the Versions + + qs = Version.objects.all().order_by('id') + # See test_should_have_due_date for order + ( + _, # addon with nothing special set + other_nhr, + recommended, + _, # promoted but not prereview addon + developer_reply, + abuse_nhr, + appeal_nhr, + multiple, + ) = list(qs) + + q_objects = Version.objects.get_due_date_reason_q_objects() + method = Version.objects.filter + + assert list(method(q_objects['needs_human_review_from_cinder'])) == [multiple] + + assert list(method(q_objects['needs_human_review_from_abuse'])) == [abuse_nhr] + + assert list(method(q_objects['needs_human_review_from_appeal'])) == [appeal_nhr] + + assert list(method(q_objects['needs_human_review_other'])) == [other_nhr] + + assert list(method(q_objects['is_pre_review_version'])) == [ + multiple, + recommended, + ] + + assert list(method(q_objects['has_developer_reply'])) == [ + multiple, + developer_reply, + ] + class TestVersion(AMOPaths, TestCase): fixtures = ['base/addon_3615', 'base/admin'] diff --git a/static/css/zamboni/reviewers.less b/static/css/zamboni/reviewers.less index a38e56859a..58bdeb841a 100644 --- a/static/css/zamboni/reviewers.less +++ b/static/css/zamboni/reviewers.less @@ -36,8 +36,9 @@ .ed-sprite-needs-admin-content-review { background-position: 0 -128px; } .ed-sprite-needs-admin-code-review { background-position: 0 -128px; } .ed-sprite-needs-admin-theme-review { background-position: 0 -128px; } -.ed-sprite-reviewer { background-position: 0 -144px; } -.ed-sprite-info { background-position: 0 -160px; } +.ed-sprite-has-developer-reply { background-position: 0 -144px; } +.ed-sprite-needs-human-review-other { background-position: 0 -160px; } +.ed-sprite-is-pre-review-version { background-position: 0 -160px; } .ed-sprite-sources-provided { background-position: 0 -192px; } .ed-sprite-webextension { background-position: 0 -208px; } .ed-sprite-expired-info { background-position: 0 -272px; }