flag correct versions for a developer appeal

src/olympia/abuse/cinder.py

@@ -15,6 +15,7 @@
     create_signed_url_for_file_backup,
 )
 from olympia.users.utils import get_task_user
+from olympia.versions.models import Version
 
 
 log = olympia.core.logger.getLogger('z.abuse')
@@ -299,7 +300,7 @@ def report(self, *args, **kwargs):
         # It doesn't make sense to report a non fxa user
         raise NotImplementedError
 
-    def appeal(self, *args, **kwargs):
+    def appeal(self, **kwargs):
         # It doesn't make sense to report a non fxa user
         raise NotImplementedError
 
@@ -600,11 +601,19 @@ def post_report(self, job):
         # 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):
-        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
+    def appeal(self, *, decision_cinder_id, **kwargs):
+        if self.version_string:
+            # if this was a reporter appeal we have version_string from the abuse report
+            related_versions = {self.version_string}
+        else:
+            # otherwise get the affected versions from the activity log
+            related_versions = set(
+                Version.unfiltered.filter(
+                    versionlog__activity_log__contentdecisionlog__decision__cinder_id=decision_cinder_id
+                ).values_list('version', flat=True)
+            )
         self.flag_for_human_review(related_versions=related_versions, appeal=True)
-        return super().appeal(*args, **kwargs)
+        return super().appeal(decision_cinder_id=decision_cinder_id, **kwargs)
 
     def post_queue_move(self, *, job):
         # When the move is to AMO reviewers we need to flag versions for review

src/olympia/abuse/tests/test_cinder.py

@@ -136,9 +136,14 @@ def test_build_report_payload(self):
     def test_report(self):
         self._test_report(self._create_dummy_target())
 
-    def _test_appeal(self, appealer, cinder_instance=None):
-        fake_decision_id = 'decision-id-to-appeal-666'
-        cinder_instance = cinder_instance or self.CinderClass(
+    def _test_appeal(
+        self,
+        appealer,
+        *,
+        cinder_entity_instance=None,
+        appealed_decision_id='decision-id-to-appeal-666',
+    ):
+        cinder_entity_instance = cinder_entity_instance or self.CinderClass(
             self._create_dummy_target()
         )
 
@@ -149,8 +154,8 @@ def _test_appeal(self, appealer, cinder_instance=None):
             status=201,
         )
         assert (
-            cinder_instance.appeal(
-                decision_cinder_id=fake_decision_id,
+            cinder_entity_instance.appeal(
+                decision_cinder_id=appealed_decision_id,
                 appeal_text='reason',
                 appealer=appealer,
             )
@@ -163,8 +168,8 @@ def _test_appeal(self, appealer, cinder_instance=None):
             status=400,
         )
         with self.assertRaises(ConnectionError):
-            cinder_instance.appeal(
-                decision_cinder_id=fake_decision_id,
+            cinder_entity_instance.appeal(
+                decision_cinder_id=appealed_decision_id,
                 appeal_text='reason',
                 appealer=appealer,
             )
@@ -1204,7 +1209,8 @@ def test_report_with_version(self):
     def test_appeal_anonymous(self):
         addon = self._create_dummy_target()
         self._test_appeal(
-            CinderUnauthenticatedReporter('itsme', '[email protected]'), self.CinderClass(addon)
+            CinderUnauthenticatedReporter('itsme', '[email protected]'),
+            cinder_entity_instance=self.CinderClass(addon),
         )
         assert (
             addon.current_version.needshumanreview_set.get().reason
@@ -1214,23 +1220,60 @@ def test_appeal_anonymous(self):
 
     def test_appeal_logged_in(self):
         addon = self._create_dummy_target()
-        self._test_appeal(CinderUser(user_factory()), self.CinderClass(addon))
+        self._test_appeal(
+            CinderUser(user_factory()), cinder_entity_instance=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):
+    def test_appeal_specific_version_from_report(self):
+        """For an appeal from a reporter, version_string is set on the CinderClass
+        instance, from AbuseReport.addon_version_string. If version_string is defined,
+        and the version exists, we should flag that version rather than current_version.
+        """
+        addon = self._create_dummy_target()
+        other_version = version_factory(
+            addon=addon,
+            channel=amo.CHANNEL_UNLISTED,
+            file_kw={'status': amo.STATUS_AWAITING_REVIEW},
+        )
+        self._test_appeal(
+            CinderUser(user_factory()),
+            cinder_entity_instance=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
+
+    def test_appeal_specific_version_from_action(self):
+        """For an appeal from a developer, version_string will be None on the
+        CinderClass instance. If version_string is falsely we collect and flag the addon
+        versions from the appealled decision rather the current_version."""
         addon = self._create_dummy_target()
         other_version = version_factory(
             addon=addon,
             channel=amo.CHANNEL_UNLISTED,
             file_kw={'status': amo.STATUS_AWAITING_REVIEW},
         )
+        decision = ContentDecision.objects.create(
+            action=DECISION_ACTIONS.AMO_DISABLE_ADDON, cinder_id='some_id', addon=addon
+        )
+        ActivityLog.objects.create(
+            amo.LOG.FORCE_DISABLE, addon, other_version, decision, user=user_factory()
+        )
         self._test_appeal(
             CinderUser(user_factory()),
-            self.CinderClass(addon, version_string=other_version.version),
+            cinder_entity_instance=self.CinderClass(addon, version_string=None),
+            appealed_decision_id=decision.cinder_id,
         )
         assert not addon.current_version.needshumanreview_set.exists()
         assert (
@@ -1248,7 +1291,7 @@ def test_appeal_no_current_version(self):
         assert not addon.current_version
         self._test_appeal(
             CinderUser(user_factory()),
-            self.CinderClass(addon),
+            cinder_entity_instance=self.CinderClass(addon),
         )
         assert (
             version.needshumanreview_set.get().reason
@@ -1263,7 +1306,9 @@ def test_appeal_waffle_switch_off(self):
         # 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.
         self.expected_queries_for_report = TestCinderAddon.expected_queries_for_report
-        self._test_appeal(CinderUser(user_factory()), self.CinderClass(addon))
+        self._test_appeal(
+            CinderUser(user_factory()), cinder_entity_instance=self.CinderClass(addon)
+        )
         assert addon.current_version.needshumanreview_set.count() == 0
 
     def test_report_with_ongoing_appeal(self):