Don't inherit due date and/or NeedsHumanReview for ABUSE_ADDON_VIOLATION/CINDER_ESCALATION (#23343)

* Don't inherit due date for ABUSE_ADDON_VIOLATION/CINDER_ESCALATION

*Only* for those reasons: as soon as a version has more reasons
for needing human review its due date can be inherited from again.

* Also don't inherit NHR itself

* Implement filtering for reasons inside get_queryset_for_pending_queues()

This fixes due date display/sorting in the review queue when there are multiple
different due dates set and some would be filtered out

Author: diox

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()

src/olympia/addons/tests/test_models.py

@@ -2483,6 +2483,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 = [
@@ -3865,6 +3905,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(

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

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):

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,
         )
 
 

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:

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

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(