send cinder emails to targets from reviewer tools (#21807)

* send cinder emails to targets from reviewer tools

* add tests

* update test_resolve_job_in_cinder_exception

* add docstring to CinderActions

src/olympia/abuse/models.py

@@ -341,7 +341,9 @@ class CinderJob(ModelBase):
             ],
         )
         self.policies.add(*CinderPolicy.objects.filter(uuid__in=policy_ids))
-        self.get_action_helper(existing_decision, override=override).process()
+        action_helper = self.get_action_helper(existing_decision, override=override)
+        if action_helper.process_action():
+            action_helper.process_notifications()
 
     def appeal(self, *, abuse_report, appeal_text, user, is_reporter):
         appealer_entity = None
@@ -386,12 +388,20 @@ class CinderJob(ModelBase):
                     reporter_appeal_date=datetime.now(), appellant_job=appeal_job
                 )
 
-    def resolve_job(self, reasoning, decision, policies):
+    def resolve_job(self, *, decision, log_entry):
         """This is called for reviewer tools originated decisions.
         See process_decision for cinder originated decisions."""
         entity_helper = self.get_entity_helper(self.abuse_reports[0])
+        policies = list(
+            {
+                review_action.reason.cinder_policy
+                for review_action in log_entry.reviewactionreasonlog_set.all()
+                if review_action.reason.cinder_policy_id
+            }
+        )
         decision_id = entity_helper.create_decision(
-            reasoning=reasoning, policy_uuids=[policy.uuid for policy in policies]
+            reasoning=log_entry.details.get('comments', ''),
+            policy_uuids=[policy.uuid for policy in policies],
         )
         existing_decision = (self.appealed_jobs.first() or self).decision_action
         with atomic():
@@ -402,8 +412,17 @@ class CinderJob(ModelBase):
             )
             self.policies.set(policies)
         action_helper = self.get_action_helper(existing_decision)
-        action_helper.notify_reporters()
+        # FIXME: pass down the log_entry id to where it's needed in a less hacky way
-        entity_helper.close_job(job_id=self.job_id)
+        action_helper.log_entry_id = log_entry.id
+        # FIXME: pass down the versions that are being rejected in a less hacky way
+        action_helper.affected_versions = [
+            version_log.version for version_log in log_entry.versionlog_set.all()
+        ]
+        action_helper.process_notifications(
+            policy_text=log_entry.details.get('comments')
+        )
+        if (report := self.initial_abuse_report) and report.is_handled_by_reviewers:
+            entity_helper.close_job(job_id=self.job_id)
 
 
 class AbuseReportQuerySet(BaseQuerySet):

src/olympia/abuse/tasks.py

@@ -8,6 +8,7 @@ import requests
 from django_statsd.clients import statsd
 
 from olympia import amo
+from olympia.activity.models import ActivityLog
 from olympia.addons.models import Addon
 from olympia.amo.celery import task
 from olympia.amo.decorators import use_primary_db
@@ -108,11 +109,11 @@ def appeal_to_cinder(
 
 @task
 @use_primary_db
-def resolve_job_in_cinder(*, cinder_job_id, reasoning, decision, policy_ids):
+def resolve_job_in_cinder(*, cinder_job_id, decision, log_entry_id):
     try:
         cinder_job = CinderJob.objects.get(id=cinder_job_id)
-        policies = CinderPolicy.objects.filter(id__in=policy_ids)
+        log_entry = ActivityLog.objects.get(id=log_entry_id)
-        cinder_job.resolve_job(reasoning, decision, policies)
+        cinder_job.resolve_job(decision=decision, log_entry=log_entry)
     except Exception:
         statsd.incr('abuse.tasks.resolve_job_in_cinder.failure')
         raise

src/olympia/abuse/tests/test_models.py

@@ -10,9 +10,12 @@ from django.db.utils import IntegrityError
 import pytest
 import responses
 
+from olympia import amo
+from olympia.activity.models import ActivityLog
 from olympia.amo.tests import TestCase, addon_factory, collection_factory, user_factory
 from olympia.constants.abuse import APPEAL_EXPIRATION_DAYS
 from olympia.ratings.models import Rating
+from olympia.reviewers.models import ReviewActionReason
 
 from ..cinder import (
     CinderAddon,
@@ -644,7 +647,12 @@ class TestCinderJob(TestCase):
         policy_a = CinderPolicy.objects.create(uuid='123-45', name='aaa', text='AAA')
         policy_b = CinderPolicy.objects.create(uuid='678-90', name='bbb', text='BBB')
 
-        with mock.patch.object(CinderActionBanUser, 'process') as cinder_action_mock:
+        with mock.patch.object(
+            CinderActionBanUser, 'process_action'
+        ) as action_mock, mock.patch.object(
+            CinderActionBanUser, 'process_notifications'
+        ) as notify_mock:
+            action_mock.return_value = True
             cinder_job.process_decision(
                 decision_id='12345',
                 decision_date=new_date,
@@ -656,7 +664,8 @@ class TestCinderJob(TestCase):
         assert cinder_job.decision_date == new_date
         assert cinder_job.decision_action == CinderJob.DECISION_ACTIONS.AMO_BAN_USER
         assert cinder_job.decision_notes == 'teh notes'
-        assert cinder_action_mock.call_count == 1
+        assert action_mock.call_count == 1
+        assert notify_mock.call_count == 1
         assert list(cinder_job.policies.all()) == [policy_a, policy_b]
 
     def test_appeal_as_target(self):
@@ -762,10 +771,11 @@ class TestCinderJob(TestCase):
 
     def test_resolve_job(self):
         cinder_job = CinderJob.objects.create(job_id='999')
+        addon_developer = user_factory()
         abuse_report = AbuseReport.objects.create(
-            guid=addon_factory().guid,
+            guid=addon_factory(users=[addon_developer]).guid,
             reason=AbuseReport.REASONS.POLICY_VIOLATION,
-            location=AbuseReport.LOCATION.AMO,
+            location=AbuseReport.LOCATION.ADDON,
             cinder_job=cinder_job,
             reporter=user_factory(),
         )
@@ -782,26 +792,41 @@ class TestCinderJob(TestCase):
             status=200,
         )
         policies = [CinderPolicy.objects.create(name='policy', uuid='12345678')]
+        review_action_reason = ReviewActionReason.objects.create(
+            cinder_policy=policies[0]
+        )
+
+        log_entry = ActivityLog.objects.create(
+            amo.LOG.REJECT_VERSION,
+            abuse_report.target,
+            abuse_report.target.current_version,
+            review_action_reason,
+            details={'comments': 'some review text'},
+            user=user_factory(),
+        )
 
         cinder_job.resolve_job(
-            'some text',
+            decision=CinderJob.DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON,
-            CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON,
+            log_entry=log_entry,
-            policies,
         )
 
         request = responses.calls[0].request
         request_body = json.loads(request.body)
         assert request_body['policy_uuids'] == ['12345678']
-        assert request_body['reasoning'] == 'some text'
+        assert request_body['reasoning'] == 'some review text'
         assert request_body['entity']['id'] == str(abuse_report.target.id)
         cinder_job.reload()
         assert cinder_job.decision_action == (
-            CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON
+            CinderJob.DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON
         )
         self.assertCloseToNow(cinder_job.decision_date)
         assert list(cinder_job.policies.all()) == policies
-        assert len(mail.outbox) == 1
+        assert len(mail.outbox) == 2
         assert mail.outbox[0].to == [abuse_report.reporter.email]
+        assert mail.outbox[1].to == [addon_developer.email]
+        assert str(log_entry.id) in mail.outbox[1].extra_headers['Message-ID']
+        assert 'some review text' in mail.outbox[1].body
+        assert str(abuse_report.target.current_version.version) in mail.outbox[1].body
 
     def test_abuse_reports(self):
         job = CinderJob.objects.create(job_id='fake_job_id')

src/olympia/abuse/tests/test_tasks.py

@@ -11,10 +11,11 @@ from freezegun import freeze_time
 
 from olympia import amo
 from olympia.abuse.tasks import flag_high_abuse_reports_addons_according_to_review_tier
+from olympia.activity.models import ActivityLog
 from olympia.amo.tests import TestCase, addon_factory, days_ago, user_factory
 from olympia.constants.reviewers import EXTRA_REVIEW_TARGET_PER_DAY_CONFIG_KEY
 from olympia.files.models import File
-from olympia.reviewers.models import NeedsHumanReview, UsageTier
+from olympia.reviewers.models import NeedsHumanReview, ReviewActionReason, UsageTier
 from olympia.versions.models import Version
 from olympia.zadmin.models import set_config
 
@@ -604,20 +605,29 @@ def test_resolve_job_in_cinder(statsd_incr_mock):
         json={'external_id': cinder_job.job_id},
         status=200,
     )
-    policy = CinderPolicy.objects.create(name='policy', uuid='12345678')
     statsd_incr_mock.reset_mock()
+    review_action_reason = ReviewActionReason.objects.create(
+        cinder_policy=CinderPolicy.objects.create(name='policy', uuid='12345678')
+    )
+    log_entry = ActivityLog.objects.create(
+        amo.LOG.FORCE_DISABLE,
+        abuse_report.target,
+        abuse_report.target.current_version,
+        review_action_reason,
+        details={'comments': 'some review text'},
+        user=user_factory(),
+    )
 
     resolve_job_in_cinder.delay(
         cinder_job_id=cinder_job.id,
-        reasoning='some text',
         decision=CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON,
-        policy_ids=[policy.id],
+        log_entry_id=log_entry.id,
     )
 
     request = responses.calls[0].request
     request_body = json.loads(request.body)
     assert request_body['policy_uuids'] == ['12345678']
-    assert request_body['reasoning'] == 'some text'
+    assert request_body['reasoning'] == 'some review text'
     assert request_body['entity']['id'] == str(abuse_report.target.id)
     cinder_job.reload()
     assert cinder_job.decision_action == CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON
@@ -632,7 +642,7 @@ def test_resolve_job_in_cinder(statsd_incr_mock):
 @mock.patch('olympia.abuse.tasks.statsd.incr')
 def test_resolve_job_in_cinder_exception(statsd_incr_mock):
     cinder_job = CinderJob.objects.create(job_id='999')
-    AbuseReport.objects.create(
+    abuse_report = AbuseReport.objects.create(
         guid=addon_factory().guid,
         reason=AbuseReport.REASONS.POLICY_VIOLATION,
         location=AbuseReport.LOCATION.AMO,
@@ -644,15 +654,23 @@ def test_resolve_job_in_cinder_exception(statsd_incr_mock):
         json={'uuid': '123'},
         status=500,
     )
-    policy = CinderPolicy.objects.create(name='policy', uuid='12345678')
+    log_entry = ActivityLog.objects.create(
+        amo.LOG.FORCE_DISABLE,
+        abuse_report.target,
+        abuse_report.target.current_version,
+        ReviewActionReason.objects.create(
+            cinder_policy=CinderPolicy.objects.create(name='policy', uuid='12345678')
+        ),
+        details={'comments': 'some review text'},
+        user=user_factory(),
+    )
     statsd_incr_mock.reset_mock()
 
     with pytest.raises(ConnectionError):
         resolve_job_in_cinder.delay(
             cinder_job_id=cinder_job.id,
-            reasoning='some text',
             decision=CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON,
-            policy_ids=[policy.id],
+            log_entry_id=log_entry.id,
         )
 
     assert statsd_incr_mock.call_count == 1

src/olympia/abuse/tests/test_utils.py

@@ -209,7 +209,7 @@ class TestCinderActionUser(BaseTestCinderAction, TestCase):
     def _test_ban_user(self):
         self.cinder_job.update(decision_action=CinderJob.DECISION_ACTIONS.AMO_BAN_USER)
         action = self.ActionClass(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         self.user.reload()
         self.assertCloseToNow(self.user.banned)
@@ -217,6 +217,9 @@ class TestCinderActionUser(BaseTestCinderAction, TestCase):
         activity = ActivityLog.objects.get(action=amo.LOG.ADMIN_USER_BANNED.id)
         assert activity.arguments == [self.user]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         subject = f'Mozilla Add-ons: {self.user.name}'
         self._test_owner_takedown_email(subject, 'has been suspended')
         return subject
@@ -240,16 +243,20 @@ class TestCinderActionUser(BaseTestCinderAction, TestCase):
     def _test_reporter_ignore_initial_or_appeal(self):
         self.cinder_job.update(decision_action=CinderJob.DECISION_ACTIONS.AMO_APPROVE)
         action = CinderActionApproveInitialDecision(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         self.user.reload()
         assert not self.user.banned
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         return f'Mozilla Add-ons: {self.user.name}'
 
     def _test_approve_appeal_or_override(self, CinderActionClass):
         self.cinder_job.update(decision_action=CinderJob.DECISION_ACTIONS.AMO_APPROVE)
         self.user.update(banned=self.days_ago(1), deleted=True)
-        CinderActionClass(self.cinder_job).process()
+        action = CinderActionClass(self.cinder_job)
+        assert action.process_action()
 
         self.user.reload()
         assert not self.user.banned
@@ -257,16 +264,22 @@ class TestCinderActionUser(BaseTestCinderAction, TestCase):
         activity = ActivityLog.objects.get(action=amo.LOG.ADMIN_USER_UNBAN.id)
         assert activity.arguments == [self.user]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_restore_email(f'Mozilla Add-ons: {self.user.name}')
 
     def test_target_appeal_decline(self):
         self.user.update(banned=self.days_ago(1), deleted=True)
         action = CinderActionTargetAppealRemovalAffirmation(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         self.user.reload()
         assert self.user.banned
         assert ActivityLog.objects.count() == 0
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_affirmation_email(f'Mozilla Add-ons: {self.user.name}')
 
 
@@ -285,13 +298,16 @@ class TestCinderActionAddon(BaseTestCinderAction, TestCase):
             decision_action=CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON
         )
         action = self.ActionClass(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.addon.reload().status == amo.STATUS_DISABLED
         assert ActivityLog.objects.count() == 1
         activity = ActivityLog.objects.get(action=amo.LOG.FORCE_DISABLE.id)
         assert activity.arguments == [self.addon]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         subject = f'Mozilla Add-ons: {self.addon.name}'
         self._test_owner_takedown_email(subject, 'permanently disabled')
         assert f'Your Extension {self.addon.name}' in mail.outbox[-1].body
@@ -317,22 +333,27 @@ class TestCinderActionAddon(BaseTestCinderAction, TestCase):
         self.addon.update(status=amo.STATUS_DISABLED)
         ActivityLog.objects.all().delete()
         action = CinderActionClass(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.addon.reload().status == amo.STATUS_APPROVED
         assert ActivityLog.objects.count() == 1
         activity = ActivityLog.objects.get(action=amo.LOG.FORCE_ENABLE.id)
         assert activity.arguments == [self.addon]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_restore_email(f'Mozilla Add-ons: {self.addon.name}')
 
     def _test_reporter_ignore_initial_or_appeal(self):
         self.cinder_job.update(decision_action=CinderJob.DECISION_ACTIONS.AMO_APPROVE)
         action = CinderActionApproveInitialDecision(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.addon.reload().status == amo.STATUS_APPROVED
         assert ActivityLog.objects.count() == 0
+        assert len(mail.outbox) == 0
+        action.process_notifications()
         return f'Mozilla Add-ons: {self.addon.name}'
 
     def test_escalate_addon(self):
@@ -343,7 +364,7 @@ class TestCinderActionAddon(BaseTestCinderAction, TestCase):
         )
         ActivityLog.objects.all().delete()
         action = CinderActionEscalateAddon(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.addon.reload().status == amo.STATUS_APPROVED
         assert (
@@ -374,7 +395,7 @@ class TestCinderActionAddon(BaseTestCinderAction, TestCase):
         assert not other_version.due_date
         ActivityLog.objects.all().delete()
         self.cinder_job.abusereport_set.update(addon_version=other_version.version)
-        action.process()
+        assert action.process_action()
         assert not listed_version.reload().needshumanreview_set.exists()
         assert not unlisted_version.reload().needshumanreview_set.exists()
         other_version.reload()
@@ -389,17 +410,21 @@ class TestCinderActionAddon(BaseTestCinderAction, TestCase):
         )
         assert activity.arguments == [other_version]
         assert activity.user == self.task_user
+        action.process_notifications()
         assert len(mail.outbox) == 0
 
     def test_target_appeal_decline(self):
         self.addon.update(status=amo.STATUS_DISABLED)
         ActivityLog.objects.all().delete()
         action = CinderActionTargetAppealRemovalAffirmation(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         self.addon.reload()
         assert self.addon.status == amo.STATUS_DISABLED
         assert ActivityLog.objects.count() == 0
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_affirmation_email(f'Mozilla Add-ons: {self.addon.name}')
 
     def test_notify_owners_with_manual_policy_block(self):
@@ -426,17 +451,24 @@ class TestCinderActionAddon(BaseTestCinderAction, TestCase):
         assert 'Bad policy: This is bad thing' not in mail_item.body
         assert 'some other policy justification' in mail_item.body
 
-    def test_reject_version(self):
+    def _test_reject_version(self):
         self.cinder_job.update(
             decision_action=CinderJob.DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON
         )
         cinder_action = CinderActionRejectVersion(self.cinder_job)
-        cinder_action.addon_rejected_versions = ['2.3', '3.45']
+        cinder_action.affected_versions = [
-        cinder_action.notify_owners(self.addon.authors.all())
+            version_factory(addon=self.addon, version='2.3'),
-        mail_item = mail.outbox[0]
+            version_factory(addon=self.addon, version='3.45'),
-        self._check_owner_email(
+        ]
-            mail_item, f'Mozilla Add-ons: {self.addon.name}', 'have been disabled'
+
-        )
+        # note: process_action isn't implemented for this action currently.
+
+        subject = f'Mozilla Add-ons: {self.addon.name}'
+
+        assert len(mail.outbox) == 0
+        cinder_action.process_notifications()
+        mail_item = mail.outbox[-1]
+        self._check_owner_email(mail_item, subject, 'have been disabled')
 
         assert 'right to appeal' in mail_item.body
         assert (
@@ -450,6 +482,23 @@ class TestCinderActionAddon(BaseTestCinderAction, TestCase):
         )
         assert 'Bad policy: This is bad thing' in mail_item.body
         assert 'Affected versions: 2.3, 3.45' in mail_item.body
+        return subject
+
+    def test_reject_version(self):
+        subject = self._test_reject_version()
+        assert len(mail.outbox) == 3
+        self._test_reporter_takedown_email(subject)
+
+    def test_reject_version_after_reporter_appeal(self):
+        original_job = CinderJob.objects.create(job_id='original')
+        self.cinder_job.appealed_jobs.add(original_job)
+        self.abuse_report_no_auth.update(cinder_job=original_job)
+        self.abuse_report_auth.update(
+            cinder_job=original_job, appellant_job=self.cinder_job
+        )
+        subject = self._test_reject_version()
+        assert len(mail.outbox) == 2
+        self._test_reporter_appeal_takedown_email(subject)
 
 
 class TestCinderActionCollection(BaseTestCinderAction, TestCase):
@@ -466,7 +515,7 @@ class TestCinderActionCollection(BaseTestCinderAction, TestCase):
             decision_action=CinderJob.DECISION_ACTIONS.AMO_DELETE_COLLECTION
         )
         action = self.ActionClass(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.collection.reload()
         assert self.collection.deleted
@@ -475,6 +524,9 @@ class TestCinderActionCollection(BaseTestCinderAction, TestCase):
         activity = ActivityLog.objects.get(action=amo.LOG.COLLECTION_DELETED.id)
         assert activity.arguments == [self.collection]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         subject = f'Mozilla Add-ons: {self.collection.name}'
         self._test_owner_takedown_email(subject, 'permanently removed')
         return subject
@@ -498,17 +550,20 @@ class TestCinderActionCollection(BaseTestCinderAction, TestCase):
     def _test_reporter_ignore_initial_or_appeal(self):
         self.cinder_job.update(decision_action=CinderJob.DECISION_ACTIONS.AMO_APPROVE)
         action = CinderActionApproveInitialDecision(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.collection.reload()
         assert not self.collection.deleted
         assert ActivityLog.objects.count() == 0
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         return f'Mozilla Add-ons: {self.collection.name}'
 
     def _test_approve_appeal_or_override(self, CinderActionClass):
         self.collection.update(deleted=True)
         action = CinderActionClass(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.collection.reload()
         assert not self.collection.deleted
@@ -516,16 +571,22 @@ class TestCinderActionCollection(BaseTestCinderAction, TestCase):
         activity = ActivityLog.objects.get(action=amo.LOG.COLLECTION_UNDELETED.id)
         assert activity.arguments == [self.collection]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_restore_email(f'Mozilla Add-ons: {self.collection.name}')
 
     def test_target_appeal_decline(self):
         self.collection.update(deleted=True)
         action = CinderActionTargetAppealRemovalAffirmation(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         self.collection.reload()
         assert self.collection.deleted
         assert ActivityLog.objects.count() == 0
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_affirmation_email(f'Mozilla Add-ons: {self.collection.name}')
 
 
@@ -546,13 +607,16 @@ class TestCinderActionRating(BaseTestCinderAction, TestCase):
             decision_action=CinderJob.DECISION_ACTIONS.AMO_DELETE_RATING
         )
         action = self.ActionClass(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert self.rating.reload().deleted
         assert ActivityLog.objects.count() == 1
         activity = ActivityLog.objects.get(action=amo.LOG.DELETE_RATING.id)
         assert activity.arguments == [self.rating.addon, self.rating]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         subject = f'Mozilla Add-ons: "Saying ..." for {self.rating.addon.name}'
         self._test_owner_takedown_email(subject, 'permanently removed')
         return subject
@@ -576,23 +640,29 @@ class TestCinderActionRating(BaseTestCinderAction, TestCase):
     def _test_reporter_ignore_initial_or_appeal(self):
         self.cinder_job.update(decision_action=CinderJob.DECISION_ACTIONS.AMO_APPROVE)
         action = CinderActionApproveInitialDecision(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert not self.rating.reload().deleted
         assert ActivityLog.objects.count() == 0
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         return f'Mozilla Add-ons: "Saying ..." for {self.rating.addon.name}'
 
     def _test_approve_appeal_or_override(self, CinderActionClass):
         self.rating.delete()
         ActivityLog.objects.all().delete()
         action = CinderActionClass(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         assert not self.rating.reload().deleted
         assert ActivityLog.objects.count() == 1
         activity = ActivityLog.objects.get(action=amo.LOG.UNDELETE_RATING.id)
         assert activity.arguments == [self.rating, self.rating.addon]
         assert activity.user == self.task_user
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_restore_email(
             f'Mozilla Add-ons: "Saying ..." for {self.rating.addon.name}'
         )
@@ -601,11 +671,14 @@ class TestCinderActionRating(BaseTestCinderAction, TestCase):
         self.rating.delete()
         ActivityLog.objects.all().delete()
         action = CinderActionTargetAppealRemovalAffirmation(self.cinder_job)
-        action.process()
+        assert action.process_action()
 
         self.rating.reload()
         assert self.rating.deleted
         assert ActivityLog.objects.count() == 0
+        assert len(mail.outbox) == 0
+
+        action.process_notifications()
         self._test_owner_affirmation_email(
             f'Mozilla Add-ons: "Saying ..." for {self.rating.addon.name}'
         )

src/olympia/abuse/utils.py

@@ -27,7 +27,19 @@ class CinderAction:
         self.target = self.cinder_job.target
         self.is_third_party_initiated = True  # will not always be true in the future
 
-    def process(self):
+        if isinstance(self.target, Addon):
+            self.addon_version = (
+                self.target.current_version
+                or self.target.find_latest_version(channel=None, exclude=())
+            )
+
+    def process_action(self):
+        """This method should return True (or a truthy value) when an action has taken
+        place, and a falsey value when the intended action didn't occur.
+        Typically the truthy value would indicate email notifications should be sent."""
+        raise NotImplementedError
+
+    def process_notifications(self, *, policy_text=None):
         raise NotImplementedError
 
     def get_target_name(self):
@@ -54,7 +66,7 @@ class CinderAction:
     def owner_template_path(self):
         return f'abuse/emails/{self.__class__.__name__}.txt'
 
-    def notify_owners(self, owners, *, policy_text=None):
+    def notify_owners(self, owners, *, policy_text):
         name = self.get_target_name()
         reference_id = f'ref:{self.cinder_job.decision_id}'
         context_dict = {
@@ -67,7 +79,9 @@ class CinderAction:
             'target_url': absolutify(self.target.get_url_path()),
             'type': self.get_target_type(),
             'SITE_URL': settings.SITE_URL,
-            'version_list': ', '.join(getattr(self, 'addon_rejected_versions', ())),
+            'version_list': ', '.join(
+                version.version for version in getattr(self, 'affected_versions', ())
+            ),
         }
         if policy_text is not None:
             context_dict['manual_policy_text'] = policy_text
@@ -154,13 +168,17 @@ class CinderActionBanUser(CinderAction):
     reporter_template_path = 'abuse/emails/reporter_takedown_user.txt'
     reporter_appeal_template_path = 'abuse/emails/reporter_appeal_takedown.txt'
 
-    def process(self):
+    def process_action(self):
+        """This will return True if a user has been banned."""
         if isinstance(self.target, UserProfile) and not self.target.banned:
             UserProfile.objects.filter(
                 pk=self.target.pk
             ).ban_and_disable_related_content()
-            self.notify_reporters()
+            return True
-            self.notify_owners([self.target])
+
+    def process_notifications(self, *, policy_text=None):
+        self.notify_reporters()
+        self.notify_owners([self.target], policy_text=policy_text)
 
 
 class CinderActionDisableAddon(CinderAction):
@@ -169,16 +187,18 @@ class CinderActionDisableAddon(CinderAction):
     reporter_template_path = 'abuse/emails/reporter_takedown_addon.txt'
     reporter_appeal_template_path = 'abuse/emails/reporter_appeal_takedown.txt'
 
-    def process(self):
+    def process_action(self):
+        """This will return True if an add-on has been disabled."""
         if isinstance(self.target, Addon) and self.target.status != amo.STATUS_DISABLED:
-            self.addon_version = (
-                self.target.current_version
-                or self.target.find_latest_version(channel=None, exclude=())
-            )
             self.target.force_disable(skip_activity_log=True)
-            self.log_entry = log_create(amo.LOG.FORCE_DISABLE, self.target)
+            self.log_entry_id = (
-            self.notify_reporters()
+                log_entry := log_create(amo.LOG.FORCE_DISABLE, self.target)
-            self.notify_owners(self.target.authors.all())
+            ) and log_entry.id
+            return True
+
+    def process_notifications(self, *, policy_text=None):
+        self.notify_reporters()
+        self.notify_owners(self.target.authors.all(), policy_text=policy_text)
 
     def send_mail(self, subject, message, recipients):
         from olympia.activity.utils import send_activity_mail
@@ -186,9 +206,7 @@ class CinderActionDisableAddon(CinderAction):
         """We send addon related via activity mail instead for the integration"""
 
         if version := getattr(self, 'addon_version', None):
-            unique_id = (
+            unique_id = getattr(self, 'log_entry_id', None) or random.randrange(100000)
-                self.log_entry.id if self.log_entry else random.randrange(100000)
-            )
             send_activity_mail(
                 subject, message, version, recipients, settings.ADDONS_EMAIL, unique_id
             )
@@ -208,7 +226,9 @@ class CinderActionRejectVersion(CinderActionDisableAddon):
 class CinderActionEscalateAddon(CinderAction):
     valid_targets = [Addon]
 
-    def process(self):
+    def process_action(self):
+        """This will return True if an add-on has had a version flagged for
+        human review."""
         from olympia.reviewers.models import NeedsHumanReview
 
         if isinstance(self.target, Addon):
@@ -240,6 +260,11 @@ class CinderActionEscalateAddon(CinderAction):
                 self.target.set_needs_human_review_on_latest_versions(
                     reason=reason, ignore_reviewed=False, unique_reason=True
                 )
+            return True
+
+    def process_notifications(self, *, policy_text=None):
+        # we don't send any emails for escalations
+        pass
 
 
 class CinderActionDeleteCollection(CinderAction):
@@ -248,12 +273,16 @@ class CinderActionDeleteCollection(CinderAction):
     reporter_template_path = 'abuse/emails/reporter_takedown_collection.txt'
     reporter_appeal_template_path = 'abuse/emails/reporter_appeal_takedown.txt'
 
-    def process(self):
+    def process_action(self):
+        """This will return True if a collection has been deleted."""
         if isinstance(self.target, Collection) and not self.target.deleted:
             log_create(amo.LOG.COLLECTION_DELETED, self.target)
             self.target.delete(clear_slug=False)
-            self.notify_reporters()
+            return True
-            self.notify_owners([self.target.author])
+
+    def process_notifications(self, *, policy_text=None):
+        self.notify_reporters()
+        self.notify_owners([self.target.author], policy_text=policy_text)
 
 
 class CinderActionDeleteRating(CinderAction):
@@ -262,37 +291,54 @@ class CinderActionDeleteRating(CinderAction):
     reporter_template_path = 'abuse/emails/reporter_takedown_rating.txt'
     reporter_appeal_template_path = 'abuse/emails/reporter_appeal_takedown.txt'
 
-    def process(self):
+    def process_action(self):
+        """This will return True if a rating has been deleted."""
         if isinstance(self.target, Rating) and not self.target.deleted:
             self.target.delete(clear_flags=False)
-            self.notify_reporters()
+            return True
-            self.notify_owners([self.target.user])
+
+    def process_notifications(self, *, policy_text=None):
+        self.notify_reporters()
+        self.notify_owners([self.target.user], policy_text=policy_text)
 
 
 class CinderActionTargetAppealApprove(CinderAction):
     valid_targets = [Addon, UserProfile, Collection, Rating]
     description = 'Reported content is within policy, after appeal'
 
-    def process(self):
+    def process_action(self):
+        """This will return True if we've reversed an action,
+        e.g. enabled a disabled add-on."""
         target = self.target
         if isinstance(target, Addon) and target.status == amo.STATUS_DISABLED:
             target.force_enable()
-            self.notify_owners(target.authors.all())
+            return True
 
         elif isinstance(target, UserProfile) and target.banned:
             UserProfile.objects.filter(
                 pk=target.pk
             ).unban_and_reenable_related_content()
-            self.notify_owners([target])
+            return True
 
         elif isinstance(target, Collection) and target.deleted:
             target.undelete()
             log_create(amo.LOG.COLLECTION_UNDELETED, target)
-            self.notify_owners([target.author])
+            return True
 
         elif isinstance(target, Rating) and target.deleted:
             target.undelete()
-            self.notify_owners([target.user])
+            return True
+
+    def process_notifications(self, *, policy_text=None):
+        target = self.target
+        if isinstance(target, Addon):
+            self.notify_owners(target.authors.all(), policy_text=policy_text)
+        elif isinstance(target, UserProfile):
+            self.notify_owners([target], policy_text=policy_text)
+        elif isinstance(target, Collection):
+            self.notify_owners([target.author], policy_text=policy_text)
+        elif isinstance(target, Rating):
+            self.notify_owners([target.user], policy_text=policy_text)
 
 
 class CinderActionOverrideApprove(CinderActionTargetAppealApprove):
@@ -305,27 +351,38 @@ class CinderActionApproveInitialDecision(CinderAction):
     reporter_template_path = 'abuse/emails/reporter_ignore.txt'
     reporter_appeal_template_path = 'abuse/emails/reporter_appeal_ignore.txt'
 
-    def process(self):
+    def process_action(self):
-        self.notify_reporters()
+        """This will always return True."""
+        return True
         # If it's an initial decision approve there is nothing else to do
 
+    def process_notifications(self, *, policy_text=None):
+        self.notify_reporters()
+
 
 class CinderActionTargetAppealRemovalAffirmation(CinderAction):
     valid_targets = [Addon, UserProfile, Collection, Rating]
     description = 'Reported content is still offending, after appeal.'
 
-    def process(self):
+    def process_action(self):
+        """This will always return True."""
+        return True
+
+    def process_notifications(self, *, policy_text=None):
         target = self.target
         if isinstance(target, Addon):
-            self.notify_owners(target.authors.all())
+            self.notify_owners(target.authors.all(), policy_text=policy_text)
         elif isinstance(target, UserProfile):
-            self.notify_owners([target])
+            self.notify_owners([target], policy_text=policy_text)
         elif isinstance(target, Collection):
-            self.notify_owners([target.author])
+            self.notify_owners([target.author], policy_text=policy_text)
         elif isinstance(target, Rating):
-            self.notify_owners([target.user])
+            self.notify_owners([target.user], policy_text=policy_text)
 
 
 class CinderActionNotImplemented(CinderAction):
-    def process(self):
+    def process_action(self):
+        return True
+
+    def process_notifications(self, *, policy_text=None):
         pass

src/olympia/reviewers/tests/test_utils.py

@@ -1,5 +1,5 @@
 from datetime import datetime, timedelta
-from unittest.mock import patch
+from unittest.mock import call, patch
 
 from django.conf import settings
 from django.core import mail
@@ -12,6 +12,7 @@ import pytest
 import responses
 
 from olympia import amo
+from olympia.abuse.models import AbuseReport, CinderJob
 from olympia.activity.models import ActivityLog, ActivityLogToken, ReviewActionReasonLog
 from olympia.addons.models import Addon, AddonApprovalsCounter, AddonReviewerFlags
 from olympia.amo.templatetags.jinja_helpers import absolutify
@@ -1017,6 +1018,37 @@ class TestReviewHelper(TestReviewHelperBase):
         assert base_fragment not in message.body
         assert message.reply_to == [reply_email]
 
+    @patch('olympia.reviewers.utils.resolve_job_in_cinder.delay')
+    def test_resolve_abuse_reports(self, mock_resolve_task):
+        log_entry = ActivityLog.objects.create(
+            action=amo.LOG.APPROVE_VERSION.id, user=user_factory()
+        )
+        self.helper.handler.log_entry = log_entry
+        cinder_job1 = CinderJob.objects.create(job_id='1')
+        cinder_job2 = CinderJob.objects.create(job_id='2')
+        self.helper.set_data(
+            {**self.get_data(), 'resolve_cinder_jobs': [cinder_job1, cinder_job2]}
+        )
+
+        self.helper.handler.resolve_abuse_reports(
+            CinderJob.DECISION_ACTIONS.AMO_APPROVE
+        )
+
+        mock_resolve_task.assert_has_calls(
+            [
+                call(
+                    cinder_job_id=cinder_job1.id,
+                    decision=CinderJob.DECISION_ACTIONS.AMO_APPROVE,
+                    log_entry_id=log_entry.id,
+                ),
+                call(
+                    cinder_job_id=cinder_job2.id,
+                    decision=CinderJob.DECISION_ACTIONS.AMO_APPROVE,
+                    log_entry_id=log_entry.id,
+                ),
+            ]
+        )
+
     def test_email_links(self):
         expected = {
             'extension_nominated_to_approved': 'addon_url',
@@ -2079,7 +2111,7 @@ class TestReviewHelper(TestReviewHelperBase):
         self.addon.update(status=amo.STATUS_NOMINATED)
         assert self.get_helper()
 
-    def test_reject_multiple_versions(self):
+    def _test_reject_multiple_versions(self, extra_data):
         old_version = self.review_version
         self.review_version = version_factory(addon=self.addon, version='3.0')
         AutoApprovalSummary.objects.create(
@@ -2093,9 +2125,9 @@ class TestReviewHelper(TestReviewHelperBase):
         assert self.file.status == amo.STATUS_APPROVED
         assert self.addon.current_version.is_public()
 
-        data = self.get_data().copy()
+        self.helper.set_data(
-        data['versions'] = self.addon.versions.all()
+            {**self.get_data(), 'versions': self.addon.versions.all(), **extra_data}
-        self.helper.set_data(data)
+        )
         self.helper.handler.reject_multiple_versions()
 
         self.addon.reload()
@@ -2115,11 +2147,6 @@ class TestReviewHelper(TestReviewHelperBase):
         assert len(mail.outbox) == 1
         message = mail.outbox[0]
         assert message.to == [self.addon.authors.all()[0].email]
-        assert message.subject == (
-            'Mozilla Add-ons: Delicious Bookmarks has been disabled on '
-            'addons.mozilla.org'
-        )
-        assert 'your add-on Delicious Bookmarks has been disabled' in message.body
         log_token = ActivityLogToken.objects.get()
         assert log_token.uuid.hex in message.reply_to[0]
 
@@ -2139,6 +2166,30 @@ class TestReviewHelper(TestReviewHelperBase):
         assert not flags.auto_approval_disabled_until_next_approval_unlisted
         assert flags.auto_approval_disabled_until_next_approval
 
+    def test_reject_multiple_versions(self):
+        self._test_reject_multiple_versions({})
+        message = mail.outbox[0]
+        assert message.subject == (
+            'Mozilla Add-ons: Delicious Bookmarks has been disabled on '
+            'addons.mozilla.org'
+        )
+        assert 'your add-on Delicious Bookmarks has been disabled' in message.body
+
+    def test_reject_multiple_versions_resolving_abuse_report(self):
+        responses.add(
+            responses.POST,
+            f'{settings.CINDER_SERVER_URL}create_decision',
+            json={'uuid': '12345'},
+            status=201,
+        )
+        cinder_job = CinderJob.objects.create(job_id='1')
+        AbuseReport.objects.create(guid=self.addon.guid, cinder_job=cinder_job)
+        self._test_reject_multiple_versions({'resolve_cinder_jobs': [cinder_job]})
+        message = mail.outbox[0]
+        assert message.subject == ('Mozilla Add-ons: Delicious Bookmarks [ref:12345]')
+        assert 'Extension Delicious Bookmarks was manually reviewed' in message.body
+        assert 'those versions of your Extension have been disabled' in message.body
+
     def test_reject_multiple_versions_with_delay(self):
         old_version = self.review_version
         self.review_version = version_factory(addon=self.addon, version='3.0')

src/olympia/reviewers/tests/test_views.py

@@ -5583,11 +5583,11 @@ class TestReview(ReviewBase):
             ),
         )
         assert self.get_addon().status == amo.STATUS_DISABLED
+        log_entry = ActivityLog.objects.get(action=amo.LOG.FORCE_DISABLE.id)
         mock_resolve_task.assert_called_once_with(
             cinder_job_id=cinder_job.id,
-            reasoning='something',
             decision=CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON,
-            policy_ids=[reason.cinder_policy.id],
+            log_entry_id=log_entry.id,
         )
 
     @override_switch('enable-cinder-reporting', active=True)
@@ -5628,11 +5628,11 @@ class TestReview(ReviewBase):
             ),
         )
 
+        log_entry = ActivityLog.objects.get(action=amo.LOG.APPROVE_VERSION.id)
         mock_resolve_task.assert_called_once_with(
             cinder_job_id=cinder_job.id,
-            reasoning='something',
             decision=CinderJob.DECISION_ACTIONS.AMO_APPROVE,
-            policy_ids=[reason.cinder_policy.id],
+            log_entry_id=log_entry.id,
         )
 
 

src/olympia/reviewers/utils.py

@@ -936,20 +936,12 @@ class ReviewBase:
 
     def resolve_abuse_reports(self, decision):
         if cinder_jobs := self.data.get('resolve_cinder_jobs', ()):
-            policy_ids = list(
-                {
-                    reason.cinder_policy_id
-                    for reason in self.data.get('reasons', ())
-                    if reason.cinder_policy_id
-                }
-            )
             # with appeals and escaltions there could be multiple jobs
             for cinder_job in cinder_jobs:
                 resolve_job_in_cinder.delay(
                     cinder_job_id=cinder_job.id,
-                    reasoning=self.data.get('comments', ''),
                     decision=decision,
-                    policy_ids=policy_ids,
+                    log_entry_id=self.log_entry.id,
                 )
 
     def clear_all_needs_human_review_flags_in_channel(self, mad_too=True):
@@ -1029,6 +1021,10 @@ class ReviewBase:
     def notify_email(
         self, template, subject, perm_setting='reviewer_reviewed', version=None
     ):
+        if self.data.get('resolve_cinder_jobs', ()):
+            # if we're resolving cinder jobs we email inside that task
+            # TODO: remove this function and always send cinder style emails!
+            return
         """Notify the authors that their addon has been reviewed."""
         if version is None:
             version = self.version
@@ -1157,12 +1153,13 @@ class ReviewBase:
             # The counter can be incremented.
             AddonApprovalsCounter.increment_for_addon(addon=self.addon)
             self.set_human_review_date()
-            self.resolve_abuse_reports(CinderJob.DECISION_ACTIONS.AMO_APPROVE)
         else:
             # Automatic approval, reset the counter.
             AddonApprovalsCounter.reset_for_addon(addon=self.addon)
 
         self.log_action(amo.LOG.APPROVE_VERSION)
+        if self.human_review:
+            self.resolve_abuse_reports(CinderJob.DECISION_ACTIONS.AMO_APPROVE)
         template = '%s_to_approved' % self.review_type
         if self.review_type in ['extension_pending', 'theme_pending']:
             subject = 'Mozilla Add-ons: %s %s Updated'