From de4a53e232b03f6e5443a7a22a305fbeab184aa0 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Thu, 1 Aug 2024 13:40:59 +0100 Subject: [PATCH] don't try to resolve an already closed job in cinder --- src/olympia/abuse/models.py | 4 ++- src/olympia/abuse/tests/test_models.py | 45 +++++++++++++++++--------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index 9db039c0f8df..686ab6a02ce9 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -1114,7 +1114,9 @@ def notify_reviewer_decision( 'reasoning': self.notes, 'policy_uuids': [policy.uuid for policy in policies], } - if cinder_job := getattr(self, 'cinder_job', None): + if not overriden_action and ( + cinder_job := getattr(self, 'cinder_job', None) + ): decision_cinder_id = entity_helper.create_job_decision( job_id=cinder_job.job_id, **create_decision_kw ) diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index b1549c907cac..15c38c67ddb7 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -934,12 +934,7 @@ def test_process_decision_escalate_addon(self): def _test_resolve_job(self, activity_action, cinder_action, *, expect_target_email): addon_developer = user_factory() addon = addon_factory(users=[addon_developer]) - cinder_job = CinderJob.objects.create( - job_id='999', - decision=CinderDecision.objects.create( - addon=addon, action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON - ), - ) + cinder_job = CinderJob.objects.create(job_id='999') flags = version_review_flags_factory( version=addon.current_version, pending_rejection=self.days_ago(1), @@ -1251,7 +1246,7 @@ def test_resolve_job_escalation(self): ) responses.add( responses.POST, - f'{settings.CINDER_SERVER_URL}jobs/{cinder_job.job_id}/decision', + f'{settings.CINDER_SERVER_URL}create_decision', json={'uuid': '123'}, status=201, ) @@ -1275,7 +1270,6 @@ def test_resolve_job_escalation(self): request_body = json.loads(request.body) assert request_body['policy_uuids'] == ['12345678'] assert request_body['reasoning'] == 'some review text' - assert 'entity' not in request_body cinder_job.reload() assert cinder_job.decision.action == DECISION_ACTIONS.AMO_DISABLE_ADDON self.assertCloseToNow(cinder_job.decision.date) @@ -2242,8 +2236,8 @@ def _test_notify_reviewer_decision( cinder_action, *, expect_email=True, - expect_create_decision_call=True, - expect_create_job_decision_call=False, + expect_create_decision_call, + expect_create_job_decision_call, extra_log_details=None, ): create_decision_response = responses.add( @@ -2339,7 +2333,11 @@ def test_notify_reviewer_decision_new_decision(self): addon = addon_factory(users=[addon_developer]) decision = CinderDecision(addon=addon) self._test_notify_reviewer_decision( - decision, amo.LOG.REJECT_VERSION, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON + decision, + amo.LOG.REJECT_VERSION, + DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + expect_create_decision_call=True, + expect_create_job_decision_call=False, ) assert parse.quote(f'/firefox/addon/{addon.slug}/') in mail.outbox[0].body assert '/developers/' not in mail.outbox[0].body @@ -2351,7 +2349,11 @@ def test_notify_reviewer_decision_updated_decision(self): addon=addon, action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON ) self._test_notify_reviewer_decision( - decision, amo.LOG.REJECT_VERSION, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON + decision, + amo.LOG.REJECT_VERSION, + DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + expect_create_decision_call=True, + expect_create_job_decision_call=False, ) assert parse.quote(f'/firefox/addon/{addon.slug}/') in mail.outbox[0].body assert '/developers/' not in mail.outbox[0].body @@ -2363,7 +2365,11 @@ def test_notify_reviewer_decision_unlisted_version(self): ) decision = CinderDecision(addon=addon) self._test_notify_reviewer_decision( - decision, amo.LOG.REJECT_VERSION, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON + decision, + amo.LOG.REJECT_VERSION, + DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + expect_create_decision_call=True, + expect_create_job_decision_call=False, ) assert '/firefox/' not in mail.outbox[0].body assert ( @@ -2397,8 +2403,8 @@ def test_notify_reviewer_decision_updated_decision_no_email_to_owner(self): amo.LOG.CONFIRM_AUTO_APPROVED, DECISION_ACTIONS.AMO_APPROVE, expect_email=False, - expect_create_decision_call=False, - expect_create_job_decision_call=True, + expect_create_decision_call=True, + expect_create_job_decision_call=False, ) def test_no_create_decision_for_approve_without_a_job(self): @@ -2411,6 +2417,7 @@ def test_no_create_decision_for_approve_without_a_job(self): amo.LOG.APPROVE_VERSION, DECISION_ACTIONS.AMO_APPROVE_VERSION, expect_create_decision_call=False, + expect_create_job_decision_call=False, expect_email=True, ) @@ -2424,6 +2431,7 @@ def test_notify_reviewer_decision_auto_approve_email_for_non_human_review(self): DECISION_ACTIONS.AMO_APPROVE_VERSION, expect_email=True, expect_create_decision_call=False, + expect_create_job_decision_call=False, extra_log_details={'human_review': False}, ) assert 'automatically screened and tentatively approved' in mail.outbox[0].body @@ -2438,6 +2446,7 @@ def test_notify_reviewer_decision_auto_approve_email_for_human_review(self): DECISION_ACTIONS.AMO_APPROVE_VERSION, expect_email=True, expect_create_decision_call=False, + expect_create_job_decision_call=False, extra_log_details={'human_review': True}, ) assert 'has been approved' in mail.outbox[0].body @@ -2480,6 +2489,8 @@ def test_notify_reviewer_decision_rejection_blocking(self): decision, amo.LOG.REJECT_VERSION, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + expect_create_decision_call=True, + expect_create_job_decision_call=False, extra_log_details={ 'is_addon_being_blocked': True, 'is_addon_being_disabled': False, @@ -2506,6 +2517,8 @@ def test_notify_reviewer_decision_rejection_blocking_addon_being_disabled(self): decision, amo.LOG.REJECT_VERSION, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + expect_create_decision_call=True, + expect_create_job_decision_call=False, extra_log_details={ 'is_addon_being_blocked': True, 'is_addon_being_disabled': True, @@ -2532,6 +2545,8 @@ def test_notify_reviewer_decision_rejection_addon_already_disabled(self): decision, amo.LOG.REJECT_VERSION, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + expect_create_decision_call=True, + expect_create_job_decision_call=False, ) assert ( 'Users who have previously installed those versions will be able to'