diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index 6f30987b4f16..61aef192e955 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -19,6 +19,7 @@ from olympia.bandwagon.models import Collection from olympia.ratings.models import Rating from olympia.users.models import UserProfile +from olympia.versions.models import Version POLICY_DOCUMENT_URL = ( @@ -52,15 +53,24 @@ def __init__(self, decision): ) def log_action(self, activity_log_action, *extra_args, extra_details=None): + # if we have a previous log associated with this decision, dupe some of it + previous_log = self.decision.activities.first() + previous_versions = ( + (arg for arg in previous_log.arguments if isinstance(arg, Version)) + if previous_log + else () + ) + previous_details = (previous_log and previous_log.details) or {} return log_create( activity_log_action, self.target, + *previous_versions, self.decision, *(self.decision.policies.all()), *extra_args, details={ + **previous_details, 'comments': self.decision.notes, - 'cinder_action': self.decision.action, **(extra_details or {}), }, ) diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index f1c797724f06..b4c8879af936 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -177,25 +177,24 @@ def get_cinder_reporter(cls, abuse): @classmethod def handle_already_removed(cls, abuse_report): - decision = ContentDecision( + entity_helper = cls.get_entity_helper( + abuse_report.target, + addon_version_string=abuse_report.addon_version, + resolved_in_reviewer_tools=False, + ) + decision = ContentDecision.objects.create( addon=abuse_report.addon, rating=abuse_report.rating, collection=abuse_report.collection, user=abuse_report.user, action=DECISION_ACTIONS.AMO_CLOSED_NO_ACTION, action_date=datetime.now(), + cinder_id=entity_helper.create_decision( + action=DECISION_ACTIONS.AMO_CLOSED_NO_ACTION.api_value, + reasoning='', + policy_uuids=(), + ), ) - entity_helper = cls.get_entity_helper( - abuse_report.target, - addon_version_string=abuse_report.addon_version, - resolved_in_reviewer_tools=False, - ) - decision.cinder_id = entity_helper.create_decision( - action=decision.action.api_value, - reasoning='', - policy_uuids=(), - ) - decision.save() decision.get_action_helper().notify_reporters( reporter_abuse_reports=[abuse_report], is_appeal=False ) @@ -302,8 +301,7 @@ def process_decision( decision_notes, policy_ids, ): - """This is called for cinder originated decisions. - See resolve_job for reviewer tools originated decisions.""" + """This is called for cinder originated decisions only.""" # We need either an AbuseReport or ContentDecision for the target props abuse_report_or_decision = ( self.appealed_decisions.first() or self.abusereport_set.first() @@ -330,7 +328,8 @@ def process_decision( if not self.decision: self.update(decision=decision) - decision.process_action() + # no need to report - it came from Cinder + decision.execute_action_and_notify() def process_queue_move(self, *, new_queue, notes): CinderQueueMove.objects.create(cinder_job=self, notes=notes, to_queue=new_queue) @@ -353,48 +352,6 @@ def process_queue_move(self, *, new_queue, notes): 'Job %s has moved, but not in or out of AMO handled queue', self.id ) - def resolve_job(self, *, log_entry): - """This is called for reviewer tools originated decisions. - See process_decision for cinder originated decisions.""" - abuse_report_or_decision = ( - self.appealed_decisions.first() or self.abusereport_set.first() - ) - if isinstance(abuse_report_or_decision, AbuseReport) and self.target_addon_id: - # set the cached_property of AbuseReport.addon - abuse_report_or_decision.addon = self.target_addon - entity_helper = self.get_entity_helper( - abuse_report_or_decision.target, - resolved_in_reviewer_tools=self.resolvable_in_reviewer_tools, - ) - - decision = ContentDecision( - addon=abuse_report_or_decision.addon, - rating=abuse_report_or_decision.rating, - collection=abuse_report_or_decision.collection, - user=abuse_report_or_decision.user, - override_of=self.decision, - ) - if is_first_decision := self.decision is None: - decision.cinder_job = self - - decision.notify_reviewer_decision( - log_entry=log_entry, - entity_helper=entity_helper, - appealed_action=getattr(self.appealed_decisions.first(), 'action', None), - ) - if is_first_decision: - self.update(decision=decision) - - if decision.is_delayed: - version_list = log_entry.versionlog_set.values_list('version', flat=True) - self.pending_rejections.add( - *VersionReviewerFlags.objects.filter(version__in=version_list) - ) - else: - self.pending_rejections.clear() - if decision.addon_id: - self.clear_needs_human_review_flags() - def clear_needs_human_review_flags(self): from olympia.reviewers.models import NeedsHumanReview @@ -979,6 +936,9 @@ class ContentDecision(ModelBase): user = models.ForeignKey(UserProfile, null=True, on_delete=models.SET_NULL) rating = models.ForeignKey(Rating, null=True, on_delete=models.SET_NULL) collection = models.ForeignKey(Collection, null=True, on_delete=models.SET_NULL) + activities = models.ManyToManyField( + to='activity.ActivityLog', through='activity.ContentDecisionLog' + ) class Meta: db_table = 'abuse_cinderdecision' @@ -1084,10 +1044,21 @@ def get_action_helper_class(cls, decision_action): DECISION_ACTIONS.AMO_LEGAL_FORWARD: ContentActionForwardToLegal, }.get(decision_action, ContentActionNotImplemented) - def get_action_helper(self, *, overridden_action=None, appealed_action=None): + def get_action_helper(self): # Base case when it's a new decision, that wasn't an appeal ContentActionClass = self.get_action_helper_class(self.action) skip_reporter_notify = False + any_cinder_job = self.originating_job + overridden_action = ( + self.override_of + and self.override_of.action_date + and self.override_of.action + ) + appealed_action = ( + getattr(any_cinder_job.appealed_decisions.first(), 'action', None) + if any_cinder_job + else None + ) if appealed_action: # target appeal @@ -1233,130 +1204,94 @@ def appeal(self, *, abuse_report, appeal_text, user, is_reporter): **({'reporter_report': abuse_report} if is_reporter else {}), ) - def notify_reviewer_decision( - self, - *, - log_entry, - entity_helper, - appealed_action=None, - ): - """Calling this method calls Cinder to create a decision, or notifies the - content owner/reporter by email, or both. - - If a decision is created in cinder the instance will be saved, along with - relevant policies; if a cinder decision isn't need the instance won't be saved. + def report_to_cinder(self, entity_helper): + """Report the decision to Cinder if: + - not already reported, and + - has an associated Job in Cinder, or not an action we skip reporting for. """ - if not DECISION_ACTIONS.has_constant( - log_entry.details.get('cinder_action', '') - ): - raise ImproperlyConfigured( - 'Missing or invalid cinder_action in activity log details passed to ' - 'notify_reviewer_decision' - ) - overridden_action = ( - self.override_of - and self.override_of.action_date - and self.override_of.action - ) - self.action = DECISION_ACTIONS.for_constant( - log_entry.details['cinder_action'] - ).value - self.notes = log_entry.details.get('comments', '') - policies = {cpl.cinder_policy for cpl in log_entry.cinderpolicylog_set.all()} + if self.cinder_id: + # We don't need to report the decision if it's already been reported + return any_cinder_job = self.originating_job - current_cinder_job = getattr(self, 'cinder_job', None) if self.action not in DECISION_ACTIONS.SKIP_DECISION or any_cinder_job: # we don't create cinder decisions for approvals that aren't resolving a job create_decision_kw = { - 'action': self.action.api_value, + 'action': DECISION_ACTIONS.for_value(self.action).api_value, 'reasoning': self.notes, - 'policy_uuids': [policy.uuid for policy in policies], + 'policy_uuids': list(self.policies.values_list('uuid', flat=True)), } - if current_cinder_job: + if current_cinder_job := getattr(self, 'cinder_job', None): decision_cinder_id = entity_helper.create_job_decision( job_id=current_cinder_job.job_id, **create_decision_kw ) else: decision_cinder_id = entity_helper.create_decision(**create_decision_kw) - with atomic(): - self.cinder_id = decision_cinder_id - self.action_date = datetime.now() - self.save() - self.policies.set(policies) - self.contentdecisionlog_set.create(activity_log=log_entry) - - action_helper = self.get_action_helper( - overridden_action=overridden_action, appealed_action=appealed_action - ) - - # TODO: generalize this hack -we shouldn't need to special case specific actions - # This will be rewritten for https://mozilla-hub.atlassian.net/browse/AMOENG-1125 - if self.action == DECISION_ACTIONS.AMO_LEGAL_FORWARD: - action_helper.process_action() - - if any_cinder_job: - any_cinder_job.notify_reporters(action_helper) - version_numbers = log_entry.versionlog_set.values_list( - 'version__version', flat=True - ) - is_auto_approval = ( - self.action == DECISION_ACTIONS.AMO_APPROVE_VERSION - and not log_entry.details.get('human_review', True) - ) - action_helper.notify_owners( - log_entry_id=log_entry.id, - extra_context={ - 'auto_approval': is_auto_approval, - 'delayed_rejection_days': log_entry.details.get( - 'delayed_rejection_days' - ), - 'is_addon_being_blocked': log_entry.details.get( - 'is_addon_being_blocked' - ), - 'is_addon_disabled': log_entry.details.get('is_addon_being_disabled') - or self.target.is_disabled, - # Because we expand the reason/policy text into notes in the reviewer - # tools, we don't want to duplicate it as policies too. - 'policies': policies if not self.notes else (), - 'version_list': ', '.join(ver_str for ver_str in version_numbers), - 'has_attachment': hasattr(log_entry, 'attachmentlog'), - 'dev_url': absolutify(self.target.get_dev_url('versions')) - if self.addon_id - else None, - }, - ) + self.update(cinder_id=decision_cinder_id) - def process_action(self, *, release_hold=False): - """currently only called by decisions from cinder. - see https://mozilla-hub.atlassian.net/browse/AMOENG-1125 - """ - assert not self.action_date # we should not be attempting to process twice + def execute_action_and_notify(self, *, release_hold=False): + """Execute the action for the decision, if not already carried out. + The action may be held for 2nd level approval. + If the action has been carried out, notify interested parties""" + action_helper = self.get_action_helper() cinder_job = self.originating_job - appealed_action = ( - getattr(cinder_job.appealed_decisions.first(), 'action', None) - if cinder_job - else None - ) + if not self.action_date: + # We should not be attempting to process twice + if release_hold or not action_helper.should_hold_action(): + # We set the action_date because .can_be_appealed depends on it + self.action_date = datetime.now() + action_helper.process_action() + # But only save it afterwards in case process_action failed + self.save(update_fields=('action_date',)) + else: + action_helper.hold_action() - overridden_action = ( - self.override_of - and self.override_of.action_date - and self.override_of.action - ) - action_helper = self.get_action_helper( - overridden_action=overridden_action, - appealed_action=appealed_action, - ) - if release_hold or not action_helper.should_hold_action(): - self.action_date = datetime.now() - log_entry = action_helper.process_action() + log_entry = self.activities.first() + + if self.action_date: if cinder_job: cinder_job.notify_reporters(action_helper) - action_helper.notify_owners(log_entry_id=getattr(log_entry, 'id', None)) - self.save(update_fields=('action_date',)) - else: - action_helper.hold_action() + + version_numbers = ( + log_entry.versionlog_set.values_list('version__version', flat=True) + if log_entry + else [] + ) + details = (log_entry and log_entry.details) or {} + is_auto_approval = ( + self.action == DECISION_ACTIONS.AMO_APPROVE_VERSION + and not details.get('human_review', True) + ) + action_helper.notify_owners( + log_entry_id=getattr(log_entry, 'id', None), + extra_context={ + 'auto_approval': is_auto_approval, + 'delayed_rejection_days': details.get('delayed_rejection_days'), + 'is_addon_being_blocked': details.get('is_addon_being_blocked'), + 'is_addon_disabled': details.get('is_addon_being_disabled') + or getattr(self.target, 'is_disabled', False), + 'version_list': ', '.join(ver_str for ver_str in version_numbers), + 'has_attachment': hasattr(log_entry, 'attachmentlog'), + 'dev_url': absolutify(self.target.get_dev_url('versions')) + if self.addon_id + else None, + # Because we expand the reason/policy text into notes in the + # reviewer tools, we don't want to duplicate it as policies too. + **({'policies': ()} if self.notes else {}), + }, + ) + if cinder_job and self.addon_id: + if self.is_delayed: + cinder_job.pending_rejections.add( + *VersionReviewerFlags.objects.filter( + version__in=log_entry.versionlog_set.values_list( + 'version', flat=True + ) + ) + ) + else: + cinder_job.pending_rejections.clear() + cinder_job.clear_needs_human_review_flags() def get_target_review_url(self): return reverse('reviewers.decision_review', kwargs={'decision_id': self.id}) diff --git a/src/olympia/abuse/tasks.py b/src/olympia/abuse/tasks.py index 427fd72d1d78..8d871c86dd3e 100644 --- a/src/olympia/abuse/tasks.py +++ b/src/olympia/abuse/tasks.py @@ -9,7 +9,6 @@ import olympia.core.logger 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 @@ -123,36 +122,20 @@ def appeal_to_cinder( @task @use_primary_db -def resolve_job_in_cinder(*, cinder_job_id, log_entry_id): +def report_decision_to_cinder_and_notify(*, decision_id): try: - cinder_job = CinderJob.objects.get(id=cinder_job_id) - log_entry = ActivityLog.objects.get(id=log_entry_id) - cinder_job.resolve_job(log_entry=log_entry) - except Exception: - statsd.incr('abuse.tasks.resolve_job_in_cinder.failure') - raise - else: - statsd.incr('abuse.tasks.resolve_job_in_cinder.success') - - -@task -@use_primary_db -def notify_addon_decision_to_cinder(*, log_entry_id, addon_id=None): - try: - log_entry = ActivityLog.objects.get(id=log_entry_id) - addon = Addon.unfiltered.get(id=addon_id) - decision = ContentDecision(addon=addon) - decision.notify_reviewer_decision( - log_entry=log_entry, - entity_helper=CinderJob.get_entity_helper( - decision.target, resolved_in_reviewer_tools=True - ), + decision = ContentDecision.objects.get(id=decision_id) + entity_helper = CinderJob.get_entity_helper( + decision.target, + resolved_in_reviewer_tools=True, ) + decision.report_to_cinder(entity_helper) + decision.execute_action_and_notify() except Exception: - statsd.incr('abuse.tasks.notify_addon_decision_to_cinder.failure') + statsd.incr('abuse.tasks.report_decision_to_cinder_and_notify.failure') raise else: - statsd.incr('abuse.tasks.notify_addon_decision_to_cinder.success') + statsd.incr('abuse.tasks.report_decision_to_cinder_and_notify.success') @task diff --git a/src/olympia/abuse/tests/test_actions.py b/src/olympia/abuse/tests/test_actions.py index 1331167a7627..7fabe5960714 100644 --- a/src/olympia/abuse/tests/test_actions.py +++ b/src/olympia/abuse/tests/test_actions.py @@ -344,10 +344,7 @@ def _test_ban_user(self): assert ActivityLog.objects.count() == 1 assert activity.arguments == [self.user, self.decision, self.policy] assert activity.user == self.task_user - assert activity.details == { - 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_BAN_USER, - } + assert activity.details == {'comments': self.decision.notes} self.user.reload() self.assertCloseToNow(self.user.banned) @@ -411,10 +408,7 @@ def _test_approve_appeal_or_override(self, ContentActionClass): assert activity.log == amo.LOG.ADMIN_USER_UNBAN assert activity.arguments == [self.user, self.decision, self.policy] assert activity.user == self.task_user - assert activity.details == { - 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_APPROVE, - } + assert activity.details == {'comments': self.decision.notes} assert len(mail.outbox) == 0 self.cinder_job.notify_reporters(action) @@ -467,10 +461,7 @@ def test_hold_action(self): assert ActivityLog.objects.count() == 1 assert activity.arguments == [self.user, self.decision, self.policy] assert activity.user == self.task_user - assert activity.details == { - 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_BAN_USER, - } + assert activity.details == {'comments': self.decision.notes} @override_switch('dsa-cinder-forwarded-review', active=True) @@ -833,10 +824,7 @@ def test_hold_action(self): assert ActivityLog.objects.count() == 1 assert activity.arguments == [self.addon, self.decision, self.policy] assert activity.user == self.task_user - assert activity.details == { - 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_DISABLE_ADDON, - } + assert activity.details == {'comments': self.decision.notes} def test_forward_to_reviewers_no_job(self): self.decision.update(action=DECISION_ACTIONS.AMO_LEGAL_FORWARD) @@ -1013,10 +1001,7 @@ def test_hold_action(self): assert ActivityLog.objects.count() == 1 assert activity.arguments == [self.collection, self.decision, self.policy] assert activity.user == self.task_user - assert activity.details == { - 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_DELETE_COLLECTION, - } + assert activity.details == {'comments': self.decision.notes} class TestContentActionRating(BaseTestContentAction, TestCase): @@ -1047,7 +1032,6 @@ def _test_delete_rating(self): assert activity.user == self.task_user assert activity.details == { 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_DELETE_RATING, 'addon_id': self.rating.addon_id, 'addon_title': str(self.rating.addon.name), 'body': self.rating.body, @@ -1120,7 +1104,6 @@ def _test_approve_appeal_or_override(self, ContentActionClass): assert activity.user == self.task_user assert activity.details == { 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_APPROVE, 'addon_id': self.rating.addon_id, 'addon_title': str(self.rating.addon.name), 'body': self.rating.body, @@ -1184,7 +1167,4 @@ def test_hold_action(self): self.rating.addon, ] assert activity.user == self.task_user - assert activity.details == { - 'comments': self.decision.notes, - 'cinder_action': DECISION_ACTIONS.AMO_DELETE_RATING, - } + assert activity.details == {'comments': self.decision.notes} diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index 24f7e602aab2..2ffa0248dda9 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -42,6 +42,7 @@ ContentActionBanUser, ContentActionDeleteCollection, ContentActionDeleteRating, + ContentActionDisableAddon, ContentActionOverrideApprove, ContentActionTargetAppealApprove, ContentActionTargetAppealRemovalAffirmation, @@ -1294,372 +1295,6 @@ def test_process_queue_move_other_queue_movement(self): cinder_job=cinder_job, to_queue='amo-env-some-other-queue', notes='?' ).exists() - 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') - flags = version_review_flags_factory( - version=addon.current_version, - pending_rejection=self.days_ago(1), - pending_rejection_by=user_factory(), - pending_content_rejection=False, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, - ) - # pretend there is a pending rejection that's resolving this job - cinder_job.pending_rejections.add(flags) - abuse_report = AbuseReport.objects.create( - guid=addon.guid, - reason=AbuseReport.REASONS.POLICY_VIOLATION, - location=AbuseReport.LOCATION.ADDON, - cinder_job=cinder_job, - reporter=user_factory(), - ) - responses.add( - responses.POST, - f'{settings.CINDER_SERVER_URL}jobs/{cinder_job.job_id}/decision', - json={'uuid': uuid.uuid4().hex}, - status=201, - ) - policies = [CinderPolicy.objects.create(name='policy', uuid='12345678')] - - log_entry = ActivityLog.objects.create( - activity_action, - abuse_report.target, - abuse_report.target.current_version, - *policies, - details={ - 'comments': 'some review text', - 'cinder_action': cinder_action.constant, - }, - user=user_factory(), - ) - - cinder_job.resolve_job(log_entry=log_entry) - - request = responses.calls[0].request - 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 == cinder_action - self.assertCloseToNow(cinder_job.decision.action_date) - assert list(cinder_job.decision.policies.all()) == policies - assert len(mail.outbox) == (2 if expect_target_email else 1) - assert mail.outbox[0].to == [abuse_report.reporter.email] - assert 'requested the developer' not in mail.outbox[0].body - if expect_target_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 - ) - assert 'days' not in mail.outbox[1].body - assert cinder_job.pending_rejections.count() == 0 - assert not NeedsHumanReview.objects.filter( - is_active=True, reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION - ).exists() - assert NeedsHumanReview.objects.filter(is_active=True).count() == 2 - assert ( - log_entry.reload().contentdecisionlog_set.get().decision - == cinder_job.decision - ) - - def test_resolve_job_notify_owner(self): - self._test_resolve_job( - amo.LOG.REJECT_VERSION, - DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, - expect_target_email=True, - ) - - def test_resolve_job_no_email_to_owner(self): - self._test_resolve_job( - amo.LOG.CONFIRM_AUTO_APPROVED, - DECISION_ACTIONS.AMO_APPROVE, - expect_target_email=False, - ) - - def test_resolve_job_delayed(self): - cinder_job = CinderJob.objects.create(job_id='999') - addon_developer = user_factory() - abuse_report = AbuseReport.objects.create( - guid=addon_factory(users=[addon_developer]).guid, - reason=AbuseReport.REASONS.POLICY_VIOLATION, - location=AbuseReport.LOCATION.ADDON, - cinder_job=cinder_job, - reporter=user_factory(), - ) - policies = [CinderPolicy.objects.create(name='policy', uuid='12345678')] - responses.add( - responses.POST, - f'{settings.CINDER_SERVER_URL}jobs/{cinder_job.job_id}/decision', - json={'uuid': uuid.uuid4().hex}, - status=201, - ) - log_entry = ActivityLog.objects.create( - amo.LOG.REJECT_VERSION_DELAYED, - abuse_report.target, - abuse_report.target.current_version, - *policies, - details={ - 'comments': 'some review text', - 'delayed_rejection_days': '14', - 'cinder_action': 'AMO_REJECT_VERSION_WARNING_ADDON', - }, - user=user_factory(), - ) - NeedsHumanReview.objects.create( - reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, - version=abuse_report.target.current_version, - ) - assert abuse_report.target.current_version.due_date - - cinder_job.resolve_job(log_entry=log_entry) - - cinder_job.reload() - assert cinder_job.decision.action == ( - DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON - ) - self.assertCloseToNow(cinder_job.decision.action_date) - assert list(cinder_job.decision.policies.all()) == policies - assert set(cinder_job.pending_rejections.all()) == set( - VersionReviewerFlags.objects.filter( - version=abuse_report.target.current_version - ) - ) - assert len(mail.outbox) == 2 - assert mail.outbox[0].to == [abuse_report.reporter.email] - assert 'requested the developer' in mail.outbox[0].body - 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 - assert '14 day(s)' in mail.outbox[1].body - assert not NeedsHumanReview.objects.filter(is_active=True).exists() - abuse_report.target.current_version.reload() - assert not abuse_report.target.current_version.due_date - assert ( - log_entry.reload().contentdecisionlog_set.get().decision - == cinder_job.decision - ) - - def test_resolve_job_appeal_not_third_party(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - appeal_job = CinderJob.objects.create( - job_id='999', - ) - CinderJob.objects.create( - job_id='998', - decision=ContentDecision.objects.create( - addon=addon, action=DECISION_ACTIONS.AMO_APPROVE, appeal_job=appeal_job - ), - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, - ) - responses.add( - responses.POST, - f'{settings.CINDER_SERVER_URL}jobs/{appeal_job.job_id}/decision', - json={'uuid': uuid.uuid4().hex}, - status=201, - ) - policies = [CinderPolicy.objects.create(name='policy', uuid='12345678')] - log_entry = ActivityLog.objects.create( - amo.LOG.FORCE_DISABLE, - addon, - addon.current_version, - *policies, - details={ - 'comments': 'some review text', - 'cinder_action': 'AMO_DISABLE_ADDON', - }, - user=user_factory(), - ) - - appeal_job.resolve_job(log_entry=log_entry) - - request = responses.calls[0].request - 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 - appeal_job.reload() - assert appeal_job.decision.action == DECISION_ACTIONS.AMO_DISABLE_ADDON - self.assertCloseToNow(appeal_job.decision.action_date) - assert list(appeal_job.decision.policies.all()) == policies - assert len(mail.outbox) == 1 - - assert mail.outbox[0].to == [addon_developer.email] - assert str(log_entry.id) in mail.outbox[0].extra_headers['Message-ID'] - assert 'some review text' in mail.outbox[0].body - assert 'days' not in mail.outbox[0].body - assert 'in an assessment performed on our own initiative' in mail.outbox[0].body - assert appeal_job.pending_rejections.count() == 0 - assert not NeedsHumanReview.objects.filter( - is_active=True, reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL - ).exists() - assert NeedsHumanReview.objects.filter(is_active=True).count() == 2 - assert ( - log_entry.reload().contentdecisionlog_set.get().decision - == appeal_job.decision - ) - - def test_resolve_job_appeal_with_new_report(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - appeal_job = CinderJob.objects.create( - job_id='999', - ) - AbuseReport.objects.create( - reporter_email='reporter@email.com', cinder_job=appeal_job, guid=addon.guid - ) - CinderJob.objects.create( - job_id='998', - decision=ContentDecision.objects.create( - addon=addon, action=DECISION_ACTIONS.AMO_APPROVE, appeal_job=appeal_job - ), - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, - ) - responses.add( - responses.POST, - f'{settings.CINDER_SERVER_URL}jobs/{appeal_job.job_id}/decision', - json={'uuid': uuid.uuid4().hex}, - status=201, - ) - policies = [CinderPolicy.objects.create(name='policy', uuid='12345678')] - log_entry = ActivityLog.objects.create( - amo.LOG.FORCE_DISABLE, - addon, - addon.current_version, - *policies, - details={ - 'comments': 'some review text', - 'cinder_action': 'AMO_DISABLE_ADDON', - }, - user=user_factory(), - ) - - appeal_job.resolve_job(log_entry=log_entry) - - appeal_job.reload() - assert appeal_job.decision.action == DECISION_ACTIONS.AMO_DISABLE_ADDON - assert len(mail.outbox) == 2 - - assert mail.outbox[1].to == [addon_developer.email] - assert str(log_entry.id) in mail.outbox[1].extra_headers['Message-ID'] - assert 'assessment performed on our own initiative' not in mail.outbox[1].body - assert mail.outbox[0].to == ['reporter@email.com'] - assert not NeedsHumanReview.objects.filter( - is_active=True, reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL - ).exists() - # We are only removing NHR with the reason matching what we're doing. - assert NeedsHumanReview.objects.filter( - is_active=True, reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION - ).exists() - assert NeedsHumanReview.objects.filter(is_active=True).count() == 2 - assert ( - log_entry.reload().contentdecisionlog_set.get().decision - == appeal_job.decision - ) - - def test_resolve_job_forwarded(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - cinder_job = CinderJob.objects.create(job_id='999') - CinderJob.objects.create(forwarded_to_job=cinder_job) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.CINDER_ESCALATION, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ADDON_REVIEW_APPEAL, - ) - NeedsHumanReview.objects.create( - version=addon.current_version, - reason=NeedsHumanReview.REASONS.ABUSE_ADDON_VIOLATION, - ) - abuse_report = AbuseReport.objects.create( - guid=addon.guid, - reason=AbuseReport.REASONS.POLICY_VIOLATION, - location=AbuseReport.LOCATION.ADDON, - cinder_job=cinder_job, - reporter=user_factory(), - ) - responses.add( - responses.POST, - f'{settings.CINDER_SERVER_URL}jobs/{cinder_job.job_id}/decision', - json={'uuid': uuid.uuid4().hex}, - status=201, - ) - policies = [CinderPolicy.objects.create(name='policy', uuid='12345678')] - - log_entry = ActivityLog.objects.create( - amo.LOG.FORCE_DISABLE, - abuse_report.target, - abuse_report.target.current_version, - *policies, - details={ - 'comments': 'some review text', - 'cinder_action': 'AMO_DISABLE_ADDON', - }, - user=user_factory(), - ) - - cinder_job.resolve_job(log_entry=log_entry) - - request = responses.calls[0].request - request_body = json.loads(request.body) - assert request_body['policy_uuids'] == ['12345678'] - assert request_body['reasoning'] == 'some review text' - cinder_job.reload() - assert cinder_job.decision.action == DECISION_ACTIONS.AMO_DISABLE_ADDON - self.assertCloseToNow(cinder_job.decision.action_date) - assert list(cinder_job.decision.policies.all()) == policies - assert len(mail.outbox) == 2 - assert mail.outbox[0].to == [abuse_report.reporter.email] - assert 'requested the developer' not in mail.outbox[0].body - 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 not NeedsHumanReview.objects.filter( - is_active=True, reason=NeedsHumanReview.REASONS.CINDER_ESCALATION - ).exists() - assert NeedsHumanReview.objects.filter(is_active=True).count() == 2 - def test_all_abuse_reports(self): job = CinderJob.objects.create(job_id='fake_job_id') assert list(job.all_abuse_reports) == [] @@ -2383,6 +2018,16 @@ def test_get_action_helper(self): decision = ContentDecision.objects.create( action=DECISION_ACTIONS.AMO_DISABLE_ADDON, addon=addon ) + overriden_decision = ContentDecision.objects.create( + action=DECISION_ACTIONS.AMO_DISABLE_ADDON, + addon=addon, + action_date=datetime.now(), + ) + appealed_decision = ContentDecision.objects.create( + action=DECISION_ACTIONS.AMO_DISABLE_ADDON, + addon=addon, + appeal_job=CinderJob.objects.create(job_id='1234'), + ) targets = { ContentActionBanUser: {'user': user_factory()}, ContentActionDeleteCollection: {'collection': collection_factory()}, @@ -2430,6 +2075,7 @@ def test_get_action_helper(self): appealed_action, ), ActionClass in action_existing_to_class.items(): decision.update( + override_of=None, **{ 'action': new_action, 'addon': None, @@ -2437,11 +2083,17 @@ def test_get_action_helper(self): 'collection': None, 'user': None, **targets.get(ActionClass, {'addon': addon}), - } - ) - helper = decision.get_action_helper( - appealed_action=appealed_action, overridden_action=overridden_action + }, ) + if overridden_action: + decision.update(override_of=overriden_decision) + overriden_decision.update(action=overridden_action) + if appealed_action: + appealed_decision.appeal_job.update(decision=decision) + appealed_decision.update(action=appealed_action) + else: + appealed_decision.appeal_job.update(decision=None) + helper = decision.get_action_helper() assert helper.__class__ == ActionClass assert helper.decision == decision assert helper.reporter_template_path == ActionClass.reporter_template_path @@ -2459,6 +2111,7 @@ def test_get_action_helper(self): overridden_action, ), ActionClass in action_existing_to_class_no_reporter_emails.items(): decision.update( + override_of=None, **{ 'action': new_action, 'addon': None, @@ -2466,11 +2119,12 @@ def test_get_action_helper(self): 'collection': None, 'user': None, **targets.get(ActionClass, {'addon': addon}), - } - ) - helper = decision.get_action_helper( - appealed_action=None, overridden_action=overridden_action + }, ) + if overridden_action: + decision.update(override_of=overriden_decision) + overriden_decision.update(action=overridden_action) + helper = decision.get_action_helper() assert helper.reporter_template_path is None assert helper.reporter_appeal_template_path is None assert ActionClass.reporter_template_path is not None @@ -2872,61 +2526,33 @@ def test_appeal_improperly_configured_author(self): is_reporter=False, ) - def _test_notify_reviewer_decision( - self, - decision, - activity_action, - cinder_action, - *, - expect_email=True, - expect_create_decision_call, - expect_create_job_decision_call, - extra_log_details=None, - expected_decision_object_count=1, + def _test_report_to_cinder( + self, decision, *, expect_create_decision_call, expect_create_job_decision_call ): + cinder_job_id = (job := getattr(decision, 'cinder_job', None)) and job.job_id create_decision_response = responses.add( responses.POST, f'{settings.CINDER_SERVER_URL}create_decision', json={'uuid': uuid.uuid4().hex}, status=201, ) - cinder_job_id = (job := getattr(decision, 'cinder_job', None)) and job.job_id create_job_decision_response = responses.add( responses.POST, f'{settings.CINDER_SERVER_URL}jobs/{cinder_job_id}/decision', json={'uuid': uuid.uuid4().hex}, status=201, ) - policies = [ - CinderPolicy.objects.create( - name='policy', uuid='12345678', text='some policy text' - ) - ] - entity_helper = CinderJob.get_entity_helper( - decision.addon, resolved_in_reviewer_tools=True - ) - addon_version = decision.addon.versions.all()[0] - cinder_action = cinder_action or getattr(activity_action, 'cinder_action', None) - log_entry = ActivityLog.objects.create( - activity_action, - decision.addon, - addon_version, - *policies, - details={ - 'comments': 'some review text', - 'cinder_action': cinder_action.constant, - **(extra_log_details or {}), - }, - user=user_factory(), + decision.policies.add( + CinderPolicy.objects.create(name='policy', uuid='12345678') ) + decision.update(notes='some review text') - decision.notify_reviewer_decision( - log_entry=log_entry, - entity_helper=entity_helper, + decision.report_to_cinder( + CinderJob.get_entity_helper( + decision.target, resolved_in_reviewer_tools=False + ) ) - assert decision.action == cinder_action - assert decision.notes == 'some review text' if expect_create_decision_call: assert create_decision_response.call_count == 1 assert create_job_decision_response.call_count == 0 @@ -2936,11 +2562,8 @@ def _test_notify_reviewer_decision( assert request_body['reasoning'] == 'some review text' assert request_body['entity']['id'] == str(decision.addon.id) assert request_body['enforcement_actions_slugs'] == [ - cinder_action.api_value + decision.action.api_value ] - self.assertCloseToNow(decision.action_date) - assert list(decision.policies.all()) == policies - assert decision.id elif expect_create_job_decision_call: assert create_decision_response.call_count == 0 assert create_job_decision_response.call_count == 1 @@ -2950,436 +2573,186 @@ def _test_notify_reviewer_decision( assert request_body['reasoning'] == 'some review text' assert 'entity' not in request_body assert request_body['enforcement_actions_slugs'] == [ - cinder_action.api_value + decision.action.api_value ] - self.assertCloseToNow(decision.action_date) - assert list(decision.policies.all()) == policies - assert decision.id else: assert create_decision_response.call_count == 0 assert create_job_decision_response.call_count == 0 - assert CinderPolicy.contentdecision_set.through.objects.count() == 0 - assert not decision.id - assert ContentDecision.objects.count() == expected_decision_object_count - if expected_decision_object_count > 0: - assert log_entry.reload().contentdecisionlog_set.get().decision == decision - - if expect_email: - assert len(mail.outbox) == 1 - assert mail.outbox[0].to == [decision.addon.authors.first().email] - assert str(log_entry.id) in mail.outbox[0].extra_headers['Message-ID'] - assert str(addon_version) in mail.outbox[0].body - assert 'days' not in mail.outbox[0].body - assert 'some review text' in mail.outbox[0].body - assert 'some policy text' not in mail.outbox[0].body - AttachmentLog.objects.create( - activity_log=log_entry, - file=ContentFile('Pseudo File', name='attachment.txt'), - ) - decision.notify_reviewer_decision( - log_entry=log_entry, - entity_helper=entity_helper, - ) - assert 'An attachment was provided.' not in mail.outbox[0].body - assert 'To respond or view the file,' not in mail.outbox[0].body - assert 'An attachment was provided.' in mail.outbox[1].body - assert 'To respond or view the file,' in mail.outbox[1].body - else: - assert len(mail.outbox) == 0 - - def test_notify_reviewer_decision_first_decision(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - decision = ContentDecision(addon=addon) - self._test_notify_reviewer_decision( - 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 - - def test_notify_reviewer_decision_override_decision(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - previous_decision = ContentDecision.objects.create( - addon=addon, - action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, - action_date=datetime.now(), - ) - decision = ContentDecision(addon=addon, override_of=previous_decision) - self._test_notify_reviewer_decision( - decision, - amo.LOG.REJECT_VERSION, - DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, - expect_create_decision_call=True, - expect_create_job_decision_call=False, - expected_decision_object_count=2, - ) - assert parse.quote(f'/firefox/addon/{addon.slug}/') in mail.outbox[0].body - assert '/developers/' not in mail.outbox[0].body - def test_notify_reviewer_decision_unlisted_version(self): - addon_developer = user_factory() - addon = addon_factory( - users=[addon_developer], version_kw={'channel': amo.CHANNEL_UNLISTED} + def test_report_to_cinder_disable(self): + decision = ContentDecision.objects.create( + addon=addon_factory(), action=DECISION_ACTIONS.AMO_DISABLE_ADDON ) - decision = ContentDecision(addon=addon) - self._test_notify_reviewer_decision( + self._test_report_to_cinder( 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 ( - f'{settings.SITE_URL}/en-US/developers/addon/{addon.id}/' - in mail.outbox[0].body - ) - def test_notify_reviewer_decision_first_decision_no_email_to_owner(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - decision = ContentDecision(addon=addon) - decision.cinder_job = CinderJob.objects.create(job_id='1234') - self._test_notify_reviewer_decision( - decision, - amo.LOG.CONFIRM_AUTO_APPROVED, - DECISION_ACTIONS.AMO_APPROVE, - expect_email=False, - expect_create_decision_call=False, - expect_create_job_decision_call=True, - ) - - def test_notify_reviewer_decision_override_decision_no_email_to_owner(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - previous_decision = ContentDecision.objects.create( - addon=addon, - action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, - action_date=datetime.now(), - ) - previous_decision.cinder_job = CinderJob.objects.create( - job_id='1234', decision=previous_decision - ) - decision = ContentDecision(addon=addon, override_of=previous_decision) - self._test_notify_reviewer_decision( - decision, - amo.LOG.CONFIRM_AUTO_APPROVED, - DECISION_ACTIONS.AMO_APPROVE, - expect_email=False, - expect_create_decision_call=True, - expect_create_job_decision_call=False, - expected_decision_object_count=2, + def test_report_to_cinder_approve_no_job(self): + decision = ContentDecision.objects.create( + addon=addon_factory(), action=DECISION_ACTIONS.AMO_APPROVE ) - - def test_no_create_decision_for_approve_without_a_job(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - decision = ContentDecision(addon=addon) - assert not hasattr(decision, 'cinder_job') - self._test_notify_reviewer_decision( + self._test_report_to_cinder( decision, - amo.LOG.APPROVE_VERSION, - DECISION_ACTIONS.AMO_APPROVE_VERSION, expect_create_decision_call=False, expect_create_job_decision_call=False, - expect_email=True, - expected_decision_object_count=0, ) - def test_notify_reviewer_decision_auto_approve_email_for_non_human_review(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - decision = ContentDecision(addon=addon) - self._test_notify_reviewer_decision( - decision, - amo.LOG.APPROVE_VERSION, - DECISION_ACTIONS.AMO_APPROVE_VERSION, - expect_email=True, - expect_create_decision_call=False, - expect_create_job_decision_call=False, - expected_decision_object_count=0, - extra_log_details={'human_review': False}, + def test_report_to_cinder_approve_with_job(self): + decision = ContentDecision.objects.create( + addon=addon_factory(), action=DECISION_ACTIONS.AMO_APPROVE ) - assert 'automatically screened and tentatively approved' in mail.outbox[0].body - - def test_notify_reviewer_decision_auto_approve_email_for_human_review(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - decision = ContentDecision(addon=addon) - self._test_notify_reviewer_decision( + CinderJob.objects.create(job_id='123', decision=decision) + self._test_report_to_cinder( decision, - amo.LOG.APPROVE_VERSION, - DECISION_ACTIONS.AMO_APPROVE_VERSION, - expect_email=True, expect_create_decision_call=False, - expect_create_job_decision_call=False, - expected_decision_object_count=0, - extra_log_details={'human_review': True}, - ) - assert 'has been approved' in mail.outbox[0].body - - def test_notify_reviewer_decision_no_cinder_action_in_activity_log(self): - addon = addon_factory() - log_entry = ActivityLog.objects.create( - amo.LOG.APPROVE_VERSION, - addon, - addon.current_version, - details={'comments': 'some review text'}, - user=user_factory(), + expect_create_job_decision_call=True, ) - with self.assertRaises(ImproperlyConfigured): - ContentDecision().notify_reviewer_decision( - log_entry=log_entry, entity_helper=None - ) - - def test_notify_reviewer_decision_invalid_cinder_action_in_activity_log(self): + def test_report_to_cinder_approve_with_job_via_override(self): addon = addon_factory() - log_entry = ActivityLog.objects.create( - amo.LOG.APPROVE_VERSION, - addon, - addon.current_version, - details={'comments': 'some review text', 'cinder_action': 'NOT_AN_ACTION'}, - user=user_factory(), - ) - - with self.assertRaises(ImproperlyConfigured): - ContentDecision().notify_reviewer_decision( - log_entry=log_entry, entity_helper=None - ) - - def test_notify_reviewer_decision_rejection_blocking(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - decision = ContentDecision(addon=addon) - self._test_notify_reviewer_decision( - 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, - }, - ) - assert ( - 'Users who have previously installed those versions will be able to' - not in mail.outbox[0].body - ) - assert ( - "users who have previously installed those versions won't be able to" - in mail.outbox[0].body + first_decision = ContentDecision.objects.create( + addon=addon, action=DECISION_ACTIONS.AMO_DISABLE_ADDON ) - assert ( - 'You may upload a new version which addresses the policy violation(s)' - in mail.outbox[0].body + override = ContentDecision.objects.create( + addon=addon, action=DECISION_ACTIONS.AMO_APPROVE, override_of=first_decision ) - - def test_notify_reviewer_decision_rejection_blocking_addon_being_disabled(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer]) - decision = ContentDecision(addon=addon) - self._test_notify_reviewer_decision( - decision, - amo.LOG.REJECT_VERSION, - DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + CinderJob.objects.create(job_id='123', decision=first_decision) + self._test_report_to_cinder( + override, expect_create_decision_call=True, expect_create_job_decision_call=False, - extra_log_details={ - 'is_addon_being_blocked': True, - 'is_addon_being_disabled': True, - }, - ) - assert ( - 'Users who have previously installed those versions will be able to' - not in mail.outbox[0].body - ) - assert ( - "users who have previously installed those versions won't be able to" - in mail.outbox[0].body - ) - assert ( - 'You may upload a new version which addresses the policy violation(s)' - not in mail.outbox[0].body ) - def test_notify_reviewer_decision_rejection_addon_already_disabled(self): - addon_developer = user_factory() - addon = addon_factory(users=[addon_developer], status=amo.STATUS_DISABLED) - decision = ContentDecision(addon=addon) - self._test_notify_reviewer_decision( - 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' - in mail.outbox[0].body - ) - assert ( - "users who have previously installed those versions won't be able to" - not in mail.outbox[0].body - ) - assert ( - 'You may upload a new version which addresses the policy violation(s)' - not in mail.outbox[0].body - ) - - def test_notify_reviewer_decision_legal_forward(self): - """Test a reviewer "decision" to forward to legal. Because there is no job there - is no decision though, so we don't expect any decision to be notified to Cinder. - """ - addon_developer = user_factory() - # Set to disabled because we already don't create decisions for approvals. - addon = addon_factory(users=[addon_developer], status=amo.STATUS_DISABLED) - decision = ContentDecision(addon=addon) - # Check there isn't a job already so our .get later isn't a false positive. - assert not CinderJob.objects.exists() - responses.add( - responses.POST, - f'{settings.CINDER_SERVER_URL}create_report', - json={'job_id': '123456'}, - status=201, - ) - self._test_notify_reviewer_decision( - decision, - amo.LOG.REQUEST_LEGAL, - None, - # as above, we arne't making a decision on a job, so no call is expected - expect_create_decision_call=False, - expect_create_job_decision_call=False, - expected_decision_object_count=0, - # and certainly no email to the developer - expect_email=False, - ) - assert CinderJob.objects.get().job_id == '123456' - - def _test_process_action_ban_user_outcome(self, decision): + def _check_notify_emails(self, decision, log_entry): + assert len(mail.outbox) == 1 + assert mail.outbox[0].to == [decision.addon.authors.first().email] + assert str(log_entry.id) in mail.outbox[0].extra_headers['Message-ID'] + assert 'days' not in mail.outbox[0].body + assert 'some review text' in mail.outbox[0].body + assert 'some policy text' not in mail.outbox[0].body + AttachmentLog.objects.create( + activity_log=log_entry, + file=ContentFile('Pseudo File', name='attachment.txt'), + ) + decision.execute_action_and_notify() + assert 'An attachment was provided.' not in mail.outbox[0].body + assert 'To respond or view the file,' not in mail.outbox[0].body + assert 'An attachment was provided.' in mail.outbox[1].body + assert 'To respond or view the file,' in mail.outbox[1].body + + def _test_execute_action_ban_user_outcome(self, decision): self.assertCloseToNow(decision.action_date) self.assertCloseToNow(decision.user.reload().banned) - assert ( - ActivityLog.objects.filter(action=amo.LOG.ADMIN_USER_BANNED.id).count() == 1 - ) + alog = ActivityLog.objects.filter(action=amo.LOG.ADMIN_USER_BANNED.id).get() + assert alog.contentdecisionlog_set.get().decision == decision assert 'appeal' in mail.outbox[0].body - def test_process_action_ban_user_held(self): + def test_execute_action_ban_user_held(self): user = user_factory(email='superstarops@mozilla.com') decision = ContentDecision.objects.create( user=user, action=DECISION_ACTIONS.AMO_BAN_USER ) assert decision.action_date is None - decision.process_action() + decision.execute_action_and_notify() assert decision.action_date is None assert not user.reload().banned - assert ( - ActivityLog.objects.filter( - action=amo.LOG.HELD_ACTION_ADMIN_USER_BANNED.id - ).count() - == 1 - ) + alog = ActivityLog.objects.filter( + action=amo.LOG.HELD_ACTION_ADMIN_USER_BANNED.id + ).get() + assert alog.contentdecisionlog_set.get().decision == decision assert len(mail.outbox) == 0 - decision.process_action(release_hold=True) - self._test_process_action_ban_user_outcome(decision) + decision.execute_action_and_notify(release_hold=True) + self._test_execute_action_ban_user_outcome(decision) - def test_process_action_ban_user(self): + def test_execute_action_ban_user(self): user = user_factory() decision = ContentDecision.objects.create( user=user, action=DECISION_ACTIONS.AMO_BAN_USER ) assert decision.action_date is None - decision.process_action() - self._test_process_action_ban_user_outcome(decision) + decision.execute_action_and_notify() + self._test_execute_action_ban_user_outcome(decision) - def _test_process_action_disable_addon_outcome(self, decision): + def _test_execute_action_disable_addon_outcome(self, decision): self.assertCloseToNow(decision.action_date) assert decision.addon.reload().status == amo.STATUS_DISABLED - assert ActivityLog.objects.filter(action=amo.LOG.FORCE_DISABLE.id).count() == 1 + alog = ActivityLog.objects.filter(action=amo.LOG.FORCE_DISABLE.id).get() + assert alog.contentdecisionlog_set.get().decision == decision assert 'appeal' in mail.outbox[0].body - def test_process_action_disable_addon_held(self): + def test_execute_action_disable_addon_held(self): addon = addon_factory(users=[user_factory()]) self.make_addon_promoted(addon, RECOMMENDED, approve_version=True) decision = ContentDecision.objects.create( addon=addon, action=DECISION_ACTIONS.AMO_DISABLE_ADDON ) assert decision.action_date is None - decision.process_action() + decision.execute_action_and_notify() assert decision.action_date is None assert addon.reload().status == amo.STATUS_APPROVED - assert ( - ActivityLog.objects.filter( - action=amo.LOG.HELD_ACTION_FORCE_DISABLE.id - ).count() - == 1 - ) + alog = ActivityLog.objects.filter( + action=amo.LOG.HELD_ACTION_FORCE_DISABLE.id + ).get() + assert alog.contentdecisionlog_set.get().decision == decision assert len(mail.outbox) == 0 - decision.process_action(release_hold=True) - self._test_process_action_disable_addon_outcome(decision) + decision.execute_action_and_notify(release_hold=True) + self._test_execute_action_disable_addon_outcome(decision) - def test_process_action_disable_addon(self): + def test_execute_action_disable_addon(self): addon = addon_factory(users=[user_factory()]) decision = ContentDecision.objects.create( addon=addon, action=DECISION_ACTIONS.AMO_DISABLE_ADDON ) assert decision.action_date is None - decision.process_action() - self._test_process_action_disable_addon_outcome(decision) + decision.execute_action_and_notify() + self._test_execute_action_disable_addon_outcome(decision) - def _test_process_action_delete_collection_outcome(self, decision): + def _test_execute_action_delete_collection_outcome(self, decision): self.assertCloseToNow(decision.action_date) assert decision.collection.reload().deleted - assert ( - ActivityLog.objects.filter(action=amo.LOG.COLLECTION_DELETED.id).count() - == 1 - ) + alog = ActivityLog.objects.filter(action=amo.LOG.COLLECTION_DELETED.id).get() + assert alog.contentdecisionlog_set.get().decision == decision assert 'appeal' in mail.outbox[0].body - def test_process_action_delete_collection_held(self): + def test_execute_action_delete_collection_held(self): collection = collection_factory(author=self.task_user) decision = ContentDecision.objects.create( collection=collection, action=DECISION_ACTIONS.AMO_DELETE_COLLECTION ) assert decision.action_date is None - decision.process_action() + decision.execute_action_and_notify() assert decision.action_date is None assert not collection.reload().deleted - assert ( - ActivityLog.objects.filter( - action=amo.LOG.HELD_ACTION_COLLECTION_DELETED.id - ).count() - == 1 - ) + alog = ActivityLog.objects.filter( + action=amo.LOG.HELD_ACTION_COLLECTION_DELETED.id + ).get() + assert alog.contentdecisionlog_set.get().decision == decision assert len(mail.outbox) == 0 - decision.process_action(release_hold=True) - self._test_process_action_delete_collection_outcome(decision) + decision.execute_action_and_notify(release_hold=True) + self._test_execute_action_delete_collection_outcome(decision) - def test_process_action_delete_collection(self): + def test_execute_action_delete_collection(self): collection = collection_factory(author=user_factory()) decision = ContentDecision.objects.create( collection=collection, action=DECISION_ACTIONS.AMO_DELETE_COLLECTION ) assert decision.action_date is None - decision.process_action() - self._test_process_action_delete_collection_outcome(decision) + decision.execute_action_and_notify() + self._test_execute_action_delete_collection_outcome(decision) - def _test_process_action_delete_rating_outcome(self, decision): + def _test_execute_action_delete_rating_outcome(self, decision): self.assertCloseToNow(decision.action_date) assert decision.rating.reload().deleted - assert ActivityLog.objects.filter(action=amo.LOG.DELETE_RATING.id).count() == 1 + alog = ActivityLog.objects.filter(action=amo.LOG.DELETE_RATING.id).get() + assert alog.contentdecisionlog_set.get().decision == decision assert 'appeal' in mail.outbox[0].body - def test_process_action_delete_rating_held(self): + def test_execute_action_delete_rating_held(self): user = user_factory() addon = addon_factory(users=[user]) rating = Rating.objects.create( @@ -3397,28 +2770,51 @@ def test_process_action_delete_rating_held(self): assert decision.action_date is None mail.outbox.clear() - decision.process_action() + decision.execute_action_and_notify() assert decision.action_date is None assert not rating.reload().deleted - assert ( - ActivityLog.objects.filter( - action=amo.LOG.HELD_ACTION_DELETE_RATING.id - ).count() - == 1 - ) + alog = ActivityLog.objects.filter( + action=amo.LOG.HELD_ACTION_DELETE_RATING.id + ).get() + assert alog.contentdecisionlog_set.get().decision == decision assert len(mail.outbox) == 0 - decision.process_action(release_hold=True) - self._test_process_action_delete_rating_outcome(decision) + decision.execute_action_and_notify(release_hold=True) + self._test_execute_action_delete_rating_outcome(decision) - def test_process_action_delete_rating(self): + def test_execute_action_delete_rating(self): rating = Rating.objects.create(addon=addon_factory(), user=user_factory()) decision = ContentDecision.objects.create( rating=rating, action=DECISION_ACTIONS.AMO_DELETE_RATING ) assert decision.action_date is None - decision.process_action() - self._test_process_action_delete_rating_outcome(decision) + decision.execute_action_and_notify() + self._test_execute_action_delete_rating_outcome(decision) + + def test_execute_action_with_action_date_already(self): + decision = ContentDecision.objects.create( + addon=addon_factory(users=[user_factory()]), + action=DECISION_ACTIONS.AMO_DISABLE_ADDON, + action_date=datetime.now(), + notes='some review text', + ) + log_entry = ActivityLog.objects.create( + amo.LOG.FORCE_DISABLE, + decision.addon, + decision, + details={'comments': 'some review text'}, + user=user_factory(), + ) + + with mock.patch.object( + ContentActionDisableAddon, 'process_action' + ) as process_mock, mock.patch.object( + ContentActionDisableAddon, 'hold_action' + ) as hold_mock: + decision.execute_action_and_notify() + process_mock.assert_not_called() + hold_mock.assert_not_called() + self._check_notify_emails(decision, log_entry) def test_get_target_review_url(self): addon = addon_factory() diff --git a/src/olympia/abuse/tests/test_tasks.py b/src/olympia/abuse/tests/test_tasks.py index 6b1f9153ef0e..258f5227bc1f 100644 --- a/src/olympia/abuse/tests/test_tasks.py +++ b/src/olympia/abuse/tests/test_tasks.py @@ -29,9 +29,8 @@ from ..tasks import ( appeal_to_cinder, handle_escalate_action, - notify_addon_decision_to_cinder, + report_decision_to_cinder_and_notify, report_to_cinder, - resolve_job_in_cinder, sync_cinder_policies, ) @@ -637,8 +636,7 @@ def test_addon_appeal_to_cinder_authenticated_author(): @pytest.mark.django_db -@mock.patch('olympia.abuse.tasks.statsd.incr') -def test_resolve_job_in_cinder(statsd_incr_mock): +def test_report_decision_to_cinder_and_notify_with_job(): cinder_job = CinderJob.objects.create(job_id='999') abuse_report = AbuseReport.objects.create( guid=addon_factory().guid, @@ -652,140 +650,105 @@ def test_resolve_job_in_cinder(statsd_incr_mock): json={'uuid': uuid.uuid4().hex}, status=201, ) - statsd_incr_mock.reset_mock() + cinder_policy = CinderPolicy.objects.create(name='policy', uuid='12345678') - log_entry = ActivityLog.objects.create( + decision = ContentDecision.objects.create( + addon=abuse_report.addon, + action=DECISION_ACTIONS.AMO_DISABLE_ADDON, + action_date=datetime.now(), + notes='some review text', + ) + decision.policies.add(cinder_policy) + cinder_job.update(decision=decision) + ActivityLog.objects.create( amo.LOG.FORCE_DISABLE, - abuse_report.target, - abuse_report.target.current_version, + decision.addon, + decision.addon.current_version, + decision, cinder_policy, - details={'comments': 'some review text', 'cinder_action': 'AMO_DISABLE_ADDON'}, + details={'comments': 'some review text'}, user=user_factory(), ) - resolve_job_in_cinder.delay( - cinder_job_id=cinder_job.id, - log_entry_id=log_entry.id, - ) + with mock.patch('olympia.abuse.tasks.statsd.incr') as statsd_incr_mock: + report_decision_to_cinder_and_notify.delay(decision_id=decision.id) request = responses.calls[0].request request_body = json.loads(request.body) assert request_body['policy_uuids'] == ['12345678'] assert request_body['reasoning'] == 'some review text' - cinder_job.reload() - assert cinder_job.decision.action == DECISION_ACTIONS.AMO_DISABLE_ADDON - - assert statsd_incr_mock.call_count == 1 - assert statsd_incr_mock.call_args[0] == ( - 'abuse.tasks.resolve_job_in_cinder.success', - ) - - -@pytest.mark.django_db -@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') - abuse_report = AbuseReport.objects.create( - guid=addon_factory().guid, - reason=AbuseReport.REASONS.POLICY_VIOLATION, - location=AbuseReport.LOCATION.AMO, - cinder_job=cinder_job, - ) - responses.add( - responses.POST, - f'{settings.CINDER_SERVER_URL}jobs/999/decision', - json={'uuid': uuid.uuid4().hex}, - status=500, - ) - log_entry = ActivityLog.objects.create( - amo.LOG.FORCE_DISABLE, - abuse_report.target, - abuse_report.target.current_version, - cinder_policy=CinderPolicy.objects.create(name='policy', uuid='12345678'), - details={'comments': 'some review text', 'cinder_action': 'AMO_DISABLE_ADDON'}, - user=user_factory(), - ) - statsd_incr_mock.reset_mock() - - with pytest.raises(ConnectionError): - resolve_job_in_cinder.delay( - cinder_job_id=cinder_job.id, - log_entry_id=log_entry.id, - ) + assert 'entity' not in request_body assert statsd_incr_mock.call_count == 1 assert statsd_incr_mock.call_args[0] == ( - 'abuse.tasks.resolve_job_in_cinder.failure', + 'abuse.tasks.report_decision_to_cinder_and_notify.success', ) @pytest.mark.django_db -@mock.patch('olympia.abuse.tasks.statsd.incr') -def test_notify_addon_decision_to_cinder(statsd_incr_mock): +def test_report_decision_to_cinder_and_notify(): responses.add( responses.POST, f'{settings.CINDER_SERVER_URL}create_decision', json={'uuid': uuid.uuid4().hex}, status=201, ) - addon = addon_factory() - statsd_incr_mock.reset_mock() cinder_policy = CinderPolicy.objects.create(name='policy', uuid='12345678') - log_entry = ActivityLog.objects.create( + decision = ContentDecision.objects.create( + addon=addon_factory(), + action=DECISION_ACTIONS.AMO_DISABLE_ADDON, + action_date=datetime.now(), + notes='some review text', + ) + decision.policies.add(cinder_policy) + ActivityLog.objects.create( amo.LOG.FORCE_DISABLE, - addon, - addon.current_version, + decision.addon, + decision.addon.current_version, + decision, cinder_policy, - details={'comments': 'some review text', 'cinder_action': 'AMO_DISABLE_ADDON'}, + details={'comments': 'some review text'}, user=user_factory(), ) - notify_addon_decision_to_cinder.delay( - log_entry_id=log_entry.id, - addon_id=addon.id, - ) + with mock.patch('olympia.abuse.tasks.statsd.incr') as statsd_incr_mock: + report_decision_to_cinder_and_notify.delay(decision_id=decision.id) request = responses.calls[0].request request_body = json.loads(request.body) assert request_body['policy_uuids'] == ['12345678'] assert request_body['reasoning'] == 'some review text' - assert request_body['entity']['id'] == str(addon.id) - assert ContentDecision.objects.get().action == DECISION_ACTIONS.AMO_DISABLE_ADDON + assert request_body['entity']['id'] == str(decision.addon_id) assert statsd_incr_mock.call_count == 1 assert statsd_incr_mock.call_args[0] == ( - 'abuse.tasks.notify_addon_decision_to_cinder.success', + 'abuse.tasks.report_decision_to_cinder_and_notify.success', ) @pytest.mark.django_db @mock.patch('olympia.abuse.tasks.statsd.incr') -def test_notify_addon_decision_to_cinder_exception(statsd_incr_mock): - addon = addon_factory() +def test_report_decision_to_cinder_and_notify_exception(statsd_incr_mock): responses.add( responses.POST, f'{settings.CINDER_SERVER_URL}create_decision', json={'uuid': uuid.uuid4().hex}, status=500, ) - log_entry = ActivityLog.objects.create( - amo.LOG.FORCE_DISABLE, - addon, - addon.current_version, - cinder_policy=CinderPolicy.objects.create(name='policy', uuid='12345678'), - details={'comments': 'some review text', 'cinder_action': 'AMO_DISABLE_ADDON'}, - user=user_factory(), + decision = ContentDecision.objects.create( + addon=addon_factory(), + action=DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, + action_date=datetime.now(), + notes='some review text', ) statsd_incr_mock.reset_mock() with pytest.raises(ConnectionError): - notify_addon_decision_to_cinder.delay( - log_entry_id=log_entry.id, addon_id=addon.id - ) + report_decision_to_cinder_and_notify.delay(decision_id=decision.id) assert statsd_incr_mock.call_count == 1 assert statsd_incr_mock.call_args[0] == ( - 'abuse.tasks.notify_addon_decision_to_cinder.failure', + 'abuse.tasks.report_decision_to_cinder_and_notify.failure', ) diff --git a/src/olympia/lib/settings_base.py b/src/olympia/lib/settings_base.py index 4165eb64fecf..4bd6faee9c2d 100644 --- a/src/olympia/lib/settings_base.py +++ b/src/olympia/lib/settings_base.py @@ -881,8 +881,7 @@ def get_db_config(environ_var, atomic_requests=True): 'olympia.abuse.tasks.appeal_to_cinder': {'queue': 'amo'}, 'olympia.abuse.tasks.handle_escalate_action': {'queue': 'amo'}, 'olympia.abuse.tasks.report_to_cinder': {'queue': 'amo'}, - 'olympia.abuse.tasks.notify_addon_decision_to_cinder': {'queue': 'amo'}, - 'olympia.abuse.tasks.resolve_job_in_cinder': {'queue': 'amo'}, + 'olympia.abuse.tasks.report_decision_to_cinder_and_notify': {'queue': 'amo'}, 'olympia.abuse.tasks.sync_cinder_policies': {'queue': 'amo'}, 'olympia.accounts.tasks.clear_sessions_event': {'queue': 'amo'}, 'olympia.accounts.tasks.delete_user_event': {'queue': 'amo'}, diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index 9afdb933f578..7219f80b5574 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -1050,7 +1050,7 @@ def test_logs(self): self.helper.handler.log_action(amo.LOG.APPROVE_VERSION) assert self.check_log_count(amo.LOG.APPROVE_VERSION.id) == 1 - def test_log_action_sets_policies_and_reasons_with_allow_reasons(self): + def test_create_content_decision_sets_policies_and_reasons_with_allow_reasons(self): self.grant_permission(self.user, 'Addons:Review') self.file.update(status=amo.STATUS_AWAITING_REVIEW) self.helper = self.get_helper() @@ -1074,17 +1074,20 @@ def test_log_action_sets_policies_and_reasons_with_allow_reasons(self): } self.helper.set_data(data) self.helper.handler.review_action = self.helper.actions.get('public') - self.helper.handler.log_action(amo.LOG.APPROVE_VERSION) + self.helper.handler.create_content_decision(amo.LOG.APPROVE_VERSION) assert ReviewActionReasonLog.objects.count() == 2 assert CinderPolicyLog.objects.count() == 1 assert ( - ActivityLog.objects.get(action=amo.LOG.APPROVE_VERSION.id).details[ - 'cinder_action' - ] - == 'AMO_APPROVE_VERSION' + ActivityLog.objects.get(action=amo.LOG.APPROVE_VERSION.id) + .contentdecisionlog_set.get() + .decision.action + == DECISION_ACTIONS.AMO_APPROVE_VERSION ) - def test_log_action_sets_policies_with_allow_policies(self): + @patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay') + def test_create_content_decision_sets_policies_with_allow_policies( + self, report_mock + ): self.grant_permission(self.user, 'Addons:Review') cinder_job = CinderJob.objects.create( target_addon=self.addon, resolvable_in_reviewer_tools=True @@ -1115,80 +1118,18 @@ def test_log_action_sets_policies_with_allow_policies(self): self.helper.handler.review_action = self.helper.actions.get( 'resolve_reports_job' ) - self.helper.handler.log_action(amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION) - assert ReviewActionReasonLog.objects.count() == 0 - assert CinderPolicyLog.objects.count() == 2 - assert ( - ActivityLog.objects.get( - action=amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION.id - ).details['cinder_action'] - == 'AMO_IGNORE' - ) - - def test_log_action_override_policies(self): - self.grant_permission(self.user, 'Addons:Review') - cinder_job = CinderJob.objects.create( - target_addon=self.addon, resolvable_in_reviewer_tools=True - ) - other_policy = CinderPolicy.objects.create( - uuid='y', default_cinder_action=DECISION_ACTIONS.AMO_DISABLE_ADDON - ) - self.helper = self.get_helper() - data = { - 'cinder_policies': [ - CinderPolicy.objects.create( - uuid='z', default_cinder_action=DECISION_ACTIONS.AMO_IGNORE - ), - ], - 'cinder_jobs_to_resolve': [cinder_job], - } - self.helper.set_data(data) - self.helper.handler.review_action = self.helper.actions.get( - 'resolve_reports_job' - ) - self.helper.handler.log_action( - amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION, policies=[other_policy] - ) - assert ReviewActionReasonLog.objects.count() == 0 - assert CinderPolicyLog.objects.count() == 1 - assert CinderPolicyLog.objects.first().cinder_policy == other_policy - assert ( - ActivityLog.objects.get( - action=amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION.id - ).details['cinder_action'] - == 'AMO_DISABLE_ADDON' # from other_policy - ) - - def test_log_action_override_action(self): - self.grant_permission(self.user, 'Addons:Review') - cinder_job = CinderJob.objects.create( - target_addon=self.addon, resolvable_in_reviewer_tools=True - ) - self.helper = self.get_helper() - data = { - 'cinder_policies': [ - CinderPolicy.objects.create( - uuid='z', default_cinder_action=DECISION_ACTIONS.AMO_IGNORE - ), - ], - 'cinder_jobs_to_resolve': [cinder_job], - } - self.helper.set_data(data) - self.helper.handler.review_action = self.helper.actions.get( - 'resolve_reports_job' - ) - self.helper.handler.log_action( - amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION, - cinder_action=DECISION_ACTIONS.AMO_DISABLE_ADDON, + self.helper.handler.create_content_decision( + amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION ) assert ReviewActionReasonLog.objects.count() == 0 - assert CinderPolicyLog.objects.count() == 1 + assert CinderPolicyLog.objects.count() == 2 assert ( - ActivityLog.objects.get( - action=amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION.id - ).details['cinder_action'] - == 'AMO_DISABLE_ADDON' + ActivityLog.objects.get(action=amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION.id) + .contentdecisionlog_set.get() + .decision.action + == DECISION_ACTIONS.AMO_IGNORE ) + report_mock.assert_called_once() def test_log_action_override_user(self): # ActivityLog.user will default to self.user in log_action. @@ -1228,43 +1169,37 @@ def test_log_action_attachment_file(self): file_content = attachment_log.file.read().decode('utf-8') assert file_content == text - @patch('olympia.reviewers.utils.notify_addon_decision_to_cinder.delay') - @patch('olympia.reviewers.utils.resolve_job_in_cinder.delay') - def test_notify_decision_calls_resolve_job_in_cinder( - self, mock_resolve, mock_report + @patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay') + def test_create_content_decision_calls_report_decision_to_cinder_and_notify( + self, mock_report ): - 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') - # without 'cinder_jobs_to_resolve', notify_addon_decision_to_cinder is called + # Without 'cinder_jobs_to_resolve', report_decision_to_cinder_and_notify the + # decision created is not linked to any job self.helper.set_data(self.get_data()) - self.helper.handler.notify_decision() - mock_report.assert_called_once_with( - addon_id=self.addon.id, log_entry_id=log_entry.id - ) + self.helper.handler.create_content_decision(amo.LOG.APPROVE_VERSION) + decision = ContentDecision.objects.get() + mock_report.assert_called_once_with(decision_id=decision.id) + assert not hasattr(decision, 'cinder_job') - # with 'cinder_jobs_to_resolve', resolve_job_in_cinder is called instead + # With 'cinder_jobs_to_resolve', report_decision_to_cinder_and_notify the + # decision created is linked to a job self.helper.set_data( {**self.get_data(), 'cinder_jobs_to_resolve': [cinder_job1, cinder_job2]} ) - self.helper.handler.notify_decision() + self.helper.handler.create_content_decision(amo.LOG.APPROVE_VERSION) - mock_resolve.assert_has_calls( + job_decision1, job_decision2 = ContentDecision.objects.all()[1:] + mock_report.assert_has_calls( [ - call( - cinder_job_id=cinder_job1.id, - log_entry_id=log_entry.id, - ), - call( - cinder_job_id=cinder_job2.id, - log_entry_id=log_entry.id, - ), + call(decision_id=job_decision1.id), + call(decision_id=job_decision2.id), ] ) + assert job_decision1.cinder_job == cinder_job1 + assert job_decision2.cinder_job == cinder_job2 def test_send_reviewer_reply(self): self.setup_data(amo.STATUS_APPROVED) @@ -1831,7 +1766,8 @@ def test_public_addon_confirm_auto_approval(self): .filter(action=amo.LOG.CONFIRM_AUTO_APPROVED.id) .get() ) - assert activity.arguments == [self.addon, self.review_version] + decision = ContentDecision.objects.get() + assert activity.arguments == [self.addon, self.review_version, decision] assert activity.details['comments'] == '' assert activity.details['human_review'] is True assert self.review_version.reload().human_review_date @@ -1870,7 +1806,8 @@ def test_public_with_unreviewed_version_addon_confirm_auto_approval(self): .filter(action=amo.LOG.CONFIRM_AUTO_APPROVED.id) .get() ) - assert activity.arguments == [self.addon, self.current_version] + decision = ContentDecision.objects.get() + assert activity.arguments == [self.addon, self.current_version, decision] assert activity.details['comments'] == '' assert activity.details['human_review'] is True @@ -1906,7 +1843,8 @@ def test_public_with_disabled_version_addon_confirm_auto_approval(self): .filter(action=amo.LOG.CONFIRM_AUTO_APPROVED.id) .get() ) - assert activity.arguments == [self.addon, self.current_version] + decision = ContentDecision.objects.get() + assert activity.arguments == [self.addon, self.current_version, decision] assert activity.details['comments'] == '' def test_addon_with_versions_pending_rejection_confirm_auto_approval(self): @@ -1949,7 +1887,8 @@ def test_addon_with_versions_pending_rejection_confirm_auto_approval(self): .filter(action=amo.LOG.CONFIRM_AUTO_APPROVED.id) .get() ) - assert activity.arguments == [self.addon, self.review_version] + decision = ContentDecision.objects.get() + assert activity.arguments == [self.addon, self.review_version, decision] assert activity.details['comments'] == '' # None of the versions should be pending rejection anymore. @@ -2180,7 +2119,13 @@ def test_unlisted_version_addon_confirm_multiple_versions(self): .filter(action=amo.LOG.CONFIRM_AUTO_APPROVED.id) .get() ) - assert activity.arguments == [self.addon, second_unlisted, first_unlisted] + decision = ContentDecision.objects.get() + assert activity.arguments == [ + self.addon, + second_unlisted, + first_unlisted, + decision, + ] def test_unlisted_manual_approval_clear_pending_rejection(self): self.grant_permission(self.user, 'Addons:ReviewUnlisted') @@ -2399,7 +2344,13 @@ def _test_reject_multiple_versions(self, extra_data, human_review=True): .filter(action=amo.LOG.REJECT_VERSION.id) .get() ) - assert log.arguments == [self.addon, self.review_version, old_version] + decision = ContentDecision.objects.get() + assert log.arguments == [ + self.addon, + self.review_version, + old_version, + decision, + ] # listed auto approvals should be disabled until the next manual # approval. @@ -2502,7 +2453,8 @@ def _test_reject_multiple_versions_with_delay(self, extra_data): .filter(action=amo.LOG.REJECT_VERSION_DELAYED.id) .get() ) - assert log.arguments == [self.addon, self.review_version, old_version] + decision = ContentDecision.objects.get() + assert log.arguments == [self.addon, self.review_version, old_version, decision] # The flag to prevent the authors from being notified several times # about pending rejections should have been reset, and auto approvals @@ -2721,7 +2673,8 @@ def test_reject_multiple_versions_content_review_with_delay(self): .filter(action=amo.LOG.REJECT_CONTENT_DELAYED.id) .get() ) - assert log.arguments == [self.addon, self.review_version, old_version] + decision = ContentDecision.objects.get() + assert log.arguments == [self.addon, self.review_version, old_version, decision] def test_unreject_latest_version_approved_addon(self): first_version = self.review_version @@ -2869,7 +2822,8 @@ def test_approve_multiple_versions_unlisted(self): .filter(action=amo.LOG.APPROVE_VERSION.id) .get() ) - assert log.arguments == [self.addon, self.review_version, old_version] + decision = ContentDecision.objects.get() + assert log.arguments == [self.addon, self.review_version, old_version, decision] def test_reject_multiple_versions_unlisted(self): old_version = self.review_version @@ -2924,7 +2878,8 @@ def test_reject_multiple_versions_unlisted(self): .filter(action=amo.LOG.REJECT_VERSION.id) .get() ) - assert log.arguments == [self.addon, self.review_version, old_version] + decision = ContentDecision.objects.get() + assert log.arguments == [self.addon, self.review_version, old_version, decision] def _setup_reject_multiple_versions_delayed(self, content_review): # Do a rejection with delay. @@ -2974,7 +2929,8 @@ def _setup_reject_multiple_versions_delayed(self, content_review): .filter(action=delayed_action.id) .get() ) - assert log.arguments == [self.addon, self.review_version, old_version] + decision = ContentDecision.objects.get() + assert log.arguments == [self.addon, self.review_version, old_version, decision] # The request user is recorded as scheduling the rejection. assert log.user == original_user @@ -3476,52 +3432,55 @@ def test_enable_addon_version_is_awaiting_review_fall_back_to_nominated(self): assert self.addon.status == amo.STATUS_NOMINATED assert len(mail.outbox) == 1 - def _notify_decision_called_everywhere_checkbox_shown(self, actions): - # these two functions are to verify we call log_action before it's accessed - def log_check(): - assert self.helper.handler.log_entry - - def log_action(*args, **kwargs): - self.helper.handler.log_entry = object() - + def _create_content_decision_called_everywhere_checkbox_shown(self, actions): + job, _ = CinderJob.objects.get_or_create(job_id='1234') + policy, _ = CinderPolicy.objects.get_or_create( + default_cinder_action=DECISION_ACTIONS.AMO_APPROVE + ) self.helper.handler.data = { 'versions': [self.review_version], - 'cinder_jobs_to_resolve': [CinderJob()], + 'cinder_jobs_to_resolve': [job], + 'cinder_policies': [policy], } + all_actions = self.helper.actions resolves_actions = { key: action - for key, action in self.helper.actions.items() + for key, action in all_actions.items() if action.get('resolves_cinder_jobs', False) } assert list(resolves_actions) == list(actions) with ( - patch.object( - self.helper.handler, 'notify_decision', wraps=log_check - ) as notify_mock, - patch.object( - self.helper.handler, 'log_action', wraps=log_action - ) as log_action_mock, + patch( + 'olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay' + ) as report_task_mock, + patch.object(self.helper.handler, 'log_action') as log_action_mock, ): for action_name, action in resolves_actions.items(): + self.helper.handler.review_action = all_actions[action_name] action['method']() - notify_mock.assert_called_once() - notify_mock.reset_mock() + + decision = ContentDecision.objects.get() + report_task_mock.assert_called_once_with(decision_id=decision.id) + report_task_mock.reset_mock() log_entry = log_action_mock.call_args.args[0] assert ( getattr(log_entry, 'hide_developer', False) != actions[action_name]['should_email'] ) assert ( - getattr(log_entry, 'cinder_action', None) - == actions[action_name]['cinder_action'] + decision.action == actions[action_name]['cinder_action'] + or policy.default_cinder_action ) + assert job.decision == decision + + job.update(decision=None) + decision.delete() log_action_mock.assert_called_once() log_action_mock.reset_mock() - self.helper.handler.log_entry = None self.helper.handler.version = self.review_version - def test_notify_decision_called_everywhere_checkbox_shown_listed(self): + def test_create_content_decision_called_everywhere_checkbox_shown_listed(self): self.grant_permission(self.user, 'Reviews:Admin') self.grant_permission(self.user, 'Addons:Review') AutoApprovalSummary.objects.create( @@ -3531,7 +3490,7 @@ def test_notify_decision_called_everywhere_checkbox_shown_listed(self): target_addon=self.addon, resolvable_in_reviewer_tools=True ) self.setup_data(amo.STATUS_APPROVED) - self._notify_decision_called_everywhere_checkbox_shown( + self._create_content_decision_called_everywhere_checkbox_shown( { 'public': { 'should_email': True, @@ -3562,7 +3521,7 @@ def test_notify_decision_called_everywhere_checkbox_shown_listed(self): ) self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_DISABLED) assert self.addon.status == amo.STATUS_APPROVED - self._notify_decision_called_everywhere_checkbox_shown( + self._create_content_decision_called_everywhere_checkbox_shown( { 'confirm_auto_approved': { 'should_email': False, @@ -3584,7 +3543,7 @@ def test_notify_decision_called_everywhere_checkbox_shown_listed(self): } ) self.setup_data(amo.STATUS_DISABLED, file_status=amo.STATUS_DISABLED) - self._notify_decision_called_everywhere_checkbox_shown( + self._create_content_decision_called_everywhere_checkbox_shown( { 'confirm_auto_approved': { 'should_email': False, @@ -3602,7 +3561,7 @@ def test_notify_decision_called_everywhere_checkbox_shown_listed(self): } ) - def test_notify_decision_called_everywhere_checkbox_shown_unlisted(self): + def test_create_content_decision_called_everywhere_checkbox_shown_unlisted(self): self.grant_permission(self.user, 'Reviews:Admin') self.grant_permission(self.user, 'Addons:Review') self.grant_permission(self.user, 'Addons:ReviewUnlisted') @@ -3613,7 +3572,7 @@ def test_notify_decision_called_everywhere_checkbox_shown_unlisted(self): target_addon=self.addon, resolvable_in_reviewer_tools=True ) self.setup_data(amo.STATUS_APPROVED, channel=amo.CHANNEL_UNLISTED) - self._notify_decision_called_everywhere_checkbox_shown( + self._create_content_decision_called_everywhere_checkbox_shown( { 'public': { 'should_email': True, @@ -3643,7 +3602,7 @@ def test_notify_decision_called_everywhere_checkbox_shown_unlisted(self): } ) self.setup_data(amo.STATUS_DISABLED, file_status=amo.STATUS_DISABLED) - self._notify_decision_called_everywhere_checkbox_shown( + self._create_content_decision_called_everywhere_checkbox_shown( { 'approve_multiple_versions': { 'should_email': True, @@ -3723,9 +3682,14 @@ def test_resolve_appeal_job(self): assert CinderPolicyLog.objects.count() == 4 activity_log_qs = ActivityLog.objects.filter(action=amo.LOG.DENY_APPEAL_JOB.id) assert activity_log_qs.count() == 2 + decision_qs = ContentDecision.objects.filter(action_date__isnull=False) + assert decision_qs.count() == 2 log2, log1 = list(activity_log_qs.all()) - assert log1.details['cinder_action'] == 'AMO_DISABLE_ADDON' - assert log2.details['cinder_action'] == 'AMO_APPROVE' + decision1, decision2 = list(decision_qs.all()) + assert decision1.action == DECISION_ACTIONS.AMO_DISABLE_ADDON + assert decision2.action == DECISION_ACTIONS.AMO_APPROVE + assert decision1.activities.get() == log1 + assert decision2.activities.get() == log2 assert set(appeal_job1.reload().decision.policies.all()) == { policy_a, policy_b, diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 29fd71da6e0b..e4d0fc547948 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -2555,7 +2555,7 @@ def test_comment(self): comment_version = amo.LOG.COMMENT_VERSION assert ActivityLog.objects.filter(action=comment_version.id).count() == 1 - @mock.patch('olympia.reviewers.utils.resolve_job_in_cinder.delay') + @mock.patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay') def test_resolve_reports_job(self, resolve_mock): cinder_job = CinderJob.objects.create( job_id='123', target_addon=self.addon, resolvable_in_reviewer_tools=True @@ -2586,10 +2586,13 @@ def test_resolve_reports_job(self, resolve_mock): action=amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION.id ) assert activity_log_qs.count() == 1 - assert activity_log_qs[0].details['cinder_action'] == 'AMO_IGNORE' - resolve_mock.assert_called_once() + decision = ContentDecision.objects.get() + resolve_mock.assert_called_once_with(decision_id=decision.id) + assert decision.activities.all().get() == activity_log_qs.get() + assert decision.action == DECISION_ACTIONS.AMO_IGNORE + self.assertCloseToNow(decision.action_date) - @mock.patch('olympia.reviewers.utils.resolve_job_in_cinder.delay') + @mock.patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay') def test_resolve_appeal_job(self, resolve_mock): appeal_job1 = CinderJob.objects.create( job_id='1', resolvable_in_reviewer_tools=True, target_addon=self.addon @@ -2627,12 +2630,19 @@ def test_resolve_appeal_job(self, resolve_mock): activity_log_qs = ActivityLog.objects.filter(action=amo.LOG.DENY_APPEAL_JOB.id) assert activity_log_qs.count() == 2 + assert ContentDecision.objects.count() == 5 # 2 new + assert ContentDecision.objects.filter(action_date__isnull=False).count() == 2 + decision2, decision1 = list( + ContentDecision.objects.filter(action_date__isnull=False) + ) log1, log2 = list(activity_log_qs.all()) - assert log1.details['cinder_action'] == 'AMO_DISABLE_ADDON' - assert log2.details['cinder_action'] == 'AMO_APPROVE' + assert decision1.activities.get() == log1 + assert decision1.action == DECISION_ACTIONS.AMO_DISABLE_ADDON + assert decision2.activities.get() == log2 + assert decision2.action == DECISION_ACTIONS.AMO_APPROVE assert resolve_mock.call_count == 2 - @mock.patch('olympia.reviewers.utils.resolve_job_in_cinder.delay') + @mock.patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay') def test_request_legal_review(self, resolve_mock): appeal_job = CinderJob.objects.create( job_id='1', resolvable_in_reviewer_tools=True, target_addon=self.addon @@ -2660,9 +2670,11 @@ def test_request_legal_review(self, resolve_mock): activity_log_qs = ActivityLog.objects.filter(action=amo.LOG.REQUEST_LEGAL.id) assert activity_log_qs.count() == 1 - log = activity_log_qs.get() - assert log.details['cinder_action'] == 'AMO_LEGAL_FORWARD' - assert resolve_mock.call_count == 1 + decision = ContentDecision.objects.last() + resolve_mock.assert_called_once_with(decision_id=decision.id) + assert decision.activities.all().get() == activity_log_qs.get() + assert decision.action == DECISION_ACTIONS.AMO_LEGAL_FORWARD + self.assertCloseToNow(decision.action_date) def test_reviewer_reply(self): reason = ReviewActionReason.objects.create( @@ -5751,7 +5763,7 @@ def test_resolve_abuse_reports_checkbox(self): self.assertContains(response, 'Show detail on 1 reports') self.assertContains(response, 'Its baaaad') - @mock.patch('olympia.reviewers.utils.resolve_job_in_cinder.delay') + @mock.patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay') def test_abuse_reports_resolved_as_disable_addon_with_disable_action( self, mock_resolve_task ): @@ -5789,12 +5801,13 @@ def test_abuse_reports_resolved_as_disable_addon_with_disable_action( ) 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, - log_entry_id=log_entry.id, - ) + decision = ContentDecision.objects.get() + mock_resolve_task.assert_called_once_with(decision_id=decision.id) + assert decision.activities.all().get() == log_entry + assert decision.action == DECISION_ACTIONS.AMO_DISABLE_ADDON + self.assertCloseToNow(decision.action_date) - @mock.patch('olympia.reviewers.utils.resolve_job_in_cinder.delay') + @mock.patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay') @mock.patch('olympia.reviewers.utils.sign_file') def test_abuse_reports_resolved_as_approve_with_approve_latest_version_action( self, sign_file_mock, mock_resolve_task @@ -5827,10 +5840,11 @@ def test_abuse_reports_resolved_as_approve_with_approve_latest_version_action( ) log_entry = ActivityLog.objects.get(action=amo.LOG.APPROVE_VERSION.id) - mock_resolve_task.assert_called_once_with( - cinder_job_id=cinder_job.id, - log_entry_id=log_entry.id, - ) + decision = ContentDecision.objects.get() + mock_resolve_task.assert_called_once_with(decision_id=decision.id) + assert decision.activities.all().get() == log_entry + assert decision.action == DECISION_ACTIONS.AMO_APPROVE_VERSION + self.assertCloseToNow(decision.action_date) class TestAbuseReportsView(ReviewerTest): diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index daa4816d8de9..625da0237c5c 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -14,7 +14,7 @@ import olympia.core.logger from olympia import amo from olympia.abuse.models import CinderJob, CinderPolicy, ContentDecision -from olympia.abuse.tasks import notify_addon_decision_to_cinder, resolve_job_in_cinder +from olympia.abuse.tasks import report_decision_to_cinder_and_notify from olympia.access import acl from olympia.activity.models import ActivityLog, AttachmentLog from olympia.activity.utils import notify_about_activity_log @@ -935,17 +935,68 @@ def set_promoted(self, versions=None): for version in versions: self.addon.promotedaddon.approve_for_version(version) - def notify_decision(self): + def create_content_decision( + self, activity_action, *, log_action_kw=None, action_completed=True + ): + reasons = ( + self.data.get('reasons', []) + if self.review_action and self.review_action.get('allows_reasons') + else [] + ) + policies = [ + reason.cinder_policy + for reason in reasons + if getattr(reason, 'cinder_policy', None) + ] + if self.review_action and self.review_action.get('requires_policies'): + policies.extend(self.data.get('cinder_policies', [])) + + cinder_action = getattr(activity_action, 'cinder_action', None) + if not cinder_action and policies: + cinder_action = ( + # If there isn't a cinder_action from the activity action already, get + # it from the policy. There should only be one in the list as form + # validation raises for multiple cinder actions. + (actions := self.get_cinder_actions_from_policies(policies)) + and actions[0].value + ) + assert cinder_action + + decision_kw = { + 'addon': self.addon, + 'action': cinder_action, + 'action_date': datetime.now() if action_completed else None, + 'notes': self.data.get('comments', ''), + } + + decisions = [] + + def create_decision(): + decision = ContentDecision.objects.create(**decision_kw) + decision.policies.set(policies) + decisions.append(decision) + return decision + if cinder_jobs := self.data.get('cinder_jobs_to_resolve', ()): # with appeals and escalations there could be multiple jobs - for cinder_job in cinder_jobs: - resolve_job_in_cinder.delay( - cinder_job_id=cinder_job.id, log_entry_id=self.log_entry.id - ) + for job in cinder_jobs: + decision = create_decision() + if job.decision: + decision.update(override_of=job.decision) + else: + job.update(decision=decision) else: - notify_addon_decision_to_cinder.delay( - log_entry_id=self.log_entry.id, addon_id=self.addon.id - ) + create_decision() + + self.log_action( + activity_action, + decisions=decisions, + reasons=reasons, + policies=policies, + **(log_action_kw or {}), + ) + for decision in decisions: + report_decision_to_cinder_and_notify.delay(decision_id=decision.id) def clear_all_needs_human_review_flags_in_channel(self, mad_too=True): """Clear needs_human_review flags on all versions in the same channel. @@ -956,9 +1007,9 @@ def clear_all_needs_human_review_flags_in_channel(self, mad_too=True): are supposed to automatically get the update to that version, so we don't need to care about older ones anymore. """ - # Do a mass UPDATE. The NeedsHumanReview coming from - # abuse/appeal/escalations are only cleared in CinderJob.resolve_job() - # if the reviewer has selected to resolve all jobs of that type though. + # Do a mass UPDATE. The NeedsHumanReview coming from abuse/appeal/escalations + # are only cleared in ContentDecision.execute_action_and_notify() if the + # reviewer has selected to resolve all jobs of that type though. NeedsHumanReview.objects.filter( version__addon=self.addon, version__channel=self.version.channel, @@ -1012,38 +1063,14 @@ def log_action( timestamp=None, user=None, extra_details=None, + decisions=None, + reasons=None, policies=None, - cinder_action=None, ): - reasons = ( - self.data.get('reasons', []) - if self.review_action and self.review_action.get('allows_reasons') - else [] - ) - if policies is None: - policies = [ - reason.cinder_policy - for reason in reasons - if getattr(reason, 'cinder_policy', None) - ] - if self.review_action and self.review_action.get('requires_policies'): - policies.extend(self.data.get('cinder_policies', [])) - - cinder_action = cinder_action or getattr(action, 'cinder_action', None) - if not cinder_action and policies: - cinder_action = ( - # If there isn't a cinder_action from the activity action already, get - # it from the policy. There should only be one in the list as form - # validation raises for multiple cinder actions. - (actions := self.get_cinder_actions_from_policies(policies)) - and actions[0] - ) - details = { 'comments': self.data.get('comments', ''), 'reviewtype': self.review_type.split('_')[1], 'human_review': self.human_review, - 'cinder_action': cinder_action and cinder_action.constant, **(extra_details or {}), } if version is None and self.version: @@ -1066,7 +1093,7 @@ def log_action( if timestamp is None: timestamp = datetime.now() - args = (*args, *reasons, *policies) + args = (*args, *(reasons or ()), *(policies or ()), *(decisions or ())) kwargs = {'user': user or self.user, 'created': timestamp, 'details': details} self.log_entry = ActivityLog.objects.create(action, *args, **kwargs) @@ -1111,8 +1138,7 @@ def process_comment(self): def resolve_reports_job(self): if self.data.get('cinder_jobs_to_resolve', ()): - self.log_action(amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION) - self.notify_decision() # notify cinder + self.create_content_decision(amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION) def resolve_appeal_job(self): # It's possible to have multiple appeal jobs, so handle them seperately. @@ -1126,15 +1152,24 @@ def resolve_appeal_job(self): previous_action_id = min( decision.action for decision in job.appealed_decisions.all() ) + # notify cinder + decision = ContentDecision.objects.create( + addon=self.addon, + action=previous_action_id, + action_date=datetime.now(), + notes=self.data.get('comments', ''), + ) + decision.policies.set(list(previous_policies)) + if job.decision: + decision.update(override_of=job.decision) + else: + job.update(decision=decision) self.log_action( amo.LOG.DENY_APPEAL_JOB, - policies=list(previous_policies), - cinder_action=DECISION_ACTIONS.for_value(previous_action_id), - ) - # notify cinder - resolve_job_in_cinder.delay( - cinder_job_id=job.id, log_entry_id=self.log_entry.id + decisions=[decision], + policies=previous_policies, ) + report_decision_to_cinder_and_notify.delay(decision_id=decision.id) def approve_latest_version(self): """Approve the add-on latest version (potentially setting the add-on to @@ -1181,11 +1216,12 @@ def approve_latest_version(self): # Automatic approval, reset the counter. AddonApprovalsCounter.reset_for_addon(addon=self.addon) - self.log_action(amo.LOG.APPROVE_VERSION) if self.human_review or self.addon.type != amo.ADDON_LPAPP: # Don't notify decisions (to cinder or owners) for auto-approved langpacks log.info('Sending email for %s' % (self.addon)) - self.notify_decision() + self.create_content_decision(amo.LOG.APPROVE_VERSION) + else: + self.log_action(amo.LOG.APPROVE_VERSION) self.log_public_message() def reject_latest_version(self): @@ -1210,10 +1246,8 @@ def reject_latest_version(self): self.clear_specific_needs_human_review_flags(self.version) self.set_human_review_date() - self.log_action(amo.LOG.REJECT_VERSION) log.info('Sending email for %s' % (self.addon)) - # This call has to happen after log_action - we need self.log_entry - self.notify_decision() + self.create_content_decision(amo.LOG.REJECT_VERSION) self.log_sandbox_message() def request_admin_review(self): @@ -1266,8 +1300,6 @@ def confirm_auto_approved(self): # For unlisted, we just use self.version. version = self.version - self.log_action(amo.LOG.CONFIRM_AUTO_APPROVED, version=version) - if self.human_review: self.set_promoted() # Mark the approval as confirmed (handle DoesNotExist, it may have @@ -1299,7 +1331,11 @@ def confirm_auto_approved(self): pending_content_rejection=None, ) self.set_human_review_date(version) - self.notify_decision() + self.create_content_decision( + amo.LOG.CONFIRM_AUTO_APPROVED, log_action_kw={'version': version} + ) + else: + self.log_action(amo.LOG.CONFIRM_AUTO_APPROVED, version=version) def reject_multiple_versions(self): """Reject a list of versions. @@ -1382,25 +1418,6 @@ def reject_multiple_versions(self): version ) - extra_details = {} - for key in [ - 'delayed_rejection_days', - 'is_addon_being_blocked', - 'is_addon_being_disabled', - ]: - if key in self.data: - extra_details[key] = self.data[key] - - for action_id, user_and_versions in actions_to_record.items(): - for user, versions in user_and_versions.items(): - self.log_action( - action_id, - versions=versions, - timestamp=now, - user=user, - extra_details=extra_details, - ) - addonreviewerflags = {} # A human rejection (delayed or not) implies the next version in the # same channel should be manually reviewed. @@ -1423,10 +1440,24 @@ def reject_multiple_versions(self): defaults=addonreviewerflags, ) - if actions_to_record: - # if we didn't record any actions we didn't do anything so nothing to notify - log.info('Sending email for %s' % (self.addon)) - self.notify_decision() + keys = [ + 'delayed_rejection_days', + 'is_addon_being_blocked', + 'is_addon_being_disabled', + ] + extra_details = {key: self.data[key] for key in keys if key in self.data} + for action_id, user_and_versions in actions_to_record.items(): + for user, versions in user_and_versions.items(): + log.info('Sending email for %s' % (self.addon)) + self.create_content_decision( + action_id, + log_action_kw={ + 'versions': versions, + 'timestamp': now, + 'user': user, + 'extra_details': extra_details, + }, + ) def unreject_latest_version(self): """Un-reject the latest version.""" @@ -1502,22 +1533,19 @@ def enable_addon(self): """Force enable the add-on.""" self.version = None self.addon.force_enable(skip_activity_log=True) - self.log_action(amo.LOG.FORCE_ENABLE) log.info('Sending email for %s' % (self.addon)) - self.notify_decision() + self.create_content_decision(amo.LOG.FORCE_ENABLE) def disable_addon(self): """Force disable the add-on and all versions.""" self.addon.force_disable(skip_activity_log=True) - self.log_action(amo.LOG.FORCE_DISABLE) log.info('Sending email for %s' % (self.addon)) - self.notify_decision() + self.create_content_decision(amo.LOG.FORCE_DISABLE) def request_legal_review(self): """Forward add-on and/or job to legal via Cinder.""" - self.log_action(amo.LOG.REQUEST_LEGAL) log.info('Forwarding %s for legal review' % (self.addon)) - self.notify_decision() + self.create_content_decision(amo.LOG.REQUEST_LEGAL, action_completed=False) class ReviewAddon(ReviewBase): @@ -1560,8 +1588,6 @@ def approve_latest_version(self): self.set_file(amo.STATUS_APPROVED, self.file) - self.log_action(amo.LOG.APPROVE_VERSION) - if self.human_review: self.set_promoted() self.clear_specific_needs_human_review_flags(self.version) @@ -1594,7 +1620,7 @@ def approve_latest_version(self): % (self.addon, self.file.file.name if self.file else '') ) log.info('Sending email for %s' % (self.addon)) - self.notify_decision() + self.create_content_decision(amo.LOG.APPROVE_VERSION) def block_multiple_versions(self): versions = self.data['versions'] @@ -1624,12 +1650,10 @@ def confirm_multiple_versions(self): self.clear_specific_needs_human_review_flags(version) self.set_human_review_date(version) - self.log_action( + self.create_content_decision( amo.LOG.CONFIRM_AUTO_APPROVED, - versions=self.data['versions'], - timestamp=timestamp, + log_action_kw={'versions': self.data['versions'], 'timestamp': timestamp}, ) - self.notify_decision() def approve_multiple_versions(self): """Set multiple unlisted add-on versions files to public.""" @@ -1657,10 +1681,6 @@ def approve_multiple_versions(self): self.clear_specific_needs_human_review_flags(version) log.info('Making %s files %s public' % (self.addon, version.file.file.name)) - self.log_action( - amo.LOG.APPROVE_VERSION, versions=self.data['versions'], timestamp=timestamp - ) - if self.human_review: self.set_promoted(versions=self.data['versions']) # An approval took place so we can reset this. @@ -1669,7 +1689,19 @@ def approve_multiple_versions(self): defaults={'auto_approval_disabled_until_next_approval_unlisted': False}, ) log.info('Sending email(s) for %s' % (self.addon)) - self.notify_decision() + self.create_content_decision( + amo.LOG.APPROVE_VERSION, + log_action_kw={ + 'versions': self.data['versions'], + 'timestamp': timestamp, + }, + ) + else: + self.log_action( + amo.LOG.APPROVE_VERSION, + versions=self.data['versions'], + timestamp=timestamp, + ) def unreject_multiple_versions(self): """Un-reject a list of versions.""" diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py index 47656ba54931..206c30cce682 100644 --- a/src/olympia/reviewers/views.py +++ b/src/olympia/reviewers/views.py @@ -1264,16 +1264,20 @@ def decision_review(request, decision_id): data = form.cleaned_data match data.get('choice'): case 'yes': - decision.process_action(release_hold=True) + decision.execute_action_and_notify(release_hold=True) case 'no': + # TODO: Make use of Cinder override to create new decision, and notify + # that new decision to Cinder decision.update(action=DECISION_ACTIONS.AMO_APPROVE, notes='') decision.policies.set( CinderPolicy.objects.filter( default_cinder_action=DECISION_ACTIONS.AMO_APPROVE ) ) - decision.process_action(release_hold=True) + decision.execute_action_and_notify(release_hold=True) case 'forward': + # TODO: Refactor so we can push this through the normal ContentDecision + # execution flow. decision.update(action_date=datetime.now()) for job in cinder_jobs_qs: handle_escalate_action.delay(job_pk=job.id)