Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Make reviewer pending rejection input a datetime widget and allow changing it through an action #23001

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/olympia/abuse/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,14 @@ def process_action(self):
)


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'
Expand Down
5 changes: 5 additions & 0 deletions src/olympia/abuse/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
ContentActionApproveInitialDecision,
ContentActionApproveNoAction,
ContentActionBanUser,
ContentActionChangePendingRejectionDate,
ContentActionDeleteCollection,
ContentActionDeleteRating,
ContentActionDisableAddon,
Expand Down Expand Up @@ -1048,6 +1049,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):
Expand Down Expand Up @@ -1275,6 +1279,7 @@ def execute_action_and_notify(self, *, release_hold=False):
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),
Expand Down
Original file line number Diff line number Diff line change
@@ -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 %}
15 changes: 12 additions & 3 deletions src/olympia/constants/abuse.py
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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',
Expand Down
11 changes: 11 additions & 0 deletions src/olympia/constants/activity.py
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,17 @@ class REQUEST_LEGAL(_LOG):
cinder_action = DECISION_ACTIONS.AMO_LEGAL_FORWARD


class CHANGE_PENDING_REJECTION(_LOG):
id = 199
format = _('{addon} {version} pending rejection changed.')
short = _('Pending rejection changed')
keep = True
review_queue = True
reviewer_review_action = True
cinder_action = DECISION_ACTIONS.AMO_CHANGE_PENDING_REJECTION_DATE
# 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})
Expand Down
119 changes: 85 additions & 34 deletions src/olympia/reviewers/forms.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import zipfile
from datetime import timedelta
from datetime import datetime, timedelta

from django import forms
from django.conf import settings
Expand All @@ -19,7 +19,6 @@
import olympia.core.logger
from olympia import amo, ratings
from olympia.abuse.models import CinderJob, CinderPolicy
from olympia.access import acl
from olympia.amo.forms import AMOModelForm
from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT
from olympia.files.utils import SafeZip
Expand Down Expand Up @@ -175,7 +174,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
Expand Down Expand Up @@ -327,8 +326,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:
Expand All @@ -344,6 +344,28 @@ 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 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)


class ReviewForm(forms.Form):
# Hack to restore behavior from pre Django 1.10 times.
# Django 1.10 enabled `required` rendering for required widgets. That
Expand All @@ -370,35 +392,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=DelayedRejectionDateWidget,
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:',
Expand Down Expand Up @@ -471,13 +487,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', ())
Expand All @@ -497,8 +514,37 @@ 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.'),
)

# 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):
if self.cleaned_data.get('delayed_rejection_date'):
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'
)
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:
Expand All @@ -507,18 +553,23 @@ def clean_version_pk(self):
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)
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
Expand Down
24 changes: 11 additions & 13 deletions src/olympia/reviewers/templates/reviewers/review.html
Original file line number Diff line number Diff line change
Expand Up @@ -246,21 +246,19 @@ <h3 id="history">
{{ form.operating_systems.errors }}
{{ form.applications.errors }}
</div>
<div class="review-actions-section data-toggle review-delayed-rejection"
data-value="{{ actions_delayable|join(' ') }}">
<div class="review-delayed-rejection-inner">
{{ form.delayed_rejection }}
{{ form.delayed_rejection.errors }}

<div class="review-delayed-rejection-deadline">
{{ form.delayed_rejection_days }}
<label for="{{ form.delayed_rejection_days.auto_id }}">
{{ form.delayed_rejection_days.label }}
</label>
{{ form.delayed_rejection_days.errors }}
{% if form.delayed_rejection and form.delayed_rejection_date %}
<div class="review-actions-section data-toggle review-delayed-rejection"
data-value="{{ actions_delayable|join(' ') }}">
<div class="review-delayed-rejection-inner">
{{ form.delayed_rejection }}
<div class="review-delayed-rejection-deadline">
{{ form.delayed_rejection_date }} UTC
{{ form.delayed_rejection_date.errors }}
</div>
{{ form.delayed_rejection.errors }}
</div>
</div>
</div>
{% endif %}
{% if switch_is_active('enable-activity-log-attachments') %}
<div class="review-actions-section data-toggle review-actions-attachment-reply"
data-value="{{ actions_attachments|join(' ') }}"
Expand Down
Loading
Loading