From 71bbeeacf40c2757bcb3f5dd9a46eb9cdc1b1f8e Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 15 Jun 2026 14:31:46 +0100 Subject: [PATCH 1/4] Implement approvals in ContentActionApproveInitialDecision --- src/olympia/abuse/actions.py | 321 +++++-- ...on.txt => ContentActionApproveVersion.txt} | 0 src/olympia/abuse/tests/test_actions.py | 814 ++++++++++++------ src/olympia/reviewers/forms.py | 8 + src/olympia/reviewers/tests/test_forms.py | 7 +- src/olympia/reviewers/tests/test_utils.py | 58 +- src/olympia/reviewers/tests/test_views.py | 89 ++ src/olympia/reviewers/utils.py | 51 +- 8 files changed, 1017 insertions(+), 331 deletions(-) rename src/olympia/abuse/templates/abuse/emails/{ContentActionApproveInitialDecision.txt => ContentActionApproveVersion.txt} (100%) diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index ceae663ba8eb..06b55623483f 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -28,6 +28,7 @@ from olympia.constants.permissions import ADDONS_HIGH_IMPACT_APPROVE from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT from olympia.files.models import File +from olympia.lib.crypto.signing import sign_file from olympia.ratings.models import Rating from olympia.users.models import UserProfile from olympia.versions.models import Version, VersionReviewerFlags @@ -68,11 +69,17 @@ def __init__(self, decision): f'{self.valid_targets}' ) - def log_action(self, activity_log_action, *extra_args, extra_details=None): + def log_action( + self, + activity_log_action, + *extra_args, + extra_details=None, + skip_private_notes=False, + ): user_kw = ( {'user': self.decision.reviewer_user} if self.decision.reviewer_user else {} ) - if self.decision.private_notes: + if self.decision.private_notes and not skip_private_notes: # If the decision contained private notes, add a separate action # for them. log_create( @@ -356,40 +363,45 @@ def get_owners(self): return [self.target] -class ContentActionDisableAddon(ContentAction): - description = 'Add-on has been disabled' +class ContentActionAddon(ContentAction): + """Base class for content actions for Addons.""" + valid_targets = (Addon,) - reporter_template_path = 'abuse/emails/reporter_takedown_addon.txt' - reporter_appeal_template_path = 'abuse/emails/reporter_appeal_takedown.txt' - action = DECISION_ACTIONS.AMO_DISABLE_ADDON def is_human_reviewer(self): return bool( (user := self.decision.reviewer_user) and user.id != settings.TASK_USER_ID ) - def should_hold_action(self): - return bool( - self.target.status != amo.STATUS_DISABLED - # is a high profile add-on - and any(self.target.promoted_groups(currently_approved=False).high_profile) - ) + @property + def target_versions(self): + return self.decision.target_versions.all() - def log_action(self, activity_log_action, *extra_args, extra_details=None): + def log_action( + self, + activity_log_action, + *extra_args, + extra_details=None, + skip_private_notes=False, + ): from olympia.activity.models import AttachmentLog extra_details = {'human_review': self.is_human_reviewer()} | ( extra_details or {} ) - if ( + if 'versions' not in extra_details and ( target_versions := self.target_versions.no_transforms() .only('pk', 'version', 'file') .order_by('-pk') ): extra_args = (*target_versions, *extra_args) extra_details['versions'] = [version.version for version in target_versions] + activity_log = super().log_action( - activity_log_action, *extra_args, extra_details=extra_details + activity_log_action, + *extra_args, + extra_details=extra_details, + skip_private_notes=skip_private_notes, ) # move any attachments to latest decision if attachment := AttachmentLog.objects.filter( @@ -399,9 +411,71 @@ def log_action(self, activity_log_action, *extra_args, extra_details=None): activity_log.attachmentlog = attachment # update fk return activity_log - @property - def target_versions(self): - return self.decision.target_versions.all() + def set_human_review_date(self, version): + if self.is_human_reviewer() and not version.human_review_date: + version.update(human_review_date=datetime.now()) + + def clear_specific_needs_human_review_flags(self, version): + """Clear needs_human_review flags on a specific version.""" + from olympia.reviewers.models import NeedsHumanReview + + from .models import CinderJob + + qs = version.needshumanreview_set.filter(is_active=True) + if not hasattr(self, 'unresolved_jobs'): + # this isn't going to change between iterations, so be efficient + self.unresolved_jobs = ( + CinderJob.objects.for_addon(self.target) + .unresolved() + .resolvable_in_reviewer_tools() + .exists() + ) + if self.unresolved_jobs: + qs = qs.exclude( + reason__in=NeedsHumanReview.REASONS.ABUSE_OR_APPEAL_RELATED.values + ) + qs.update(is_active=False) + # Because the updating of needs human review was made with a queryset + # the post_save signal was not triggered so let's recheck the due date + # explicitly. + version.reset_due_date() + + def _clear_all_needs_human_review_flags_in_channel(self, channel): + """Clear needs_human_review flags on all versions in the same channel. + + Doesn't clear abuse or appeal related flags. + To be called when approving a listed version: For listed, the version + reviewers are approving is always the latest listed one, and then users + are supposed to automatically get the update to that version, so we + don't need to care about older ones anymore. + """ + from olympia.reviewers.models import NeedsHumanReview + + # Do a mass UPDATE. The NeedsHumanReview coming from abuse/appeal/escalations + # are only cleared in ContentDecision.execute_action() if the + # reviewer has selected to resolve all jobs of that type though. + NeedsHumanReview.objects.filter( + version__addon=self.target, version__channel=channel, is_active=True + ).exclude( + reason__in=NeedsHumanReview.REASONS.ABUSE_OR_APPEAL_RELATED.values + ).update(is_active=False) + # Trigger a check of all due dates on the add-on since we mass-updated + # versions. + self.target.update_all_due_dates() + + +class ContentActionDisableAddon(ContentActionAddon): + description = 'Add-on has been disabled' + reporter_template_path = 'abuse/emails/reporter_takedown_addon.txt' + reporter_appeal_template_path = 'abuse/emails/reporter_appeal_takedown.txt' + action = DECISION_ACTIONS.AMO_DISABLE_ADDON + + def should_hold_action(self): + return bool( + self.target.status != amo.STATUS_DISABLED + # is a high profile add-on + and any(self.target.promoted_groups(currently_approved=False).high_profile) + ) @property def versions_force_disable_will_affect(self): @@ -591,33 +665,6 @@ def process_action(self, release_hold=False): amo.LOG.REJECT_CONTENT if self.content_review else amo.LOG.REJECT_VERSION ) - def clear_specific_needs_human_review_flags(self, version): - """Clear needs_human_review flags on a specific version.""" - from olympia.reviewers.models import NeedsHumanReview - - from .models import CinderJob - - qs = version.needshumanreview_set.filter(is_active=True) - unresolved_jobs = ( - CinderJob.objects.for_addon(version.addon) - .unresolved() - .resolvable_in_reviewer_tools() - .exists() - ) - if unresolved_jobs: - qs = qs.exclude( - reason__in=NeedsHumanReview.REASONS.ABUSE_OR_APPEAL_RELATED.values - ) - qs.update(is_active=False) - # Because the updating of needs human review was made with a queryset - # the post_save signal was not triggered so let's recheck the due date - # explicitly. - version.reset_due_date() - - def set_human_review_date(self, version): - if self.is_human_reviewer() and not version.human_review_date: - version.update(human_review_date=datetime.now()) - def hold_action(self): # Even if the action is held, we want to always prevent auto-approval # in relevant channels. @@ -1046,8 +1093,7 @@ def hold_action(self): return None -class ContentActionForwardToLegal(ContentAction): - valid_targets = (Addon,) +class ContentActionForwardToLegal(ContentActionAddon): action = DECISION_ACTIONS.AMO_LEGAL_FORWARD def process_action(self, release_hold=False): @@ -1057,9 +1103,8 @@ def process_action(self, release_hold=False): return self.log_action(amo.LOG.REQUEST_LEGAL) -class ContentActionChangePendingRejectionDate(ContentAction): +class ContentActionChangePendingRejectionDate(ContentActionAddon): description = 'Add-on pending rejection date has changed' - valid_targets = (Addon,) action = DECISION_ACTIONS.AMO_CHANGE_PENDING_REJECTION_DATE def get_owners(self): @@ -1336,9 +1381,7 @@ def process_action(self, release_hold=False): return None -class ContentActionApproveInitialDecision( - AnyTargetMixin, NoActionMixin, AnyOwnerEmailMixin, ContentAction -): +class ContentActionApproveVersion(ContentActionAddon): description = ( 'Reported content is within policy, initial decision, approving versions' ) @@ -1346,6 +1389,178 @@ class ContentActionApproveInitialDecision( reporter_appeal_template_path = 'abuse/emails/reporter_appeal_approve.txt' action = DECISION_ACTIONS.AMO_APPROVE_VERSION + def get_owners(self): + if ( + isinstance(self.target, Addon) + and (self.is_human_reviewer or self.target.type != amo.ADDON_LPAPP) + and self.decision.activities.filter( + # FORCE_ENABLE is logged via the reviewer tools enable_addon action. + action__in=(amo.LOG.APPROVE_VERSION.id, amo.LOG.FORCE_ENABLE.id) + ).exists() + ): + # Don't notify decisions (to cinder or owners) for auto-approved langpacks + # or if the decision wasn't (freshly) approving any versions. + return self.target.authors.all() + else: + return () + + def _set_promoted(self, versions): + group = self.target.promoted_groups(currently_approved=False) + if group and versions: + channel = versions[0].channel + if (channel == amo.CHANNEL_LISTED and any(group.listed_pre_review)) or ( + channel == amo.CHANNEL_UNLISTED and any(group.unlisted_pre_review) + ): + # These addons shouldn't be be attempted for auto approval + # anyway, but double check that the cron job isn't trying to + # approve it. + assert self.is_human_reviewer + for version in versions: + self.target.approve_for_version(version) + + def process_version(self, version): + from olympia.reviewers.models import NeedsHumanReview + + # Sign addon. + assert not version.is_blocked + + if version.file.status == amo.STATUS_AWAITING_REVIEW: + if version.file.is_experiment: + self.log_action( + amo.LOG.EXPERIMENT_SIGNED, + version.file, + extra_details={'versions': [version.version]}, + skip_private_notes=True, + ) + sign_file(version.file) + if version.channel == amo.CHANNEL_UNLISTED: + self.log_action( + amo.LOG.UNLISTED_SIGNED, + version.file, + extra_details={'versions': [version.version]}, + skip_private_notes=True, + ) + + # Save files first, because set_addon checks to make sure there + # is at least one public file or it won't make the addon public. + version.file.update( + datestatuschanged=datetime.now(), + approval_date=datetime.now(), + original_status=amo.STATUS_NULL, + status_disabled_reason=File.STATUS_DISABLED_REASONS.NONE, + status=amo.STATUS_APPROVED, + ) + already_approved = False + else: + already_approved = True + + self.set_human_review_date(version) + + if self.is_human_reviewer(): + # Clear pending rejection since we approved that version. + VersionReviewerFlags.objects.filter(version=version).update( + pending_rejection=None, + pending_rejection_by=None, + pending_content_rejection=None, + ) + if version.channel == amo.CHANNEL_UNLISTED: + self.clear_specific_needs_human_review_flags(version) + elif ( + version.channel == amo.CHANNEL_UNLISTED + and version.needshumanreview_set.filter(is_active=True) + and (delay := self.target.auto_approval_delayed_until_unlisted) + and delay < datetime.now() + ): + # if we're auto-approving because its past the approval delay, + # flag it. + NeedsHumanReview.objects.create( + version=version, + reason=NeedsHumanReview.REASONS.AUTO_APPROVED_PAST_APPROVAL_DELAY, + ) + return already_approved + + def process_action(self, release_hold=False): + if not self.decision.reviewer_user: + # This action should only be used by reviewer tools, not cinder webhook + raise NotImplementedError + + if not isinstance(self.target, Addon) or not ( + versions := list(self.target_versions) + ): + return None + + was_public = self.target.is_public() + already_approved_versions, newly_approved_versions = [], [] + for version in versions: + if self.process_version(version): + already_approved_versions.append(version) + else: + newly_approved_versions.append(version) + + self._set_promoted(versions) + if not was_public and newly_approved_versions: + self.target.update_status() + + channel = versions[0].channel + if self.is_human_reviewer(): + if channel == amo.CHANNEL_LISTED: + # No need for a human review anymore in this channel. + self._clear_all_needs_human_review_flags_in_channel(amo.CHANNEL_LISTED) + # The counter can be incremented. + AddonApprovalsCounter.increment_for_addon(addon=self.target) + + # An approval took place so we can reset this. + AddonReviewerFlags.objects.update_or_create( + addon=self.target, + defaults={ + 'auto_approval_disabled_until_next_approval' + if channel == amo.CHANNEL_LISTED + else 'auto_approval_disabled_until_next_approval_unlisted': False + }, + ) + self.target.reviewerflags.reload() + elif channel == amo.CHANNEL_LISTED: + # Automatic approval, reset the counter. + AddonApprovalsCounter.reset_for_addon(addon=self.target) + + if newly_approved_versions: + approve_log = self.log_action( + amo.LOG.APPROVE_VERSION, + *newly_approved_versions, + extra_details={ + 'versions': [version.version for version in newly_approved_versions] + }, + ) + if not was_public and self.target.is_public(): + log.info('Making %s public' % (self.target)) + else: + log.info( + 'Making %s files [%s] public' + % ( + self.target, + ', '.join( + version.file.file.name + for version in newly_approved_versions + ), + ) + ) + if already_approved_versions: + confirm_approve_log = self.log_action( + amo.LOG.CONFIRM_AUTO_APPROVED, + *already_approved_versions, + extra_details={ + 'versions': [ + version.version for version in already_approved_versions + ] + }, + skip_private_notes=bool(newly_approved_versions), + ) + return ( + (newly_approved_versions and approve_log) + or (already_approved_versions and confirm_approve_log) + or None + ) + class ContentActionTargetAppealRemovalAffirmation( AnyTargetMixin, AnyOwnerEmailMixin, ContentAction diff --git a/src/olympia/abuse/templates/abuse/emails/ContentActionApproveInitialDecision.txt b/src/olympia/abuse/templates/abuse/emails/ContentActionApproveVersion.txt similarity index 100% rename from src/olympia/abuse/templates/abuse/emails/ContentActionApproveInitialDecision.txt rename to src/olympia/abuse/templates/abuse/emails/ContentActionApproveVersion.txt diff --git a/src/olympia/abuse/tests/test_actions.py b/src/olympia/abuse/tests/test_actions.py index 02bb2b1f2f24..51aa3c8856c7 100644 --- a/src/olympia/abuse/tests/test_actions.py +++ b/src/olympia/abuse/tests/test_actions.py @@ -2,10 +2,12 @@ import uuid from datetime import date, datetime, timedelta from inspect import isclass +from unittest.mock import patch from django.conf import settings from django.core import mail from django.core.files.base import ContentFile +from django.test.utils import override_settings from django.urls import reverse import responses @@ -47,8 +49,8 @@ from ..actions import ( ContentAction, - ContentActionApproveInitialDecision, ContentActionApproveListingContent, + ContentActionApproveVersion, ContentActionBanUser, ContentActionBlockAddon, ContentActionDelayedMidHardBlockAddon, @@ -76,7 +78,7 @@ ) -class BaseTestContentAction: +class BaseContentActionMixin: def setUp(self): addon = addon_factory() self.past_negative_decision = ContentDecision.objects.create( @@ -87,7 +89,7 @@ def setUp(self): ) self.decision = ContentDecision.objects.create( cinder_id='ab89', - action=DECISION_ACTIONS.AMO_APPROVE, + action=self.default_decision_action, private_notes="extra note's", reasoning='some réasoning', addon=addon, @@ -124,6 +126,82 @@ def setUp(self): # action. We need it for the ActivityLog creation to work. set_user(self.task_user) + def _check_owner_email(self, mail_item, subject, snippet): + user = getattr(self, 'user', getattr(self, 'author', None)) + assert mail_item.to == [user.email] + assert mail_item.subject == subject + ' [ref:ab89]' + assert snippet in mail_item.body + assert '[ref:ab89]' in mail_item.body + assert '"' not in mail_item.body + assert '<b>' not in mail_item.body + assert ''' not in mail_item.body + assert self.decision.reasoning in mail_item.body + assert self.decision.private_notes not in mail_item.body + + def test_log_action_user(self): + # just an arbitrary activity class + reviewer = user_factory() + self.decision.update(reviewer_user=reviewer) + assert ( + self.ActionClass(self.decision).log_action(amo.LOG.ADMIN_USER_UNBAN).user + == reviewer + ) + + def test_log_action_saves_policy_texts(self): + # Update the policy with a placeholder - these aren't supposed to be + # used with Cinder originated policy decisions, but we should handle + # this gracefully. + self.policy.update(text='This is {JUDGEMENT} thing') + assert self.ActionClass(self.decision).log_action( + amo.LOG.ADMIN_USER_UNBAN + ).details['policy_texts'] == [ + 'Parent Policy, specifically Bad policy: This is thing' + ] + # change the decision to one that was made by an AMO reviewer + self.decision.update(reviewer_user=user_factory()) + assert ( + # no policy text - the text will be included in the decision notes + 'policy_texts' + not in self.ActionClass(self.decision) + .log_action(amo.LOG.ADMIN_USER_UNBAN) + .details + ) + + # except if the review has directly specified the policies with the placeholders + self.decision.update( + metadata={ + ContentDecision.POLICY_DYNAMIC_VALUES: { + self.policy.uuid: {'JUDGEMENT': 'a Térrible'} + } + } + ) + assert self.ActionClass(self.decision).log_action( + amo.LOG.ADMIN_USER_UNBAN + ).details['policy_texts'] == [ + 'Parent Policy, specifically Bad policy: This is a Térrible thing' + ] + + def test_email_content_not_escaped(self): + unsafe_str = '' + self.decision.update(reasoning=unsafe_str) + action_helper = self.ActionClass(self.decision) + action_helper.notify_owners() + assert unsafe_str in mail.outbox[0].body + + action_helper = ContentActionApproveListingContent(self.decision) + mail.outbox.clear() + action_helper.notify_reporters( + reporter_abuse_reports=[self.abuse_report_auth], is_appeal=True + ) + assert unsafe_str in mail.outbox[0].body + + def test_should_be_skipped_by_automation(self): + # should_be_skipped_by_automation is a classmethod, default is to + # return False. + assert not self.ActionClass.should_be_skipped_by_automation() + + +class NegativeContentActionMixin: def _test_reporter_takedown_email(self, subject): assert mail.outbox[0].to == ['email@domain.com'] assert mail.outbox[1].to == [self.abuse_report_auth.reporter.email] @@ -150,49 +228,6 @@ def _test_reporter_takedown_email(self, subject): assert self.decision.private_notes not in mail.outbox[0].body assert self.decision.private_notes not in mail.outbox[1].body - def _test_reporter_content_approve_email(self, subject): - assert mail.outbox[0].to == ['email@domain.com'] - assert mail.outbox[1].to == [self.abuse_report_auth.reporter.email] - assert mail.outbox[0].subject == ( - subject + f' [ref:ab89/{self.abuse_report_no_auth.id}]' - ) - assert mail.outbox[1].subject == ( - subject + f' [ref:ab89/{self.abuse_report_auth.id}]' - ) - assert 'does not violate Mozilla' in mail.outbox[0].body - assert 'does not violate Mozilla' in mail.outbox[1].body - assert 'was correct' not in mail.outbox[0].body - assert ( - reverse( - 'abuse.appeal_reporter', - kwargs={ - 'abuse_report_id': self.abuse_report_no_auth.id, - 'decision_cinder_id': self.decision.cinder_id, - }, - ) - in mail.outbox[0].body - ) - assert ( - reverse( - 'abuse.appeal_reporter', - kwargs={ - 'abuse_report_id': self.abuse_report_auth.id, - 'decision_cinder_id': self.decision.cinder_id, - }, - ) - in mail.outbox[1].body - ) - assert f'[ref:ab89/{self.abuse_report_no_auth.id}]' in mail.outbox[0].body - assert f'[ref:ab89/{self.abuse_report_auth.id}]' in mail.outbox[1].body - assert '"' not in mail.outbox[0].body - assert '"' not in mail.outbox[1].body - assert '<b>' not in mail.outbox[0].body - assert '<b>' not in mail.outbox[1].body - assert self.decision.reasoning not in mail.outbox[0].body - assert self.decision.reasoning not in mail.outbox[1].body - assert self.decision.private_notes not in mail.outbox[0].body - assert self.decision.private_notes not in mail.outbox[1].body - def _test_reporter_appeal_takedown_email(self, subject): assert mail.outbox[0].to == [self.abuse_report_auth.reporter.email] assert mail.outbox[0].subject == ( @@ -207,33 +242,6 @@ def _test_reporter_appeal_takedown_email(self, subject): assert self.decision.reasoning not in mail.outbox[0].body assert self.decision.private_notes not in mail.outbox[0].body - def _test_reporter_appeal_approve_email(self, subject): - assert mail.outbox[0].to == [self.abuse_report_auth.reporter.email] - assert mail.outbox[0].subject == ( - subject + f' [ref:ab89/{self.abuse_report_auth.id}]' - ) - assert 'does not violate Mozilla' in mail.outbox[0].body - assert 'right to appeal' not in mail.outbox[0].body - assert 'was correct' in mail.outbox[0].body - assert f'[ref:ab89/{self.abuse_report_auth.id}]' in mail.outbox[0].body - assert '"' not in mail.outbox[0].body - assert '<b>' not in mail.outbox[0].body - assert ''' not in mail.outbox[0].body - assert self.decision.reasoning in mail.outbox[0].body - assert self.decision.private_notes not in mail.outbox[0].body - - def _check_owner_email(self, mail_item, subject, snippet): - user = getattr(self, 'user', getattr(self, 'author', None)) - assert mail_item.to == [user.email] - assert mail_item.subject == subject + ' [ref:ab89]' - assert snippet in mail_item.body - assert '[ref:ab89]' in mail_item.body - assert '"' not in mail_item.body - assert '<b>' not in mail_item.body - assert ''' not in mail_item.body - assert self.decision.reasoning in mail_item.body - assert self.decision.private_notes not in mail_item.body - def _test_owner_takedown_email(self, subject, snippet): mail_item = mail.outbox[-1] self._check_owner_email(mail_item, subject, snippet) @@ -297,26 +305,6 @@ def test_approve_override_success(self): self._test_approve_appeal_or_override(ContentActionOverrideApprove) assert 'After reviewing your appeal' not in mail.outbox[0].body - def _test_reporter_no_action_taken(self, *, ActionClass, action): - raise NotImplementedError - - def _test_reporter_content_approved_action_taken(self): - # For most ActionClasses, there is no action taken. - return self._test_reporter_no_action_taken( - ActionClass=ContentActionApproveListingContent, - action=DECISION_ACTIONS.AMO_APPROVE, - ) - - def test_owner_content_approve_report_email(self): - # This isn't called by cinder actions, but is triggered by reviewer actions - subject = self._test_reporter_no_action_taken( - ActionClass=ContentActionApproveInitialDecision, - action=DECISION_ACTIONS.AMO_APPROVE, - ) - assert len(mail.outbox) == 3 - self._test_reporter_content_approve_email(subject) - assert 'has been approved' in mail.outbox[-1].body - def test_notify_reporters_reporters_provided(self): action_helper = self.ActionClass(self.decision) action_helper.notify_reporters( @@ -330,12 +318,25 @@ def test_notify_reporters_reporters_provided(self): assert 'have therefore removed' in mail.outbox[0].body assert f'[ref:ab89/{self.abuse_report_no_auth.id}]' in mail.outbox[0].body - def test_reporter_ignore_invalid_report(self): - self.decision.policies.first().update() - subject = self._test_reporter_no_action_taken( - ActionClass=ContentActionIgnore, action=DECISION_ACTIONS.AMO_IGNORE + def test_notify_2nd_level_approvers(self): + self.ActionClass(self.decision).notify_2nd_level_approvers() + assert len(mail.outbox) == 0 + + user = user_factory() + self.grant_permission(user, ':'.join(ADDONS_HIGH_IMPACT_APPROVE)) + self.ActionClass(self.decision).notify_2nd_level_approvers() + assert len(mail.outbox) == 1 + assert mail.outbox[0].subject == ( + 'A new item has entered the second level approval queue' ) - assert len(mail.outbox) == 2 + assert mail.outbox[0].to == [user.email] + assert reverse('reviewers.decision_review', args=[self.decision.id]) in ( + mail.outbox[0].body + ) + + +class PositiveContentActionMixin: + def _test_reporter_content_approve_email(self, subject): assert mail.outbox[0].to == ['email@domain.com'] assert mail.outbox[1].to == [self.abuse_report_auth.reporter.email] assert mail.outbox[0].subject == ( @@ -344,98 +345,98 @@ def test_reporter_ignore_invalid_report(self): assert mail.outbox[1].subject == ( subject + f' [ref:ab89/{self.abuse_report_auth.id}]' ) - assert f'[ref:ab89/{self.abuse_report_no_auth.id}]' in mail.outbox[0].body - assert f'[ref:ab89/{self.abuse_report_auth.id}]' in mail.outbox[1].body - - for idx in range(0, 1): - assert 'were unable to identify a violation' in mail.outbox[idx].body - assert 'right to appeal' not in mail.outbox[idx].body - assert 'This is bad thing' in mail.outbox[idx].body # policy text - assert 'Bad policy' not in mail.outbox[idx].body # policy name - assert 'Parent' not in mail.outbox[idx].body # parent policy text - - def test_email_content_not_escaped(self): - unsafe_str = '' - self.decision.update(reasoning=unsafe_str) - action_helper = self.ActionClass(self.decision) - action_helper.notify_owners() - assert unsafe_str in mail.outbox[0].body - - action_helper = ContentActionApproveListingContent(self.decision) - mail.outbox.clear() - action_helper.notify_reporters( - reporter_abuse_reports=[self.abuse_report_auth], is_appeal=True - ) - assert unsafe_str in mail.outbox[0].body - - def test_log_action_user(self): - # just an arbitrary activity class - reviewer = user_factory() - self.decision.update(reviewer_user=reviewer) + assert 'does not violate Mozilla' in mail.outbox[0].body + assert 'does not violate Mozilla' in mail.outbox[1].body + assert 'was correct' not in mail.outbox[0].body assert ( - self.ActionClass(self.decision).log_action(amo.LOG.ADMIN_USER_UNBAN).user - == reviewer + reverse( + 'abuse.appeal_reporter', + kwargs={ + 'abuse_report_id': self.abuse_report_no_auth.id, + 'decision_cinder_id': self.decision.cinder_id, + }, + ) + in mail.outbox[0].body ) - - def test_log_action_saves_policy_texts(self): - # Update the policy with a placeholder - these aren't supposed to be - # used with Cinder originated policy decisions, but we should handle - # this gracefully. - self.policy.update(text='This is {JUDGEMENT} thing') - assert self.ActionClass(self.decision).log_action( - amo.LOG.ADMIN_USER_UNBAN - ).details['policy_texts'] == [ - 'Parent Policy, specifically Bad policy: This is thing' - ] - # change the decision to one that was made by an AMO reviewer - self.decision.update(reviewer_user=user_factory()) assert ( - # no policy text - the text will be included in the decision notes - 'policy_texts' - not in self.ActionClass(self.decision) - .log_action(amo.LOG.ADMIN_USER_UNBAN) - .details + reverse( + 'abuse.appeal_reporter', + kwargs={ + 'abuse_report_id': self.abuse_report_auth.id, + 'decision_cinder_id': self.decision.cinder_id, + }, + ) + in mail.outbox[1].body ) + assert f'[ref:ab89/{self.abuse_report_no_auth.id}]' in mail.outbox[0].body + assert f'[ref:ab89/{self.abuse_report_auth.id}]' in mail.outbox[1].body + assert '"' not in mail.outbox[0].body + assert '"' not in mail.outbox[1].body + assert '<b>' not in mail.outbox[0].body + assert '<b>' not in mail.outbox[1].body + assert self.decision.reasoning not in mail.outbox[0].body + assert self.decision.reasoning not in mail.outbox[1].body + assert self.decision.private_notes not in mail.outbox[0].body + assert self.decision.private_notes not in mail.outbox[1].body - # except if the review has directly specified the policies with the placeholders - self.decision.update( - metadata={ - ContentDecision.POLICY_DYNAMIC_VALUES: { - self.policy.uuid: {'JUDGEMENT': 'a Térrible'} - } - } + def _test_reporter_appeal_approve_email(self, subject): + assert mail.outbox[0].to == [self.abuse_report_auth.reporter.email] + assert mail.outbox[0].subject == ( + subject + f' [ref:ab89/{self.abuse_report_auth.id}]' ) - assert self.ActionClass(self.decision).log_action( - amo.LOG.ADMIN_USER_UNBAN - ).details['policy_texts'] == [ - 'Parent Policy, specifically Bad policy: This is a Térrible thing' - ] + assert 'does not violate Mozilla' in mail.outbox[0].body + assert 'right to appeal' not in mail.outbox[0].body + assert 'was correct' in mail.outbox[0].body + assert f'[ref:ab89/{self.abuse_report_auth.id}]' in mail.outbox[0].body + assert '"' not in mail.outbox[0].body + assert '<b>' not in mail.outbox[0].body + assert ''' not in mail.outbox[0].body + assert self.decision.reasoning in mail.outbox[0].body + assert self.decision.private_notes not in mail.outbox[0].body + + def _test_reporter_no_action_taken(self, *, ActionClass, action): + raise NotImplementedError - def test_notify_2nd_level_approvers(self): - self.ActionClass(self.decision).notify_2nd_level_approvers() - assert len(mail.outbox) == 0 + def _test_reporter_content_approved_action_taken(self): + # For most ActionClasses, there is no action taken. + return self._test_reporter_no_action_taken( + ActionClass=ContentActionApproveListingContent, + action=DECISION_ACTIONS.AMO_APPROVE, + ) - user = user_factory() - self.grant_permission(user, ':'.join(ADDONS_HIGH_IMPACT_APPROVE)) - self.ActionClass(self.decision).notify_2nd_level_approvers() - assert len(mail.outbox) == 1 + def test_reporter_ignore_invalid_report(self): + self.decision.policies.first().update() + subject = self._test_reporter_no_action_taken( + ActionClass=ContentActionIgnore, action=DECISION_ACTIONS.AMO_IGNORE + ) + assert len(mail.outbox) == 2 + assert mail.outbox[0].to == ['email@domain.com'] + assert mail.outbox[1].to == [self.abuse_report_auth.reporter.email] assert mail.outbox[0].subject == ( - 'A new item has entered the second level approval queue' + subject + f' [ref:ab89/{self.abuse_report_no_auth.id}]' ) - assert mail.outbox[0].to == [user.email] - assert reverse('reviewers.decision_review', args=[self.decision.id]) in ( - mail.outbox[0].body + assert mail.outbox[1].subject == ( + subject + f' [ref:ab89/{self.abuse_report_auth.id}]' ) + assert f'[ref:ab89/{self.abuse_report_no_auth.id}]' in mail.outbox[0].body + assert f'[ref:ab89/{self.abuse_report_auth.id}]' in mail.outbox[1].body - def test_should_be_skipped_by_automation(self): - # should_be_skipped_by_automation is a classmethod, default is to - # return False. - assert not self.ActionClass.should_be_skipped_by_automation() + for idx in range(0, 1): + assert 'were unable to identify a violation' in mail.outbox[idx].body + assert 'right to appeal' not in mail.outbox[idx].body + assert 'This is bad thing' in mail.outbox[idx].body # policy text + assert 'Bad policy' not in mail.outbox[idx].body # policy name + assert 'Parent' not in mail.outbox[idx].body # parent policy text -class TestContentActionBanUser(BaseTestContentAction, TestCase): +class TestContentActionBanUser( + PositiveContentActionMixin, + NegativeContentActionMixin, + BaseContentActionMixin, + TestCase, +): ActionClass = ContentActionBanUser - takedown_decision_action = DECISION_ACTIONS.AMO_BAN_USER + default_decision_action = DECISION_ACTIONS.AMO_BAN_USER def setUp(self): super().setUp() @@ -443,13 +444,13 @@ def setUp(self): self.cinder_job.abusereport_set.update(user=self.user, guid=None) self.decision.update(addon=None, user=self.user) self.past_negative_decision.update( - addon=None, user=self.user, action=self.takedown_decision_action + addon=None, user=self.user, action=self.default_decision_action ) def _test_ban_user(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) - assert action_helper.action == self.takedown_decision_action + assert action_helper.action == self.default_decision_action activity = action_helper.process_action() assert activity.log == amo.LOG.ADMIN_USER_BANNED assert activity.arguments == [self.user, self.decision, self.policy] @@ -476,7 +477,7 @@ def _test_ban_user(self): return subject def test_log_action_no_notes(self): - self.decision.update(private_notes='', action=self.takedown_decision_action) + self.decision.update(private_notes='', action=self.default_decision_action) action_helper = self.ActionClass(self.decision) action_helper.process_action() assert ActivityLog.objects.count() == 1 @@ -567,7 +568,7 @@ def test_target_appeal_decline(self): self._test_owner_affirmation_email(f'Mozilla Add-ons: {self.user.name}') def test_should_hold_action(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) assert action_helper.should_hold_action() is False @@ -591,7 +592,7 @@ def test_should_hold_action(self): assert action_helper.should_hold_action() is False def test_hold_action(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) activity = action_helper.hold_action() assert activity.log == amo.LOG.HELD_ACTION_ADMIN_USER_BANNED @@ -610,11 +611,13 @@ def test_hold_action(self): @override_switch('dsa-cinder-forwarded-review', active=True) -class TestContentActionDisableAddon(BaseTestContentAction, TestCase): +class TestContentActionDisableAddon( + NegativeContentActionMixin, BaseContentActionMixin, TestCase +): ActionClass = ContentActionDisableAddon activity_log_action = amo.LOG.FORCE_DISABLE disable_snippet = 'permanently disabled' - takedown_decision_action = DECISION_ACTIONS.AMO_DISABLE_ADDON + default_decision_action = DECISION_ACTIONS.AMO_DISABLE_ADDON def setUp(self): super().setUp() @@ -631,7 +634,7 @@ def setUp(self): self.decision.update(addon=self.addon) self.decision.target_versions.set((self.version, self.old_version)) self.past_negative_decision.update( - addon=self.addon, action=self.takedown_decision_action + addon=self.addon, action=self.default_decision_action ) self.past_negative_decision.target_versions.set( (self.version, self.old_version) @@ -666,9 +669,9 @@ def test_addon_version_has_no_target_version(self): assert self.ActionClass(self.decision).addon_version == self.another_version def _process_action_and_notify(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) - assert action_helper.action == self.takedown_decision_action + assert action_helper.action == self.default_decision_action activity = action_helper.process_action() assert activity assert activity.log == self.activity_log_action @@ -693,7 +696,7 @@ def _process_action_and_notify(self): action_helper.notify_owners() def test_log_action_no_notes(self): - self.decision.update(private_notes='', action=self.takedown_decision_action) + self.decision.update(private_notes='', action=self.default_decision_action) action_helper = self.ActionClass(self.decision) action_helper.process_action() assert not ActivityLog.objects.filter( @@ -701,7 +704,7 @@ def test_log_action_no_notes(self): ).exists() def test_already_taken_down(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) self.addon.update(status=amo.STATUS_DISABLED) action_helper = self.ActionClass(self.decision) assert action_helper.process_action() is None @@ -794,7 +797,7 @@ def test_target_appeal_decline_no_manual_reasoning_text(self): def test_notify_owners_with_manual_reasoning_text(self): self.decision.update( - action=self.takedown_decision_action, + action=self.default_decision_action, reasoning='some other policy justification', ) self.ActionClass(self.decision).notify_owners( @@ -818,7 +821,7 @@ def test_notify_owners_with_manual_reasoning_text(self): assert 'some other policy justification' in mail_item.body def test_notify_owners_with_for_third_party_decision(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) self.ActionClass(self.decision).notify_owners() mail_item = mail.outbox[0] self._check_owner_email( @@ -833,7 +836,7 @@ def test_notify_owners_with_for_proactive_decision(self): self.abuse_report_auth.delete() self.abuse_report_no_auth.delete() self.decision.refresh_from_db() - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) self.ActionClass(self.decision).notify_owners() mail_item = mail.outbox[0] self._check_owner_email( @@ -844,7 +847,7 @@ def test_notify_owners_with_for_proactive_decision(self): assert 'based on a report we received from a third party' not in mail_item.body def test_notify_owners_non_public_url(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) self.addon.update(status=amo.STATUS_DISABLED, _current_version=None) assert self.addon.get_url_path() == '' @@ -860,7 +863,7 @@ def test_notify_owners_non_public_url(self): ) def test_should_hold_action(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) assert action_helper.should_hold_action() is False @@ -871,7 +874,7 @@ def test_should_hold_action(self): assert action_helper.should_hold_action() is False def test_hold_action(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) activity = action_helper.hold_action() assert activity.log == amo.LOG.HELD_ACTION_FORCE_DISABLE @@ -1134,18 +1137,12 @@ def test_approve_override_success_but_not_approved(self): ContentActionOverrideApprove ) - def test_owner_content_approve_report_email(self): - pass # Covered by TestContentApproveContentListing - - def test_reporter_ignore_invalid_report(self): - pass # Covered by TestContentApproveContentListing - class TestContentActionRejectVersion(TestContentActionDisableAddon): ActionClass = ContentActionRejectVersion activity_log_action = amo.LOG.REJECT_VERSION disable_snippet = 'versions of your Extension have been disabled' - takedown_decision_action = DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON + default_decision_action = DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON def setUp(self): super().setUp() @@ -1157,7 +1154,7 @@ def _test_reject_version(self, *, content_review, expected_emails_from_action=0) old_version_original_status = self.old_version.file.status version_original_status = self.version.file.status self.decision.update( - action=self.takedown_decision_action, + action=self.default_decision_action, metadata={'content_review': content_review}, ) NeedsHumanReview(version=self.old_version).save(_no_automatic_activity_log=True) @@ -1333,7 +1330,7 @@ def test_approve_override_success_for_delayed_reject(self): def test_log_action_no_notes(self): self.decision.update( private_notes='', - action=self.takedown_decision_action, + action=self.default_decision_action, reviewer_user=user_factory(), ) action_helper = self.ActionClass(self.decision) @@ -1345,7 +1342,7 @@ def test_log_action_no_notes(self): def test_already_taken_down(self): self.decision.update( - action=self.takedown_decision_action, reviewer_user=user_factory() + action=self.default_decision_action, reviewer_user=user_factory() ) self.version.file.update(status=amo.STATUS_DISABLED) self.old_version.file.update(status=amo.STATUS_DISABLED) @@ -1676,7 +1673,7 @@ def test_execute_action_delayed_after_reporter_appeal(self): def test_hold_action(self): NeedsHumanReview(version=self.old_version).save(_no_automatic_activity_log=True) NeedsHumanReview(version=self.version).save(_no_automatic_activity_log=True) - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) activity = action_helper.hold_action() assert activity.log == amo.LOG.HELD_ACTION_REJECT_VERSIONS @@ -1714,7 +1711,7 @@ def test_hold_action_human(self): user = user_factory() NeedsHumanReview(version=self.old_version).save(_no_automatic_activity_log=True) NeedsHumanReview(version=self.version).save(_no_automatic_activity_log=True) - self.decision.update(action=self.takedown_decision_action, reviewer_user=user) + self.decision.update(action=self.default_decision_action, reviewer_user=user) action_helper = self.ActionClass(self.decision) activity = action_helper.hold_action() assert activity.arguments == [ @@ -2000,7 +1997,7 @@ def _test_approve_appeal_or_override_but_not_approved(self, ActionClass): class TestContentActionBlockAddon(TestContentActionDisableAddon): ActionClass = ContentActionBlockAddon - takedown_decision_action = DECISION_ACTIONS.AMO_BLOCK_ADDON + default_decision_action = DECISION_ACTIONS.AMO_BLOCK_ADDON def setUp(self): super().setUp() @@ -2051,7 +2048,7 @@ def _process_action_and_notify(self): def test_already_taken_down(self): """For a block action, this shouldn't affect the block, only the disable""" - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) self.addon.update(status=amo.STATUS_DISABLED) File.objects.filter(version__addon=self.addon).update( status=amo.STATUS_DISABLED @@ -2075,7 +2072,7 @@ def test_already_taken_down(self): ) def test_already_blocked(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) BlockVersion.objects.create(block=self.addon.block, version=self.version) BlockVersion.objects.create(block=self.addon.block, version=self.old_version) action_helper = self.ActionClass(self.decision) @@ -2086,7 +2083,7 @@ def test_should_hold_action(self): PromotedGroup.objects.get_or_create( group_id=PROMOTED_GROUP_CHOICES.RECOMMENDED, high_profile=True ) - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) assert action_helper.should_hold_action() is False @@ -2191,9 +2188,11 @@ def test_should_be_skipped_by_automation_unresolved_appeal(self): ) -class TestContentActionDelayedShortSoftBlockAddon(BaseTestContentAction, TestCase): +class TestContentActionDelayedShortSoftBlockAddon( + NegativeContentActionMixin, BaseContentActionMixin, TestCase +): ActionClass = ContentActionDelayedShortSoftBlockAddon - takedown_decision_action = DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON + default_decision_action = DECISION_ACTIONS.AMO_FU_DELAY_SHORT_SOFT_BLOCK_ADDON block_type = BlockType.SOFT_BLOCKED def setUp(self): @@ -2218,7 +2217,7 @@ def setUp(self): self.cinder_job.abusereport_set.update(guid=self.addon.guid) self.decision.update(addon=self.addon) self.past_negative_decision.update( - addon=self.addon, action=self.takedown_decision_action + addon=self.addon, action=self.default_decision_action ) self.past_negative_decision.target_versions.set( (self.version, self.old_version) @@ -2227,7 +2226,7 @@ def setUp(self): def _test_process_action(self, version_ids, followup_action): assert not BlocklistSubmission.objects.exists() action_helper = self.ActionClass(self.decision, followup_action) - assert action_helper.action == self.takedown_decision_action + assert action_helper.action == self.default_decision_action action_helper.process_action() assert BlocklistSubmission.objects.count() == 1 submission = BlocklistSubmission.objects.get() @@ -2268,7 +2267,7 @@ def _test_process_action(self, version_ids, followup_action): def test_process_action_standalone(self): # Note: this isn't currently a codepath that's possible - the class is only used # as a follow-up action. - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) assert not self.decision.target_versions.exists() self._test_process_action([self.another_version.id, self.version.id], None) # shouldn't change the addon or version.file statues. @@ -2279,7 +2278,7 @@ def test_process_action_standalone(self): def test_process_action_followup_from_disable_addon(self): self.decision.update(action=DECISION_ACTIONS.AMO_DISABLE_ADDON) followup = ContentDecisionFollowupAction.objects.create( - decision=self.decision, action=self.takedown_decision_action + decision=self.decision, action=self.default_decision_action ) self.addon.update(status=amo.STATUS_DISABLED) # typically this _would_ be set, but it shouldn't be used anyway @@ -2295,7 +2294,7 @@ def test_process_action_followup_from_disable_addon(self): def test_process_action_with_multiple_followups(self): self.decision.update(action=DECISION_ACTIONS.AMO_DISABLE_ADDON) followup = ContentDecisionFollowupAction.objects.create( - decision=self.decision, action=self.takedown_decision_action + decision=self.decision, action=self.default_decision_action ) another_followup = ContentDecisionFollowupAction.objects.create( decision=self.decision, @@ -2319,7 +2318,7 @@ def test_process_action_with_multiple_followups(self): def test_process_action_followup_from_reject_version(self): self.decision.update(action=DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON) followup = ContentDecisionFollowupAction.objects.create( - decision=self.decision, action=self.takedown_decision_action + decision=self.decision, action=self.default_decision_action ) self.version.file.update(status=amo.STATUS_DISABLED) self.old_version.file.update(status=amo.STATUS_DISABLED) @@ -2335,7 +2334,7 @@ def test_process_action_followup_from_reject_version(self): def test_primary_action_emails_mention_followups(self): self.decision.update(action=DECISION_ACTIONS.AMO_DISABLE_ADDON) followup = ContentDecisionFollowupAction.objects.create( - decision=self.decision, action=self.takedown_decision_action + decision=self.decision, action=self.default_decision_action ) action_helper = ContentActionDisableAddon(self.decision) @@ -2347,12 +2346,6 @@ def test_primary_action_emails_mention_followups(self): assert followup.description_with_eta in email_body assert f'days, on {future_date.strftime("%Y-%m-%d")}' in email_body - def test_owner_content_approve_report_email(self): - pass # Covered by TestContentApproveContentListing - - def test_reporter_ignore_invalid_report(self): - pass # Covered by TestContentApproveContentListing - def _test_approve_appeal_or_override(self, ActionClass): yet_another_version = version_factory(addon=self.addon) self.decision.update(action=DECISION_ACTIONS.AMO_APPROVE) @@ -2409,7 +2402,7 @@ def _test_approve_appeal_or_override(self, ActionClass): def test_approve_appeal_success(self): self.past_negative_decision.update( - appeal_job=self.cinder_job, action=self.takedown_decision_action + appeal_job=self.cinder_job, action=self.default_decision_action ) self._test_approve_appeal_or_override(ContentActionTargetAppealApprove) # TODO: once we add support for emails, re-enable this? @@ -2417,7 +2410,7 @@ def test_approve_appeal_success(self): def test_approve_override_success(self): self.decision.update(override_of=self.past_negative_decision) - self.past_negative_decision.update(action=self.takedown_decision_action) + self.past_negative_decision.update(action=self.default_decision_action) self._test_approve_appeal_or_override(ContentActionOverrideApprove) # TODO: once we add support for emails, re-enable this? # assert 'After reviewing your appeal' not in mail.outbox[0].body @@ -2428,7 +2421,7 @@ def test_approve_appeal_success_followup(self): ) ContentDecisionFollowupAction.objects.create( decision=self.past_negative_decision, - action=self.takedown_decision_action, + action=self.default_decision_action, action_date=datetime.now(), ) self._test_approve_appeal_or_override(ContentActionTargetAppealApprove) @@ -2439,7 +2432,7 @@ def test_approve_appeal_success_followup_with_multiple_followups(self): ) ContentDecisionFollowupAction.objects.create( decision=self.past_negative_decision, - action=self.takedown_decision_action, + action=self.default_decision_action, action_date=datetime.now(), ) # These follow-up actions are redundant, but shouldn't cause errors. @@ -2460,7 +2453,7 @@ def test_approve_override_success_followup(self): self.past_negative_decision.update(action=DECISION_ACTIONS.AMO_DISABLE_ADDON) ContentDecisionFollowupAction.objects.create( decision=self.past_negative_decision, - action=self.takedown_decision_action, + action=self.default_decision_action, action_date=datetime.now(), ) self._test_approve_appeal_or_override(ContentActionOverrideApprove) @@ -2479,7 +2472,7 @@ class TestContentActionDelayedMidHardBlockAddon( TestContentActionDelayedShortSoftBlockAddon ): ActionClass = ContentActionDelayedMidHardBlockAddon - takedown_decision_action = DECISION_ACTIONS.AMO_FU_DELAY_MID_HARD_BLOCK_ADDON + default_decision_action = DECISION_ACTIONS.AMO_FU_DELAY_MID_HARD_BLOCK_ADDON block_type = BlockType.BLOCKED def setUp(self): @@ -2496,9 +2489,11 @@ def test_description(self): ) -class TestContentActionApproveListingContent(BaseTestContentAction, TestCase): +class TestContentActionApproveListingContent( + PositiveContentActionMixin, BaseContentActionMixin, TestCase +): ActionClass = ContentActionApproveListingContent - takedown_decision_action = DECISION_ACTIONS.AMO_APPROVE + default_decision_action = DECISION_ACTIONS.AMO_APPROVE activity_log_action = amo.LOG.APPROVE_LISTING_CONTENT def setUp(self): @@ -2516,7 +2511,7 @@ def setUp(self): self.decision.update(addon=self.addon) self.decision.target_versions.set((self.version, self.old_version)) self.past_negative_decision.update( - addon=self.addon, action=self.takedown_decision_action + addon=self.addon, action=self.default_decision_action ) self.past_negative_decision.target_versions.set( (self.version, self.old_version) @@ -2702,36 +2697,327 @@ def test_content_approve_rejected_listing_content_but_awaiting_approval(self): assert 'It is now available' not in mail.outbox[-1].body assert 'information on its availability.' in mail.outbox[-1].body - def test_approve_appeal_success(self): - # This test doesn't apply for this ActionClass as it's for appeals following - # that action that result in Approve. - pass + def test_email_content_not_escaped(self): + self.addon.update(status=amo.STATUS_REJECTED) + super().test_email_content_not_escaped() - def test_approve_override_success(self): - # This test doesn't apply for this ActionClass as it's for overrides following - # that action that result in Approve. - pass - def test_notify_reporters_reporters_provided(self): - # This test doesn't apply for this ActionClass because it's emailing about - # content removal. - pass +# Those tests can call signing when making things public. We want to test that +# it works correctly, so we set ENABLE_ADDON_SIGNING to True and mock the +# actual signing call below in setUp(). +@override_settings(ENABLE_ADDON_SIGNING=True) +class TestContentActionApproveVersion( + PositiveContentActionMixin, BaseContentActionMixin, TestCase +): + ActionClass = ContentActionApproveVersion + default_decision_action = DECISION_ACTIONS.AMO_APPROVE_VERSION + activity_log_action = amo.LOG.APPROVE_VERSION + + def setUp(self): + super().setUp() + self.author = user_factory() + self.reviewer = user_factory() + self.addon = addon_factory(users=(self.author,), name='Bad Addön') + self.old_version = self.addon.current_version + self.version = version_factory( + addon=self.addon, file_kw={'status': amo.STATUS_AWAITING_REVIEW} + ) + self.another_version = version_factory( + addon=self.addon, file_kw={'status': amo.STATUS_DISABLED} + ) + self.addon.reload() + ActivityLog.objects.all().delete() + self.cinder_job.abusereport_set.update(guid=self.addon.guid) + self.decision.update(addon=self.addon) + self.decision.target_versions.set((self.version, self.old_version)) + self.past_negative_decision.update( + addon=self.addon, action=self.default_decision_action + ) + self.past_negative_decision.target_versions.set( + (self.version, self.old_version) + ) + patcher = patch('olympia.abuse.actions.sign_file') + self.addCleanup(patcher.stop) + self.sign_file_mock = patcher.start() + + def _test_reporter_no_action_taken(self, *, ActionClass, action): + self.decision.update(action=action) + action_helper = ActionClass(self.decision) + assert action_helper.process_action() is None + + assert self.addon.reload().status == amo.STATUS_APPROVED + assert ActivityLog.objects.count() == 0 + assert len(mail.outbox) == 0 + self.cinder_job.notify_reporters(action_helper) + action_helper.notify_owners() + return f'Mozilla Add-ons: {self.addon.name}' + + def _test_reporter_content_approved_action_taken(self): + # override because Addon versions can get signed + self.decision.update(action=self.default_decision_action) + assert self.decision.target_versions.exists() + action_helper = self.ActionClass(self.decision) + # process_action is only available for reviewer tools decisions. + with self.assertRaises(NotImplementedError): + action_helper.process_action() + + self.decision.update(reviewer_user=self.reviewer) + activity = action_helper.process_action() + + assert self.version.file.reload().status == amo.STATUS_APPROVED + self.assertCloseToNow(self.version.file.approval_date) + assert self.old_version.file.approval_date is None # would have been set before + self.sign_file_mock.assert_called_with(self.version.file) + self.sign_file_mock.assert_called_once() # we didn't call it with old_version + + assert activity.log == amo.LOG.APPROVE_VERSION + # versions in the args + assert activity.arguments == [ + self.addon, + self.decision, + self.policy, + self.version, + ] + assert activity.user == self.reviewer + # exclude this extra activity log - we'll test specificially for it elsewhere + activity_log_qs = ActivityLog.objects.exclude(action=amo.LOG.UNLISTED_SIGNED.id) + assert activity_log_qs.count() == 3 + second_activity = ( + activity_log_qs.exclude(pk=activity.pk) + .exclude(action=amo.LOG.CONFIRM_AUTO_APPROVED.id) + .get() + ) + assert second_activity.log == amo.LOG.REVIEWER_PRIVATE_COMMENT + assert second_activity.arguments == [self.addon, self.decision] + assert second_activity.user == self.reviewer + assert second_activity.details == {'comments': self.decision.private_notes} + third_activity = ( + activity_log_qs.exclude(pk=activity.pk) + .exclude(action=amo.LOG.REVIEWER_PRIVATE_COMMENT.id) + .get() + ) + assert third_activity.log == amo.LOG.CONFIRM_AUTO_APPROVED + assert third_activity.arguments == [ + self.addon, + self.decision, + self.policy, + self.old_version, + ] + assert third_activity.user == self.reviewer + + # get this again, to replicate how send_notifications works + action_helper = self.ActionClass(self.decision) + assert len(mail.outbox) == 0 + if self.decision.cinder_job: + self.decision.cinder_job.notify_reporters(action_helper) + action_helper.notify_owners() + return f'Mozilla Add-ons: {self.addon.name}' + + def test_reporter_appeal_approve(self): + original_job = CinderJob.objects.create( + job_id='original', + decision=ContentDecision.objects.create( + addon=self.decision.addon, + user=self.decision.user, + rating=self.decision.rating, + collection=self.decision.collection, + action=self.default_decision_action, + ), + ) + self.cinder_job.appealed_decisions.add(original_job.final_decision) + self.abuse_report_no_auth.update(cinder_job=original_job) + self.abuse_report_auth.update(cinder_job=original_job) + CinderAppeal.objects.create( + decision=original_job.final_decision, reporter_report=self.abuse_report_auth + ) + self.cinder_job.reload() + subject = self._test_reporter_content_approved_action_taken() + assert len(mail.outbox) == 2 # one for the author, one for the reporter + self._test_reporter_appeal_approve_email(subject) + + def test_execute_action(self): + # testing the case of: listed versions; human review + for version in (self.version, self.old_version): + VersionReviewerFlags.objects.create( + version=version, + pending_rejection=datetime.now(), + pending_rejection_by=self.task_user, + pending_content_rejection=False, + ) + AddonReviewerFlags.objects.create( + addon=self.addon, + auto_approval_disabled_until_next_approval=True, + auto_approval_disabled_until_next_approval_unlisted=True, + ) + # test the vanilla case, where there is no cinder_job + self.decision.update(cinder_job=None) + self._test_reporter_content_approved_action_taken() + + self.assertCloseToNow(self.version.reload().human_review_date) + self.assertCloseToNow(self.old_version.reload().human_review_date) + assert self.version.reviewerflags.reload().pending_rejection is None + assert self.old_version.reviewerflags.reload().pending_rejection is None + assert ( + AddonApprovalsCounter.objects.get(addon=self.addon).content_review_status + == AddonApprovalsCounter.CONTENT_REVIEW_STATUSES.PASS + ) + assert self.addon.auto_approval_disabled_until_next_approval is False + assert self.addon.auto_approval_disabled_until_next_approval_unlisted is True + + assert len(mail.outbox) == 1 + assert mail.outbox[0].recipients() == [self.author.email] + assert 'has been approved' in mail.outbox[0].body + + def test_execute_action_no_files_awaiting_review(self): + self.version.file.update(status=amo.STATUS_APPROVED) + self.decision.update( + cinder_job=None, + action=self.default_decision_action, + reviewer_user=self.reviewer, + ) + assert self.decision.target_versions.exists() + action_helper = self.ActionClass(self.decision) + + activity = action_helper.process_action() + assert activity.log == amo.LOG.CONFIRM_AUTO_APPROVED + # versions in the args + assert activity.arguments == [ + self.addon, + self.decision, + self.policy, + self.version, + self.old_version, + ] + assert activity.user == self.reviewer + + assert self.addon.reload().status == amo.STATUS_APPROVED + assert ActivityLog.objects.count() == 2 + second_activity = ActivityLog.objects.exclude(pk=activity.pk).get() + assert second_activity.log == amo.LOG.REVIEWER_PRIVATE_COMMENT + assert second_activity.arguments == [self.addon, self.decision] + assert second_activity.user == self.reviewer + assert second_activity.details == {'comments': self.decision.private_notes} + + # get this again, to replicate how send_notifications works + action_helper = self.ActionClass(self.decision) + action_helper.notify_owners() + assert len(mail.outbox) == 0 + + def test_execute_action_unlisted(self): + # testing the case of: unlisted versions; human review + self.make_addon_unlisted(self.addon) + assert self.addon.status == amo.STATUS_NULL + ActivityLog.objects.all().delete() + for version in (self.version, self.old_version): + VersionReviewerFlags.objects.create( + version=version, + pending_rejection=datetime.now(), + pending_rejection_by=self.task_user, + pending_content_rejection=False, + ) + AddonReviewerFlags.objects.create( + addon=self.addon, + auto_approval_disabled_until_next_approval=True, + auto_approval_disabled_until_next_approval_unlisted=True, + ) + # test the vanilla case, where there is no cinder_job + self.decision.update(cinder_job=None) + self._test_reporter_content_approved_action_taken() + + self.assertCloseToNow(self.version.reload().human_review_date) + self.assertCloseToNow(self.old_version.reload().human_review_date) + assert self.version.reviewerflags.reload().pending_rejection is None + assert self.old_version.reviewerflags.reload().pending_rejection is None + assert not AddonApprovalsCounter.objects.filter(addon=self.addon).exists() + assert self.addon.reload().auto_approval_disabled_until_next_approval is True + assert self.addon.auto_approval_disabled_until_next_approval_unlisted is False + assert ActivityLog.objects.get(action=amo.LOG.UNLISTED_SIGNED.id).arguments == [ + self.addon, + self.decision, + self.policy, + self.version.file, + ] + + assert len(mail.outbox) == 1 + assert mail.outbox[0].recipients() == [self.author.email] + assert 'has been approved' in mail.outbox[0].body + + def test_execute_action_promoted(self): + self.make_addon_promoted(self.addon, PROMOTED_GROUP_CHOICES.RECOMMENDED) + assert not self.addon.promoted_groups() + self.test_execute_action() + assert self.addon.promoted_groups() + assert self.version.promoted_versions.filter( + promoted_group__group_id=PROMOTED_GROUP_CHOICES.RECOMMENDED + ).exists() + assert self.old_version.promoted_versions.filter( + promoted_group__group_id=PROMOTED_GROUP_CHOICES.RECOMMENDED + ).exists() + + def test_execute_action_not_human(self): + # testing the case of: listed versions; not human + self.reviewer = self.task_user + for version in (self.version, self.old_version): + VersionReviewerFlags.objects.create( + version=version, + pending_rejection=datetime.now(), + pending_rejection_by=self.task_user, + pending_content_rejection=False, + ) + AddonReviewerFlags.objects.create( + addon=self.addon, + auto_approval_disabled_until_next_approval=True, + auto_approval_disabled_until_next_approval_unlisted=True, + ) + AddonApprovalsCounter.objects.create( + addon=self.addon, + content_review_status=AddonApprovalsCounter.CONTENT_REVIEW_STATUSES.UNREVIEWED, + counter=1, + ) + # test the vanilla case, where there is no cinder_job + self.decision.update(cinder_job=None) + self._test_reporter_content_approved_action_taken() + + assert self.version.reload().human_review_date is None + assert self.old_version.reload().human_review_date is None + self.assertCloseToNow(self.version.reviewerflags.reload().pending_rejection) + self.assertCloseToNow(self.old_version.reviewerflags.reload().pending_rejection) + aacounter = AddonApprovalsCounter.objects.get(addon=self.addon) + assert ( + aacounter.content_review_status + == AddonApprovalsCounter.CONTENT_REVIEW_STATUSES.UNREVIEWED + ) + assert aacounter.counter == 0 + assert self.addon.auto_approval_disabled_until_next_approval is True + assert self.addon.auto_approval_disabled_until_next_approval_unlisted is True + + assert len(mail.outbox) == 1 + assert mail.outbox[0].recipients() == [self.author.email] + assert 'has been approved' in mail.outbox[0].body def test_email_content_not_escaped(self): - self.addon.update(status=amo.STATUS_REJECTED) + ActivityLog.objects.create( + amo.LOG.APPROVE_VERSION, + self.addon, + self.decision, + self.policy, + self.version, + self.old_version, + user=self.reviewer, + ) super().test_email_content_not_escaped() class TestContentActionRejectListingContent(TestContentActionDisableAddon): ActionClass = ContentActionRejectListingContent - takedown_decision_action = DECISION_ACTIONS.AMO_REJECT_LISTING_CONTENT + default_decision_action = DECISION_ACTIONS.AMO_REJECT_LISTING_CONTENT disable_snippet = 'until you address the violations and request a further review' activity_log_action = amo.LOG.REJECT_LISTING_CONTENT def _process_action_and_notify(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) - assert action_helper.action == self.takedown_decision_action + assert action_helper.action == self.default_decision_action activity = action_helper.process_action() assert activity assert activity.log == self.activity_log_action @@ -2752,7 +3038,7 @@ def _process_action_and_notify(self): # get this again, to replicate how send_notifications works action_helper = self.ActionClass(self.decision) - assert action_helper.action == self.takedown_decision_action + assert action_helper.action == self.default_decision_action self.cinder_job.notify_reporters(action_helper) action_helper.notify_owners() @@ -2794,7 +3080,7 @@ def test_execute_action(self): assert not AddonReviewerFlags.objects.filter(addon=self.addon).exists() def test_hold_action(self): - self.decision.update(action=self.takedown_decision_action) + self.decision.update(action=self.default_decision_action) action_helper = self.ActionClass(self.decision) activity = action_helper.hold_action() assert activity.log == amo.LOG.HELD_ACTION_REJECT_LISTING_CONTENT @@ -2927,9 +3213,14 @@ def test_should_be_skipped_by_automation(self): ) -class TestContentActionCollection(BaseTestContentAction, TestCase): +class TestContentActionCollection( + PositiveContentActionMixin, + NegativeContentActionMixin, + BaseContentActionMixin, + TestCase, +): ActionClass = ContentActionDeleteCollection - takedown_decision_action = DECISION_ACTIONS.AMO_DELETE_COLLECTION + default_decision_action = DECISION_ACTIONS.AMO_DELETE_COLLECTION def setUp(self): super().setUp() @@ -2950,7 +3241,7 @@ def setUp(self): def _test_delete_collection(self): self.decision.update(action=DECISION_ACTIONS.AMO_DELETE_COLLECTION) action_helper = self.ActionClass(self.decision) - assert action_helper.action == self.takedown_decision_action + assert action_helper.action == self.default_decision_action log_entry = action_helper.process_action() assert self.collection.reload() @@ -3095,9 +3386,14 @@ def test_hold_action(self): assert second_activity.details == {'comments': self.decision.private_notes} -class TestContentActionRating(BaseTestContentAction, TestCase): +class TestContentActionRating( + PositiveContentActionMixin, + NegativeContentActionMixin, + BaseContentActionMixin, + TestCase, +): ActionClass = ContentActionDeleteRating - takedown_decision_action = DECISION_ACTIONS.AMO_DELETE_RATING + default_decision_action = DECISION_ACTIONS.AMO_DELETE_RATING def setUp(self): super().setUp() @@ -3115,7 +3411,7 @@ def setUp(self): def _test_delete_rating(self): self.decision.update(action=DECISION_ACTIONS.AMO_DELETE_RATING) action_helper = self.ActionClass(self.decision) - assert action_helper.action == self.takedown_decision_action + assert action_helper.action == self.default_decision_action activity = action_helper.process_action() assert activity.log == amo.LOG.DELETE_RATING assert activity.arguments == [ @@ -3303,7 +3599,7 @@ def test_hold_action(self): class TestContentActionLegalTakedownDisableAddon(TestContentActionDisableAddon): ActionClass = ContentActionLegalTakedownDisableAddon - takedown_decision_action = DECISION_ACTIONS.AMO_LEGAL_DISABLE_ADDON + default_decision_action = DECISION_ACTIONS.AMO_LEGAL_DISABLE_ADDON def test_execute_action(self): self._process_action_and_notify() diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index 76acb3861167..7330590d39a0 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -116,6 +116,7 @@ class VersionsChoiceWidget(forms.SelectMultiple): actions_filters = { amo.CHANNEL_UNLISTED: { amo.STATUS_APPROVED: [ + 'review_with_policy_approve', 'review_with_policy', 'block_multiple_versions', 'confirm_multiple_versions', @@ -123,6 +124,7 @@ class VersionsChoiceWidget(forms.SelectMultiple): 'reply', ], amo.STATUS_AWAITING_REVIEW: [ + 'review_with_policy_approve', 'review_with_policy', 'approve_multiple_versions', 'reject_multiple_versions', @@ -141,6 +143,7 @@ class VersionsChoiceWidget(forms.SelectMultiple): 'reply', ], amo.STATUS_AWAITING_REVIEW: [ + 'review_with_policy_approve', 'review_with_policy', 'approve_multiple_versions', 'reject_multiple_versions', @@ -183,6 +186,10 @@ def create_option(self, *args, **kwargs): # allow confirmation of its auto-approval. if obj.deleted and obj.was_auto_approved: actions.append('confirm_multiple_versions') + # If the version is the current version and was auto-approved, make it + # possible to confirm the auto-approval via policy-selection + if obj == obj.addon.current_version and obj.was_auto_approved: + actions.append('review_with_policy_approve') option['attrs']['class'] = 'data-toggle' option['attrs']['data-value'] = ' '.join(actions) @@ -304,6 +311,7 @@ def create_option( hide_for_these_actions = ['appeal_override', 'resolve_reports_job'] elif is_developer_appeal: hide_for_these_actions = [ + 'review_with_policy_approve', 'review_with_policy', 'reject', 'reject_multiple_versions', diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index da2686ca7567..4fc077437e75 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -604,7 +604,7 @@ def test_versions_required_when_enforcement_is_on_versions(self): action = form.cleaned_data['action'] assert action == 'review_with_policy' assert form.cleaned_data['cinder_policies'] == [reject_policy] - assert not form.helper.get_actions()[action]['multiple_versions'] + assert form.helper.get_actions()[action]['multiple_versions'] assert form.errors == {'versions': ['This field is required.']} def test_cinder_jobs_filtered_for_resolve_reports_job_and_appeal_deny(self): @@ -884,6 +884,7 @@ def test_versions_queryset_contains_pending_files_for_listed( 'reject_multiple_versions', 'reply', 'set_needs_human_review_multiple_versions', + 'review_with_policy_approve', ] assert option1.attrib.get('value') == str(self.version.pk) @@ -891,6 +892,7 @@ def test_versions_queryset_contains_pending_files_for_listed( assert option2.attrib.get('class') == 'data-toggle' assert option2.attrib.get('data-value').split(' ') == [ # That version is pending. + 'review_with_policy_approve', 'review_with_policy', 'approve_multiple_versions', 'reject_multiple_versions', @@ -1017,6 +1019,7 @@ def test_versions_queryset_contains_pending_files_for_unlisted( assert option1.attrib.get('class') == 'data-toggle' assert option1.attrib.get('data-value').split(' ') == [ # That version is approved. + 'review_with_policy_approve', 'review_with_policy', 'block_multiple_versions', 'confirm_multiple_versions', @@ -1030,6 +1033,7 @@ def test_versions_queryset_contains_pending_files_for_unlisted( assert option2.attrib.get('class') == 'data-toggle' assert option2.attrib.get('data-value').split(' ') == [ # That version is pending. + 'review_with_policy_approve', 'review_with_policy', 'approve_multiple_versions', 'reject_multiple_versions', @@ -1521,6 +1525,7 @@ def test_cinder_jobs_to_resolve_choices(self): assert label_1.attr['class'] == 'data-toggle-hide' assert label_1.attr['data-value'] == ' '.join( ( + 'review_with_policy_approve', 'review_with_policy', 'reject', 'reject_multiple_versions', diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index f875a9d119b3..d24be20610e5 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -1109,7 +1109,12 @@ def test_actions_enforcement_actions(self): addon_status=amo.STATUS_NOMINATED, file_status=amo.STATUS_AWAITING_REVIEW, ) + assert actions['review_with_policy_approve']['enforcement_actions'] == ( + DECISION_ACTIONS.AMO_APPROVE, + DECISION_ACTIONS.AMO_APPROVE_VERSION, + ) assert actions['review_with_policy']['enforcement_actions'] == ( + DECISION_ACTIONS.AMO_REJECT_LISTING_CONTENT, DECISION_ACTIONS.AMO_DISABLE_ADDON, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, @@ -1217,8 +1222,8 @@ def test_actions_with_enable_policy_review_selection(self): self.grant_permission(self.user, 'Addons:Review') self.grant_permission(self.user, 'Reviews:Admin') expected = [ + 'review_with_policy_approve', 'review_with_policy', - 'public', 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', @@ -4136,14 +4141,63 @@ def test_review_with_policy_with_disable_addon(self): self.check_subject(message) assert 'disabled' in message.body + @override_switch('enable-policy-review-selection', active=True) + def test_review_with_policy_with_version_approval(self): + self.grant_permission(self.user, 'Addons:Review') + self.setup_data(amo.STATUS_NOMINATED, file_status=amo.STATUS_AWAITING_REVIEW) + assert not ContentDecision.objects.exists() + data = { + 'action': 'review_with_policy', + 'cinder_policies': [ + policy := CinderPolicy.objects.create( + uuid='z', + enforcement_actions=[ + DECISION_ACTIONS.AMO_APPROVE_VERSION.api_value + ], + ), + ], + 'versions': [self.review_version], + 'most_important_policy_actions': filter_enforcement_actions( + policy.split_enforcement_actions, Addon + ), + } + self.helper.set_data(data) + self.helper.handler.review_action = self.helper.actions['review_with_policy'] + with patch('olympia.abuse.actions.sign_file') as sign_file_mock: + self.helper.handler.review_with_policy() + sign_file_mock.assert_called_once() + + self.addon.reload() + assert self.addon.status == amo.STATUS_APPROVED + assert self.review_version.file.reload().status == amo.STATUS_APPROVED + assert ActivityLog.objects.count() == 2 + activity_log = ActivityLog.objects.first() + assert activity_log.action == amo.LOG.APPROVE_VERSION.id + assert activity_log.arguments == [ + self.addon, + ContentDecision.objects.get(), + policy, + self.review_version, + ] + activity_log = ActivityLog.objects.last() + assert activity_log.action == amo.LOG.CHANGE_STATUS.id + assert activity_log.arguments[0] == self.addon + + assert len(mail.outbox) == 1 + message = mail.outbox[0] + self.check_subject(message) + assert 'approved' in message.body + def test_enable_addon(self): self.grant_permission(self.user, 'Reviews:Admin') - self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_APPROVED) + self.setup_data(amo.STATUS_NULL, file_status=amo.STATUS_APPROVED) other_version = version_factory(addon=self.addon) version_factory(addon=self.addon, file_kw={'status': amo.STATUS_DISABLED}) Addon.disable_all_files( [self.addon], File.STATUS_DISABLED_REASONS.ADDON_DISABLE ) + self.addon.update(status=amo.STATUS_NULL) + ActivityLog.objects.all().delete() self.helper.handler.enable_addon() diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 4d908d4f9523..2e8f8879002a 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -6930,6 +6930,95 @@ def test_review_with_review_with_policy_action_multiple_reject(self): assert self.version.file.reload().status == amo.STATUS_DISABLED assert old_version.file.reload().status == amo.STATUS_DISABLED + @override_switch('enable-policy-review-selection', active=True) + def test_review_with_review_with_policy_action_approve(self): + responses.add( + responses.POST, + f'{settings.CINDER_SERVER_URL}v1/create_decision', + json={'uuid': uuid.uuid4().hex}, + status=201, + ) + self.grant_permission(self.reviewer, 'Addons:Review') + policy = CinderPolicy.objects.create( + uuid='1', + name='policy 1', + expose_in_reviewer_tools=True, + enforcement_actions=[DECISION_ACTIONS.AMO_APPROVE.api_value], + ) + self.addon.update(status=amo.STATUS_REJECTED) + response = self.client.post( + self.url, + self.get_dict( + action='review_with_policy_approve', + cinder_policies=[policy.id], + # no versions + ), + ) + assert response.status_code == 302, response.context['form'].errors + assert self.get_addon().status == amo.STATUS_APPROVED + log_entry = ActivityLog.objects.get( + action=amo.LOG.APPROVE_REJECTED_LISTING_CONTENT.id + ) + assert ( + log_entry.contentdecisionlog_set.get().decision.action + == DECISION_ACTIONS.AMO_APPROVE + ) + + @override_switch('enable-policy-review-selection', active=True) + @mock.patch('olympia.abuse.actions.sign_file') + def test_review_with_review_with_policy_action_approve_versions(self, sign_mock): + responses.add( + responses.POST, + f'{settings.CINDER_SERVER_URL}v1/create_decision', + json={'uuid': uuid.uuid4().hex}, + status=201, + ) + self.grant_permission(self.reviewer, 'Addons:Review') + policy = CinderPolicy.objects.create( + uuid='1', + name='policy 1', + expose_in_reviewer_tools=True, + enforcement_actions=[DECISION_ACTIONS.AMO_APPROVE_VERSION.api_value], + ) + old_version = self.version + old_version.file.update(status=amo.STATUS_DISABLED) + NeedsHumanReview.objects.create(version=old_version) + self.version = version_factory( + addon=self.addon, + version='3.0', + file_kw={'status': amo.STATUS_AWAITING_REVIEW}, + ) + response = self.client.post( + self.url, + self.get_dict( + action='review_with_policy_approve', + cinder_policies=[policy.id], + versions=[self.version.pk], + ), + ) + assert response.status_code == 302, response.context['form'].errors + assert self.get_addon().status == amo.STATUS_APPROVED + log_entry = ActivityLog.objects.get(action=amo.LOG.APPROVE_VERSION.id) + assert ( + log_entry.contentdecisionlog_set.get().decision.action + == DECISION_ACTIONS.AMO_APPROVE_VERSION + ) + self.version.reload() + assert not self.version.needshumanreview_set.filter(is_active=True).exists() + file_ = self.version.file.reload() + assert file_.status == amo.STATUS_APPROVED + assert not self.version.pending_rejection + + # we clear all NHR for a listed channel approval + assert ( + not old_version.reload() + .needshumanreview_set.filter(is_active=True) + .exists() + ) + assert old_version.file.status == amo.STATUS_DISABLED + + sign_mock.assert_called_once_with(file_) + def test_enforcement_actions_rendered(self): response = self.client.get(self.url) doc = pq(response.content) diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index f61082bd303a..bf4a2936d157 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -533,9 +533,12 @@ def get_actions(self): use_content_rejection = self.content_review and waffle.switch_is_active( 'enable-content-rejection' ) + policy_selection_enabled = waffle.switch_is_active( + 'enable-policy-review-selection' + ) # Special logic for availability of reject/approve multiple action: - if version_is_unlisted: + if version_is_unlisted or policy_selection_enabled: can_reject_multiple = is_appropriate_reviewer can_approve_multiple = is_appropriate_reviewer elif use_content_rejection: @@ -562,12 +565,8 @@ def get_actions(self): ) can_approve_multiple = False - policy_selection_enabled = waffle.switch_is_active( - 'enable-policy-review-selection' - ) - # Definitions for all actions. - actions['review_with_policy'] = { + actions['review_with_policy_approve'] = { 'method': self.handler.review_with_policy, 'policy_enforcement': True, 'minimal': False, @@ -580,15 +579,30 @@ def get_actions(self): and is_appropriate_reviewer ), 'enforcement_actions': ( - # TODO: expand functionality to handle non-negative policies, - # and content review - # DECISION_ACTIONS.AMO_APPROVE, - # DECISION_ACTIONS.AMO_REJECT_LISTING_CONTENT, - # DECISION_ACTIONS.AMO_APPROVE_VERSION, + DECISION_ACTIONS.AMO_APPROVE, + DECISION_ACTIONS.AMO_APPROVE_VERSION, + ), + 'multiple_versions': can_approve_multiple, + 'resolves_cinder_jobs': True, + 'can_attach': True, + } + actions['review_with_policy'] = { + 'method': self.handler.review_with_policy, + 'policy_enforcement': True, + 'minimal': False, + 'details': ('Select a policy to perform on the version or add-on.'), + 'label': 'Review Negatively', + 'available': ( + policy_selection_enabled + and not is_static_theme + and not self.content_review + and is_appropriate_reviewer + ), + 'enforcement_actions': ( + DECISION_ACTIONS.AMO_REJECT_LISTING_CONTENT, DECISION_ACTIONS.AMO_DISABLE_ADDON, DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON, DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, - # DECISION_ACTIONS.AMO_IGNORE, DECISION_ACTIONS.AMO_BLOCK_ADDON, ), 'multiple_versions': can_reject_multiple, @@ -605,7 +619,8 @@ def get_actions(self): ), 'label': 'Approve', 'available': ( - not self.content_review + not policy_selection_enabled + and not self.content_review and addon_is_reviewable and version_is_unreviewed and (is_appropriate_reviewer or not self.human_review) @@ -721,7 +736,8 @@ def get_actions(self): 'minimal': True, 'comments': False, 'available': ( - not self.content_review + not policy_selection_enabled + and not self.content_review and current_or_latest_listed_version_was_auto_approved and is_appropriate_reviewer_post_review ), @@ -736,7 +752,7 @@ def get_actions(self): 'This will approve the selected versions. ' 'The comments will be sent to the developer.' ), - 'available': (can_approve_multiple), + 'available': (not policy_selection_enabled and can_approve_multiple), 'enforcement_actions': (DECISION_ACTIONS.AMO_APPROVE,), 'resolves_cinder_jobs': True, } @@ -820,7 +836,10 @@ def get_actions(self): ), 'comments': False, 'available': ( - not is_static_theme and version_is_unlisted and is_appropriate_reviewer + not policy_selection_enabled + and not is_static_theme + and version_is_unlisted + and is_appropriate_reviewer ), 'resolves_cinder_jobs': True, } From 1c2cd0b8143b18cd6ddcd3512ec41080ccfff7d3 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 22 Jun 2026 18:36:07 +0100 Subject: [PATCH 2/4] actually set AutoApprovalSummary to confirmed --- src/olympia/abuse/actions.py | 6 +++++- src/olympia/abuse/tests/test_actions.py | 8 +++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index 06b55623483f..99020530f462 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -1419,7 +1419,7 @@ def _set_promoted(self, versions): self.target.approve_for_version(version) def process_version(self, version): - from olympia.reviewers.models import NeedsHumanReview + from olympia.reviewers.models import AutoApprovalSummary, NeedsHumanReview # Sign addon. assert not version.is_blocked @@ -1463,6 +1463,10 @@ def process_version(self, version): pending_rejection_by=None, pending_content_rejection=None, ) + try: + version.autoapprovalsummary.update(confirmed=True) + except AutoApprovalSummary.DoesNotExist: + pass if version.channel == amo.CHANNEL_UNLISTED: self.clear_specific_needs_human_review_flags(version) elif ( diff --git a/src/olympia/abuse/tests/test_actions.py b/src/olympia/abuse/tests/test_actions.py index 51aa3c8856c7..bd92a4f61c36 100644 --- a/src/olympia/abuse/tests/test_actions.py +++ b/src/olympia/abuse/tests/test_actions.py @@ -44,7 +44,7 @@ from olympia.files.models import File from olympia.promoted.models import PromotedGroup from olympia.ratings.models import Rating -from olympia.reviewers.models import NeedsHumanReview +from olympia.reviewers.models import AutoApprovalSummary, NeedsHumanReview from olympia.versions.models import VersionReviewerFlags from ..actions import ( @@ -2876,6 +2876,10 @@ def test_execute_action_no_files_awaiting_review(self): reviewer_user=self.reviewer, ) assert self.decision.target_versions.exists() + AutoApprovalSummary.objects.create( + version=self.version, verdict=amo.AUTO_APPROVED, weight=151 + ) + # no autoapproval summary for old_version action_helper = self.ActionClass(self.decision) activity = action_helper.process_action() @@ -2897,6 +2901,8 @@ def test_execute_action_no_files_awaiting_review(self): assert second_activity.arguments == [self.addon, self.decision] assert second_activity.user == self.reviewer assert second_activity.details == {'comments': self.decision.private_notes} + assert self.version.autoapprovalsummary.reload().confirmed is True + assert hasattr(self.old_version, 'autoapprovalsummary') is False # get this again, to replicate how send_notifications works action_helper = self.ActionClass(self.decision) From e3f62883ca8f69a4a8983c17314ca71164543eb4 Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 23 Jun 2026 12:16:33 +0100 Subject: [PATCH 3/4] minor pr fixes --- src/olympia/abuse/actions.py | 16 ++++++---------- src/olympia/reviewers/utils.py | 4 ++-- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index 99020530f462..426614e92892 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -1391,13 +1391,11 @@ class ContentActionApproveVersion(ContentActionAddon): def get_owners(self): if ( - isinstance(self.target, Addon) - and (self.is_human_reviewer or self.target.type != amo.ADDON_LPAPP) - and self.decision.activities.filter( - # FORCE_ENABLE is logged via the reviewer tools enable_addon action. - action__in=(amo.LOG.APPROVE_VERSION.id, amo.LOG.FORCE_ENABLE.id) - ).exists() - ): + self.is_human_reviewer or self.target.type != amo.ADDON_LPAPP + ) and self.decision.activities.filter( + # FORCE_ENABLE is logged via the reviewer tools enable_addon action. + action__in=(amo.LOG.APPROVE_VERSION.id, amo.LOG.FORCE_ENABLE.id) + ).exists(): # Don't notify decisions (to cinder or owners) for auto-approved langpacks # or if the decision wasn't (freshly) approving any versions. return self.target.authors.all() @@ -1488,9 +1486,7 @@ def process_action(self, release_hold=False): # This action should only be used by reviewer tools, not cinder webhook raise NotImplementedError - if not isinstance(self.target, Addon) or not ( - versions := list(self.target_versions) - ): + if not (versions := list(self.target_versions)): return None was_public = self.target.is_public() diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index bf4a2936d157..e13f8f862928 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -571,7 +571,7 @@ def get_actions(self): 'policy_enforcement': True, 'minimal': False, 'details': ('Select a policy to perform on the version or add-on.'), - 'label': 'Review', + 'label': 'Positive Review', 'available': ( policy_selection_enabled and not is_static_theme @@ -591,7 +591,7 @@ def get_actions(self): 'policy_enforcement': True, 'minimal': False, 'details': ('Select a policy to perform on the version or add-on.'), - 'label': 'Review Negatively', + 'label': 'Negative Review', 'available': ( policy_selection_enabled and not is_static_theme From 95595ffbe4016472e81eb5969324fccdd18faa2c Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Tue, 23 Jun 2026 18:34:44 +0100 Subject: [PATCH 4/4] change auto_approve command to create ContentDecision directly --- .../management/commands/auto_approve.py | 58 ++++++++++++++++--- src/olympia/reviewers/tests/test_commands.py | 34 ++++++++++- 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/src/olympia/reviewers/management/commands/auto_approve.py b/src/olympia/reviewers/management/commands/auto_approve.py index 3049f4f15bbc..94819dab2dfb 100644 --- a/src/olympia/reviewers/management/commands/auto_approve.py +++ b/src/olympia/reviewers/management/commands/auto_approve.py @@ -4,13 +4,17 @@ from django.conf import settings from django.core.management.base import BaseCommand from django.db import transaction +from django.utils.functional import cached_property import waffle from django_statsd.clients import statsd import olympia.core.logger from olympia import amo +from olympia.abuse.models import CinderPolicy, ContentDecision +from olympia.abuse.tasks import report_decision_to_cinder_and_notify from olympia.amo.decorators import use_primary_db +from olympia.constants.abuse import DECISION_ACTIONS from olympia.constants.reviewers import WAIT_ON_SCANNERS_TIMEOUT from olympia.files.utils import lock from olympia.lib.crypto.signing import SigningError @@ -39,6 +43,16 @@ class ApprovalNotAvailableError(Exception): class Command(BaseCommand): help = 'Auto-approve add-on versions based on predefined criteria' + # The comment is not translated on purpose, to behave like + # regular human approval does. + LISTED_COMMENT = ( + 'This version has been screened and approved for the ' + 'public. Keep in mind that other reviewers may look into ' + 'this version in the future and determine that it ' + 'requires changes or should be taken down.' + ) + UNLISTED_COMMENT = 'automatic validation' + def add_arguments(self, parser): """Handle command arguments.""" parser.add_argument( @@ -189,6 +203,14 @@ def process(self, version_id): @statsd.timer('reviewers.auto_approve.approve') def approve(self, version): + """Approve a version, either by calling ReviewHelper or by creating a + ContentDecision and calling its execute_action() method.""" + if waffle.switch_is_active('enable-policy-review-selection'): + self.approve_with_action_class(version) + else: + self.approve_with_reviewer_helper(version) + + def approve_with_reviewer_helper(self, version): """Do the approval itself, caling ReviewHelper to change the status, sign the files, send the e-mail, etc.""" helper = ReviewHelper( @@ -201,20 +223,38 @@ def approve(self, version): if not approve_action: raise ApprovalNotAvailableError if version.channel == amo.CHANNEL_LISTED: - helper.handler.data = { - # The comment is not translated on purpose, to behave like - # regular human approval does. - 'comments': 'This version has been screened and approved for the ' - 'public. Keep in mind that other reviewers may look into ' - 'this version in the future and determine that it ' - 'requires changes or should be taken down.' - '\r\n\r\nThank you!' - } + helper.handler.data = {'comments': self.LISTED_COMMENT} else: helper.handler.data = {'comments': 'automatic validation'} approve_action['method']() statsd.incr('reviewers.auto_approve.approve.success') + @cached_property + def approve_policy(self): + return CinderPolicy.objects.filter( + enforcement_actions=DECISION_ACTIONS.AMO_APPROVE_VERSION + ).first() + + def approve_with_action_class(self, version): + """Do the approval itself, caling the report_decision_to_cinder_and_notify, + which changes the status, signs the files, sends the e-mail, etc.""" + decision = ContentDecision.objects.create( + addon=version.addon, + action=DECISION_ACTIONS.AMO_APPROVE_VERSION, + reasoning=( + self.LISTED_COMMENT + if version.channel == amo.CHANNEL_LISTED + else self.UNLISTED_COMMENT + ), + reviewer_user_id=settings.TASK_USER_ID, + ) + if self.approve_policy: + decision.policies.set([self.approve_policy]) + decision.target_versions.set([version]) + decision.execute_action() + report_decision_to_cinder_and_notify.delay(decision_id=decision.id) + statsd.incr('reviewers.auto_approve.approve.success') + def disapprove(self, version): """Handle a version we are not auto-approving, setting NeedsHumanReview if necessary to "transfer" it to the reviewers queue. diff --git a/src/olympia/reviewers/tests/test_commands.py b/src/olympia/reviewers/tests/test_commands.py index e4f6403f246f..0238fffeafc7 100644 --- a/src/olympia/reviewers/tests/test_commands.py +++ b/src/olympia/reviewers/tests/test_commands.py @@ -9,6 +9,7 @@ from django.test.testcases import TransactionTestCase import responses +from waffle.testutils import override_switch from olympia import amo from olympia.abuse.models import AbuseReport, CinderJob, CinderPolicy, ContentDecision @@ -299,7 +300,7 @@ def test_fetch_candidates(self): @mock.patch('olympia.reviewers.management.commands.auto_approve.statsd.incr') @mock.patch('olympia.reviewers.management.commands.auto_approve.ReviewHelper') - def test_approve(self, review_helper_mock, statsd_incr_mock): + def test_approve_with_review_helper(self, review_helper_mock, statsd_incr_mock): review_helper_mock.return_value.actions = {'public': mock.MagicMock()} command = auto_approve.Command() command.approve(self.version) @@ -320,6 +321,37 @@ def test_approve(self, review_helper_mock, statsd_incr_mock): {}, ) + @override_switch('enable-policy-review-selection', active=True) + @mock.patch('olympia.reviewers.management.commands.auto_approve.statsd.incr') + @mock.patch( + 'olympia.reviewers.management.commands.auto_approve.report_decision_to_cinder_and_notify.delay' + ) + @mock.patch( + 'olympia.reviewers.management.commands.auto_approve.ContentDecision.execute_action' + ) + def test_approve_with_action_class( + self, execute_action_mock, report_decision_mock, statsd_incr_mock + ): + report_decision_mock.return_value = mock.MagicMock() + command = auto_approve.Command() + command.approve(self.version) + decision = ContentDecision.objects.get() + assert report_decision_mock.call_count == 1 + assert report_decision_mock.call_args == ( + (), + {'decision_id': decision.id}, + ) + assert decision.target == self.addon + assert decision.target_versions.get() == self.version + assert decision.action == DECISION_ACTIONS.AMO_APPROVE_VERSION + assert decision.reasoning == auto_approve.Command.LISTED_COMMENT + assert decision.reviewer_user_id == settings.TASK_USER_ID + assert statsd_incr_mock.call_count == 1 + assert statsd_incr_mock.call_args == ( + ('reviewers.auto_approve.approve.success',), + {}, + ) + @mock.patch('olympia.reviewers.utils.sign_file') def test_full_run(self, sign_file_mock): # Simple integration test with as few mocks as possible.