diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py index 89bd0a233c85..f17fdb2e7895 100644 --- a/src/olympia/addons/models.py +++ b/src/olympia/addons/models.py @@ -319,8 +319,7 @@ def get_queryset_for_pending_queues( select_related_fields_for_listed=False, ) versions_due_qs = ( - Version.unfiltered.annotate_due_date_reasons() - .filter(due_date__isnull=False, addon=OuterRef('pk')) + Version.unfiltered.filter(due_date__isnull=False, addon=OuterRef('pk')) .no_transforms() .order_by('due_date') ) @@ -366,15 +365,10 @@ def get_queryset_for_pending_queues( first_version_id=Subquery( versions_due_qs.filter(addon=OuterRef('pk')).values('pk')[:1] ), - needs_human_review_from_cinder=Exists( - versions_due_qs.filter(needs_human_review_from_cinder=True) - ), - needs_human_review_from_abuse=Exists( - versions_due_qs.filter(needs_human_review_from_abuse=True) - ), - needs_human_review_from_appeal=Exists( - versions_due_qs.filter(needs_human_review_from_appeal=True) - ), + **{ + name: Exists(versions_due_qs.filter(q)) + for name, q in Version.unfiltered.get_due_date_reason_qs().items() + }, ) .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 cfae53a7a44f..3fad538fc343 100644 --- a/src/olympia/addons/tests/test_models.py +++ b/src/olympia/addons/tests/test_models.py @@ -3600,9 +3600,14 @@ def _test_pending_queue_needs_human_review_from(self, reason, annotated_field): 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 @@ def test_pending_queue_needs_human_review_from_cinder(self): 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/versions/models.py b/src/olympia/versions/models.py index 3bdabde3846a..e6601ce2403a 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 @@ -9,7 +11,7 @@ from django.core.exceptions import ObjectDoesNotExist from django.core.files.storage import default_storage as storage from django.db import models, transaction -from django.db.models import Case, Exists, F, OuterRef, Q, When +from django.db.models import Case, F, Q, When from django.dispatch import receiver from django.urls import reverse from django.utils.encoding import force_str @@ -176,23 +178,16 @@ def should_have_due_date(self, negate=False): """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.annotate_due_date_reasons(), 'exclude' if negate else 'filter' - ) + method = getattr(self, 'exclude' if negate else 'filter') return ( method( - Q(needs_human_review_from_cinder=True) - | Q(needs_human_review_from_abuse=True) - | Q(needs_human_review_from_appeal=True) - | Q(needs_human_review_other=True) - | Q(is_pre_review_version=True) - | Q(has_developer_reply=True) + functools.reduce(operator.or_, self.get_due_date_reason_qs().values()) ) .using('default') .distinct() ) - def annotate_due_date_reasons(self): + def get_due_date_reason_qs(cls): from olympia.reviewers.models import NeedsHumanReview requires_manual_listed_approval_and_is_listed = Q( @@ -241,7 +236,6 @@ def annotate_due_date_reasons(self): is_needs_human_review = Q( ~Q(file__status=amo.STATUS_DISABLED) | Q(file__is_signed=True), needshumanreview__is_active=True, - id=OuterRef('pk'), ) # Developer replies always trigger a due date even if the version has # been disabled and is not signed. @@ -249,44 +243,32 @@ def annotate_due_date_reasons(self): needshumanreview__is_active=True, needshumanreview__reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY, ) - return self.annotate( - needs_human_review_from_cinder=Exists( - self.filter( - is_needs_human_review, - needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, - ) - ), - needs_human_review_from_abuse=Exists( - self.filter( - is_needs_human_review, - needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, - ) + return { + 'needs_human_review_from_cinder': Q( + is_needs_human_review, + needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, ), - needs_human_review_from_appeal=Exists( - self.filter( - is_needs_human_review, - needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, - ) + 'needs_human_review_from_abuse': Q( + is_needs_human_review, + needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, ), - needs_human_review_other=Exists( - self.filter( - 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, - ) - ), - ) + 'needs_human_review_from_appeal': Q( + is_needs_human_review, + needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, ), - is_pre_review_version=Exists( - self.filter(is_pre_review_version, id=OuterRef('pk')) - ), - has_developer_reply=models.Exists( - self.filter(has_developer_reply, id=OuterRef('pk')) + '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, + ) + ), ), - ) + 'is_pre_review_version': is_pre_review_version, + 'has_developer_reply': has_developer_reply, + } class UnfilteredVersionManagerForRelations(VersionManager): @@ -1018,8 +1000,7 @@ def reset_due_date(self, due_date=None): If due_date is None then a new due date will only be set if the version doesn't already have one; otherwise the provided due_date will be be used to overwrite - any value. - """ + any value.""" if self.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: @@ -1028,7 +1009,7 @@ def reset_due_date(self, due_date=None): log.info('Version %r (%s) due_date set to %s', self, self.id, due_date) self.update(due_date=due_date, _signal=False) elif self.due_date: - # otherwise it shouldn't have a due_date so clear it + # 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 ) diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index 654ced601198..876820b01002 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -462,77 +462,45 @@ def test_should_have_due_date(self): multiple, ] - def test_annotate_due_date_reasons(self): + def test_get_due_date_reason_qs(self): self.test_should_have_due_date() # to set up the Versions - qs = Version.objects.annotate_due_date_reasons().order_by('id') + qs = Version.objects.all().order_by('id') # See test_should_have_due_date for order ( - random_addon, + _, # addon with nothing special set other_nhr, recommended, - strategic, + _, # promoted but not prereview addon developer_reply, abuse_nhr, appeal_nhr, multiple, ) = list(qs) - assert random_addon.needs_human_review_from_cinder is False - assert random_addon.needs_human_review_from_abuse is False - assert random_addon.needs_human_review_from_appeal is False - assert random_addon.needs_human_review_other is False - assert random_addon.is_pre_review_version is False - assert random_addon.has_developer_reply is False - - assert other_nhr.needs_human_review_from_cinder is False - assert other_nhr.needs_human_review_from_abuse is False - assert other_nhr.needs_human_review_from_appeal is False - assert other_nhr.needs_human_review_other is True - assert other_nhr.is_pre_review_version is False - assert other_nhr.has_developer_reply is False - - assert recommended.needs_human_review_from_cinder is False - assert recommended.needs_human_review_from_abuse is False - assert recommended.needs_human_review_from_appeal is False - assert recommended.needs_human_review_other is False - assert recommended.is_pre_review_version is True - assert recommended.has_developer_reply is False - - assert strategic.needs_human_review_from_cinder is False - assert strategic.needs_human_review_from_abuse is False - assert strategic.needs_human_review_from_appeal is False - assert strategic.needs_human_review_other is False - assert strategic.is_pre_review_version is False - assert strategic.has_developer_reply is False - - assert developer_reply.needs_human_review_from_cinder is False - assert developer_reply.needs_human_review_from_abuse is False - assert developer_reply.needs_human_review_from_appeal is False - assert developer_reply.needs_human_review_other is False - assert developer_reply.is_pre_review_version is False - assert developer_reply.has_developer_reply is True - - assert abuse_nhr.needs_human_review_from_cinder is False - assert abuse_nhr.needs_human_review_from_abuse is True - assert abuse_nhr.needs_human_review_from_appeal is False - assert abuse_nhr.needs_human_review_other is False - assert abuse_nhr.is_pre_review_version is False - assert abuse_nhr.has_developer_reply is False - - assert appeal_nhr.needs_human_review_from_cinder is False - assert appeal_nhr.needs_human_review_from_abuse is False - assert appeal_nhr.needs_human_review_from_appeal is True - assert appeal_nhr.needs_human_review_other is False - assert appeal_nhr.is_pre_review_version is False - assert appeal_nhr.has_developer_reply is False - - assert multiple.needs_human_review_from_cinder is True - assert multiple.needs_human_review_from_abuse is False - assert multiple.needs_human_review_from_appeal is False - assert multiple.needs_human_review_other is False - assert multiple.is_pre_review_version is True - assert multiple.has_developer_reply is True + q_objects = Version.objects.get_due_date_reason_qs() + 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'])) == [ + multiple, + 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):