diff --git a/src/olympia/abuse/cinder.py b/src/olympia/abuse/cinder.py index dcd32680a7d..579b9accd21 100644 --- a/src/olympia/abuse/cinder.py +++ b/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 diff --git a/src/olympia/abuse/tests/test_cinder.py b/src/olympia/abuse/tests/test_cinder.py index df490d4b7a5..9a511e67d51 100644 --- a/src/olympia/abuse/tests/test_cinder.py +++ b/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', 'm@r.io'), self.CinderClass(addon) + CinderUnauthenticatedReporter('itsme', 'm@r.io'), + cinder_entity_instance=self.CinderClass(addon), ) assert ( addon.current_version.needshumanreview_set.get().reason @@ -1214,14 +1220,20 @@ 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, @@ -1230,7 +1242,9 @@ def test_appeal_specific_version(self): ) self._test_appeal( CinderUser(user_factory()), - self.CinderClass(addon, version_string=other_version.version), + cinder_entity_instance=self.CinderClass( + addon, version_string=other_version.version + ), ) assert not addon.current_version.needshumanreview_set.exists() assert ( @@ -1240,6 +1254,37 @@ def test_appeal_specific_version(self): 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 falsey we collect and flag the addon + versions from the appealled decision rather the current_version.""" + addon = self._create_dummy_target() + flagged_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 + ) + # An activity log links the flagged_version to the decision, even though the + # version_string on the CinderClass below is set to None + ActivityLog.objects.create( + amo.LOG.FORCE_DISABLE, addon, flagged_version, decision, user=user_factory() + ) + self._test_appeal( + CinderUser(user_factory()), + cinder_entity_instance=self.CinderClass(addon, version_string=None), + appealed_decision_id=decision.cinder_id, + ) + assert not addon.current_version.needshumanreview_set.exists() + assert ( + flagged_version.needshumanreview_set.get().reason + == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL + ) + assert not addon.current_version.reload().due_date + assert flagged_version.reload().due_date + def test_appeal_no_current_version(self): addon = self._create_dummy_target( status=amo.STATUS_NULL, file_kw={'status': amo.STATUS_DISABLED} @@ -1248,7 +1293,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 +1308,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):