From 383c870be4d77e1a9d761af5552fdca08c9b9e4b Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Mon, 8 Jun 2026 13:14:21 +0100 Subject: [PATCH] Allow appeals to be accepted with override --- src/olympia/constants/abuse.py | 2 +- src/olympia/reviewers/forms.py | 94 ++++++---- .../reviewers/templates/reviewers/review.html | 2 +- src/olympia/reviewers/tests/test_forms.py | 88 ++++++--- src/olympia/reviewers/tests/test_utils.py | 89 ++++++++- src/olympia/reviewers/tests/test_views.py | 11 +- src/olympia/reviewers/utils.py | 174 ++++++++++++++---- 7 files changed, 348 insertions(+), 112 deletions(-) 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):