Skip to content

Allow appeals to be accepted with override #25004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
eviljeff merged 1 commit into from
Jun 11, 2026
Merged

Allow appeals to be accepted with override #25004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/olympia/constants/abuse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
94 changes: 59 additions & 35 deletions src/olympia/reviewers/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
*(
Expand All @@ -278,7 +278,7 @@ def create_option(
'<details><summary>Show detail on {} reports</summary><ul>{}</ul>'
'</details>',
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 '',
Expand All @@ -292,21 +292,25 @@ 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
# rejecting versions: that would cause the rejection to be no-op and
# 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
Expand Down Expand Up @@ -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:
Expand All @@ -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')
Expand Down Expand Up @@ -648,23 +668,33 @@ 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',
'Invalid policies selected with more than one primary enforcement '
'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
Expand All @@ -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'
Expand All @@ -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')
Expand Down
2 changes: 1 addition & 1 deletion src/olympia/reviewers/templates/reviewers/review.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ <h4>Enforcement actions:</h4>
</div>
</div>
<div class="data-toggle"
data-value="resolve_appeal_job">
data-value="appeal_deny">
<label for="id_appeal_action">{{ form.appeal_action.label }}</label>
<div class="review-actions-policies-select">
{{ form.appeal_action }}
Expand Down
Loading
Loading