diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py
index ceae663ba8eb..426614e92892 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 (
+ 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 AutoApprovalSummary, 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,
+ )
+ 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 (
+ 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 (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..bd92a4f61c36 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
@@ -42,13 +44,13 @@
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 (
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,333 @@ 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()
+ 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()
+ 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}
+ 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)
+ 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 +3044,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 +3086,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 +3219,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 +3247,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 +3392,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 +3417,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 +3605,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/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.
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..e13f8f862928 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,17 +565,33 @@ 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_approve'] = {
+ 'method': self.handler.review_with_policy,
+ 'policy_enforcement': True,
+ 'minimal': False,
+ 'details': ('Select a policy to perform on the version or add-on.'),
+ 'label': 'Positive Review',
+ 'available': (
+ policy_selection_enabled
+ and not is_static_theme
+ and not self.content_review
+ and is_appropriate_reviewer
+ ),
+ 'enforcement_actions': (
+ 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',
+ 'label': 'Negative Review',
'available': (
policy_selection_enabled
and not is_static_theme
@@ -580,15 +599,10 @@ 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_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,
}