allow unsigned versions to be flagged for NHR (#22857)

src/olympia/abuse/cinder.py

@@ -494,9 +494,7 @@ class CinderAddonHandledByReviewers(CinderAddon):
         # No special appeal queue for reviewer handled jobs
         return self.queue
 
-    def flag_for_human_review(
-        self, *, reported_versions, appeal=False, forwarded=False
-    ):
+    def flag_for_human_review(self, *, related_versions, appeal=False, forwarded=False):
         from olympia.reviewers.models import NeedsHumanReview
 
         waffle_switch_name = (
@@ -525,62 +523,64 @@ class CinderAddonHandledByReviewers(CinderAddon):
         version_objs = (
             set(
                 self.addon.versions(manager='unfiltered_for_relations')
-                .filter(version__in=reported_versions)
-                .exclude(
-                    needshumanreview__reason=reason,
-                    needshumanreview__is_active=True,
-                )
+                .filter(version__in=related_versions)
                 .no_transforms()
             )
-            if reported_versions
+            if related_versions
             else set()
         )
-        nhr_object = None
-        # We need custom save() and post_save to be triggered, so we can't
-        # optimize this via bulk_create().
-        for version in version_objs:
-            nhr_object = NeedsHumanReview(
-                version=version, reason=reason, is_active=True
-            )
-            nhr_object.save(_no_automatic_activity_log=True)
         # If we have more versions specified than versions we flagged, flag latest
         # to be safe. (Either because there was an unknown version, or a None)
-        if len(version_objs) != len(reported_versions) or len(reported_versions) == 0:
-            version_objs = version_objs.union(
-                self.addon.set_needs_human_review_on_latest_versions(
-                    reason=reason,
-                    ignore_reviewed=False,
-                    unique_reason=True,
-                    skip_activity_log=True,
-                )
+        if len(version_objs) != len(related_versions) or len(related_versions) == 0:
+            version_objs.add(
+                self.addon.versions(manager='unfiltered_for_relations')
+                .filter(channel=amo.CHANNEL_LISTED)
+                .no_transforms()
+                .first()
             )
-        if version_objs:
-            version_objs = sorted(version_objs, key=lambda v: v.id)
-            # we just need this for get_reason_display
-            nhr_object = nhr_object or NeedsHumanReview(
-                version=version_objs[-1],
-                reason=reason,
-                is_active=True,
+        version_objs = sorted(version_objs, key=lambda v: v.id)
+        log.debug(
+            'Found %s versions potentially needing NHR [%s]',
+            len(version_objs),
+            ','.join(v.version for v in version_objs),
+        )
+        existing_nhrs = {
+            nhr.version
+            for nhr in NeedsHumanReview.objects.filter(
+                version__in=version_objs, is_active=True, reason=reason
             )
+        }
+        # We need custom save() and post_save to be triggered, so we can't
+        # optimize this via bulk_create().
+        nhr = None
+        for version in version_objs:
+            if version in existing_nhrs:
+                # if there's already an active NHR for this reason, don't duplicate it
+                continue
+            nhr = NeedsHumanReview(version=version, reason=reason, is_active=True)
+            nhr.save(_no_automatic_activity_log=True)
+
+        if nhr:
             activity.log_create(
                 amo.LOG.NEEDS_HUMAN_REVIEW_CINDER,
                 *version_objs,
-                details={'comments': nhr_object.get_reason_display()},
+                details={'comments': nhr.get_reason_display()},
                 user=core.get_user() or get_task_user(),
             )
 
     def post_report(self, job):
         if not job.is_appeal:
-            reported_version = self.version_string
             self.flag_for_human_review(
-                reported_versions={reported_version}, appeal=False
+                related_versions={self.version_string}, appeal=False
             )
         # If our report was added to an appeal job (i.e. an appeal was ongoing,
         # and a report was made against the add-on), don't flag the add-on for
         # human review again: we should already have one because of the appeal.
 
     def appeal(self, *args, **kwargs):
-        self.flag_for_human_review(reported_versions=set(), appeal=True)
+        related_versions = {self.version_string} if self.version_string else set()
+        # TODO: use the version(s) we took action on, rather than the versions reported
+        self.flag_for_human_review(related_versions=related_versions, appeal=True)
         return super().appeal(*args, **kwargs)
 
     def workflow_recreate(self, *, job):
@@ -592,7 +592,7 @@ class CinderAddonHandledByReviewers(CinderAddon):
         reported_versions = set(
             job.abusereport_set.values_list('addon_version', flat=True)
         )
-        self.flag_for_human_review(reported_versions=reported_versions, forwarded=True)
+        self.flag_for_human_review(related_versions=reported_versions, forwarded=True)
 
 
 class CinderReport(CinderEntity):

src/olympia/abuse/models.py

@@ -1172,7 +1172,9 @@ class ContentDecision(ModelBase):
             raise CantBeAppealed
 
         entity_helper = CinderJob.get_entity_helper(
-            self.target, resolved_in_reviewer_tools=resolvable_in_reviewer_tools
+            self.target,
+            resolved_in_reviewer_tools=resolvable_in_reviewer_tools,
+            addon_version_string=getattr(abuse_report, 'addon_version', None),
         )
         appeal_id = entity_helper.appeal(
             decision_cinder_id=self.cinder_id,
@@ -1201,8 +1203,8 @@ class ContentDecision(ModelBase):
         entity_helper,
         appealed_action=None,
     ):
-        """Calling this method calls cinder to create a decision, or notfies the content
-        owner/reporter by email, or both.
+        """Calling this method calls Cinder to create a decision, or notifies the
+        content owner/reporter by email, or both.
 
         If a decision is created in cinder the instance will be saved, along with
         relevant policies; if a cinder decision isn't need the instance won't be saved.

src/olympia/abuse/tests/test_cinder.py

@@ -1125,7 +1125,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
         # Make sure this is testing the case where no user is set (we fall back
         # to the task user).
         assert core.get_user() is None
-        addon.current_version.file.update(is_signed=True)
         # Trigger switch_is_active to ensure it's cached to make db query
         # count more predictable.
         waffle.switch_is_active('dsa-abuse-reports-review')
@@ -1134,25 +1133,24 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
         assert addon.current_version.needshumanreview_set.count() == 0
         job = CinderJob.objects.create(job_id='1234-xyz')
         cinder_instance = self.CinderClass(addon)
-        with self.assertNumQueries(13):
+        with self.assertNumQueries(11):
             # - 1 Fetch Cinder Decision
             # - 2 Fetch Version
-            # - 3 Fetch Translations for that Version
-            # - 4 Fetch NeedsHumanReview
-            # - 5 Create NeedsHumanReview
-            # - 6 Fetch NeedsHumanReview
-            # - 7 Update due date on Versions
-            # - 8 Fetch Latest signed Version
-            # - 9 Fetch task user
-            # - 10 Create ActivityLog
-            # - 11 Create ActivityLogComment
-            # - 12 Update ActivityLogComment
-            # - 13 Create VersionLog
+            # - 3 Fetch NeedsHumanReview
+            # - 4 Create NeedsHumanReview
+            # - 5 Query if due_date is needed for version
+            # - 6 Update due date on Version
+            # - 7 Fetch task user
+            # - 8 Create ActivityLog
+            # - 9 Update ActivityLog
+            # - 10 Create ActivityLogComment
+            # - 11 Create VersionLog
             cinder_instance.post_report(job)
         assert (
             addon.current_version.needshumanreview_set.get().reason
             == NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION
         )
+        assert addon.current_version.reload().due_date
         assert ActivityLog.objects.for_versions(addon.current_version).filter(
             action=amo.LOG.NEEDS_HUMAN_REVIEW_CINDER.id
         )
@@ -1160,7 +1158,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
     @override_switch('dsa-abuse-reports-review', active=False)
     def test_report_waffle_switch_off(self):
         addon = self._create_dummy_target()
-        addon.current_version.file.update(is_signed=True)
         # Trigger switch_is_active to ensure it's cached to make db query
         # count more predictable.
         waffle.switch_is_active('dsa-abuse-reports-review')
@@ -1173,10 +1170,8 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
 
     def test_report_with_version(self):
         addon = self._create_dummy_target()
-        addon.current_version.file.update(is_signed=True)
         other_version = version_factory(
-            addon=addon,
-            file_kw={'is_signed': True, 'status': amo.STATUS_AWAITING_REVIEW},
+            addon=addon, file_kw={'status': amo.STATUS_AWAITING_REVIEW}
         )
         responses.add(
             responses.POST,
@@ -1188,7 +1183,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
             guid=addon.guid, addon_version=other_version.version
         )
         report = CinderReport(abuse_report)
-        cinder_instance = self.CinderClass(addon, version_string=other_version)
+        cinder_instance = self.CinderClass(addon, version_string=other_version.version)
         assert cinder_instance.report(report=report, reporter=None)
         job = CinderJob.objects.create(job_id='1234-xyz')
         assert not addon.current_version.needshumanreview_set.exists()
@@ -1201,10 +1196,10 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
             other_version.needshumanreview_set.get().reason
             == NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION
         )
+        assert other_version.reload().due_date
 
     def test_appeal_anonymous(self):
         addon = self._create_dummy_target()
-        addon.current_version.file.update(is_signed=True)
         self._test_appeal(
             CinderUnauthenticatedReporter('itsme', 'm@r.io'), self.CinderClass(addon)
         )
@@ -1212,20 +1207,37 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
             addon.current_version.needshumanreview_set.get().reason
             == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL
         )
+        assert addon.current_version.reload().due_date
 
     def test_appeal_logged_in(self):
         addon = self._create_dummy_target()
-        addon.current_version.file.update(is_signed=True)
         self._test_appeal(CinderUser(user_factory()), self.CinderClass(addon))
         assert (
             addon.current_version.needshumanreview_set.get().reason
             == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL
         )
+        assert addon.current_version.reload().due_date
+
+    def test_appeal_specific_version(self):
+        addon = self._create_dummy_target()
+        other_version = version_factory(
+            addon=addon, file_kw={'status': amo.STATUS_AWAITING_REVIEW}
+        )
+        self._test_appeal(
+            CinderUser(user_factory()),
+            self.CinderClass(addon, version_string=other_version.version),
+        )
+        assert not addon.current_version.needshumanreview_set.exists()
+        assert (
+            other_version.needshumanreview_set.get().reason
+            == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL
+        )
+        assert not addon.current_version.reload().due_date
+        assert other_version.reload().due_date
 
     @override_switch('dsa-appeals-review', active=False)
     def test_appeal_waffle_switch_off(self):
         addon = self._create_dummy_target()
-        addon.current_version.file.update(is_signed=True)
         # We are no longer doing the queries for the activitylog, needshumanreview
         # etc since the waffle switch is off. So we're back to the same number of
         # queries made by the reports that go to Cinder.
@@ -1235,7 +1247,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
 
     def test_report_with_ongoing_appeal(self):
         addon = self._create_dummy_target()
-        addon.current_version.file.update(is_signed=True)
         job = CinderJob.objects.create(job_id='1234-xyz')
         job.appealed_decisions.add(
             ContentDecision.objects.create(
@@ -1255,7 +1266,6 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
 
     def test_report_with_ongoing_forwarded_appeal(self):
         addon = self._create_dummy_target()
-        addon.current_version.file.update(is_signed=True)
         job = CinderJob.objects.create(job_id='1234-xyz')
         CinderJob.objects.create(forwarded_to_job=job)
         job.appealed_decisions.add(
@@ -1380,10 +1390,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
     def _setup_workflow_move_test(self):
         addon = self._create_dummy_target()
         listed_version = addon.current_version
-        listed_version.file.update(is_signed=True)
-        unlisted_version = version_factory(
-            addon=addon, channel=amo.CHANNEL_UNLISTED, file_kw={'is_signed': True}
-        )
+        unlisted_version = version_factory(addon=addon, channel=amo.CHANNEL_UNLISTED)
         ActivityLog.objects.all().delete()
         cinder_instance = self.CinderClass(addon)
         cinder_job = CinderJob.objects.create(target_addon=addon, job_id='1')
@@ -1407,15 +1414,14 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
             listed_version.reload().needshumanreview_set.get().reason
             == NeedsHumanReview.REASONS.CINDER_ESCALATION
         )
-        assert (
-            unlisted_version.reload().needshumanreview_set.get().reason
-            == NeedsHumanReview.REASONS.CINDER_ESCALATION
-        )
+        assert not unlisted_version.reload().needshumanreview_set.exists()
+        assert listed_version.reload().due_date
+        assert not unlisted_version.reload().due_date
         assert ActivityLog.objects.count() == 1
         activity = ActivityLog.objects.filter(
             action=amo.LOG.NEEDS_HUMAN_REVIEW_CINDER.id
         ).get()
-        assert activity.arguments == [listed_version, unlisted_version]
+        assert activity.arguments == [listed_version]
         assert activity.user == self.task_user
 
     def test_workflow_move(self):
@@ -1433,8 +1439,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
             self._setup_workflow_move_test()
         )
         other_version = version_factory(
-            addon=listed_version.addon,
-            file_kw={'status': amo.STATUS_DISABLED, 'is_signed': True},
+            addon=listed_version.addon, file_kw={'status': amo.STATUS_DISABLED}
         )
         assert not other_version.due_date
         cinder_job.abusereport_set.update(addon_version=other_version.version)
@@ -1444,8 +1449,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
 
         assert not listed_version.reload().needshumanreview_set.exists()
         assert not unlisted_version.reload().needshumanreview_set.exists()
-        other_version.reload()
-        assert other_version.due_date
+        assert other_version.reload().due_date
         assert (
             other_version.needshumanreview_set.get().reason
             == NeedsHumanReview.REASONS.CINDER_ESCALATION
@@ -1528,7 +1532,7 @@ class TestCinderAddonHandledByReviewers(TestCinderAddon):
 
         other_version = version_factory(
             addon=listed_version.addon,
-            file_kw={'status': amo.STATUS_DISABLED, 'is_signed': True},
+            file_kw={'status': amo.STATUS_DISABLED},
         )
         assert not other_version.due_date
         ActivityLog.objects.all().delete()

src/olympia/abuse/tests/test_models.py

@@ -2698,6 +2698,56 @@ class TestContentDecision(TestCase):
         abuse_report.reload()
         assert abuse_report.cinderappeal
 
+    def test_appeal_as_reporter_specific_version(self):
+        addon = addon_factory(version_kw={'human_review_date': datetime.now()})
+        original_version = addon.current_version
+        version_factory(addon=addon, human_review_date=datetime.now())
+        abuse_report = AbuseReport.objects.create(
+            guid=addon.guid,
+            reason=AbuseReport.REASONS.ILLEGAL,
+            reporter=user_factory(),
+            addon_version=original_version.version,
+        )
+        abuse_report.update(
+            cinder_job=CinderJob.objects.create(
+                target_addon=addon,
+                resolvable_in_reviewer_tools=True,
+                decision=ContentDecision.objects.create(
+                    cinder_id='4815162342-lost',
+                    action_date=self.days_ago(179),
+                    action=DECISION_ACTIONS.AMO_APPROVE,
+                    addon=addon,
+                ),
+            )
+        )
+        responses.add(
+            responses.POST,
+            f'{settings.CINDER_SERVER_URL}appeal',
+            json={'external_id': '2432615184-tsol'},
+            status=201,
+        )
+        assert not original_version.due_date
+
+        abuse_report.cinder_job.decision.appeal(
+            abuse_report=abuse_report,
+            appeal_text='appeal text',
+            user=abuse_report.reporter,
+            is_reporter=True,
+        )
+
+        abuse_report.cinder_job.reload()
+        assert abuse_report.cinder_job.decision.appeal_job
+        assert abuse_report.cinder_job.decision.appeal_job.job_id == '2432615184-tsol'
+        assert abuse_report.cinder_job.decision.appeal_job.target_addon == addon
+        abuse_report.reload()
+        assert abuse_report.cinderappeal
+        assert CinderAppeal.objects.count() == 1
+        appeal_text_obj = CinderAppeal.objects.get()
+        assert appeal_text_obj.text == 'appeal text'
+        assert appeal_text_obj.decision == abuse_report.cinder_job.decision
+        assert appeal_text_obj.reporter_report == abuse_report
+        assert original_version.reload().due_date
+
     def test_appeal_improperly_configured_reporter(self):
         cinder_job = CinderJob.objects.create(
             decision=ContentDecision.objects.create(

src/olympia/abuse/tests/test_tasks.py

@@ -591,6 +591,7 @@ def test_addon_appeal_to_cinder_authenticated_reporter():
 @pytest.mark.django_db
 def test_addon_appeal_to_cinder_authenticated_author():
     user = user_factory(fxa_id='fake-fxa-id')
+    user_factory(pk=settings.TASK_USER_ID)
     addon = addon_factory(users=[user])
     decision = ContentDecision.objects.create(
         cinder_id='4815162342-abc',

src/olympia/versions/models.py

@@ -239,7 +239,7 @@ class VersionManager(ManagerBase):
         # Versions that haven't been disabled or have ever been signed and have
         # the explicit needs human review flag should have a due date (it gets
         # dropped on various reviewer actions).
-        is_needs_human_review = Q(
+        is_other_needs_human_review = Q(
             ~Q(file__status=amo.STATUS_DISABLED) | Q(file__is_signed=True),
             needshumanreview__is_active=True,
         )
@@ -251,19 +251,19 @@ class VersionManager(ManagerBase):
         )
         return {
             'needs_human_review_from_cinder': Q(
-                is_needs_human_review,
+                needshumanreview__is_active=True,
                 needshumanreview__reason=NeedsHumanReview.REASONS.CINDER_ESCALATION,
             ),
             'needs_human_review_from_abuse': Q(
-                is_needs_human_review,
+                needshumanreview__is_active=True,
                 needshumanreview__reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION,
             ),
             'needs_human_review_from_appeal': Q(
-                is_needs_human_review,
+                needshumanreview__is_active=True,
                 needshumanreview__reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL,
             ),
             'needs_human_review_other': Q(
-                is_needs_human_review,
+                is_other_needs_human_review,
                 ~Q(
                     needshumanreview__reason__in=(
                         NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION.value,

src/olympia/versions/tests/test_models.py

@@ -1122,7 +1122,7 @@ class TestVersion(AMOPaths, TestCase):
 
         # Any non-disabled status with needs_human_review is enough to get a
         # due date, even if not signed.
-        NeedsHumanReview.objects.create(version=version)
+        nhr = NeedsHumanReview.objects.create(version=version)
         version.file.update(is_signed=False, status=amo.STATUS_AWAITING_REVIEW)
         assert version.should_have_due_date
 
@@ -1132,12 +1132,20 @@ class TestVersion(AMOPaths, TestCase):
         version.file.update(is_signed=False, status=amo.STATUS_DISABLED)
         assert not version.should_have_due_date
 
-        # If it was signed however, it should get a due date.
+        # If was reported for abuse or appealed should also get a due_date,
+        # even if unsigned.
+        nhr.update(reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL)
+        assert version.should_have_due_date
+
+        # Otherwise it needs to be signed to get a due date.
+        nhr.update(reason=NeedsHumanReview.REASONS.UNKNOWN)
+        assert not version.should_have_due_date
         version.file.update(is_signed=True)
         assert version.should_have_due_date
 
         # Even if deleted (which internally disables the file), as long as it
         # was signed and needs human review, it should keep the due date.
+        version.file.update(is_signed=True)
         version.delete()
         assert version.should_have_due_date
 
@@ -1255,7 +1263,10 @@ class TestVersion(AMOPaths, TestCase):
 
         version.file.update(is_signed=False)
         for reason in NeedsHumanReview.REASONS.values.keys() - [
-            NeedsHumanReview.REASONS.DEVELOPER_REPLY
+            NeedsHumanReview.REASONS.DEVELOPER_REPLY,
+            NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION,
+            NeedsHumanReview.REASONS.CINDER_ESCALATION,
+            NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL,
         ]:
             # Every other reason shouldn't result in a due date since the
             # version is disabled and not signed at this point.