diff --git a/src/olympia/constants/abuse.py b/src/olympia/constants/abuse.py
index 7b0ca92b0b23..ad0fbc7217c4 100644
--- a/src/olympia/constants/abuse.py
+++ b/src/olympia/constants/abuse.py
@@ -15,7 +15,7 @@ class DECISION_ACTIONS(EnumChoicesApiDash):
# 4 is unused
AMO_DELETE_RATING = 5, 'Rating delete'
AMO_DELETE_COLLECTION = 6, 'Collection delete'
- AMO_APPROVE = 7, 'Approved (no action)'
+ AMO_APPROVE = 7, 'Approved'
# Rejecting versions is not an available action for moderators in cinder
# - it is only handled by the reviewer tools by AMO Reviewers.
# It should not be sent by the cinder webhook, & does not have an action defined
diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py
index 583b9df7c137..c76f16fd11a5 100644
--- a/src/olympia/reviewers/forms.py
+++ b/src/olympia/reviewers/forms.py
@@ -224,7 +224,8 @@ def create_option(
# label_from_instance() on WidgetRenderedModelMultipleChoiceField returns the
# full object, not a label, this is what makes this work.
obj = label
- is_appeal = obj.is_appeal
+ is_developer_appeal = obj.is_developer_appeal
+ is_reporter_appeal = not is_developer_appeal and obj.is_appeal
queue_moves = list(obj.queue_moves.order_by('-created'))
requeued_decisions = list(
obj.decisions.filter(action=DECISION_ACTIONS.AMO_REQUEUE).order_by(
@@ -261,10 +262,9 @@ def create_option(
for appealed_decision in obj.appealed_decisions.all()
for appeal_obj in appealed_decision.appeals.all()
)
- if is_appeal
+ if is_reporter_appeal or is_developer_appeal
else ()
)
- is_developer_appeal = is_appeal and obj.is_developer_appeal
subtexts_gen = [
*internal_notes,
*(
@@ -278,7 +278,7 @@ def create_option(
'Show detail on {} reports
{}
'
'',
format_datetime(obj.created),
- '[Appeal] ' if is_appeal else '',
+ '[Appeal] ' if (is_reporter_appeal or is_developer_appeal) else '',
format_html('[Forwarded on {}] ', format_datetime(forwarded))
if forwarded
else '',
@@ -292,7 +292,7 @@ def create_option(
)
attrs = attrs or {}
- # Reviewers shouldn't use resolve_appeal_job to resolve "regular" jobs,
+ # Reviewers shouldn't use appeal_deny to resolve "regular" jobs,
# and conversely shouldn't use resolve_reports_job to resolve appeals,
# as resolving appeals is a bit more involved.
# On top of that, they shouldn't resolve *developer* appeals when
@@ -300,13 +300,17 @@ def create_option(
# that's not always what we want.
# The parent element will have `data-toggle-hide`, so data-value is
# used to hide actions that are not supposed to be used for this job.
- hide_for_these_actions = [
- 'resolve_appeal_job' if not is_appeal else 'resolve_reports_job'
- ]
- if is_developer_appeal:
- hide_for_these_actions.extend(
- ('review_with_policy', 'reject', 'reject_multiple_versions')
- )
+ if is_reporter_appeal:
+ hide_for_these_actions = ['appeal_override', 'resolve_reports_job']
+ elif is_developer_appeal:
+ hide_for_these_actions = [
+ 'review_with_policy',
+ 'reject',
+ 'reject_multiple_versions',
+ 'resolve_reports_job',
+ ]
+ else:
+ hide_for_these_actions = ['appeal_deny', 'appeal_override']
attrs['data-value'] = ' '.join(hide_for_these_actions)
return super().create_option(
name, value, label, selected, index, subindex, attrs
@@ -584,7 +588,7 @@ def is_valid(self):
else:
# we no longer strictly require comments with cinder policies
self.fields['comments'].required = False
- if selected_action == 'resolve_appeal_job':
+ if selected_action == 'appeal_deny':
self.fields['appeal_action'].required = True
result = super().is_valid()
if result:
@@ -600,27 +604,43 @@ def clean(self):
selected_action = self.cleaned_data.get('action')
selected_definition = self.helper.actions.get(selected_action, {})
# If the user select a different type of job before changing actions there could
- # be non-appeal jobs selected as cinder_jobs_to_resolve under resolve_appeal_job
+ # be non-appeal jobs selected as cinder_jobs_to_resolve under appeal_deny
# action, or appeal jobs under resolve_reports_job/a reject action. So filter
# them out.
- if selected_action == 'resolve_appeal_job':
+ require_jobs = False
+ if selected_action == 'appeal_deny':
self.cleaned_data['cinder_jobs_to_resolve'] = [
job
for job in self.cleaned_data.get('cinder_jobs_to_resolve', ())
if job.is_appeal
]
+ require_jobs = True
+ elif selected_action == 'appeal_override':
+ self.cleaned_data['cinder_jobs_to_resolve'] = [
+ job
+ for job in self.cleaned_data.get('cinder_jobs_to_resolve', ())
+ if job.is_developer_appeal
+ ]
+ require_jobs = True
elif selected_action == 'resolve_reports_job':
self.cleaned_data['cinder_jobs_to_resolve'] = [
job
for job in self.cleaned_data.get('cinder_jobs_to_resolve', ())
if not job.is_appeal
]
- elif selected_action in ('reject', 'reject_multiple_versions'):
+ require_jobs = True
+ elif selected_action in (
+ 'reject',
+ 'reject_multiple_versions',
+ 'review_with_policy',
+ ):
self.cleaned_data['cinder_jobs_to_resolve'] = [
job
for job in self.cleaned_data.get('cinder_jobs_to_resolve', ())
if not job.is_developer_appeal
]
+ if require_jobs and not self.cleaned_data.get('cinder_jobs_to_resolve'):
+ self.add_error('cinder_jobs_to_resolve', 'This field is required.')
is_policy_enforcement = selected_definition.get('policy_enforcement')
if (
is_policy_enforcement or self.cleaned_data.get('cinder_jobs_to_resolve')
@@ -648,11 +668,6 @@ def clean(self):
'cinder_policies',
'No policies selected with an associated cinder action.',
)
- elif not is_policy_enforcement and len(all_primary_actions) > 1:
- self.add_error(
- 'cinder_policies',
- 'Multiple policies selected with different cinder actions.',
- )
elif any(len(actions) != 1 for actions in all_primary_actions):
self.add_error(
'cinder_policies',
@@ -660,11 +675,26 @@ def clean(self):
'action.',
)
- if is_policy_enforcement:
- # get the most aggressive negative policy
- # TODO: if we expand to support non-negative policies, we'll need to
- # change this logic (e.g. most_postive_actions, or something).
- sorted_actions = sorted(
+ if not is_policy_enforcement:
+ if len(all_primary_actions) > 1:
+ self.add_error(
+ 'cinder_policies',
+ 'Multiple policies selected with different cinder actions.',
+ )
+ else:
+ negative_primary_actions = {
+ ea
+ for ea in all_primary_actions
+ if len(ea) and ea[0] in DECISION_ACTIONS.ADDON_NEGATIVE_SORTED
+ }
+ if not negative_primary_actions and len(all_primary_actions) > 1:
+ self.add_error(
+ 'cinder_policies',
+ 'Selecting multiple policies selected with different '
+ 'non-negative cinder actions is not supported.',
+ )
+
+ srtd_actions = sorted(
(
filter_enforcement_actions(
policy.split_enforcement_actions, Addon
@@ -675,11 +705,9 @@ def clean(self):
reverse=True,
)
- if sorted_actions:
- self.cleaned_data['most_aggressive_policy_actions'] = (
- sorted_actions[0]
- )
- if self.cleaned_data['most_aggressive_policy_actions'].primary[
+ if srtd_actions:
+ self.cleaned_data['most_important_policy_actions'] = srtd_actions[0]
+ if self.cleaned_data['most_important_policy_actions'].primary[
0
] in DECISION_ACTIONS.VERSION_SPECIFIC and selected_definition.get(
'multiple_versions'
@@ -691,10 +719,6 @@ def clean(self):
# otherwise, don't confuse logging with versions
self.cleaned_data['versions'] = []
- # TODO: once we support non-negative policies, we'll need to check that
- # the policy selection is consistent - e.g. you shouldn't be able to
- # select a positive and negative policy together
-
if selected_definition.get('delayable'):
delayed_rejection = self.cleaned_data.get('delayed_rejection')
delayed_rejection_date = self.cleaned_data.get('delayed_rejection_date')
diff --git a/src/olympia/reviewers/templates/reviewers/review.html b/src/olympia/reviewers/templates/reviewers/review.html
index e2d166fffc1c..fc82342097c1 100644
--- a/src/olympia/reviewers/templates/reviewers/review.html
+++ b/src/olympia/reviewers/templates/reviewers/review.html
@@ -226,7 +226,7 @@
Enforcement actions:
+ data-value="appeal_deny">
{{ form.appeal_action }}
diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py
index 7323d112a486..4fcd5dd201df 100644
--- a/src/olympia/reviewers/tests/test_forms.py
+++ b/src/olympia/reviewers/tests/test_forms.py
@@ -330,7 +330,7 @@ def test_policy_values_parsed(self):
'yyy': {'UNSELECTED': None},
}
- def test_appeal_action_require_with_resolve_appeal_job(self):
+ def test_appeal_action_require_with_appeal_deny(self):
self.grant_permission(self.request.user, 'Addons:Review')
self.addon.update(status=amo.STATUS_NOMINATED)
self.version.file.update(status=amo.STATUS_AWAITING_REVIEW)
@@ -343,7 +343,7 @@ def test_appeal_action_require_with_resolve_appeal_job(self):
form = self.get_form()
assert not form.is_bound
data = {
- 'action': 'resolve_appeal_job',
+ 'action': 'appeal_deny',
'comments': 'lol',
'cinder_jobs_to_resolve': [job.id],
}
@@ -456,20 +456,33 @@ def test_policy_actions_with_policy_enforcement(self):
no_action_policy = CinderPolicy.objects.create(
uuid='no', name='no action', expose_in_reviewer_tools=True
)
- action_policy_a = CinderPolicy.objects.create(
- uuid='a',
+ action_policy_disable = CinderPolicy.objects.create(
+ uuid='disable',
name='disable',
expose_in_reviewer_tools=True,
enforcement_actions=[DECISION_ACTIONS.AMO_DISABLE_ADDON.api_value],
)
- action_policy_c = CinderPolicy.objects.create(
- uuid='c',
+ action_policy_reject = CinderPolicy.objects.create(
+ uuid='reject',
name='reject',
expose_in_reviewer_tools=True,
enforcement_actions=[DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON.api_value],
)
+ action_policy_approve = CinderPolicy.objects.create(
+ uuid='approve',
+ name='approve',
+ expose_in_reviewer_tools=True,
+ enforcement_actions=[DECISION_ACTIONS.AMO_APPROVE.api_value],
+ )
+ action_policy_ignore = CinderPolicy.objects.create(
+ uuid='ignore',
+ name='ignore',
+ expose_in_reviewer_tools=True,
+ enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value],
+ )
form = self.get_form()
assert not form.is_bound
+
data = {
'action': 'review_with_policy',
'cinder_jobs_to_resolve': [job.id],
@@ -485,22 +498,26 @@ def test_policy_actions_with_policy_enforcement(self):
# multiple policies with different enforcement actions are fine when the action
# has policy_enforcement=True
- data['cinder_policies'] = [action_policy_a.id, action_policy_c.id]
+ data['cinder_policies'] = [action_policy_disable.id, action_policy_reject.id]
form = self.get_form(data=data)
assert form.is_valid(), form.errors
assert not form.errors
- assert form.cleaned_data['most_aggressive_policy_actions'] == (
+ assert form.cleaned_data['most_important_policy_actions'] == (
(DECISION_ACTIONS.AMO_DISABLE_ADDON,),
(),
)
# multiple primary enforcement action per policy are prevented if that happens
- action_policy_c.update(
+ action_policy_multiple_primary = CinderPolicy.objects.create(
+ uuid='multi',
+ name='multi',
+ expose_in_reviewer_tools=True,
enforcement_actions=[
DECISION_ACTIONS.AMO_DISABLE_ADDON.api_value,
DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON.api_value,
- ]
+ ],
)
+ data['cinder_policies'] = [action_policy_multiple_primary.id]
form = self.get_form(data=data)
assert not form.is_valid()
assert form.errors == {
@@ -510,6 +527,21 @@ def test_policy_actions_with_policy_enforcement(self):
]
}
+ ContentDecision.objects.create(
+ action=DECISION_ACTIONS.AMO_DISABLE_ADDON, addon=self.addon, appeal_job=job
+ )
+ data['action'] = 'appeal_override'
+ # and multiple enforcement actions only applies to negative policies
+ data['cinder_policies'] = [action_policy_ignore.id, action_policy_approve.id]
+ form = self.get_form(data=data)
+ assert not form.is_valid()
+ assert form.errors == {
+ 'cinder_policies': [
+ 'Selecting multiple policies selected with different '
+ 'non-negative cinder actions is not supported.'
+ ]
+ }
+
@override_switch('enable-policy-review-selection', active=True)
def test_versions_required_when_enforcement_is_on_versions(self):
self.grant_permission(self.request.user, 'Addons:Review')
@@ -559,7 +591,7 @@ def test_versions_required_when_enforcement_is_on_versions(self):
assert not form.errors
assert list(form.cleaned_data['versions']) == [self.version]
- def test_cinder_jobs_filtered_for_resolve_reports_job_and_resolve_appeal_job(self):
+ def test_cinder_jobs_filtered_for_resolve_reports_job_and_appeal_deny(self):
self.grant_permission(self.request.user, 'Addons:Review')
self.addon.update(status=amo.STATUS_NOMINATED)
self.version.file.update(status=amo.STATUS_AWAITING_REVIEW)
@@ -583,18 +615,18 @@ def test_cinder_jobs_filtered_for_resolve_reports_job_and_resolve_appeal_job(sel
)
data = {
- 'action': 'resolve_appeal_job',
+ 'action': 'appeal_deny',
'comments': 'lol',
'appeal_action': ['deny'],
'cinder_jobs_to_resolve': [report_job.id],
}
form = self.get_form(data=data)
- form.is_valid()
- assert form.cleaned_data['cinder_jobs_to_resolve'] == []
+ assert not form.is_valid()
+ assert form.errors == {'cinder_jobs_to_resolve': ['This field is required.']}
data['cinder_jobs_to_resolve'] = [report_job, appeal_job]
form = self.get_form(data=data)
- form.is_valid()
+ assert form.is_valid()
assert form.cleaned_data['cinder_jobs_to_resolve'] == [appeal_job]
data = {
@@ -603,12 +635,12 @@ def test_cinder_jobs_filtered_for_resolve_reports_job_and_resolve_appeal_job(sel
'cinder_jobs_to_resolve': [appeal_job.id],
}
form = self.get_form(data=data)
- form.is_valid()
- assert form.cleaned_data['cinder_jobs_to_resolve'] == []
+ assert not form.is_valid()
+ assert form.errors == {'cinder_jobs_to_resolve': ['This field is required.']}
data['cinder_jobs_to_resolve'] = [report_job.id, appeal_job.id]
form = self.get_form(data=data)
- form.is_valid()
+ assert form.is_valid()
assert form.cleaned_data['cinder_jobs_to_resolve'] == [report_job]
def test_cinder_jobs_filtered_for_reject_or_reject_multiple_versions(self):
@@ -651,7 +683,7 @@ def test_cinder_jobs_filtered_for_reject_or_reject_multiple_versions(self):
uuid='a',
name='ignore',
expose_in_reviewer_tools=True,
- enforcement_actions=[DECISION_ACTIONS.AMO_IGNORE.api_value],
+ enforcement_actions=[DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON.api_value],
)
data = {
@@ -662,9 +694,11 @@ def test_cinder_jobs_filtered_for_reject_or_reject_multiple_versions(self):
developer_appeal_job,
],
'versions': [self.version.pk],
+ 'cinder_policies': [policy.id],
+ 'delayed_rejection': False,
}
form = self.get_form(data=data)
- form.is_valid()
+ assert form.is_valid(), form.errors
assert form.cleaned_data['cinder_jobs_to_resolve'] == [
reporter_appeal_other_report_job
]
@@ -675,7 +709,7 @@ def test_cinder_jobs_filtered_for_reject_or_reject_multiple_versions(self):
developer_appeal_job,
]
form = self.get_form(data=data)
- form.is_valid()
+ assert form.is_valid()
assert form.cleaned_data['cinder_jobs_to_resolve'] == [
reporter_appeal_other_report_job,
report_job,
@@ -690,7 +724,7 @@ def test_cinder_jobs_filtered_for_reject_or_reject_multiple_versions(self):
],
}
form = self.get_form(data=data)
- form.is_valid()
+ assert form.is_valid()
assert form.cleaned_data['cinder_jobs_to_resolve'] == [
reporter_appeal_other_report_job
]
@@ -701,7 +735,7 @@ def test_cinder_jobs_filtered_for_reject_or_reject_multiple_versions(self):
developer_appeal_job,
]
form = self.get_form(data=data)
- form.is_valid()
+ assert form.is_valid()
assert form.cleaned_data['cinder_jobs_to_resolve'] == [
reporter_appeal_other_report_job,
report_job,
@@ -1467,18 +1501,18 @@ def test_cinder_jobs_to_resolve_choices(self):
)
assert label_0.attr['class'] == 'data-toggle-hide'
- assert label_0.attr['data-value'] == 'resolve_appeal_job'
+ assert label_0.attr['data-value'] == 'appeal_deny appeal_override'
assert label_1.attr['class'] == 'data-toggle-hide'
assert label_1.attr['data-value'] == ' '.join(
(
- 'resolve_reports_job',
'review_with_policy',
'reject',
'reject_multiple_versions',
+ 'resolve_reports_job',
)
)
assert label_2.attr['class'] == 'data-toggle-hide'
- assert label_2.attr['data-value'] == 'resolve_appeal_job'
+ assert label_2.attr['data-value'] == 'appeal_deny appeal_override'
# If we make the developer appeal a reporter appeal instead, suddenly
# the widget option is shown for reject/reject_multiple_versions.
@@ -1491,7 +1525,7 @@ def test_cinder_jobs_to_resolve_choices(self):
doc = pq(str(form['cinder_jobs_to_resolve']))
label_1 = doc('label[for="id_cinder_jobs_to_resolve_1"]')
assert label_1.attr['class'] == 'data-toggle-hide'
- assert label_1.attr['data-value'] == 'resolve_reports_job'
+ assert label_1.attr['data-value'] == 'appeal_override resolve_reports_job'
def test_cinder_policies_choices(self):
policy_exposed = CinderPolicy.objects.create(
diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py
index 1f6786d43cb9..f875a9d119b3 100644
--- a/src/olympia/reviewers/tests/test_utils.py
+++ b/src/olympia/reviewers/tests/test_utils.py
@@ -1056,7 +1056,7 @@ def test_actions_cinder_jobs_to_resolve(self):
'reject_multiple_versions',
'set_needs_human_review_multiple_versions',
'reply',
- 'resolve_appeal_job',
+ 'appeal_deny',
'request_legal_review',
'comment',
]
@@ -1395,7 +1395,7 @@ def test_record_decision_most_negative_enforcement_actions(self, report_mock):
),
],
'cinder_jobs_to_resolve': [cinder_job],
- 'most_aggressive_policy_actions': filter_enforcement_actions(
+ 'most_important_policy_actions': filter_enforcement_actions(
most.split_enforcement_actions, Addon
),
}
@@ -1403,7 +1403,7 @@ def test_record_decision_most_negative_enforcement_actions(self, report_mock):
self.helper.handler.review_action = self.helper.actions.get(
'review_with_policy'
)
- self.helper.handler.record_decision(None, action_completed=False)
+ self.helper.handler.record_policy_decision(versions=[])
assert CinderPolicyLog.objects.count() == 4
decision = (
ActivityLog.objects.get(action=amo.LOG.FORCE_DISABLE.id)
@@ -4115,7 +4115,7 @@ def test_review_with_policy_with_disable_addon(self):
enforcement_actions=[DECISION_ACTIONS.AMO_DISABLE_ADDON.api_value],
),
],
- 'most_aggressive_policy_actions': filter_enforcement_actions(
+ 'most_important_policy_actions': filter_enforcement_actions(
policy.split_enforcement_actions, Addon
),
}
@@ -4425,7 +4425,7 @@ def test_record_decision_called_everywhere_checkbox_shown_unlisted(self):
}
)
- def test_resolve_appeal_job_policies(self):
+ def test_appeal_deny_policies(self):
policy_a = CinderPolicy.objects.create(
uuid='a', text='The {THING} with the {OTHER} thing or {SOMETHING}'
)
@@ -4490,9 +4490,9 @@ def test_resolve_appeal_job_policies(self):
}
self.helper.set_data(data)
- self.helper.handler.resolve_appeal_job()
+ self.helper.handler.appeal_deny()
- assert CinderPolicyLog.objects.count() == 4
+ assert CinderPolicyLog.objects.count() == 5
activity_log_qs = ActivityLog.objects.filter(action=amo.LOG.DENY_APPEAL_JOB.id)
assert activity_log_qs.count() == 2
decision_qs = ContentDecision.objects.filter(action_date__isnull=False)
@@ -4502,7 +4502,12 @@ def test_resolve_appeal_job_policies(self):
assert decision1.action == DECISION_ACTIONS.AMO_DISABLE_ADDON
assert decision2.action == DECISION_ACTIONS.AMO_APPROVE
assert decision1.activities.get() == log1
- assert decision2.activities.get() == log2
+ assert (
+ decision2.activities.exclude(
+ action=amo.LOG.APPROVE_LISTING_CONTENT.id
+ ).get()
+ == log2
+ )
assert set(appeal_job1.reload().final_decision.policies.all()) == {
policy_a,
policy_b,
@@ -4520,7 +4525,7 @@ def test_resolve_appeal_job_policies(self):
policy_c.uuid: {'mmm': 'no!'},
}
- def test_resolve_appeal_job_versions(self):
+ def test_appeal_deny_versions(self):
old_version1 = version_factory(
addon=self.addon, file_kw={'status': amo.STATUS_DISABLED}
)
@@ -4557,7 +4562,7 @@ def test_resolve_appeal_job_versions(self):
}
self.helper.set_data(data)
- self.helper.handler.resolve_appeal_job()
+ self.helper.handler.appeal_deny()
activity_log_qs = ActivityLog.objects.filter(action=amo.LOG.DENY_APPEAL_JOB.id)
assert activity_log_qs.count() == 1
@@ -4572,6 +4577,70 @@ def test_resolve_appeal_job_versions(self):
vl1, vl2 = list(VersionLog.objects.filter(activity_log=log1))
assert [vl1.version, vl2.version] == [old_version2, old_version1]
+ def test_appeal_override_with_approve(self):
+ old_version1 = version_factory(
+ addon=self.addon,
+ file_kw={
+ 'status': amo.STATUS_DISABLED,
+ 'original_status': amo.STATUS_APPROVED,
+ },
+ )
+ old_version2 = version_factory(
+ addon=self.addon, file_kw={'status': amo.STATUS_DISABLED}
+ )
+
+ appeal_job1 = CinderJob.objects.create(
+ job_id='1', resolvable_in_reviewer_tools=True, target_addon=self.addon
+ )
+ ContentDecision.objects.create(
+ appeal_job=appeal_job1,
+ action=DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON,
+ addon=self.addon,
+ ).target_versions.add(old_version1)
+ ContentDecision.objects.create(
+ appeal_job=appeal_job1,
+ action=DECISION_ACTIONS.AMO_REJECT_VERSION_ADDON,
+ addon=self.addon,
+ ).target_versions.add(old_version2)
+ responses.add_callback(
+ responses.POST,
+ f'{settings.CINDER_SERVER_URL}v1/jobs/{appeal_job1.job_id}/decision',
+ callback=lambda r: (201, {}, json.dumps({'uuid': uuid.uuid4().hex})),
+ )
+ approve_policy = CinderPolicy.objects.create(
+ uuid='approve',
+ name='Approve',
+ enforcement_actions=[DECISION_ACTIONS.AMO_APPROVE.api_value],
+ )
+
+ self.grant_permission(self.user, 'Addons:Review')
+ self.helper = self.get_helper()
+ data = {
+ 'comments': 'Nope',
+ 'cinder_jobs_to_resolve': [appeal_job1],
+ 'cinder_policies': [approve_policy],
+ 'most_important_policy_actions': approve_policy.split_enforcement_actions,
+ }
+ self.helper.set_data(data)
+
+ self.helper.handler.appeal_override()
+
+ activity_log_qs = ActivityLog.objects.filter(action=amo.LOG.UNREJECT_VERSION.id)
+ assert activity_log_qs.count() == 1
+ decision_qs = ContentDecision.objects.filter(action_date__isnull=False)
+ assert decision_qs.count() == 1
+ log1 = activity_log_qs.first()
+ decision = decision_qs.first()
+ assert decision.action == DECISION_ACTIONS.AMO_APPROVE
+ assert decision.activities.get() == log1
+ assert list(decision.target_versions.all()) == [old_version2, old_version1]
+ assert VersionLog.objects.filter(activity_log=log1).count() == 2
+ vl1, vl2 = list(VersionLog.objects.filter(activity_log=log1))
+ assert [vl1.version, vl2.version] == [old_version1, old_version2]
+
+ assert old_version1.file.reload().status == amo.STATUS_APPROVED
+ assert old_version2.file.reload().status == amo.STATUS_AWAITING_REVIEW
+
def test_reject_multiple_versions_resets_original_status_too(self):
old_version = self.review_version
old_version.file.update(
diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py
index 8c85eea80c21..4d908d4f9523 100644
--- a/src/olympia/reviewers/tests/test_views.py
+++ b/src/olympia/reviewers/tests/test_views.py
@@ -2734,7 +2734,7 @@ def test_resolve_reports_job(self, resolve_mock):
self.assertCloseToNow(decision.action_date)
@mock.patch('olympia.reviewers.utils.report_decision_to_cinder_and_notify.delay')
- def test_resolve_appeal_job(self, resolve_mock):
+ def test_appeal_deny(self, resolve_mock):
appeal_job1 = CinderJob.objects.create(
job_id='1', resolvable_in_reviewer_tools=True, target_addon=self.addon
)
@@ -2761,7 +2761,7 @@ def test_resolve_appeal_job(self, resolve_mock):
response = self.client.post(
self.url,
{
- 'action': 'resolve_appeal_job',
+ 'action': 'appeal_deny',
'comments': 'Nope',
'cinder_jobs_to_resolve': [appeal_job1.id, appeal_job2.id],
'appeal_action': ['deny'],
@@ -2779,7 +2779,12 @@ def test_resolve_appeal_job(self, resolve_mock):
log1, log2 = list(activity_log_qs.all())
assert decision1.activities.get() == log1
assert decision1.action == DECISION_ACTIONS.AMO_DISABLE_ADDON
- assert decision2.activities.get() == log2
+ assert (
+ decision2.activities.exclude(
+ action=amo.LOG.APPROVE_LISTING_CONTENT.id
+ ).get()
+ == log2
+ )
assert decision2.action == DECISION_ACTIONS.AMO_APPROVE
assert resolve_mock.call_count == 2
diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py
index 9e2860c47af1..f61082bd303a 100644
--- a/src/olympia/reviewers/utils.py
+++ b/src/olympia/reviewers/utils.py
@@ -966,17 +966,35 @@ def get_actions(self):
DECISION_ACTIONS.AMO_CLOSED_NO_ACTION,
),
}
- actions['resolve_appeal_job'] = {
- 'method': self.handler.resolve_appeal_job,
- 'label': 'Resolve Appeals',
+ actions['appeal_deny'] = {
+ 'method': self.handler.appeal_deny,
+ 'label': 'Resolve Appeals with Denial',
'details': (
- 'Allows abuse report jobs to be resolved without an action on the '
+ 'Resolve appeal jobs by denying them, without taking an action on the '
'add-on or versions.'
),
'minimal': True,
'available': is_reviewer and has_unresolved_appeal_jobs,
'resolves_cinder_jobs': True,
}
+ actions['appeal_override'] = {
+ 'method': self.handler.appeal_override,
+ 'policy_enforcement': True,
+ 'minimal': True,
+ 'details': (
+ 'Resolve appeal jobs from developers by overriding with new decision.'
+ ),
+ 'label': 'Resolve Appeals with Override',
+ 'available': (
+ policy_selection_enabled and is_reviewer and has_unresolved_appeal_jobs
+ ),
+ 'enforcement_actions': (
+ DECISION_ACTIONS.AMO_APPROVE,
+ DECISION_ACTIONS.AMO_IGNORE,
+ ),
+ 'resolves_cinder_jobs': True,
+ 'can_attach': True,
+ }
actions['request_legal_review'] = {
'method': self.handler.request_legal_review,
'label': 'Request review from Mozilla Legal',
@@ -1123,11 +1141,10 @@ def record_decision(
call log_action; then trigger a task to notify Cinder and/or interested parties
and/or carry out the action.
- Not used by resolve_appeal_job."""
+ Not used by appeal_deny, or actions that are 'policy_enforcement': True.
+ """
# footgun: if action_completed=True then we don't call log_action here
assert not log_action_kw or action_completed
- # footgun: we need an activity_action if the action is already complete
- assert not (activity_action is None and action_completed)
decision_metadata = decision_metadata or {}
log_action_kw = log_action_kw or {}
if self.review_action and self.review_action.get('enforcement_actions'):
@@ -1143,22 +1160,7 @@ def record_decision(
else:
policies = []
- if (
- self.review_action
- and self.review_action.get('policy_enforcement')
- and 'most_aggressive_policy_actions' in self.data
- ):
- most_aggressive_policy_actions = self.data.get(
- 'most_aggressive_policy_actions'
- )
-
- # form validation/cleaning should ensure there is at least one policy
- # with a negative primary enforcement_action
- cinder_action = most_aggressive_policy_actions.primary[0]
- followup_actions = most_aggressive_policy_actions.followup
- else:
- cinder_action = getattr(activity_action, 'cinder_action', None)
- followup_actions = []
+ cinder_action = getattr(activity_action, 'cinder_action', None)
if not cinder_action and policies:
cinder_actions = [
@@ -1198,10 +1200,6 @@ def create_decision(job):
**decision_kw,
)
decision.policies.set(policies)
- ContentDecisionFollowupAction.objects.bulk_create(
- ContentDecisionFollowupAction(decision=decision, action=action)
- for action in followup_actions
- )
if versions:
decision.target_versions.set(versions)
return decision
@@ -1262,6 +1260,102 @@ def create_decision(job):
decision_id=decision.id, notify_owners=notify_owners
)
+ def record_policy_decision(self, *, versions, versions_per_job=None):
+ """Create the ContentDecision for the decision that's been made with a policy
+ selection.
+
+ Note: this is somewhat of a duplicate of record_decision, but specialized for
+ actions that use `policy_enforcement`, to reduce complexity.
+ Eventually record_decision will be replaced by this function.
+
+ versions_per_job is used for appeals: it is a mapping from CinderJob.pk to a
+ list of versions. It takes precedence over versions, when not None."""
+
+ decision_metadata = {}
+ policies = self.data.get('cinder_policies', [])
+ if 'policy_values' in self.data:
+ # We want to strip out empty values -
+ # both where the reviewer has provided no value, and unselected policies
+ decision_metadata[ContentDecision.POLICY_DYNAMIC_VALUES] = {
+ uuid_: trimmed
+ for uuid_, vals in self.data['policy_values'].items()
+ if (trimmed := {k: v for k, v in vals.items() if v})
+ }
+
+ most_important_policy_actions = self.data.get('most_important_policy_actions')
+ # form validation/cleaning should ensure there is at least one policy
+ # with a negative primary enforcement_action
+ cinder_action = most_important_policy_actions.primary[0]
+ followup_actions = most_important_policy_actions.followup
+
+ assert cinder_action
+
+ decision_kw = {
+ 'addon': self.addon,
+ 'action': cinder_action,
+ 'action_date': None,
+ # Note: there is only a single field for comments in reviewer tools
+ # regardless of whether the comments are intended to be shared with
+ # the developer or kept private. We use `reasoning` field on the
+ # decision to store that comment in both cases, the decision action
+ # will then determine whether that `reasoning` is exposed or not.
+ 'reasoning': self.data.get('comments', ''),
+ 'reviewer_user': self.user,
+ 'metadata': decision_metadata,
+ }
+
+ def create_decision(job):
+ decision = ContentDecision.objects.create(
+ cinder_job=job,
+ override_of=job.final_decision if job else None,
+ **decision_kw,
+ )
+ decision.policies.set(policies)
+ ContentDecisionFollowupAction.objects.bulk_create(
+ ContentDecisionFollowupAction(decision=decision, action=action)
+ for action in followup_actions
+ )
+ if versions_per_job is not None:
+ decision.target_versions.set(versions_per_job.get(job.id, []))
+ elif versions:
+ decision.target_versions.set(versions)
+ return decision
+
+ if cinder_jobs := self.data.get('cinder_jobs_to_resolve', ()):
+ # with appeals and escalations there could be multiple jobs
+ decisions = [create_decision(job) for job in cinder_jobs]
+ else:
+ decisions = [create_decision(None)]
+
+ log_entry = None
+ for decision in decisions:
+ # If there are multiple decisions, in theory only one should really
+ # be executed completely and log an activity, the rest should be
+ # no-op that we just need for record-keeping purposes. We will only
+ # notify the owners for that "complete" one.
+ notify_owners = False
+ log_entry_for_decision = decision.execute_action()
+ if log_entry_for_decision:
+ notify_owners = True
+ log_entry = log_entry_for_decision
+ self.log_attachment(log_entry)
+ self.update_queue_history(log_entry)
+ elif log_entry:
+ # decision.execute_action() explicitly returned None but we
+ # already have a log_entry: that means this decision was
+ # already carried out, we are just resolving multiple jobs with
+ # the same action. We need to attach the extra "no-op"
+ # decisions to the log_entry to have proper records.
+ log_entry.set_arguments(
+ [log_entry.arguments[0], decision, *log_entry.arguments[1:]]
+ )
+ del log_entry.arguments
+ log_entry.contentdecision_set.add(decision)
+ log_entry.save()
+ report_decision_to_cinder_and_notify.delay(
+ decision_id=decision.id, notify_owners=notify_owners
+ )
+
def clear_all_needs_human_review_flags_in_channel(self):
"""Clear needs_human_review flags on all versions in the same channel.
@@ -1384,7 +1478,7 @@ def resolve_reports_job(self):
if self.data.get('cinder_jobs_to_resolve', ()):
self.record_decision(amo.LOG.RESOLVE_CINDER_JOB_WITH_NO_ACTION)
- def resolve_appeal_job(self):
+ def appeal_deny(self):
# It's possible to have multiple appeal jobs, so handle them seperately.
version = self.version
self.version = None
@@ -1395,13 +1489,13 @@ def resolve_appeal_job(self):
)
# we just need a single action for this appeal
# - use min() to favor AMO_DISABLE_ADDON over AMO_REJECT_VERSION_ADDON
- previous_action_id = min(
+ cinder_action = min(
decision.action for decision in job.appealed_decisions.all()
)
previous_versions = list(
Version.unfiltered.filter(contentdecision__appeal_job=job).distinct()
)
-
+ followup_actions = []
metadata = {}
for mtda in job.appealed_decisions.all().values_list('metadata', flat=True):
if ContentDecision.POLICY_DYNAMIC_VALUES not in mtda:
@@ -1422,14 +1516,17 @@ def resolve_appeal_job(self):
# notify cinder
decision = ContentDecision.objects.create(
addon=self.addon,
- action=previous_action_id,
- action_date=datetime.now(),
+ action=cinder_action,
reasoning=self.data.get('comments', ''),
reviewer_user=self.user,
cinder_job=job,
override_of=job.final_decision,
metadata=metadata,
)
+ ContentDecisionFollowupAction.objects.bulk_create(
+ ContentDecisionFollowupAction(decision=decision, action=action)
+ for action in followup_actions
+ )
decision.policies.set(policies)
decision.target_versions.set(previous_versions)
self.log_action(
@@ -1443,6 +1540,15 @@ def resolve_appeal_job(self):
self.update_queue_history(log_entry)
report_decision_to_cinder_and_notify.delay(decision_id=decision.id)
+ def appeal_override(self):
+ versions_per_job = {
+ job.id: list(
+ Version.unfiltered.filter(contentdecision__appeal_job=job).distinct()
+ )
+ for job in self.data.get('cinder_jobs_to_resolve', ())
+ }
+ self.record_policy_decision(versions=None, versions_per_job=versions_per_job)
+
def approve_latest_version(self):
"""Approve the add-on latest version (potentially setting the add-on to
approved if it was awaiting its first review)."""
@@ -1949,9 +2055,7 @@ def review_with_policy(self):
'Policies selected for %s, [%s]'
% (self.addon, ', '.join(p.uuid for p in self.data['cinder_policies']))
)
- self.record_decision(
- None, action_completed=False, versions=self.data.get('versions')
- )
+ self.record_policy_decision(versions=self.data.get('versions'))
class ReviewAddon(ReviewBase):