From 4995c451432581b898a48a204658c8ac37f369fe Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 7 Jan 2025 11:44:29 +0000 Subject: [PATCH 1/3] flag correct versions for a developer appeal --- src/olympia/abuse/cinder.py | 19 ++++++-- src/olympia/abuse/tests/test_cinder.py | 66 +++++++++++++++++++++----- 2 files changed, 67 insertions(+), 18 deletions(-) diff --git a/src/olympia/abuse/cinder.py b/src/olympia/abuse/cinder.py index dcd32680a7db..579b9accd212 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 df490d4b7a55..3a10144ca6d1 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,23 +1220,55 @@ 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): + # version_string is set for reporter appeals on reports that specified a 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 developer appeals we should use the versions that were affected 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 +1286,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 +1301,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): From 8486ff498480e1df40103df854be42ef02a1eb6a Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Wed, 8 Jan 2025 10:07:26 +0000 Subject: [PATCH 2/3] reword test comments into docstrings --- src/olympia/abuse/tests/test_cinder.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/olympia/abuse/tests/test_cinder.py b/src/olympia/abuse/tests/test_cinder.py index 3a10144ca6d1..c9ff78bc6631 100644 --- a/src/olympia/abuse/tests/test_cinder.py +++ b/src/olympia/abuse/tests/test_cinder.py @@ -1230,7 +1230,10 @@ def test_appeal_logged_in(self): assert addon.current_version.reload().due_date def test_appeal_specific_version_from_report(self): - # version_string is set for reporter appeals on reports that specified a version + """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, @@ -1252,7 +1255,9 @@ def test_appeal_specific_version_from_report(self): assert other_version.reload().due_date def test_appeal_specific_version_from_action(self): - # for developer appeals we should use the versions that were affected + """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, From 56dc3175e6cf78279844b3d6ba018029e76bab8b Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Thu, 9 Jan 2025 12:58:07 +0000 Subject: [PATCH 3/3] test update --- src/olympia/abuse/tests/test_cinder.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/olympia/abuse/tests/test_cinder.py b/src/olympia/abuse/tests/test_cinder.py index c9ff78bc6631..9a511e67d51d 100644 --- a/src/olympia/abuse/tests/test_cinder.py +++ b/src/olympia/abuse/tests/test_cinder.py @@ -1256,10 +1256,10 @@ def test_appeal_specific_version_from_report(self): 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 + 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() - other_version = version_factory( + flagged_version = version_factory( addon=addon, channel=amo.CHANNEL_UNLISTED, file_kw={'status': amo.STATUS_AWAITING_REVIEW}, @@ -1267,8 +1267,10 @@ def test_appeal_specific_version_from_action(self): 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, other_version, decision, user=user_factory() + amo.LOG.FORCE_DISABLE, addon, flagged_version, decision, user=user_factory() ) self._test_appeal( CinderUser(user_factory()), @@ -1277,11 +1279,11 @@ def test_appeal_specific_version_from_action(self): ) assert not addon.current_version.needshumanreview_set.exists() assert ( - other_version.needshumanreview_set.get().reason + flagged_version.needshumanreview_set.get().reason == NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL ) assert not addon.current_version.reload().due_date - assert other_version.reload().due_date + assert flagged_version.reload().due_date def test_appeal_no_current_version(self): addon = self._create_dummy_target(