From 936dcce2eff33683e0632ec7226b376eaec8e5cb Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Sat, 18 Jan 2025 17:22:03 +0100 Subject: [PATCH 01/25] Make reviewer pending rejection input a datetime widget and allow changing it through an action --- src/olympia/constants/activity.py | 10 ++ src/olympia/reviewers/forms.py | 95 +++++++++++++------ .../reviewers/templates/reviewers/review.html | 10 +- src/olympia/reviewers/utils.py | 59 ++++++++---- src/olympia/reviewers/views.py | 17 +--- src/olympia/versions/models.py | 2 +- static/css/zamboni/reviewers.less | 13 --- static/js/zamboni/reviewers.js | 12 ++- 8 files changed, 125 insertions(+), 93 deletions(-) diff --git a/src/olympia/constants/activity.py b/src/olympia/constants/activity.py index 30bc90bac9c2..c36efe24bce4 100644 --- a/src/olympia/constants/activity.py +++ b/src/olympia/constants/activity.py @@ -1163,6 +1163,16 @@ class HELD_ACTION_REJECT_CONTENT_DELAYED(_LOG): admin_event = True +class CHANGE_PENDING_REJECTION(_LOG): + id = 203 + format = _('{addon} {version} pending rejection changed.') + short = _('Pending rejection changed') + keep = True + review_queue = True + reviewer_review_action = True + # Not hidden to developers. + + LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG] # Make sure there's no duplicate IDs. assert len(LOGS) == len({log.id for log in LOGS}) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index 872378fe8006..b383ffe65412 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -1,5 +1,5 @@ import zipfile -from datetime import timedelta +from datetime import datetime, timedelta from django import forms from django.conf import settings @@ -19,7 +19,7 @@ import olympia.core.logger from olympia import amo, ratings from olympia.abuse.models import CinderJob, CinderPolicy -from olympia.access import acl +from olympia.amo.admin import HTML5DateTimeInput from olympia.amo.forms import AMOModelForm from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT from olympia.files.utils import SafeZip @@ -175,7 +175,7 @@ def create_option(self, *args, **kwargs): # fine though. actions.remove('unreject_multiple_versions') if obj.pending_rejection: - actions.append('clear_pending_rejection_multiple_versions') + actions.append('change_pending_rejection_multiple_versions') if needs_human_review: actions.append('clear_needs_human_review_multiple_versions') # Setting needs human review is available if the version is not @@ -344,6 +344,20 @@ def validate_review_attachment(value): return value +class DelayedRejectionWidget(forms.RadioSelect): + def create_option(self, name, value, *args, **kwargs): + option = super().create_option(name, value, *args, **kwargs) + if not value: + option['attrs']['class'] = 'data-toggle' + if value is False: + option['attrs']['data-value'] = 'reject_multiple_versions' + else: # Empty value is reserved for clearing pending rejection. + option['attrs']['data-value'] = ( + 'change_pending_rejection_multiple_versions' + ) + return option + + class ReviewForm(forms.Form): # Hack to restore behavior from pre Django 1.10 times. # Django 1.10 enabled `required` rendering for required widgets. That @@ -370,35 +384,29 @@ class ReviewForm(forms.Form): operating_systems = forms.CharField(required=False, label='Operating systems:') applications = forms.CharField(required=False, label='Applications:') - delayed_rejection = forms.BooleanField( - # For the moment we default to immediate rejections, but in the future - # this will have to be dynamically set in __init__() to default to - # delayed for listed review, and immediate for unlisted (the default - # matters especially for unlisted where we don't intend to even show - # the inputs, so we'll always use the initial value). - # See https://github.com/mozilla/addons-server/pull/15025 + delayed_rejection = forms.NullBooleanField( initial=False, required=False, - widget=forms.RadioSelect( + widget=DelayedRejectionWidget( choices=( ( True, - 'Delay rejection, requiring developer to correct in ' 'less than…', + 'Delay rejection, requiring developer to correct before…', ), ( False, 'Reject immediately.', ), + ( + None, + 'Clear pending rejection.', + ), ) ), ) - delayed_rejection_days = forms.IntegerField( + delayed_rejection_date = forms.DateTimeField( + widget=HTML5DateTimeInput, required=False, - widget=NumberInput, - initial=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, - label='days', - min_value=1, - max_value=99, ) reasons = WidgetRenderedModelMultipleChoiceField( label='Choose one or more reasons:', @@ -471,13 +479,14 @@ def clean(self): 'attachment_file' ): raise ValidationError('Cannot upload both a file and input.') - if self.cleaned_data.get('action') == 'resolve_appeal_job': + selected_action = self.cleaned_data.get('action') + if selected_action == 'resolve_appeal_job': self.cleaned_data['cinder_jobs_to_resolve'] = [ job for job in self.cleaned_data.get('cinder_jobs_to_resolve', ()) if job.is_appeal ] - elif self.cleaned_data.get('action') == 'resolve_reports_job': + 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', ()) @@ -497,8 +506,34 @@ def clean(self): raise ValidationError( 'Multiple policies selected with different cinder actions.' ) + + if self.helper.actions.get(selected_action, {}).get('delayable'): + if 'delayed_rejection' not in self.data: + self.add_error( + 'delayed_rejection', + forms.ValidationError('This field is required.'), + ) + elif self.cleaned_data.get('delayed_rejection') and not self.data.get( + 'delayed_rejection_date' + ): + # In case reviewer selected delayed rejection option and + # somehow cleared the date widget, raise an error. + self.add_error( + 'delayed_rejection_date', + forms.ValidationError('This field is required.'), + ) + return self.cleaned_data + def clean_delayed_rejection_date(self): + if self.cleaned_data.get('delayed_rejection_date'): + now = datetime.now() + if self.cleaned_data['delayed_rejection_date'] <= now + timedelta(days=1): + raise ValidationError( + 'Delayed rejection date should be at least one day in the future' + ) + return self.cleaned_data.get('delayed_rejection_date') + def clean_version_pk(self): version_pk = self.cleaned_data.get('version_pk') if version_pk and version_pk != self.helper.version.pk: @@ -508,17 +543,15 @@ def __init__(self, *args, **kw): self.helper = kw.pop('helper') super().__init__(*args, **kw) - # Delayed rejection period needs to be readonly unless we're an admin. - user = self.helper.handler.user - rejection_period_widget_attributes = {} - rejection_period = self.fields['delayed_rejection_days'] - if not acl.action_allowed_for(user, amo.permissions.REVIEWS_ADMIN): - rejection_period.min_value = rejection_period.initial - rejection_period.max_value = rejection_period.initial - rejection_period_widget_attributes['readonly'] = 'readonly' - rejection_period_widget_attributes['min'] = rejection_period.min_value - rejection_period_widget_attributes['max'] = rejection_period.max_value - rejection_period.widget.attrs.update(rejection_period_widget_attributes) + # Set the initial rejection date from the default rejection period in + # days plus one hour to account for time it took the reviewer to + # actually do the review, otherwise if the date isn't modified it would + # be slightly less than the default period by the time the rejection is + # submitted. + # FIXME: still only allow admins ? + self.fields['delayed_rejection_date'].initial = datetime.now() + timedelta( + days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, hours=1 + ) # With the helper, we now have the add-on and can set queryset on the # versions field correctly. Small optimization: we only need to do this diff --git a/src/olympia/reviewers/templates/reviewers/review.html b/src/olympia/reviewers/templates/reviewers/review.html index 4db648aabf49..f59c37cae59b 100644 --- a/src/olympia/reviewers/templates/reviewers/review.html +++ b/src/olympia/reviewers/templates/reviewers/review.html @@ -250,15 +250,11 @@

data-value="{{ actions_delayable|join(' ') }}">
{{ form.delayed_rejection }} - {{ form.delayed_rejection.errors }} -
- {{ form.delayed_rejection_days }} - - {{ form.delayed_rejection_days.errors }} + {{ form.delayed_rejection_date }} + {{ form.delayed_rejection_date.errors }}
+ {{ form.delayed_rejection.errors }}
{% if switch_is_active('enable-activity-log-attachments') %} diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 5d211aa4a14a..43617a0f226d 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -1,5 +1,5 @@ from collections import OrderedDict, defaultdict -from datetime import datetime, timedelta +from datetime import datetime from django.conf import settings from django.contrib.humanize.templatetags.humanize import naturaltime @@ -694,13 +694,14 @@ def get_actions(self): ), 'resolves_cinder_jobs': True, } - actions['clear_pending_rejection_multiple_versions'] = { - 'method': self.handler.clear_pending_rejection_multiple_versions, - 'label': 'Clear pending rejection', + actions['change_pending_rejection_multiple_versions'] = { + 'method': self.handler.change_pending_rejection_multiple_versions, + 'label': 'Change pending rejection', 'details': ( - 'Clear pending rejection from selected versions, but ' + 'Change pending rejection from selected versions, but ' "otherwise don't change the version(s) or add-on statuses." ), + 'delayable': True, 'multiple_versions': True, 'minimal': True, 'comments': False, @@ -1450,13 +1451,13 @@ def auto_reject_multiple_versions(self): self.version = None self.file = None now = datetime.now() - if self.data.get('delayed_rejection'): - pending_rejection_deadline = now + timedelta( - days=int(self.data['delayed_rejection_days']) - ) - else: - pending_rejection_deadline = None - if pending_rejection_deadline: + if self.data.get('delayed_rejection') and self.data.get( + 'delayed_rejection_date' + ): + pending_rejection_deadline = self.data['delayed_rejection_date'] + self.data['delayed_rejection_days'] = ( + pending_rejection_deadline - now + ).days action_id = ( amo.LOG.REJECT_CONTENT_DELAYED if self.content_review @@ -1467,6 +1468,7 @@ def auto_reject_multiple_versions(self): % (self.addon, ', '.join(str(v.pk) for v in self.data['versions'])) ) else: + pending_rejection_deadline = None action_id = ( amo.LOG.REJECT_CONTENT if self.content_review @@ -1621,20 +1623,35 @@ def set_needs_human_review_multiple_versions(self): versions=self.data['versions'], ) - def clear_pending_rejection_multiple_versions(self): - """Clear pending rejection on selected versions.""" + def change_pending_rejection_multiple_versions(self): + """Change pending rejection on selected versions.""" self.file = None self.version = None + if self.data.get('delayed_rejection') and self.data.get( + 'delayed_rejection_date' + ): + pending_rejection_deadline = self.data['delayed_rejection_date'] + action = amo.LOG.CHANGE_PENDING_REJECTION + else: + pending_rejection_deadline = None + action = amo.LOG.CLEAR_PENDING_REJECTION + for version in self.data['versions']: - # Do it one by one to trigger the post_save(). + # Do it one by one to trigger the post_save() for each version. if version.pending_rejection: - version.reviewerflags.update( - pending_rejection=None, - pending_rejection_by=None, - pending_content_rejection=None, - ) + kwargs = { + 'pending_rejection': pending_rejection_deadline, + } + if not pending_rejection_deadline: + kwargs.update( + { + 'pending_rejection_by': None, + 'pending_content_rejection': None, + } + ) + version.reviewerflags.update(**kwargs) # Record a single activity log. - self.log_action(amo.LOG.CLEAR_PENDING_REJECTION, versions=self.data['versions']) + self.log_action(action, versions=self.data['versions']) def enable_addon(self): """Force enable the add-on.""" diff --git a/src/olympia/reviewers/views.py b/src/olympia/reviewers/views.py index f06850b45d79..e1a48e7d5153 100644 --- a/src/olympia/reviewers/views.py +++ b/src/olympia/reviewers/views.py @@ -68,7 +68,7 @@ get_average_daily_users_per_version_from_bigquery, ) from olympia.users.models import UserProfile -from olympia.versions.models import Version, VersionReviewerFlags +from olympia.versions.models import Version from olympia.zadmin.models import get_config, set_config from .decorators import ( @@ -1114,21 +1114,6 @@ def allow_resubmission(self, request, **kwargs): status_code = status.HTTP_409_CONFLICT return Response(status=status_code) - @drf_action( - detail=True, - methods=['post'], - permission_classes=[GroupPermission(amo.permissions.REVIEWS_ADMIN)], - ) - def clear_pending_rejections(self, request, **kwargs): - addon = get_object_or_404(Addon, pk=kwargs['pk']) - status_code = status.HTTP_202_ACCEPTED - VersionReviewerFlags.objects.filter(version__addon=addon).update( - pending_rejection=None, - pending_rejection_by=None, - pending_content_rejection=None, - ) - return Response(status=status_code) - @drf_action( detail=True, methods=['get'], diff --git a/src/olympia/versions/models.py b/src/olympia/versions/models.py index 45f6c488ba60..9ff0eaa877dc 100644 --- a/src/olympia/versions/models.py +++ b/src/olympia/versions/models.py @@ -1195,7 +1195,7 @@ def get_review_status_for_auto_approval_and_delay_reject(self): if (reviewer_flags := getattr(self, 'reviewerflags', None)) and ( rejection_date := reviewer_flags.pending_rejection ): - status = gettext('Delay-rejected, scheduled for %s') % rejection_date.date() + status = gettext('Delay-rejected, scheduled for %s') % rejection_date elif self.file.status == amo.STATUS_APPROVED: summary = getattr(self, 'autoapprovalsummary', None) if summary and summary.verdict == amo.AUTO_APPROVED: diff --git a/static/css/zamboni/reviewers.less b/static/css/zamboni/reviewers.less index cce20034c457..a68dc5f7520a 100644 --- a/static/css/zamboni/reviewers.less +++ b/static/css/zamboni/reviewers.less @@ -1088,10 +1088,6 @@ form.review-form .data-toggle { "c c"; grid-template-columns: max-content 1fr; - input[type="number"] { - width: 2.5em; - } - .review-delayed-rejection-deadline { grid-area: b; } @@ -1103,15 +1099,6 @@ form.review-form .data-toggle { li { list-style-type: none } - - li:nth-child(1) { - grid-area: a; - } - - li:nth-child(2) { - grid-column-start: 1; - grid-column-end: 3; - } } .history-comment { diff --git a/static/js/zamboni/reviewers.js b/static/js/zamboni/reviewers.js index 9ca182e05eb4..7d46a183b5e6 100644 --- a/static/js/zamboni/reviewers.js +++ b/static/js/zamboni/reviewers.js @@ -104,12 +104,16 @@ function initReviewActions() { } // Hide everything, then show the ones containing the value we're interested in. - $data_toggle.hide(); - $data_toggle.filter('[data-value~="' + value + '"]').show(); + $data_toggle.hide().prop('disabled', true) + $data_toggle.parent('label').hide() + $data_toggle.filter('[data-value~="' + value + '"]').show().prop('disabled', false); + $data_toggle.filter('[data-value~="' + value + '"]').parent('label').show(); // For data_toggle_hide, the opposite - show everything, then hide the ones containing // the value we're interested in. - $data_toggle_hide.show(); - $data_toggle_hide.filter('[data-value~="' + value + '"]').hide(); + $data_toggle_hide.show().prop('disabled', false); + $data_toggle_hide.parent('label').show(); + $data_toggle_hide.filter('[data-value~="' + value + '"]').hide().prop('disabled', true); + $data_toggle_hide.filter('[data-value~="' + value + '"]').parent('label').hide(); } $('#review-actions .action_nav #id_action > *:not(.disabled)').click( From 36a6c4bfbdc2e72adba67a274d871f5917f79686 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 20 Jan 2025 12:57:53 +0100 Subject: [PATCH 02/25] Notify developer --- src/olympia/abuse/actions.py | 8 ++++++++ src/olympia/abuse/models.py | 5 +++++ .../ContentActionChangePendingRejectionDate.txt | 6 ++++++ src/olympia/constants/abuse.py | 15 ++++++++++++--- src/olympia/constants/activity.py | 1 + src/olympia/reviewers/forms.py | 9 +++++++-- src/olympia/reviewers/utils.py | 15 ++++++++++++++- 7 files changed, 53 insertions(+), 6 deletions(-) create mode 100644 src/olympia/abuse/templates/abuse/emails/ContentActionChangePendingRejectionDate.txt diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index 8ac415442d89..b0f342eb9315 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -447,6 +447,14 @@ def process_action(self): return self.log_action(amo.LOG.REQUEST_LEGAL) +class ContentActionChangePendingRejectionDate(ContentAction): + description = 'Add-on pending rejection date has changed' + valid_targets = (Addon,) + + def get_owners(self): + return self.target.authors.all() + + class ContentActionDeleteCollection(ContentAction): valid_targets = (Collection,) description = 'Collection has been deleted' diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index a8828efb7fab..f25b8ef68ce3 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -31,6 +31,7 @@ ContentActionApproveInitialDecision, ContentActionApproveNoAction, ContentActionBanUser, + ContentActionChangePendingRejectionDate, ContentActionDeleteCollection, ContentActionDeleteRating, ContentActionDisableAddon, @@ -1070,6 +1071,9 @@ def get_action_helper_class(cls, decision_action): DECISION_ACTIONS.AMO_IGNORE: ContentActionIgnore, DECISION_ACTIONS.AMO_CLOSED_NO_ACTION: ContentActionAlreadyRemoved, DECISION_ACTIONS.AMO_LEGAL_FORWARD: ContentActionForwardToLegal, + DECISION_ACTIONS.AMO_CHANGE_PENDING_REJECTION_DATE: ( + ContentActionChangePendingRejectionDate + ), }.get(decision_action, ContentActionNotImplemented) def get_action_helper(self): @@ -1314,6 +1318,7 @@ def send_notifications(self): extra_context = { 'auto_approval': is_auto_approval, 'delayed_rejection_days': details.get('delayed_rejection_days'), + 'details': details, 'is_addon_being_blocked': details.get('is_addon_being_blocked'), 'is_addon_disabled': details.get('is_addon_being_disabled') or getattr(self.target, 'is_disabled', False), diff --git a/src/olympia/abuse/templates/abuse/emails/ContentActionChangePendingRejectionDate.txt b/src/olympia/abuse/templates/abuse/emails/ContentActionChangePendingRejectionDate.txt new file mode 100644 index 000000000000..faf6246de370 --- /dev/null +++ b/src/olympia/abuse/templates/abuse/emails/ContentActionChangePendingRejectionDate.txt @@ -0,0 +1,6 @@ +{% extends "abuse/emails/base.txt" %}{% block content %} + +As you are aware, your {{ type }} {{ name }} was manually reviewed by the Mozilla Add-ons team, at which point we found a violation of one or more Mozilla add-on policies. + +Our previous correspondence indicated that you would be required to correct the violation(s) by {{ details.old_deadline }}. However, after further assessing the circumstances - including the violation itself, the risks it presents, and the steps required to resolve it - we have determined that an alternative timeline is appropriate. Based on that determination, we have updated the deadline, and will now require you to correct your add-on violations no later than {{ details.new_deadline }}. +{% endblock %} diff --git a/src/olympia/constants/abuse.py b/src/olympia/constants/abuse.py index f2d260a31628..943658dcf74f 100644 --- a/src/olympia/constants/abuse.py +++ b/src/olympia/constants/abuse.py @@ -23,6 +23,9 @@ ('AMO_IGNORE', 11, 'Invalid report, so ignored'), ('AMO_CLOSED_NO_ACTION', 12, 'Content already removed (no action)'), ('AMO_LEGAL_FORWARD', 13, 'Forward add-on to legal'), + # Changing pending rejection date is not an available action for moderators + # in cinder - it is only performed by AMO Reviewers. + ('AMO_CHANGE_PENDING_REJECTION_DATE', 14, 'Pending rejection date changed'), ) DECISION_ACTIONS.add_subset( 'APPEALABLE_BY_AUTHOR', @@ -51,7 +54,13 @@ 'NON_OFFENDING', ('AMO_APPROVE', 'AMO_APPROVE_VERSION', 'AMO_IGNORE') ) DECISION_ACTIONS.add_subset( - 'SKIP_DECISION', ('AMO_APPROVE', 'AMO_APPROVE_VERSION', 'AMO_LEGAL_FORWARD') + 'SKIP_DECISION', + ( + 'AMO_APPROVE', + 'AMO_APPROVE_VERSION', + 'AMO_LEGAL_FORWARD', + 'AMO_CHANGE_PENDING_REJECTION_DATE', + ), ) # Illegal categories, only used when the reason is `illegal`. The constants @@ -115,12 +124,12 @@ ( 'HIDDEN_ADVERTISEMENT', 4, - 'Hidden advertisement or commercial communication, including by ' 'influencers', + 'Hidden advertisement or commercial communication, including by influencers', ), ( 'MISLEADING_INFO_GOODS_SERVICES', 5, - 'Misleading information about the characteristics of the goods and ' 'services', + 'Misleading information about the characteristics of the goods and services', ), ( 'MISLEADING_INFO_CONSUMER_RIGHTS', diff --git a/src/olympia/constants/activity.py b/src/olympia/constants/activity.py index c36efe24bce4..33d4ddbb99f3 100644 --- a/src/olympia/constants/activity.py +++ b/src/olympia/constants/activity.py @@ -1170,6 +1170,7 @@ class CHANGE_PENDING_REJECTION(_LOG): keep = True review_queue = True reviewer_review_action = True + cinder_action = DECISION_ACTIONS.AMO_CHANGE_PENDING_REJECTION_DATE # Not hidden to developers. diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index b383ffe65412..ddff74fe266d 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -327,8 +327,9 @@ def validate_review_attachment(value): valid_extensions_string = '(%s)' % ', '.join(VALID_ATTACHMENT_EXTENSIONS) raise forms.ValidationError( gettext( - 'Unsupported file type, please upload a ' - 'file {extensions}.'.format(extensions=valid_extensions_string) + 'Unsupported file type, please upload a file {extensions}.'.format( + extensions=valid_extensions_string + ) ) ) if value.size >= settings.MAX_UPLOAD_SIZE: @@ -523,6 +524,10 @@ def clean(self): forms.ValidationError('This field is required.'), ) + # FIXME: if they chose to change a pending rejection date we need + # to check all versions had the same one or raise an error. + # FIXME: and versions is required in that case + return self.cleaned_data def clean_delayed_rejection_date(self): diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 43617a0f226d..b02285ca719e 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -1627,11 +1627,13 @@ def change_pending_rejection_multiple_versions(self): """Change pending rejection on selected versions.""" self.file = None self.version = None + extra_details = {} if self.data.get('delayed_rejection') and self.data.get( 'delayed_rejection_date' ): pending_rejection_deadline = self.data['delayed_rejection_date'] action = amo.LOG.CHANGE_PENDING_REJECTION + extra_details['new_deadline'] = str(pending_rejection_deadline) else: pending_rejection_deadline = None action = amo.LOG.CLEAR_PENDING_REJECTION @@ -1639,6 +1641,7 @@ def change_pending_rejection_multiple_versions(self): for version in self.data['versions']: # Do it one by one to trigger the post_save() for each version. if version.pending_rejection: + extra_details['old_deadline'] = str(version.pending_rejection) kwargs = { 'pending_rejection': pending_rejection_deadline, } @@ -1651,7 +1654,17 @@ def change_pending_rejection_multiple_versions(self): ) version.reviewerflags.update(**kwargs) # Record a single activity log. - self.log_action(action, versions=self.data['versions']) + self.log_action( + action, versions=self.data['versions'], extra_details=extra_details + ) + if pending_rejection_deadline: + log.info('Sending email for %s' % (self.addon)) + # We avoid calling record_decision() because this action doesn't + # affect the queue (or resolve jobs), we only want to notify the + # developer(s). + notify_addon_decision_to_cinder.delay( + log_entry_id=self.log_entry.id, addon_id=self.addon.id + ) def enable_addon(self): """Force enable the add-on.""" From 816d012b2a0d4edc88a2e91736848f173fd0ced4 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 22 Jan 2025 13:00:55 +0100 Subject: [PATCH 03/25] Better validation --- src/olympia/reviewers/forms.py | 39 ++++++++++++------- .../reviewers/templates/reviewers/review.html | 20 +++++----- src/olympia/reviewers/utils.py | 2 +- 3 files changed, 38 insertions(+), 23 deletions(-) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index ddff74fe266d..f4baaad5682b 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -19,7 +19,6 @@ import olympia.core.logger from olympia import amo, ratings from olympia.abuse.models import CinderJob, CinderPolicy -from olympia.amo.admin import HTML5DateTimeInput from olympia.amo.forms import AMOModelForm from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT from olympia.files.utils import SafeZip @@ -359,6 +358,13 @@ def create_option(self, name, value, *args, **kwargs): return option +class DelayedRejectionDateWidget(forms.DateTimeInput): + input_type = 'datetime-local' + + def __init__(self, attrs=None, format='%Y-%m-%dT%H:%M'): + super().__init__(attrs, format) + + class ReviewForm(forms.Form): # Hack to restore behavior from pre Django 1.10 times. # Django 1.10 enabled `required` rendering for required widgets. That @@ -406,7 +412,7 @@ class ReviewForm(forms.Form): ), ) delayed_rejection_date = forms.DateTimeField( - widget=HTML5DateTimeInput, + widget=DelayedRejectionDateWidget, required=False, ) reasons = WidgetRenderedModelMultipleChoiceField( @@ -533,7 +539,7 @@ def clean(self): def clean_delayed_rejection_date(self): if self.cleaned_data.get('delayed_rejection_date'): now = datetime.now() - if self.cleaned_data['delayed_rejection_date'] <= now + timedelta(days=1): + if self.cleaned_data['delayed_rejection_date'] < self.min_rejection_date: raise ValidationError( 'Delayed rejection date should be at least one day in the future' ) @@ -547,16 +553,23 @@ def clean_version_pk(self): def __init__(self, *args, **kw): self.helper = kw.pop('helper') super().__init__(*args, **kw) - - # Set the initial rejection date from the default rejection period in - # days plus one hour to account for time it took the reviewer to - # actually do the review, otherwise if the date isn't modified it would - # be slightly less than the default period by the time the rejection is - # submitted. - # FIXME: still only allow admins ? - self.fields['delayed_rejection_date'].initial = datetime.now() + timedelta( - days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, hours=1 - ) + if any(action.get('delayable') for action in self.helper.actions.values()): + # Minimum delayed rejection date should be in the future. + self.min_rejection_date = datetime.now() + timedelta(days=1) + self.fields['delayed_rejection_date'].widget.attrs['min'] = ( + self.min_rejection_date.isoformat()[:16] + ) + # Default delayed rejection date should be + # REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT days in the + # future plus one hour to account for the time it's taking the + # reviewer to actually perform the review. + self.fields['delayed_rejection_date'].initial = datetime.now() + timedelta( + days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, hours=1 + ) + else: + # No delayable action available, remove the fields entirely. + del self.fields['delayed_rejection_date'] + del self.fields['delayed_rejection'] # With the helper, we now have the add-on and can set queryset on the # versions field correctly. Small optimization: we only need to do this diff --git a/src/olympia/reviewers/templates/reviewers/review.html b/src/olympia/reviewers/templates/reviewers/review.html index f59c37cae59b..92b57d88d1ba 100644 --- a/src/olympia/reviewers/templates/reviewers/review.html +++ b/src/olympia/reviewers/templates/reviewers/review.html @@ -246,17 +246,19 @@

{{ form.operating_systems.errors }} {{ form.applications.errors }} -
-
- {{ form.delayed_rejection }} -
- {{ form.delayed_rejection_date }} - {{ form.delayed_rejection_date.errors }} + {% if form.delayed_rejection and form.delayed_rejection_date %} +
+
+ {{ form.delayed_rejection }} +
+ {{ form.delayed_rejection_date }} UTC + {{ form.delayed_rejection_date.errors }} +
+ {{ form.delayed_rejection.errors }}
- {{ form.delayed_rejection.errors }}
-
+ {% endif %} {% if switch_is_active('enable-activity-log-attachments') %}
Date: Wed, 22 Jan 2025 14:54:38 +0100 Subject: [PATCH 04/25] Use new record_decision() --- src/olympia/reviewers/forms.py | 2 +- src/olympia/reviewers/utils.py | 32 +++++++++++++++++++------------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index f4baaad5682b..6842c1925bc1 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -361,6 +361,7 @@ def create_option(self, name, value, *args, **kwargs): class DelayedRejectionDateWidget(forms.DateTimeInput): input_type = 'datetime-local' + # Force the format to prevent seconds from showing up. def __init__(self, attrs=None, format='%Y-%m-%dT%H:%M'): super().__init__(attrs, format) @@ -538,7 +539,6 @@ def clean(self): def clean_delayed_rejection_date(self): if self.cleaned_data.get('delayed_rejection_date'): - now = datetime.now() if self.cleaned_data['delayed_rejection_date'] < self.min_rejection_date: raise ValidationError( 'Delayed rejection date should be at least one day in the future' diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 83b573ea36f7..250662912ffe 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -953,6 +953,7 @@ def record_decision( decision_metadata=None, action_completed=True, versions=None, + update_queue_history=True, ): """Create the ContentDecision for the decision that's been made; call log_action; then trigger a task to notify Cinder and/or interested parties @@ -1026,7 +1027,8 @@ def create_decision(): policies=policies, **(log_action_kw or {}), ) - self.update_queue_history(log_entry) + if update_queue_history: + self.update_queue_history(log_entry) for decision in decisions: log_entry = decision.execute_action() if not action_completed: @@ -1035,7 +1037,8 @@ def create_decision(): for reason in reasons ) self.log_attachment(log_entry) - self.update_queue_history(log_entry) + if update_queue_history: + self.update_queue_history(log_entry) report_decision_to_cinder_and_notify.delay(decision_id=decision.id) def clear_all_needs_human_review_flags_in_channel(self, mad_too=True): @@ -1632,11 +1635,9 @@ def change_pending_rejection_multiple_versions(self): 'delayed_rejection_date' ): pending_rejection_deadline = self.data['delayed_rejection_date'] - action = amo.LOG.CHANGE_PENDING_REJECTION extra_details['new_deadline'] = str(pending_rejection_deadline) else: pending_rejection_deadline = None - action = amo.LOG.CLEAR_PENDING_REJECTION for version in self.data['versions']: # Do it one by one to trigger the post_save() for each version. @@ -1653,17 +1654,22 @@ def change_pending_rejection_multiple_versions(self): } ) version.reviewerflags.update(**kwargs) - # Record a single activity log. - self.log_action( - action, versions=self.data['versions'], extra_details=extra_details - ) + if pending_rejection_deadline: log.info('Sending email for %s' % (self.addon)) - # We avoid calling record_decision() because this action doesn't - # affect the queue (or resolve jobs), we only want to notify the - # developer(s). - notify_addon_decision_to_cinder.delay( - log_entry_id=self.log_entry.id, addon_id=self.addon.id + self.record_decision( + amo.LOG.CHANGE_PENDING_REJECTION, + log_action_kw={ + 'versions': self.data['versions'], + 'extra_details': extra_details, + }, + update_queue_history=False, # This action doesn't affect the queue. + ) + else: + # When clearing, we don't notify the developer, so we only log the + # activity. + self.log_action( + amo.LOG.CLEAR_PENDING_REJECTION, versions=self.data['versions'] ) def enable_addon(self): From 91934937444e4c24c20413f0b73b3b077ecd3c4c Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 22 Jan 2025 15:45:17 +0100 Subject: [PATCH 05/25] Basic cleanups --- src/olympia/reviewers/tests/test_views.py | 28 ----------------------- static/js/zamboni/reviewers.js | 24 ++++++++++++++----- 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index c82c4c7b3bd5..bb5b5539688b 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -6444,9 +6444,6 @@ def setUp(self): self.allow_resubmission_url = reverse_ns( 'reviewers-addon-allow-resubmission', kwargs={'pk': self.addon.pk} ) - self.clear_pending_rejections_url = reverse_ns( - 'reviewers-addon-clear-pending-rejections', kwargs={'pk': self.addon.pk} - ) self.due_date_url = reverse_ns( 'reviewers-addon-due-date', kwargs={'pk': self.addon.pk} ) @@ -6776,31 +6773,6 @@ def test_allow_resubmission_with_non_denied_guid(self): assert response.status_code == 409 assert DeniedGuid.objects.count() == 0 - def test_clear_pending_rejections(self): - self.grant_permission(self.user, 'Reviews:Admin') - self.client.login_api(self.user) - version_factory( - addon=self.addon, file_kw={'status': amo.STATUS_AWAITING_REVIEW} - ) - for version in self.addon.versions.all(): - version_review_flags_factory( - version=version, - pending_rejection=datetime.now() + timedelta(days=7), - pending_rejection_by=user_factory(), - pending_content_rejection=False, - ) - response = self.client.post(self.clear_pending_rejections_url) - assert response.status_code == 202 - assert not VersionReviewerFlags.objects.filter( - version__addon=self.addon, pending_rejection__isnull=False - ).exists() - assert not VersionReviewerFlags.objects.filter( - version__addon=self.addon, pending_rejection_by__isnull=False - ).exists() - assert not VersionReviewerFlags.objects.filter( - version__addon=self.addon, pending_content_rejection__isnull=False - ).exists() - def test_due_date(self): user_factory(pk=settings.TASK_USER_ID) self.grant_permission(self.user, 'Reviews:Admin') diff --git a/static/js/zamboni/reviewers.js b/static/js/zamboni/reviewers.js index 7d46a183b5e6..a96151b8f793 100644 --- a/static/js/zamboni/reviewers.js +++ b/static/js/zamboni/reviewers.js @@ -104,16 +104,28 @@ function initReviewActions() { } // Hide everything, then show the ones containing the value we're interested in. - $data_toggle.hide().prop('disabled', true) - $data_toggle.parent('label').hide() - $data_toggle.filter('[data-value~="' + value + '"]').show().prop('disabled', false); - $data_toggle.filter('[data-value~="' + value + '"]').parent('label').show(); + $data_toggle.hide().prop('disabled', true); + $data_toggle.parent('label').hide(); + $data_toggle + .filter('[data-value~="' + value + '"]') + .show() + .prop('disabled', false); + $data_toggle + .filter('[data-value~="' + value + '"]') + .parent('label') + .show(); // For data_toggle_hide, the opposite - show everything, then hide the ones containing // the value we're interested in. $data_toggle_hide.show().prop('disabled', false); $data_toggle_hide.parent('label').show(); - $data_toggle_hide.filter('[data-value~="' + value + '"]').hide().prop('disabled', true); - $data_toggle_hide.filter('[data-value~="' + value + '"]').parent('label').hide(); + $data_toggle_hide + .filter('[data-value~="' + value + '"]') + .hide() + .prop('disabled', true); + $data_toggle_hide + .filter('[data-value~="' + value + '"]') + .parent('label') + .hide(); } $('#review-actions .action_nav #id_action > *:not(.disabled)').click( From 21d5ab1e4a189c566d7dcf98f9620d527a9c7fe9 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 22 Jan 2025 15:58:00 +0100 Subject: [PATCH 06/25] Simple test fix --- src/olympia/reviewers/tests/test_views.py | 1 - src/olympia/versions/tests/test_models.py | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index bb5b5539688b..877a1e6ae385 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -81,7 +81,6 @@ ApplicationsVersions, AppVersion, VersionManager, - VersionReviewerFlags, ) from olympia.versions.utils import get_review_due_date from olympia.zadmin.models import get_config diff --git a/src/olympia/versions/tests/test_models.py b/src/olympia/versions/tests/test_models.py index 8c1f96dd4106..4f30cd9df637 100644 --- a/src/olympia/versions/tests/test_models.py +++ b/src/olympia/versions/tests/test_models.py @@ -1659,19 +1659,19 @@ def test_unreviewed_files(db, addon_status, file_status, is_unreviewed): ( ( amo.STATUS_AWAITING_REVIEW, - datetime(2022, 7, 7, 7, 7, 7), + datetime(2025, 1, 22, 8, 9, 10), True, amo.AUTO_APPROVED, False, - 'Delay-rejected, scheduled for 2022-07-07', + 'Delay-rejected, scheduled for 2025-01-22 08:09:10', ), ( amo.STATUS_APPROVED, - datetime(2022, 8, 8, 8, 8, 8), + datetime(2025, 1, 23, 10, 11, 12), True, amo.AUTO_APPROVED, False, - 'Delay-rejected, scheduled for 2022-08-08', + 'Delay-rejected, scheduled for 2025-01-23 10:11:12', ), ( amo.STATUS_APPROVED, From a912f06528bb7f122cd1d00e08fda624b75df10d Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 22 Jan 2025 15:59:52 +0100 Subject: [PATCH 07/25] And update these --- src/olympia/reviewers/tests/test_forms.py | 14 +++++++------- src/olympia/reviewers/tests/test_utils.py | 18 +++++++++--------- src/olympia/reviewers/tests/test_views.py | 2 +- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index 912a72836c79..90e1cf4efadc 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -95,7 +95,7 @@ def test_actions_addon_status_null(self): actions = self.get_form().helper.get_actions() assert list(actions.keys()) == [ 'unreject_latest_version', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -134,7 +134,7 @@ def test_actions_addon_status_null_unlisted(self): 'unreject_multiple_versions', 'block_multiple_versions', 'confirm_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -163,7 +163,7 @@ def test_actions_addon_status_deleted(self): addon_status=amo.STATUS_DELETED, file_status=amo.STATUS_DISABLED ) assert list(actions.keys()) == [ - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -193,7 +193,7 @@ def test_actions_no_pending_files(self): ) assert list(actions.keys()) == [ 'reject_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -208,7 +208,7 @@ def test_actions_no_pending_files(self): addon_status=amo.STATUS_DISABLED, file_status=amo.STATUS_DISABLED ) assert list(actions.keys()) == [ - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'reply', 'enable_addon', @@ -754,7 +754,7 @@ def test_versions_queryset_contains_pending_files_for_listed_admin_reviewer(self self.test_versions_queryset_contains_pending_files_for_listed( expected_select_data_value=[ 'reject_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -977,7 +977,7 @@ def test_versions_queryset_contains_pending_files_for_unlisted_admin_reviewer(se 'unreject_multiple_versions', 'block_multiple_versions', 'confirm_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', ] diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index dd65d4212ae7..10706a595776 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -584,7 +584,7 @@ def test_actions_promoted_admin_review_needs_admin_permission(self): 'public', 'reject', 'reject_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -659,7 +659,7 @@ def test_actions_unlisted(self): 'unreject_multiple_versions', 'block_multiple_versions', 'confirm_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -800,7 +800,7 @@ def test_actions_pending_rejection_admin(self): expected = [ 'confirm_auto_approved', 'reject_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -827,7 +827,7 @@ def test_actions_pending_rejection_admin(self): 'reject', 'confirm_auto_approved', 'reject_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -864,7 +864,7 @@ def test_actions_disabled_addon(self): self.grant_permission(self.user, 'Reviews:Admin') expected = [ - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'reply', 'enable_addon', @@ -896,7 +896,7 @@ def test_actions_rejected_version(self): self.grant_permission(self.user, 'Reviews:Admin') expected = [ 'unreject_latest_version', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -958,7 +958,7 @@ def test_actions_versions_needing_human_review(self): self.grant_permission(self.user, 'Reviews:Admin') expected = [ - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -3364,7 +3364,7 @@ def test_set_needs_human_review_multiple_versions(self): assert not unselected.needshumanreview_set.filter(is_active=True).exists() assert not unselected.due_date - def test_clear_pending_rejection_multiple_versions(self): + def test_change_pending_rejection_multiple_versions(self): self.grant_permission(self.user, 'Addons:Review') self.grant_permission(self.user, 'Reviews:Admin') self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_APPROVED) @@ -3395,7 +3395,7 @@ def test_clear_pending_rejection_multiple_versions(self): .exclude(pk=unselected.pk) .order_by('pk') ) - data['action'] = 'clear_pending_rejection_multiple_versions' + data['action'] = 'change_pending_rejection_multiple_versions' self.helper.set_data(data) self.helper.process() diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 877a1e6ae385..20f433e6d6b8 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -2449,7 +2449,7 @@ def test_need_correct_reviewer_for_promoted_addon(self): 'public', 'reject', 'reject_multiple_versions', - 'clear_pending_rejection_multiple_versions', + 'change_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', From 3855f88a261074ff0a72ddecf1950344fe34bdb4 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 23 Jan 2025 13:29:49 +0100 Subject: [PATCH 08/25] form tests --- src/olympia/reviewers/tests/test_forms.py | 50 +++++++++++++++-------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index 90e1cf4efadc..47bd89d1e68e 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -4,6 +4,7 @@ from django.core.files.base import ContentFile from django.utils.encoding import force_str +from freezegun import freeze_time from pyquery import PyQuery as pq from olympia import amo @@ -24,7 +25,6 @@ version_factory, ) from olympia.constants.abuse import DECISION_ACTIONS -from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT from olympia.files.models import File from olympia.reviewers.forms import ReviewForm from olympia.reviewers.models import ( @@ -1008,28 +1008,42 @@ def test_versions_required(self): assert not form.is_valid() assert form.errors == {'versions': ['This field is required.']} - def test_delayed_rejection_days_widget_attributes(self): - # Regular reviewers can't customize the delayed rejection period. + def test_delayed_rejection_days_doesnt_show_up_for_regular_reviewers(self): + # Regular reviewers can't customize the delayed rejection period so + # the field is removed at init for them. + self.grant_permission(self.request.user, 'Addons:Review') form = self.get_form() - widget = form.fields['delayed_rejection_days'].widget - assert widget.attrs == { - 'min': REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, - 'max': REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, - 'readonly': 'readonly', - } + assert 'delayed_rejection_date' not in form.fields + assert 'delayed_rejection' not in form.fields + + @freeze_time('2025-01-23 12:52') + def test_delayed_rejection_days_shows_up_for_admin_reviewers(self): # Admin reviewers can customize the delayed rejection period. + self.grant_permission(self.request.user, 'Addons:Review') self.grant_permission(self.request.user, 'Reviews:Admin') form = self.get_form() - widget = form.fields['delayed_rejection_days'].widget - assert widget.attrs == { - 'min': 1, - 'max': 99, + assert 'delayed_rejection_date' in form.fields + assert 'delayed_rejection' in form.fields + assert form.fields['delayed_rejection_date'].widget.attrs == { + 'min': '2025-01-24T12:52' } - - def test_delayed_rejection_showing_for_unlisted_awaiting(self): - self.addon.update(status=amo.STATUS_NULL) - self.version.update(channel=amo.CHANNEL_UNLISTED) - self.test_delayed_rejection_days_widget_attributes() + assert form.fields['delayed_rejection_date'].initial == datetime( + 2025, 2, 6, 13, 52 + ) + content = str(form['delayed_rejection']) + doc = pq(content) + inputs = doc('input[type=radio]') + assert inputs[0].label.text_content().strip() == 'Delay rejection, requiring developer to correct before…' + assert inputs[0].attrib['value'] == 'True' + assert inputs[1].label.text_content().strip() == 'Reject immediately.' + assert inputs[1].attrib['value'] == 'False' + assert inputs[1].attrib['checked'] == 'checked' + assert inputs[1].attrib['class'] == 'data-toggle' + assert inputs[1].attrib['data-value'] == 'reject_multiple_versions' + assert inputs[2].label.text_content().strip() == 'Clear pending rejection.' + assert inputs[2].attrib['value'] == '' + assert inputs[2].attrib['class'] == 'data-toggle' + assert inputs[2].attrib['data-value'] == 'change_pending_rejection_multiple_versions' def test_version_pk(self): self.grant_permission(self.request.user, 'Addons:Review') From 058483d0485c461a40886530390490336915cd95 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 23 Jan 2025 14:12:13 +0100 Subject: [PATCH 09/25] Last FIXME --- src/olympia/reviewers/forms.py | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index 6842c1925bc1..837cd6e03df6 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -530,10 +530,27 @@ def clean(self): 'delayed_rejection_date', forms.ValidationError('This field is required.'), ) - - # FIXME: if they chose to change a pending rejection date we need - # to check all versions had the same one or raise an error. - # FIXME: and versions is required in that case + elif ( + selected_action == 'change_pending_rejection_multiple_versions' + and self.cleaned_data.get('delayed_rejection') + and self.cleaned_data.get('delayed_rejection_date') + and self.cleaned_data.get('versions') + ): + distinct_pending_rejection_dates = ( + self.cleaned_data['versions'] + .values_list('reviewerflags__pending_rejection') + .distinct() + .count() + ) + if distinct_pending_rejection_dates > 1: + self.add_error( + 'versions', + forms.ValidationError( + 'Can only change the delayed rejection date of multiple ' + 'versions at once if their pending rejection dates are all ' + 'the same.' + ), + ) return self.cleaned_data From 996fce6a1ee0f6b3a935eda8245e84549748fb3d Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 23 Jan 2025 14:16:44 +0100 Subject: [PATCH 10/25] More forms tests --- src/olympia/reviewers/tests/test_forms.py | 72 ++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index 47bd89d1e68e..37c34781ac1e 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -1033,7 +1033,10 @@ def test_delayed_rejection_days_shows_up_for_admin_reviewers(self): content = str(form['delayed_rejection']) doc = pq(content) inputs = doc('input[type=radio]') - assert inputs[0].label.text_content().strip() == 'Delay rejection, requiring developer to correct before…' + assert ( + inputs[0].label.text_content().strip() + == 'Delay rejection, requiring developer to correct before…' + ) assert inputs[0].attrib['value'] == 'True' assert inputs[1].label.text_content().strip() == 'Reject immediately.' assert inputs[1].attrib['value'] == 'False' @@ -1043,7 +1046,72 @@ def test_delayed_rejection_days_shows_up_for_admin_reviewers(self): assert inputs[2].label.text_content().strip() == 'Clear pending rejection.' assert inputs[2].attrib['value'] == '' assert inputs[2].attrib['class'] == 'data-toggle' - assert inputs[2].attrib['data-value'] == 'change_pending_rejection_multiple_versions' + assert ( + inputs[2].attrib['data-value'] + == 'change_pending_rejection_multiple_versions' + ) + + @freeze_time('2025-01-23 12:52') + def test_delayed_rejection_days_value_not_in_the_future(self): + self.grant_permission(self.request.user, 'Addons:Review,Reviews:Admin') + self.reason_a = ReviewActionReason.objects.create( + name='a reason', + is_active=True, + canned_response='Canned response for A', + ) + data = { + 'action': 'reject_multiple_versions', + 'comments': 'foo!', + 'delayed_rejection': 'True', + 'delayed_rejection_date': '2025-01-23T12:52', + 'reasons': [self.reason_a.pk], + 'versions': [self.version.pk], + } + form = self.get_form(data=data) + assert not form.is_valid() + assert form.errors['delayed_rejection_date'] == [ + 'Delayed rejection date should be at least one day in the future' + ] + + data['delayed_rejection_date'] = '2025-01-24T12:52' + form = self.get_form(data=data) + assert form.is_valid(), form.errors + + def test_delayable_action_missing_fields(self): + self.grant_permission(self.request.user, 'Addons:Review,Reviews:Admin') + self.reason_a = ReviewActionReason.objects.create( + name='a reason', + is_active=True, + canned_response='Canned response for A', + ) + data = { + 'action': 'reject_multiple_versions', + 'comments': 'foo!', + 'reasons': [self.reason_a.pk], + 'versions': [self.version.pk], + } + form = self.get_form(data=data) + assert not form.is_valid() + assert form.errors['delayed_rejection'] == [ + 'This field is required.' + ] + + # 'False' or '' works, we just want to ensure the field was submitted. + form = self.get_form(data=data) + data['delayed_rejection'] = '' + assert form.is_valid() + form = self.get_form(data=data) + data['delayed_rejection'] = 'False' + assert form.is_valid() + + # If 'True', we need a date. + data['delayed_rejection'] = 'True' + data['delayed_rejection_date'] = '' + form = self.get_form(data=data) + assert not form.is_valid() + assert form.errors['delayed_rejection_date'] == [ + 'This field is required.' + ] def test_version_pk(self): self.grant_permission(self.request.user, 'Addons:Review') From 8cde3087bd2c05e79aa9d0069bc9aed1fe2cc6d2 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 23 Jan 2025 14:54:30 +0100 Subject: [PATCH 11/25] test utils fixes --- src/olympia/reviewers/tests/test_utils.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index 10706a595776..cb970b597974 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -2445,7 +2445,7 @@ def _test_reject_multiple_versions_with_delay(self, extra_data): ) self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_APPROVED) - in_the_future = datetime.now() + timedelta(days=14) + in_the_future = datetime.now() + timedelta(days=14, hours=1) # Safeguards. assert isinstance(self.helper.handler, ReviewFiles) @@ -2457,7 +2457,7 @@ def _test_reject_multiple_versions_with_delay(self, extra_data): **self.get_data(), 'versions': self.addon.versions.all(), 'delayed_rejection': True, - 'delayed_rejection_days': 14, + 'delayed_rejection_date': in_the_future, **extra_data, } self.helper.set_data(data) @@ -2673,7 +2673,7 @@ def test_reject_multiple_versions_content_review_with_delay(self): amo.STATUS_APPROVED, file_status=amo.STATUS_APPROVED, content_review=True ) - in_the_future = datetime.now() + timedelta(days=14) + in_the_future = datetime.now() + timedelta(days=14, hours=1) # Safeguards. assert isinstance(self.helper.handler, ReviewFiles) @@ -2686,7 +2686,7 @@ def test_reject_multiple_versions_content_review_with_delay(self): { 'versions': self.addon.versions.all(), 'delayed_rejection': True, - 'delayed_rejection_days': 14, + 'delayed_rejection_date': in_the_future, } ) self.helper.set_data(data) @@ -2952,12 +2952,14 @@ def _setup_reject_multiple_versions_delayed(self, content_review): assert self.addon.status == amo.STATUS_APPROVED + in_the_future = datetime.now() + timedelta(days=14, hours=1) + data = self.get_data().copy() data.update( { 'versions': self.addon.versions.all(), 'delayed_rejection': True, - 'delayed_rejection_days': 14, + 'delayed_rejection_date': in_the_future, } ) self.helper.set_data(data) From 59fb08c4a57086acaeb00843920f863c4ec97a11 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 23 Jan 2025 15:12:40 +0100 Subject: [PATCH 12/25] Fixes for test_views --- src/olympia/reviewers/tests/test_views.py | 88 +++++++++++++++++++---- 1 file changed, 74 insertions(+), 14 deletions(-) diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 20f433e6d6b8..208bd51b9ad6 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -4610,7 +4610,12 @@ def test_reject_multiple_versions_with_delay(self): version=self.addon.current_version, verdict=amo.AUTO_APPROVED ) GroupUser.objects.filter(user=self.reviewer).all().delete() - self.grant_permission(self.reviewer, 'Addons:Review') + self.grant_permission(self.reviewer, 'Addons:Review,Reviews:Admin') + + in_the_future = datetime.now() + timedelta( + days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, + hours=1 + ) response = self.client.post( self.url, @@ -4620,15 +4625,10 @@ def test_reject_multiple_versions_with_delay(self): 'reasons': [reason.id], 'versions': [old_version.pk, self.version.pk], 'delayed_rejection': 'True', - 'delayed_rejection_days': ( - REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT - ), + 'delayed_rejection_date': in_the_future.isoformat()[:16], }, ) - in_the_future = datetime.now() + timedelta( - days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT - ) assert response.status_code == 302 for version in [old_version, self.version]: version.reload() @@ -5197,8 +5197,67 @@ def test_data_value_attributes(self): # data-value. assert doc('.data-toggle.review-files')[0].attrib['data-value'] == '' assert doc('.data-toggle.review-tested')[0].attrib['data-value'] == '' - elm = doc('.data-toggle.review-delayed-rejection')[0] - assert elm.attrib['data-value'] == 'reject_multiple_versions' + # Regular reviewer won't see delayed rejection inputs, need an admin. + assert not doc('.data-toggle.review-delayed-rejection') + + def test_test_data_value_attributes_admin(self): + AutoApprovalSummary.objects.create( + verdict=amo.AUTO_APPROVED, version=self.version + ) + self.grant_permission(self.reviewer, 'Addons:Review,Reviews:Admin') + response = self.client.get(self.url) + assert response.status_code == 200 + doc = pq(response.content) + + expected_actions_values = [ + 'confirm_auto_approved', + 'reject_multiple_versions', + 'change_pending_rejection_multiple_versions', + 'clear_needs_human_review_multiple_versions', + 'set_needs_human_review_multiple_versions', + 'reply', + 'disable_addon', + 'request_legal_review', + 'comment', + ] + assert [ + act.attrib['data-value'] for act in doc('.data-toggle.review-actions-desc') + ] == expected_actions_values + + assert doc('select#id_versions.data-toggle')[0].attrib['data-value'].split( + ' ' + ) == [ + 'reject_multiple_versions', + 'change_pending_rejection_multiple_versions', + 'clear_needs_human_review_multiple_versions', + 'set_needs_human_review_multiple_versions', + 'reply', + ] + + assert ( + doc('select#id_versions.data-toggle option')[0].text + == f'{self.version.version} - Auto-approved, not Confirmed' + ) + + assert doc('.data-toggle.review-comments')[0].attrib['data-value'].split( + ' ' + ) == [ + 'reject_multiple_versions', + 'set_needs_human_review_multiple_versions', + 'reply', + 'disable_addon', + 'request_legal_review', + 'comment', + ] + + assert doc('.data-toggle.review-actions-reasons')[0].attrib['data-value'].split( + ' ' + ) == ['reject_multiple_versions', 'reply', 'disable_addon',] + + assert doc('.data-toggle.review-files')[0].attrib['data-value'] == 'disable_addon' + assert doc('.data-toggle.review-tested')[0].attrib['data-value'] == 'disable_addon' + # Admins can use delayed rejections + assert doc('.data-toggle.review-delayed-rejection')[0].attrib['data-value'] == 'reject_multiple_versions change_pending_rejection_multiple_versions' def test_data_value_attributes_unlisted(self): self.version.update(channel=amo.CHANNEL_UNLISTED) @@ -5256,8 +5315,9 @@ def test_data_value_attributes_unlisted(self): # data-value. assert doc('.data-toggle.review-files')[0].attrib['data-value'] == '' assert doc('.data-toggle.review-tested')[0].attrib['data-value'] == '' - elm = doc('.data-toggle.review-delayed-rejection')[0] - assert elm.attrib['data-value'] == 'reject_multiple_versions' + + # Regular reviewer won't see delayed rejection inputs, need an admin. + assert not doc('.data-toggle.review-delayed-rejection') def test_no_data_value_attributes_unlisted_for_viewer(self): self.version.update(channel=amo.CHANNEL_UNLISTED) @@ -5280,9 +5340,9 @@ def test_no_data_value_attributes_unlisted_for_viewer(self): assert doc('.data-toggle.review-actions-reasons')[0].attrib['data-value'] == '' assert doc('.data-toggle.review-files')[0].attrib['data-value'] == '' assert doc('.data-toggle.review-tested')[0].attrib['data-value'] == '' - assert ( - doc('.data-toggle.review-delayed-rejection')[0].attrib['data-value'] == '' - ) + + # Regular reviewer won't see delayed rejection inputs, need an admin. + assert not doc('.data-toggle.review-delayed-rejection') def test_data_value_attributes_unreviewed(self): self.file.update(status=amo.STATUS_AWAITING_REVIEW) From 0eed736cbac81675d23e447c7b10d4fe7da263f9 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 23 Jan 2025 15:53:58 +0100 Subject: [PATCH 13/25] abuse tests, formatting --- src/olympia/abuse/tests/test_models.py | 67 +++++++++++++++++------ src/olympia/reviewers/tests/test_forms.py | 8 +-- src/olympia/reviewers/tests/test_views.py | 22 ++++++-- 3 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index 9cc3c97147f6..45377bc4a272 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -1,6 +1,6 @@ import json import uuid -from datetime import datetime +from datetime import datetime, timedelta from unittest import mock from django.conf import settings @@ -1170,11 +1170,10 @@ def test_process_decision(self): policy_a = CinderPolicy.objects.create(uuid='123-45', name='aaa', text='AAA') policy_b = CinderPolicy.objects.create(uuid='678-90', name='bbb', text='BBB') - with mock.patch.object( - ContentActionBanUser, 'process_action' - ) as action_mock, mock.patch.object( - ContentActionBanUser, 'notify_owners' - ) as notify_mock: + with ( + mock.patch.object(ContentActionBanUser, 'process_action') as action_mock, + mock.patch.object(ContentActionBanUser, 'notify_owners') as notify_mock, + ): action_mock.return_value = None cinder_job.process_decision( decision_cinder_id='12345', @@ -1201,11 +1200,10 @@ def test_process_decision_with_duplicate_parent(self): uuid='123-45', name='aaa', text='AAA', parent=parent_policy ) - with mock.patch.object( - ContentActionBanUser, 'process_action' - ) as action_mock, mock.patch.object( - ContentActionBanUser, 'notify_owners' - ) as notify_mock: + with ( + mock.patch.object(ContentActionBanUser, 'process_action') as action_mock, + mock.patch.object(ContentActionBanUser, 'notify_owners') as notify_mock, + ): action_mock.return_value = None cinder_job.process_decision( decision_cinder_id='12345', @@ -2889,6 +2887,42 @@ def test_execute_action_reject_version_delayed_held(self): decision.execute_action(release_hold=True) self._test_execute_action_reject_version_delayed_outcome(decision) + def test_execute_action_change_pending_rejection_date(self): + addon = addon_factory(users=[user_factory(email='author@example.com')]) + old_pending_rejection = self.days_ago(1) + new_pending_rejection = datetime.now() + timedelta(days=1) + version_review_flags_factory( + version=addon.current_version, + pending_rejection=old_pending_rejection, + pending_rejection_by=user_factory(), + pending_content_rejection=False, + ) + decision = ContentDecision.objects.create( + addon=addon, + action=DECISION_ACTIONS.AMO_CHANGE_PENDING_REJECTION_DATE, + action_date=datetime.now(), + ) + ActivityLog.objects.create( + amo.LOG.CHANGE_PENDING_REJECTION, + addon, + addon.current_version, + decision, + details={ + 'old_deadline': str(old_pending_rejection), + 'new_deadline': str(new_pending_rejection), + }, + user=user_factory(), + ) + decision.execute_action_and_notify() + assert ( + 'previous correspondence indicated that you would be required ' + f'to correct the violation(s) by {old_pending_rejection}' + ) in mail.outbox[0].body + assert ( + 'now require you to correct your add-on violations no later ' + f'than {new_pending_rejection}' + ) in mail.outbox[0].body + def test_resolve_job_forwarded(self): addon_developer = user_factory() addon = addon_factory(users=[addon_developer]) @@ -3105,11 +3139,12 @@ def test_execute_action_with_action_date_already(self): user=user_factory(), ) - with mock.patch.object( - ContentActionDisableAddon, 'process_action' - ) as process_mock, mock.patch.object( - ContentActionDisableAddon, 'hold_action' - ) as hold_mock: + with ( + mock.patch.object( + ContentActionDisableAddon, 'process_action' + ) as process_mock, + mock.patch.object(ContentActionDisableAddon, 'hold_action') as hold_mock, + ): decision.execute_action() process_mock.assert_not_called() hold_mock.assert_not_called() diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index 37c34781ac1e..e055858dd404 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -1092,9 +1092,7 @@ def test_delayable_action_missing_fields(self): } form = self.get_form(data=data) assert not form.is_valid() - assert form.errors['delayed_rejection'] == [ - 'This field is required.' - ] + assert form.errors['delayed_rejection'] == ['This field is required.'] # 'False' or '' works, we just want to ensure the field was submitted. form = self.get_form(data=data) @@ -1109,9 +1107,7 @@ def test_delayable_action_missing_fields(self): data['delayed_rejection_date'] = '' form = self.get_form(data=data) assert not form.is_valid() - assert form.errors['delayed_rejection_date'] == [ - 'This field is required.' - ] + assert form.errors['delayed_rejection_date'] == ['This field is required.'] def test_version_pk(self): self.grant_permission(self.request.user, 'Addons:Review') diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 208bd51b9ad6..ab039349ba4a 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -4613,8 +4613,7 @@ def test_reject_multiple_versions_with_delay(self): self.grant_permission(self.reviewer, 'Addons:Review,Reviews:Admin') in_the_future = datetime.now() + timedelta( - days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, - hours=1 + days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, hours=1 ) response = self.client.post( @@ -5252,12 +5251,23 @@ def test_test_data_value_attributes_admin(self): assert doc('.data-toggle.review-actions-reasons')[0].attrib['data-value'].split( ' ' - ) == ['reject_multiple_versions', 'reply', 'disable_addon',] + ) == [ + 'reject_multiple_versions', + 'reply', + 'disable_addon', + ] - assert doc('.data-toggle.review-files')[0].attrib['data-value'] == 'disable_addon' - assert doc('.data-toggle.review-tested')[0].attrib['data-value'] == 'disable_addon' + assert ( + doc('.data-toggle.review-files')[0].attrib['data-value'] == 'disable_addon' + ) + assert ( + doc('.data-toggle.review-tested')[0].attrib['data-value'] == 'disable_addon' + ) # Admins can use delayed rejections - assert doc('.data-toggle.review-delayed-rejection')[0].attrib['data-value'] == 'reject_multiple_versions change_pending_rejection_multiple_versions' + assert ( + doc('.data-toggle.review-delayed-rejection')[0].attrib['data-value'] + == 'reject_multiple_versions change_pending_rejection_multiple_versions' + ) def test_data_value_attributes_unlisted(self): self.version.update(channel=amo.CHANNEL_UNLISTED) From d3fc424324df57e2921ca1d8c1d979ba8e2fc652 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Tue, 28 Jan 2025 12:19:25 +0100 Subject: [PATCH 14/25] Update test --- src/olympia/abuse/tests/test_models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index 45377bc4a272..8d88d926af97 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -2887,7 +2887,7 @@ def test_execute_action_reject_version_delayed_held(self): decision.execute_action(release_hold=True) self._test_execute_action_reject_version_delayed_outcome(decision) - def test_execute_action_change_pending_rejection_date(self): + def test_send_notifications_change_pending_rejection_date(self): addon = addon_factory(users=[user_factory(email='author@example.com')]) old_pending_rejection = self.days_ago(1) new_pending_rejection = datetime.now() + timedelta(days=1) @@ -2913,7 +2913,7 @@ def test_execute_action_change_pending_rejection_date(self): }, user=user_factory(), ) - decision.execute_action_and_notify() + decision.send_notifications() assert ( 'previous correspondence indicated that you would be required ' f'to correct the violation(s) by {old_pending_rejection}' From 2226b3eef92e941926a8a4788a71a4d58191c9ea Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 29 Jan 2025 15:23:04 +0100 Subject: [PATCH 15/25] Update following rebase --- src/olympia/abuse/actions.py | 16 ++-- src/olympia/reviewers/tests/test_utils.py | 6 +- src/olympia/reviewers/utils.py | 91 +++++++++-------------- 3 files changed, 48 insertions(+), 65 deletions(-) diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index b0f342eb9315..8041b4b33e89 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -1,5 +1,5 @@ import random -from datetime import datetime, timedelta +from datetime import datetime from django.conf import settings from django.core.exceptions import ImproperlyConfigured @@ -383,10 +383,17 @@ class ContentActionRejectVersionDelayed(ContentActionRejectVersion): def __init__(self, decision): super().__init__(decision) - self.days = int(self.decision.metadata.get('delayed_rejection_days', 0)) + self.delayed_rejection_date = datetime.fromisoformat( + self.decision.metadata.get('delayed_rejection_date') + ) def log_action(self, activity_log_action, *extra_args, extra_details=None): - extra_details = {**(extra_details or {}), 'delayed_rejection_days': self.days} + extra_details = { + **(extra_details or {}), + 'delayed_rejection_days': ( + self.delayed_rejection_date - datetime.now() + ).days, + } return super().log_action( activity_log_action, *extra_args, extra_details=extra_details ) @@ -397,14 +404,13 @@ def process_action(self): if not self.decision.reviewer_user: # This action should only be used by reviewer tools, not cinder webhook raise NotImplementedError - pending_rejection_deadline = datetime.now() + timedelta(days=self.days) for version in self.decision.target_versions.all(): # (Re)set pending_rejection. VersionReviewerFlags.objects.update_or_create( version=version, defaults={ - 'pending_rejection': pending_rejection_deadline, + 'pending_rejection': self.delayed_rejection_date, 'pending_rejection_by': self.decision.reviewer_user, 'pending_content_rejection': self.content_review, }, diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index cb970b597974..c0705ac228cb 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -2391,7 +2391,6 @@ def _test_reject_multiple_versions(self, extra_data, human_review=True): ] assert decision.metadata == { 'content_review': False, - 'delayed_rejection_days': None, } # listed auto approvals should be disabled until the next manual @@ -2499,7 +2498,7 @@ def _test_reject_multiple_versions_with_delay(self, extra_data): assert log.arguments == [self.addon, decision, self.review_version, old_version] assert decision.metadata == { 'content_review': False, - 'delayed_rejection_days': 14, + 'delayed_rejection_date': in_the_future.isoformat(), } # The flag to prevent the authors from being notified several times @@ -2662,7 +2661,6 @@ def test_reject_multiple_versions_content_review(self): assert ContentDecision.objects.get().metadata == { 'content_review': True, - 'delayed_rejection_days': None, } def test_reject_multiple_versions_content_review_with_delay(self): @@ -2728,7 +2726,7 @@ def test_reject_multiple_versions_content_review_with_delay(self): assert log.arguments == [self.addon, decision, self.review_version, old_version] assert decision.metadata == { 'content_review': True, - 'delayed_rejection_days': 14, + 'delayed_rejection_date': in_the_future.isoformat(), } def test_unreject_latest_version_approved_addon(self): diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 250662912ffe..a8e5e08e3dbf 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -1393,8 +1393,16 @@ def reject_multiple_versions(self): channel = self.version.channel if self.version else None self.version = None self.file = None + decision_metadata = { + 'content_review': self.content_review, + } - if self.data.get('delayed_rejection'): + if self.data.get('delayed_rejection') and self.data.get( + 'delayed_rejection_date' + ): + decision_metadata['delayed_rejection_date'] = self.data.get( + 'delayed_rejection_date' + ).isoformat() action_id = ( amo.LOG.REJECT_CONTENT_DELAYED if self.content_review @@ -1436,17 +1444,17 @@ def reject_multiple_versions(self): self.record_decision( action_id, versions=self.data['versions'], - decision_metadata={ - 'content_review': self.content_review, - 'delayed_rejection_days': self.data.get('delayed_rejection_days'), - }, + decision_metadata=decision_metadata, action_completed=False, ) def auto_reject_multiple_versions(self): - """Auto reject a list of versions from previously delayed rejections. - Note: this is also used in blocklist.utils.disable_addon_for_block for both - listed and unlisted versions (human_review=False).""" + """Immediately reject a list of versions, either from previously + delayed rejections or from versions being blocked. + + Note: this is not accessible through reviewer tools UI, but because it + is triggered for rejections coming from a block being created, + self.human_review can be True.""" # self.version and self.file won't point to the versions we want to # modify in this action, so set them to None before finding the right # versions. @@ -1454,41 +1462,22 @@ def auto_reject_multiple_versions(self): self.version = None self.file = None now = datetime.now() - if self.data.get('delayed_rejection') and self.data.get( - 'delayed_rejection_date' - ): - pending_rejection_deadline = self.data['delayed_rejection_date'] - self.data['delayed_rejection_days'] = ( - pending_rejection_deadline - now - ).days - action_id = ( - amo.LOG.REJECT_CONTENT_DELAYED - if self.content_review - else amo.LOG.REJECT_VERSION_DELAYED - ) - log.info( - 'Marking %s versions %s for delayed rejection' - % (self.addon, ', '.join(str(v.pk) for v in self.data['versions'])) - ) - else: - pending_rejection_deadline = None - action_id = ( - amo.LOG.REJECT_CONTENT - if self.content_review - else amo.LOG.REJECT_VERSION - ) - log.info( - 'Making %s versions %s disabled' - % (self.addon, ', '.join(str(v.pk) for v in self.data['versions'])) - ) - # For a human review we record a single action, but for automated - # stuff, we need to split by type (content review or not) and by - # original user to match the original user(s) and action(s). + + action_id = ( + amo.LOG.REJECT_CONTENT if self.content_review else amo.LOG.REJECT_VERSION + ) + log.info( + 'Making %s versions %s disabled' + % (self.addon, ', '.join(str(v.pk) for v in self.data['versions'])) + ) + # For an immediate rejection we record a single action, but for + # applying a delayed rejection, we need to split by type (content + # review or not) and by original user to match the original user(s) + # and action(s). actions_to_record = defaultdict(lambda: defaultdict(list)) for version in self.data['versions']: file = version.file - if not pending_rejection_deadline: - self.set_file(amo.STATUS_DISABLED, file) + self.set_file(amo.STATUS_DISABLED, file) if ( not self.human_review @@ -1504,18 +1493,13 @@ def auto_reject_multiple_versions(self): # Clear needs human review flags on rejected versions, we # consider that the reviewer looked at them before rejecting. self.clear_specific_needs_human_review_flags(version) - # (Re)set pending_rejection. Could be reset to None if doing an - # immediate rejection. + # Reset pending_rejection. VersionReviewerFlags.objects.update_or_create( version=version, defaults={ - 'pending_rejection': pending_rejection_deadline, - 'pending_rejection_by': self.user - if pending_rejection_deadline - else None, - 'pending_content_rejection': self.content_review - if pending_rejection_deadline - else None, + 'pending_rejection': None, + 'pending_rejection_by': None, + 'pending_content_rejection': None, }, ) self.set_human_review_date(version) @@ -1535,12 +1519,8 @@ def auto_reject_multiple_versions(self): else 'auto_approval_disabled_until_next_approval_unlisted' ) addonreviewerflags[auto_approval_disabled_until_next_approval_flag] = True - if pending_rejection_deadline: - # Developers should be notified again once the deadline is close. - addonreviewerflags['notified_about_expiring_delayed_rejections'] = False - else: - # An immediate rejection might require the add-on status to change. - self.addon.update_status() + # The versions rejected might require the add-on status to change. + self.addon.update_status() if addonreviewerflags: AddonReviewerFlags.objects.update_or_create( addon=self.addon, @@ -1548,7 +1528,6 @@ def auto_reject_multiple_versions(self): ) keys = [ - 'delayed_rejection_days', 'is_addon_being_blocked', 'is_addon_being_disabled', ] From 04dbc90c94785f97f1c20c4787e7924246360c5f Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 29 Jan 2025 15:51:49 +0100 Subject: [PATCH 16/25] Pain --- src/olympia/abuse/actions.py | 12 +++++++++--- src/olympia/abuse/tests/test_actions.py | 7 ++++++- src/olympia/abuse/tests/test_models.py | 12 ++++++++++-- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/olympia/abuse/actions.py b/src/olympia/abuse/actions.py index 8041b4b33e89..e05375334ed1 100644 --- a/src/olympia/abuse/actions.py +++ b/src/olympia/abuse/actions.py @@ -383,9 +383,15 @@ class ContentActionRejectVersionDelayed(ContentActionRejectVersion): def __init__(self, decision): super().__init__(decision) - self.delayed_rejection_date = datetime.fromisoformat( - self.decision.metadata.get('delayed_rejection_date') - ) + if 'delayed_rejection_date' in self.decision.metadata: + self.delayed_rejection_date = datetime.fromisoformat( + self.decision.metadata.get('delayed_rejection_date') + ) + else: + # Will fail later if we try to use it to log/process the action, + # but allows us to at least instantiate the class and use other + # methods. + self.delayed_rejection_date = None def log_action(self, activity_log_action, *extra_args, extra_details=None): extra_details = { diff --git a/src/olympia/abuse/tests/test_actions.py b/src/olympia/abuse/tests/test_actions.py index fae31034a1c0..3ca8714d8653 100644 --- a/src/olympia/abuse/tests/test_actions.py +++ b/src/olympia/abuse/tests/test_actions.py @@ -784,7 +784,12 @@ def test_reject_version_after_reporter_appeal(self): def _test_reject_version_delayed(self, *, content_review): self.decision.update( action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, - metadata={'delayed_rejection_days': 14, 'content_review': content_review}, + metadata={ + 'delayed_rejection_date': ( + datetime.now() + timedelta(days=14, minutes=1) + ).isoformat(), + 'content_review': content_review, + }, ) action = ContentActionRejectVersionDelayed(self.decision) # process_action is only available for reviewer tools decisions. diff --git a/src/olympia/abuse/tests/test_models.py b/src/olympia/abuse/tests/test_models.py index 8d88d926af97..b36b9c06eb7b 100644 --- a/src/olympia/abuse/tests/test_models.py +++ b/src/olympia/abuse/tests/test_models.py @@ -2835,7 +2835,11 @@ def test_execute_action_reject_version_delayed(self): action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, notes='some review text', reviewer_user=self.reviewer_user, - metadata={'delayed_rejection_days': 14}, + metadata={ + 'delayed_rejection_date': ( + datetime.now() + timedelta(days=14, minutes=1) + ).isoformat() + }, ) decision.target_versions.set([addon.current_version]) NeedsHumanReview.objects.create( @@ -2870,7 +2874,11 @@ def test_execute_action_reject_version_delayed_held(self): action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, notes='some review text', reviewer_user=self.reviewer_user, - metadata={'delayed_rejection_days': 14}, + metadata={ + 'delayed_rejection_date': ( + datetime.now() + timedelta(days=14, minutes=1) + ).isoformat() + }, ) decision.target_versions.set([version]) assert decision.action_date is None From 4430b9a8ed1ad17ee3a9d4d275a50e8d4ef5179c Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 29 Jan 2025 16:58:37 +0100 Subject: [PATCH 17/25] In actions tests delay in the future like reviewers would do --- src/olympia/abuse/tests/test_actions.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/olympia/abuse/tests/test_actions.py b/src/olympia/abuse/tests/test_actions.py index 3ca8714d8653..fdaa7e71880c 100644 --- a/src/olympia/abuse/tests/test_actions.py +++ b/src/olympia/abuse/tests/test_actions.py @@ -782,12 +782,11 @@ def test_reject_version_after_reporter_appeal(self): self._test_reporter_appeal_takedown_email(subject) def _test_reject_version_delayed(self, *, content_review): + in_the_future = datetime.now() + timedelta(days=14, hours=1) self.decision.update( action=DECISION_ACTIONS.AMO_REJECT_VERSION_WARNING_ADDON, metadata={ - 'delayed_rejection_date': ( - datetime.now() + timedelta(days=14, minutes=1) - ).isoformat(), + 'delayed_rejection_date': in_the_future.isoformat(), 'content_review': content_review, }, ) @@ -809,9 +808,7 @@ def _test_reject_version_delayed(self, *, content_review): assert self.addon.reload().status == amo.STATUS_APPROVED assert self.version.file.status == amo.STATUS_APPROVED version_flags = VersionReviewerFlags.objects.filter(version=self.version).get() - self.assertCloseToNow( - version_flags.pending_rejection, now=datetime.now() + timedelta(14) - ) + self.assertCloseToNow(version_flags.pending_rejection, now=in_the_future) assert version_flags.pending_rejection_by == reviewer assert version_flags.pending_content_rejection == content_review assert ActivityLog.objects.count() == 1 From 771a3431d0c699a67e6c32dcb11b439510c6a373 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 30 Jan 2025 12:50:34 +0100 Subject: [PATCH 18/25] Tests --- src/olympia/reviewers/tests/test_forms.py | 38 +++++++++- src/olympia/reviewers/tests/test_utils.py | 89 ++++++++++++++++++++++- src/olympia/reviewers/tests/test_views.py | 37 ++++++++++ src/olympia/reviewers/utils.py | 6 +- 4 files changed, 165 insertions(+), 5 deletions(-) diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index e055858dd404..0a8e932f3a4e 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -1,5 +1,5 @@ import uuid -from datetime import datetime +from datetime import datetime, timedelta from django.core.files.base import ContentFile from django.utils.encoding import force_str @@ -33,7 +33,7 @@ ) from olympia.reviewers.utils import ReviewHelper from olympia.users.models import UserProfile -from olympia.versions.models import Version +from olympia.versions.models import Version, VersionReviewerFlags class TestReviewForm(TestCase): @@ -1109,6 +1109,40 @@ def test_delayable_action_missing_fields(self): assert not form.is_valid() assert form.errors['delayed_rejection_date'] == ['This field is required.'] + def test_change_pending_rejection_multiple_versions_different_dates(self): + self.grant_permission(self.request.user, 'Addons:Review,Reviews:Admin') + in_the_future = datetime.now() + timedelta(days=15) + in_the_future2 = datetime.now() + timedelta(days=55) + VersionReviewerFlags.objects.create( + version=self.version, + pending_rejection=in_the_future, + pending_rejection_by=self.request.user, + pending_content_rejection=False, + ) + new_version = version_factory(addon=self.addon) + VersionReviewerFlags.objects.create( + version=new_version, + pending_rejection=in_the_future2, + pending_rejection_by=self.request.user, + pending_content_rejection=False, + ) + + data = { + 'action': 'change_pending_rejection_multiple_versions', + 'delayed_rejection': 'True', + 'delayed_rejection_date': in_the_future.isoformat()[:16], + 'versions': [self.version.pk, new_version.pk], + } + form = self.get_form(data=data) + assert not form.is_valid() + assert form.errors == { + 'versions': [ + 'Can only change the delayed rejection date of multiple ' + 'versions at once if their pending rejection dates are all ' + 'the same.' + ] + } + def test_version_pk(self): self.grant_permission(self.request.user, 'Addons:Review') data = {'action': 'comment', 'comments': 'lol'} diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index c0705ac228cb..d0872c66cdd2 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -3364,7 +3364,7 @@ def test_set_needs_human_review_multiple_versions(self): assert not unselected.needshumanreview_set.filter(is_active=True).exists() assert not unselected.due_date - def test_change_pending_rejection_multiple_versions(self): + def test_clear_pending_rejection_multiple_versions(self): self.grant_permission(self.user, 'Addons:Review') self.grant_permission(self.user, 'Reviews:Admin') self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_APPROVED) @@ -3432,6 +3432,93 @@ def test_change_pending_rejection_multiple_versions(self): assert unselected.reviewerflags.pending_rejection_by assert unselected.reviewerflags.pending_rejection is not None + def test_change_pending_rejection_multiple_versions(self): + self.grant_permission(self.user, 'Addons:Review') + self.grant_permission(self.user, 'Reviews:Admin') + self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_APPROVED) + old_pending_rejection_date = datetime.now() + timedelta(days=1) + VersionReviewerFlags.objects.create( + version=self.review_version, + pending_rejection=old_pending_rejection_date, + pending_rejection_by=self.user, + pending_content_rejection=False, + ) + selected = version_factory(addon=self.review_version.addon) + VersionReviewerFlags.objects.create( + version=selected, + pending_rejection=old_pending_rejection_date, + pending_rejection_by=self.user, + pending_content_rejection=True, + ) + unselected = version_factory(addon=self.review_version.addon) + VersionReviewerFlags.objects.create( + version=unselected, + pending_rejection=old_pending_rejection_date, + pending_rejection_by=self.user, + pending_content_rejection=False, + ) + in_the_future = datetime.now().replace(second=0, microsecond=0) + timedelta( + days=15 + ) + data = self.get_data().copy() + data['versions'] = ( + self.addon.versions(manager='unfiltered_for_relations') + .all() + .exclude(pk=unselected.pk) + .order_by('pk') + ) + data['action'] = 'change_pending_rejection_multiple_versions' + data['delayed_rejection'] = 'True' + data['delayed_rejection_date'] = in_the_future.isoformat()[:16] + self.helper.set_data(data) + self.helper.process() + + old_deadline = old_pending_rejection_date.isoformat()[:16] + new_deadline = in_the_future.isoformat()[:16] + log_type_id = amo.LOG.CHANGE_PENDING_REJECTION.id + assert self.check_log_count(log_type_id) == 1 + activity = ActivityLog.objects.for_addons(self.helper.addon).get( + action=log_type_id + ) + assert activity.details['comments'] == '' + assert activity.details['versions'] == [ + self.review_version.version, + selected.version, + ] + assert activity.details['old_deadline'] == old_deadline + assert activity.details['new_deadline'] == new_deadline + assert len(mail.outbox) == 1 + assert ( + 'Our previous correspondence indicated that you would be required ' + f'to correct the violation(s) by {old_deadline}.' + ) in mail.outbox[0].body + assert ( + 'will now require you to correct your add-on violations no later ' + f'than {new_deadline}' + ) in mail.outbox[0].body + + self.review_version.reload() + self.review_version.reviewerflags.reload() + assert not self.review_version.human_review_date + assert self.review_version.reviewerflags.pending_content_rejection is False + assert self.review_version.reviewerflags.pending_rejection_by == self.user + assert self.review_version.reviewerflags.pending_rejection == in_the_future + + selected.reload() + selected.reviewerflags.reload() + assert not selected.human_review_date + assert selected.reviewerflags.pending_content_rejection is True + assert selected.reviewerflags.pending_rejection_by == self.user + assert selected.reviewerflags.pending_rejection == in_the_future + + unselected.reload() + unselected.reviewerflags.reload() + assert not unselected.human_review_date + assert unselected.reviewerflags.pending_content_rejection is False + assert unselected.reviewerflags.pending_rejection_by + assert unselected.reviewerflags.pending_rejection + assert unselected.reviewerflags.pending_rejection != in_the_future + def test_disable_addon(self): self.grant_permission(self.user, 'Reviews:Admin') self.setup_data(amo.STATUS_APPROVED, file_status=amo.STATUS_APPROVED) diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index ab039349ba4a..17e088b9df8e 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -4640,6 +4640,43 @@ def test_reject_multiple_versions_with_delay(self): assert version.pending_rejection self.assertCloseToNow(version.pending_rejection, now=in_the_future) + def test_change_pending_rejection_date(self): + self.grant_permission(self.reviewer, 'Addons:Review,Reviews:Admin') + old_version = self.version + in_the_future = datetime.now() + timedelta(hours=1) + in_the_future2 = datetime.now() + timedelta(days=2, hours=1) + self.version = version_factory(addon=self.addon, version='3.0') + for version in (old_version, self.version): + version_review_flags_factory( + version=version, + pending_rejection=in_the_future, + pending_rejection_by=self.reviewer, + pending_content_rejection=False, + ) + NeedsHumanReview.objects.create(version=version) + + response = self.client.post( + self.url, + { + 'action': 'change_pending_rejection_multiple_versions', + 'versions': [old_version.pk, self.version.pk], + 'delayed_rejection': 'True', + 'delayed_rejection_date': in_the_future2.isoformat()[:16], + }, + ) + assert response.status_code == 302 + for version in [old_version, self.version]: + version.reload() + version.reviewerflags.reload() + # NeedsHumanReview was *not* cleared. + assert version.needshumanreview_set.filter(is_active=True) + file_ = version.file + # ... But their status shouldn't have changed yet ... + assert file_.status == amo.STATUS_APPROVED + # ... Because they are still pending rejection, with the new date. + assert version.pending_rejection + self.assertCloseToNow(version.pending_rejection, now=in_the_future2) + def test_unreject_latest_version(self): old_version = self.version version_factory(addon=self.addon, version='2.99') diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index a8e5e08e3dbf..d86d355fb748 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -1614,14 +1614,16 @@ def change_pending_rejection_multiple_versions(self): 'delayed_rejection_date' ): pending_rejection_deadline = self.data['delayed_rejection_date'] - extra_details['new_deadline'] = str(pending_rejection_deadline) + extra_details['new_deadline'] = pending_rejection_deadline else: pending_rejection_deadline = None for version in self.data['versions']: # Do it one by one to trigger the post_save() for each version. if version.pending_rejection: - extra_details['old_deadline'] = str(version.pending_rejection) + extra_details['old_deadline'] = version.pending_rejection.isoformat()[ + :16 + ] kwargs = { 'pending_rejection': pending_rejection_deadline, } From 4ac1b687858e2871bb93bde7d0811abdba570326 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Thu, 30 Jan 2025 13:41:51 +0100 Subject: [PATCH 19/25] data (django form cleaned) contains a date, details (json) a string --- src/olympia/reviewers/tests/test_utils.py | 2 +- src/olympia/reviewers/utils.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index d0872c66cdd2..ac7e334a325a 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -3469,7 +3469,7 @@ def test_change_pending_rejection_multiple_versions(self): ) data['action'] = 'change_pending_rejection_multiple_versions' data['delayed_rejection'] = 'True' - data['delayed_rejection_date'] = in_the_future.isoformat()[:16] + data['delayed_rejection_date'] = in_the_future self.helper.set_data(data) self.helper.process() diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index d86d355fb748..5033757d166b 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -1614,7 +1614,7 @@ def change_pending_rejection_multiple_versions(self): 'delayed_rejection_date' ): pending_rejection_deadline = self.data['delayed_rejection_date'] - extra_details['new_deadline'] = pending_rejection_deadline + extra_details['new_deadline'] = pending_rejection_deadline.isoformat()[:16] else: pending_rejection_deadline = None From 407703a7f58465cf9f570e68eacfdc2534b664d6 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Fri, 31 Jan 2025 13:50:37 +0100 Subject: [PATCH 20/25] Comment + stop hardcoding error message --- src/olympia/reviewers/forms.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index 837cd6e03df6..e73f1b3109cc 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -516,10 +516,12 @@ def clean(self): ) if self.helper.actions.get(selected_action, {}).get('delayable'): + # Extra required checks are added here because the NullBooleanField + # otherwise accepts missing data as `None`. if 'delayed_rejection' not in self.data: self.add_error( 'delayed_rejection', - forms.ValidationError('This field is required.'), + self.fields['delayed_rejection'].error_messages['required'], ) elif self.cleaned_data.get('delayed_rejection') and not self.data.get( 'delayed_rejection_date' @@ -528,7 +530,7 @@ def clean(self): # somehow cleared the date widget, raise an error. self.add_error( 'delayed_rejection_date', - forms.ValidationError('This field is required.'), + self.fields['delayed_rejection'].error_messages['required'], ) elif ( selected_action == 'change_pending_rejection_multiple_versions' From a1128d64885be43823671b51a83f99ca5e212c7f Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Fri, 31 Jan 2025 17:14:54 +0100 Subject: [PATCH 21/25] Extract what we can to variables --- src/olympia/reviewers/forms.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index e73f1b3109cc..e57b7cab48c9 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -516,6 +516,8 @@ def clean(self): ) if self.helper.actions.get(selected_action, {}).get('delayable'): + delayed_rejection = self.cleaned_data.get('delayed_rejection') + delayed_rejection_date = self.cleaned_data.get('delayed_rejection_date') # Extra required checks are added here because the NullBooleanField # otherwise accepts missing data as `None`. if 'delayed_rejection' not in self.data: @@ -523,9 +525,7 @@ def clean(self): 'delayed_rejection', self.fields['delayed_rejection'].error_messages['required'], ) - elif self.cleaned_data.get('delayed_rejection') and not self.data.get( - 'delayed_rejection_date' - ): + elif delayed_rejection and not self.data.get('delayed_rejection_date'): # In case reviewer selected delayed rejection option and # somehow cleared the date widget, raise an error. self.add_error( @@ -534,8 +534,8 @@ def clean(self): ) elif ( selected_action == 'change_pending_rejection_multiple_versions' - and self.cleaned_data.get('delayed_rejection') - and self.cleaned_data.get('delayed_rejection_date') + and delayed_rejection + and delayed_rejection_date and self.cleaned_data.get('versions') ): distinct_pending_rejection_dates = ( From f1f0ac5243df4fea90a6ad6a5449e683279dc5d1 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Fri, 31 Jan 2025 18:22:07 +0100 Subject: [PATCH 22/25] Disable/enable the delay date widget dynamically --- static/js/zamboni/reviewers.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/static/js/zamboni/reviewers.js b/static/js/zamboni/reviewers.js index a96151b8f793..f4ca7283094e 100644 --- a/static/js/zamboni/reviewers.js +++ b/static/js/zamboni/reviewers.js @@ -126,8 +126,23 @@ function initReviewActions() { .filter('[data-value~="' + value + '"]') .parent('label') .hide(); + + showHideDelayedRejectionDateWidget(); + } + + function showHideDelayedRejectionDateWidget() { + var delayed_rejection_input = $('#id_delayed_rejection input[name=delayed_rejection]:checked'); + console.log(delayed_rejection_input); + var delayed_rejection_date_widget = $('#id_delayed_rejection_date'); + if (delayed_rejection_input.prop('value') == 'True') { + delayed_rejection_date_widget.prop('disabled', false); + } else { + delayed_rejection_date_widget.prop('disabled', true); + } } + $('#id_delayed_rejection input[name=delayed_rejection]').change(showHideDelayedRejectionDateWidget); + $('#review-actions .action_nav #id_action > *:not(.disabled)').click( function () { showForm(this); From be61b8ba1ba89911a6c5aa313539db0192cf8262 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Fri, 31 Jan 2025 18:53:49 +0100 Subject: [PATCH 23/25] Reformat --- static/js/zamboni/reviewers.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/static/js/zamboni/reviewers.js b/static/js/zamboni/reviewers.js index f4ca7283094e..b17e2153110b 100644 --- a/static/js/zamboni/reviewers.js +++ b/static/js/zamboni/reviewers.js @@ -131,7 +131,9 @@ function initReviewActions() { } function showHideDelayedRejectionDateWidget() { - var delayed_rejection_input = $('#id_delayed_rejection input[name=delayed_rejection]:checked'); + var delayed_rejection_input = $( + '#id_delayed_rejection input[name=delayed_rejection]:checked', + ); console.log(delayed_rejection_input); var delayed_rejection_date_widget = $('#id_delayed_rejection_date'); if (delayed_rejection_input.prop('value') == 'True') { @@ -141,7 +143,9 @@ function initReviewActions() { } } - $('#id_delayed_rejection input[name=delayed_rejection]').change(showHideDelayedRejectionDateWidget); + $('#id_delayed_rejection input[name=delayed_rejection]').change( + showHideDelayedRejectionDateWidget, + ); $('#review-actions .action_nav #id_action > *:not(.disabled)').click( function () { From e1ffe0983d8264314fe1adbd8353815ef295e3ab Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 3 Feb 2025 14:41:10 +0100 Subject: [PATCH 24/25] Update internal action name + description (keep the label to avoid wrapping) --- src/olympia/reviewers/forms.py | 6 +++--- src/olympia/reviewers/tests/test_forms.py | 18 +++++++++--------- src/olympia/reviewers/tests/test_utils.py | 18 +++++++++--------- src/olympia/reviewers/tests/test_views.py | 10 +++++----- src/olympia/reviewers/utils.py | 14 ++++++++------ 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/src/olympia/reviewers/forms.py b/src/olympia/reviewers/forms.py index e57b7cab48c9..db5f33b4f499 100644 --- a/src/olympia/reviewers/forms.py +++ b/src/olympia/reviewers/forms.py @@ -174,7 +174,7 @@ def create_option(self, *args, **kwargs): # fine though. actions.remove('unreject_multiple_versions') if obj.pending_rejection: - actions.append('change_pending_rejection_multiple_versions') + actions.append('change_or_clear_pending_rejection_multiple_versions') if needs_human_review: actions.append('clear_needs_human_review_multiple_versions') # Setting needs human review is available if the version is not @@ -353,7 +353,7 @@ def create_option(self, name, value, *args, **kwargs): option['attrs']['data-value'] = 'reject_multiple_versions' else: # Empty value is reserved for clearing pending rejection. option['attrs']['data-value'] = ( - 'change_pending_rejection_multiple_versions' + 'change_or_clear_pending_rejection_multiple_versions' ) return option @@ -533,7 +533,7 @@ def clean(self): self.fields['delayed_rejection'].error_messages['required'], ) elif ( - selected_action == 'change_pending_rejection_multiple_versions' + selected_action == 'change_or_clear_pending_rejection_multiple_versions' and delayed_rejection and delayed_rejection_date and self.cleaned_data.get('versions') diff --git a/src/olympia/reviewers/tests/test_forms.py b/src/olympia/reviewers/tests/test_forms.py index 0a8e932f3a4e..1ba707fe6a9b 100644 --- a/src/olympia/reviewers/tests/test_forms.py +++ b/src/olympia/reviewers/tests/test_forms.py @@ -95,7 +95,7 @@ def test_actions_addon_status_null(self): actions = self.get_form().helper.get_actions() assert list(actions.keys()) == [ 'unreject_latest_version', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -134,7 +134,7 @@ def test_actions_addon_status_null_unlisted(self): 'unreject_multiple_versions', 'block_multiple_versions', 'confirm_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -163,7 +163,7 @@ def test_actions_addon_status_deleted(self): addon_status=amo.STATUS_DELETED, file_status=amo.STATUS_DISABLED ) assert list(actions.keys()) == [ - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -193,7 +193,7 @@ def test_actions_no_pending_files(self): ) assert list(actions.keys()) == [ 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -208,7 +208,7 @@ def test_actions_no_pending_files(self): addon_status=amo.STATUS_DISABLED, file_status=amo.STATUS_DISABLED ) assert list(actions.keys()) == [ - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'reply', 'enable_addon', @@ -754,7 +754,7 @@ def test_versions_queryset_contains_pending_files_for_listed_admin_reviewer(self self.test_versions_queryset_contains_pending_files_for_listed( expected_select_data_value=[ 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -977,7 +977,7 @@ def test_versions_queryset_contains_pending_files_for_unlisted_admin_reviewer(se 'unreject_multiple_versions', 'block_multiple_versions', 'confirm_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', ] @@ -1048,7 +1048,7 @@ def test_delayed_rejection_days_shows_up_for_admin_reviewers(self): assert inputs[2].attrib['class'] == 'data-toggle' assert ( inputs[2].attrib['data-value'] - == 'change_pending_rejection_multiple_versions' + == 'change_or_clear_pending_rejection_multiple_versions' ) @freeze_time('2025-01-23 12:52') @@ -1128,7 +1128,7 @@ def test_change_pending_rejection_multiple_versions_different_dates(self): ) data = { - 'action': 'change_pending_rejection_multiple_versions', + 'action': 'change_or_clear_pending_rejection_multiple_versions', 'delayed_rejection': 'True', 'delayed_rejection_date': in_the_future.isoformat()[:16], 'versions': [self.version.pk, new_version.pk], diff --git a/src/olympia/reviewers/tests/test_utils.py b/src/olympia/reviewers/tests/test_utils.py index ac7e334a325a..925692f1e786 100644 --- a/src/olympia/reviewers/tests/test_utils.py +++ b/src/olympia/reviewers/tests/test_utils.py @@ -584,7 +584,7 @@ def test_actions_promoted_admin_review_needs_admin_permission(self): 'public', 'reject', 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -659,7 +659,7 @@ def test_actions_unlisted(self): 'unreject_multiple_versions', 'block_multiple_versions', 'confirm_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -800,7 +800,7 @@ def test_actions_pending_rejection_admin(self): expected = [ 'confirm_auto_approved', 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -827,7 +827,7 @@ def test_actions_pending_rejection_admin(self): 'reject', 'confirm_auto_approved', 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -864,7 +864,7 @@ def test_actions_disabled_addon(self): self.grant_permission(self.user, 'Reviews:Admin') expected = [ - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'reply', 'enable_addon', @@ -896,7 +896,7 @@ def test_actions_rejected_version(self): self.grant_permission(self.user, 'Reviews:Admin') expected = [ 'unreject_latest_version', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -958,7 +958,7 @@ def test_actions_versions_needing_human_review(self): self.grant_permission(self.user, 'Reviews:Admin') expected = [ - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -3395,7 +3395,7 @@ def test_clear_pending_rejection_multiple_versions(self): .exclude(pk=unselected.pk) .order_by('pk') ) - data['action'] = 'change_pending_rejection_multiple_versions' + data['action'] = 'change_or_clear_pending_rejection_multiple_versions' self.helper.set_data(data) self.helper.process() @@ -3467,7 +3467,7 @@ def test_change_pending_rejection_multiple_versions(self): .exclude(pk=unselected.pk) .order_by('pk') ) - data['action'] = 'change_pending_rejection_multiple_versions' + data['action'] = 'change_or_clear_pending_rejection_multiple_versions' data['delayed_rejection'] = 'True' data['delayed_rejection_date'] = in_the_future self.helper.set_data(data) diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 17e088b9df8e..1d3635b922fc 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -2449,7 +2449,7 @@ def test_need_correct_reviewer_for_promoted_addon(self): 'public', 'reject', 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -4658,7 +4658,7 @@ def test_change_pending_rejection_date(self): response = self.client.post( self.url, { - 'action': 'change_pending_rejection_multiple_versions', + 'action': 'change_or_clear_pending_rejection_multiple_versions', 'versions': [old_version.pk, self.version.pk], 'delayed_rejection': 'True', 'delayed_rejection_date': in_the_future2.isoformat()[:16], @@ -5248,7 +5248,7 @@ def test_test_data_value_attributes_admin(self): expected_actions_values = [ 'confirm_auto_approved', 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -5264,7 +5264,7 @@ def test_test_data_value_attributes_admin(self): ' ' ) == [ 'reject_multiple_versions', - 'change_pending_rejection_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', 'clear_needs_human_review_multiple_versions', 'set_needs_human_review_multiple_versions', 'reply', @@ -5303,7 +5303,7 @@ def test_test_data_value_attributes_admin(self): # Admins can use delayed rejections assert ( doc('.data-toggle.review-delayed-rejection')[0].attrib['data-value'] - == 'reject_multiple_versions change_pending_rejection_multiple_versions' + == 'reject_multiple_versions change_or_clear_pending_rejection_multiple_versions' ) def test_data_value_attributes_unlisted(self): diff --git a/src/olympia/reviewers/utils.py b/src/olympia/reviewers/utils.py index 5033757d166b..1397233ca1bd 100644 --- a/src/olympia/reviewers/utils.py +++ b/src/olympia/reviewers/utils.py @@ -694,12 +694,14 @@ def get_actions(self): ), 'resolves_cinder_jobs': True, } - actions['change_pending_rejection_multiple_versions'] = { - 'method': self.handler.change_pending_rejection_multiple_versions, + actions['change_or_clear_pending_rejection_multiple_versions'] = { + 'method': self.handler.change_or_clear_pending_rejection_multiple_versions, 'label': 'Change pending rejection', 'details': ( - 'Change pending rejection from selected versions, but ' - "otherwise don't change the version(s) or add-on statuses." + 'Change or clear pending rejection from selected versions, ' + "but otherwise don't change the version(s) or add-on " + 'statuses. Developer will be notified of the new pending ' + 'rejection date unless the action clears it.' ), 'delayable': True, 'multiple_versions': True, @@ -1605,8 +1607,8 @@ def set_needs_human_review_multiple_versions(self): versions=self.data['versions'], ) - def change_pending_rejection_multiple_versions(self): - """Change pending rejection on selected versions.""" + def change_or_clear_pending_rejection_multiple_versions(self): + """Change/clear pending rejection on selected versions.""" self.file = None self.version = None extra_details = {} From dd959654131c6957cfe623bb592ee10d0a5a06cc Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 3 Feb 2025 14:55:55 +0100 Subject: [PATCH 25/25] Reformat --- src/olympia/reviewers/tests/test_views.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 1d3635b922fc..e22cf0f57a5d 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -5301,10 +5301,12 @@ def test_test_data_value_attributes_admin(self): doc('.data-toggle.review-tested')[0].attrib['data-value'] == 'disable_addon' ) # Admins can use delayed rejections - assert ( - doc('.data-toggle.review-delayed-rejection')[0].attrib['data-value'] - == 'reject_multiple_versions change_or_clear_pending_rejection_multiple_versions' - ) + assert doc('.data-toggle.review-delayed-rejection')[0].attrib[ + 'data-value' + ].split(' ') == [ + 'reject_multiple_versions', + 'change_or_clear_pending_rejection_multiple_versions', + ] def test_data_value_attributes_unlisted(self): self.version.update(channel=amo.CHANNEL_UNLISTED)