diff --git a/src/olympia/addons/models.py b/src/olympia/addons/models.py index c645f4a95a0f..959122486904 100644 --- a/src/olympia/addons/models.py +++ b/src/olympia/addons/models.py @@ -307,6 +307,7 @@ def get_queryset_for_pending_queues( theme_review=False, show_temporarily_delayed=True, show_only_upcoming=False, + due_date_reasons_choices=None, ): filters = { 'type__in': amo.GROUP_TYPE_THEME if theme_review else amo.GROUP_TYPE_ADDON, @@ -359,13 +360,29 @@ def get_queryset_for_pending_queues( & ~Q(**{listed_delay_flag_field: datetime.max}) ) ) + version_subqs = versions_due_qs.all() + if due_date_reasons_choices: + versions_filter = Q( + versions__needshumanreview__reason__in=due_date_reasons_choices.values + ) + version_subqs = version_subqs.filter( + needshumanreview__reason__in=due_date_reasons_choices.values + ) + else: + versions_filter = None qs = ( qs.filter(**filters) .annotate( - first_version_due_date=Min('versions__due_date'), - first_version_id=Subquery( - versions_due_qs.filter(addon=OuterRef('pk')).values('pk')[:1] + # We need both first_version_due_date and first_version_id to + # be set and have the same behavior. The former is used for + # grouping (hence the Min()) and provides a way for callers to + # sort this queryset by due date, the latter to filter add-ons + # matching the reasons we care about and to instantiate the + # right Version in first_pending_version_transformer(). + first_version_due_date=Min( + 'versions__due_date', filter=versions_filter ), + first_version_id=Subquery(version_subqs.values('pk')[:1]), **{ name: Exists(versions_due_qs.filter(q)) for name, q in ( @@ -768,6 +785,21 @@ def _set_needs_human_review_on_latest_signed_version( version.reset_due_date(due_date) return version + def versions_triggering_needs_human_review_inheritance(self, channel): + """Return queryset of Versions belonging to this addon in the specified + channel that should be considered for due date and NeedsHumanReview + inheritance.""" + from olympia.reviewers.models import NeedsHumanReview + + reasons_triggering_inheritance = set( + NeedsHumanReview.REASONS.values.keys() + ) - set(NeedsHumanReview.REASONS.NO_DUE_DATE_INHERITANCE.values.keys()) + return self.versions(manager='unfiltered_for_relations').filter( + channel=channel, + needshumanreview__is_active=True, + needshumanreview__reason__in=reasons_triggering_inheritance, + ) + @property def is_guid_denied(self): return DeniedGuid.objects.filter(guid=self.guid).exists() diff --git a/src/olympia/addons/tests/test_models.py b/src/olympia/addons/tests/test_models.py index cc67c43aa319..dfdbf5201f6c 100644 --- a/src/olympia/addons/tests/test_models.py +++ b/src/olympia/addons/tests/test_models.py @@ -2397,6 +2397,46 @@ def test_set_needs_human_review_on_latest_versions_even_deleted(self): == NeedsHumanReview.REASONS.UNKNOWN ) + def test_versions_triggering_needs_human_review_inheritance(self): + addon = Addon.objects.get(id=3615) + version = addon.current_version + version.needshumanreview_set.create(reason=NeedsHumanReview.REASONS.UNKNOWN) + version2 = version_factory(addon=addon) + unlisted_version = version_factory(addon=addon, channel=amo.CHANNEL_UNLISTED) + unlisted_version.needshumanreview_set.create( + reason=NeedsHumanReview.REASONS.UNKNOWN + ) + assert set( + addon.versions_triggering_needs_human_review_inheritance(amo.CHANNEL_LISTED) + ) == {version} + assert set( + addon.versions_triggering_needs_human_review_inheritance( + amo.CHANNEL_UNLISTED + ) + ) == {unlisted_version} + + # Adding any of those NHR should not matter. + for reason_value, _ in NeedsHumanReview.REASONS.NO_DUE_DATE_INHERITANCE: + version2.needshumanreview_set.create(reason=reason_value) + + assert set( + addon.versions_triggering_needs_human_review_inheritance(amo.CHANNEL_LISTED) + ) == {version} + + # Adding any other NHR should. + nhr = version2.needshumanreview_set.create( + reason=NeedsHumanReview.REASONS.AUTO_APPROVAL_DISABLED + ) + assert set( + addon.versions_triggering_needs_human_review_inheritance(amo.CHANNEL_LISTED) + ) == {version, version2} + + # An inactive NHR should not trigger inheritance. + nhr.update(is_active=False) + assert set( + addon.versions_triggering_needs_human_review_inheritance(amo.CHANNEL_LISTED) + ) == {version} + def test_update_all_due_dates(self): addon = Addon.objects.get(id=3615) versions_that_should_have_due_date = [ @@ -3779,6 +3819,52 @@ def test_pending_queue_needs_human_scanner_action(self): NeedsHumanReview.REASONS.SCANNER_ACTION, 'needs_human_review_scanner_action' ) + def test_get_queryset_for_pending_queues_for_specific_due_date_reasons(self): + expected_addons = [ + version_factory( + addon=addon_factory( + version_kw={ + 'needshumanreview_kw': { + 'reason': NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION + }, + 'due_date': self.days_ago(48), + 'version': '0.1', + } + ), + needshumanreview_kw={ + 'reason': NeedsHumanReview.REASONS.AUTO_APPROVAL_DISABLED + }, + due_date=self.days_ago(15), + version='0.2', + ).addon, + addon_factory( + version_kw={ + 'needshumanreview_kw': { + 'reason': NeedsHumanReview.REASONS.SCANNER_ACTION + }, + 'due_date': self.days_ago(16), + 'version': '666.0', + } + ), + ] + addon_factory( + version_kw={ + 'needshumanreview_kw': { + 'reason': NeedsHumanReview.REASONS.DEVELOPER_REPLY + }, + 'due_date': self.days_ago(23), + } + ) # Should not show up + addons = Addon.objects.get_queryset_for_pending_queues( + due_date_reasons_choices=NeedsHumanReview.REASONS.extract_subset( + 'AUTO_APPROVAL_DISABLED', 'SCANNER_ACTION' + ) + ) + expected_version = expected_addons[0].versions.get(version='0.2') + assert addons[0].first_version_id == expected_version.pk + assert addons[0].first_pending_version == expected_version + assert addons[0].first_version_due_date == expected_version.due_date + 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 512d521c4daa..22beb983fd77 100644 --- a/src/olympia/reviewers/models.py +++ b/src/olympia/reviewers/models.py @@ -933,6 +933,13 @@ class NeedsHumanReview(ModelBase): 'SECOND_LEVEL_REQUEUE', ), ) + REASONS.add_subset( + 'NO_DUE_DATE_INHERITANCE', + ( + 'ABUSE_ADDON_VIOLATION', + 'CINDER_ESCALATION', + ), + ) reason = models.SmallIntegerField( default=0, choices=REASONS.choices, editable=False diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 622c683971e9..a2ced0729267 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -1762,6 +1762,34 @@ def test_due_date_reason_with_two_filters(self): ) self._test_results() + def test_different_due_dates_correct_one_is_shown_when_filtering(self): + self.url += '?due_date_reasons=needs_human_review_developer_reply' + self.expected_addons = self.get_expected_addons_by_names( + ['Pending One', 'Public'], + ) + # Create a NHR that would be filtered out and not inherited on + # "Pending One" first version with an old due date, then make a new + # version with a NHR that we are going to filter for, and a different + # due date. That new version and due date should be the ones shown. + self.addons['Pending One'].current_version.needshumanreview_set.create( + reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION + ) + self.addons['Pending One'].current_version.update(due_date=self.days_ago(42)) + self.expected_versions = self.get_expected_versions(self.expected_addons) + self.expected_versions[self.addons['Pending One']] = version_factory( + addon=self.addons['Pending One'], version='0.2' + ) + for version in self.expected_versions.values(): + version.needshumanreview_set.create( + reason=NeedsHumanReview.REASONS.DEVELOPER_REPLY + ) + version.update(due_date=self.days_ago(1)) + + self.expected_versions[self.addons['Pending One']] + doc = self._test_results() + rows = doc('#addon-queue tr.addon-row') + assert '1\xa0day ago' in rows[0].text_content() + class TestThemeQueue(QueueTest): def setUp(self): diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 1f2608635fe7..00edc29485cf 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -115,7 +115,14 @@ class Meta(AddonQueueTable.Meta): orderable = True @classmethod - def get_queryset(cls, request, *, upcoming_due_date_focus=False, **kwargs): + def get_queryset( + cls, + request, + *, + upcoming_due_date_focus=False, + due_date_reasons_choices=None, + **kwargs, + ): if request is None: admin_reviewer = True show_temporarily_delayed = True @@ -132,6 +139,7 @@ def get_queryset(cls, request, *, upcoming_due_date_focus=False, **kwargs): admin_reviewer=admin_reviewer, show_temporarily_delayed=show_temporarily_delayed, show_only_upcoming=show_only_upcoming, + due_date_reasons_choices=due_date_reasons_choices, ) def get_version(self, record): @@ -172,10 +180,12 @@ class Meta(PendingManualApprovalQueueTable.Meta): ) @classmethod - def get_queryset(cls, request, **kwargs): + def get_queryset(cls, request, due_date_reasons_choices=None, **kwargs): admin_reviewer = is_admin_reviewer(request.user) if request else True return Addon.objects.get_queryset_for_pending_queues( - admin_reviewer=admin_reviewer, theme_review=True + admin_reviewer=admin_reviewer, + theme_review=True, + due_date_reasons_choices=due_date_reasons_choices, ) diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py index 447cc79739a2..308e6a9b2269 100644 --- a/src/olympia/reviewers/views.py +++ b/src/olympia/reviewers/views.py @@ -1,5 +1,3 @@ -import functools -import operator from collections import OrderedDict from datetime import date, datetime from urllib.parse import urljoin @@ -316,7 +314,6 @@ def queue(request, tab): @permission_or_tools_listed_view_required(TableObj.permission) def _queue(request, tab): - qs = TableObj.get_queryset(request=request, upcoming_due_date_focus=True) params = {} order_by = request.GET.get('sort') if order_by is None and hasattr(TableObj, 'default_order_by'): @@ -326,11 +323,21 @@ def _queue(request, tab): filter_form = ReviewQueueFilter( request.GET if 'due_date_reasons' in request.GET else None ) - if filter_form.is_valid() and ( - due_date_reasons := filter_form.cleaned_data['due_date_reasons'] - ): - filters = [Q(**{reason: True}) for reason in due_date_reasons] - qs = qs.filter(functools.reduce(operator.or_, filters)) + due_date_reasons_choices = None + if filter_form.is_valid(): + # Build a choices subset from the submitted reasons. + due_date_reasons_choices = NeedsHumanReview.REASONS.__class__( + *( + entry + for entry in NeedsHumanReview.REASONS.entries + if entry.annotation in filter_form.cleaned_data['due_date_reasons'] + ) + ) + qs = TableObj.get_queryset( + request=request, + upcoming_due_date_focus=True, + due_date_reasons_choices=due_date_reasons_choices, + ) table = TableObj(data=qs, **params) per_page = request.GET.get('per_page', REVIEWS_PER_PAGE) try: diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 41ad2ca8d010..73971862dec1 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -430,9 +430,7 @@ def from_upload( ) previous_version_had_needs_human_review = ( - addon.versions(manager='unfiltered_for_relations') - .filter(channel=channel, needshumanreview__is_active=True) - .exists() + addon.versions_triggering_needs_human_review_inheritance(channel).exists() ) version = cls.objects.create( @@ -1006,19 +1004,20 @@ def reset_due_date(self, due_date=None, should_have_due_date=None): def generate_due_date(self): """ (Re)Generate a due date for this version, inheriting from the earliest - due date possible from any other version in the same channel if one - exists, but only if the result would be at at earlier date than - the default/existing one on the instance. + due date possible from any other version with reasons that can trigger + inheritance in the same channel if one exists, but only if the result + would be at at earlier date than the default/existing one on the + instance. """ qs = ( - Version.unfiltered.filter(addon=self.addon, channel=self.channel) + self.addon.versions_triggering_needs_human_review_inheritance(self.channel) .exclude(due_date=None) .exclude(id=self.pk) .values_list('due_date', flat=True) .order_by('-due_date') ) - standard_or_existing_due_date = self.due_date or get_review_due_date() due_date = qs.first() + standard_or_existing_due_date = self.due_date or get_review_due_date() if not due_date or due_date > standard_or_existing_due_date: due_date = standard_or_existing_due_date return due_date diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index 38b9d1f26955..25b27c080727 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -2419,6 +2419,45 @@ def test_dont_inherit_needs_human_review_from_different_channel(self): assert not upload_version.due_date assert upload_version.needshumanreview_set.count() == 0 + def test_dont_inherit_due_date_or_nhr_for_some_specific_reasons(self): + # Some NeedsHumanReview reasons don't pass their due date through + # inheritance if they are the only reason a version had a due date. + old_version = self.addon.current_version + old_version.needshumanreview_set.create( + reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION + ) + assert old_version.due_date + old_version.update(due_date=self.days_ago(1)) + new_version = Version.from_upload( + self.upload, + self.addon, + amo.CHANNEL_LISTED, + selected_apps=[self.selected_app], + parsed_data=self.dummy_parsed_data, + ) + # The version doesn't gain a NHR for INHERITANCE + assert new_version.needshumanreview_set.count() == 0 + # If it gains a NHR, it doesn't inherit the due date since the old + # version only needs human review for one of the reasons that does not + # trigger inheritance. + new_version.needshumanreview_set.create(reason=NeedsHumanReview.REASONS.UNKNOWN) + assert new_version.due_date + assert new_version.due_date > old_version.due_date + # Forcing re-generation doesn't change anything. + assert new_version.generate_due_date() > old_version.due_date + + # The above remains true for CINDER_ESCALATION which is another reason + # that doesn't trigger due date inheritance. + old_version.needshumanreview_set.create( + reason=NeedsHumanReview.REASONS.CINDER_ESCALATION + ) + assert new_version.generate_due_date() > old_version.due_date + + # If we add another reason that *does* trigger inheritance to the old + # version, suddenly we will inherit its due date. + old_version.needshumanreview_set.create(reason=NeedsHumanReview.REASONS.UNKNOWN) + assert new_version.generate_due_date() == old_version.due_date + def test_set_version_to_customs_scanners_result(self): self.create_switch('enable-customs', active=True) scanners_result = ScannerResult.objects.create(