Skip to content

Commit

Permalink
Allow regular reviewers to delay-reject again without changing the da…
Browse files Browse the repository at this point in the history
…te (#23064)

* Allow regular reviewers to delay-reject again without changing the date

They should be able to see the widget, just not customize the value.
  • Loading branch information
diox committed Feb 10, 2025
1 parent d8ac069 commit af29d9a
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 20 deletions.
22 changes: 16 additions & 6 deletions src/olympia/reviewers/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +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.forms import AMOModelForm
from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT
from olympia.files.utils import SafeZip
Expand Down Expand Up @@ -573,18 +574,27 @@ def __init__(self, *args, **kw):
self.helper = kw.pop('helper')
super().__init__(*args, **kw)
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(
now = datetime.now()
self.fields['delayed_rejection_date'].initial = now + timedelta(
days=REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT, hours=1
)
# We enforce a reasonable min value on the widget.
self.min_rejection_date = now + timedelta(days=1)
delayed_rejection_date_widget_attrs = {
'min': self.min_rejection_date.isoformat()[:16],
}
if not acl.action_allowed_for(
self.helper.handler.user, amo.permissions.REVIEWS_ADMIN
):
# Non-admin reviewers can't customize the date.
delayed_rejection_date_widget_attrs['readonly'] = 'readonly'
self.fields['delayed_rejection_date'].widget.attrs.update(
delayed_rejection_date_widget_attrs
)
else:
# No delayable action available, remove the fields entirely.
del self.fields['delayed_rejection_date']
Expand Down
41 changes: 35 additions & 6 deletions src/olympia/reviewers/tests/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,7 @@ def test_reasons_required_for_reject_multiple_versions(self):
'action': 'reject_multiple_versions',
'comments': 'lol',
'versions': self.addon.versions.all(),
'delayed_rejection': 'False',
}
)
assert form.is_bound
Expand Down Expand Up @@ -1001,20 +1002,48 @@ def test_versions_required(self):
canned_response='reason 1',
)
],
'delayed_rejection': 'False',
}
)
form.helper.actions['reject_multiple_versions']['versions'] = True
assert form.is_bound
assert not form.is_valid()
assert form.errors == {'versions': ['This field is required.']}

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.
@freeze_time('2025-02-10 12:09')
def test_delayed_rejection_date_is_readonly_for_regular_reviewers(self):
# Regular reviewers can't customize the delayed rejection period.
self.grant_permission(self.request.user, 'Addons:Review')
form = self.get_form()
assert 'delayed_rejection_date' not in form.fields
assert 'delayed_rejection' not in form.fields
assert 'delayed_rejection_date' in form.fields
assert 'delayed_rejection' in form.fields
assert form.fields['delayed_rejection_date'].widget.attrs == {
'min': '2025-02-11T12:09',
'readonly': 'readonly',
}
assert form.fields['delayed_rejection_date'].initial == datetime(
2025, 2, 24, 13, 9
)
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_or_clear_pending_rejection_multiple_versions'
)

@freeze_time('2025-01-23 12:52')
def test_delayed_rejection_days_shows_up_for_admin_reviewers(self):
Expand All @@ -1025,7 +1054,7 @@ def test_delayed_rejection_days_shows_up_for_admin_reviewers(self):
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'
'min': '2025-01-24T12:52',
}
assert form.fields['delayed_rejection_date'].initial == datetime(
2025, 2, 6, 13, 52
Expand Down
19 changes: 13 additions & 6 deletions src/olympia/reviewers/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4541,6 +4541,7 @@ def test_reject_multiple_versions(self):
'comments': 'multireject!',
'reasons': [reason.id],
'versions': [old_version.pk, self.version.pk],
'delayed_rejection': 'False',
},
)

Expand Down Expand Up @@ -5229,12 +5230,16 @@ def test_data_value_attributes(self):
' '
) == ['reject_multiple_versions', 'reply']

assert doc('.data-toggle.review-delayed-rejection')[0].attrib[
'data-value'
].split(' ') == [
'reject_multiple_versions',
]

# We don't have approve/reject actions so these have an empty
# data-value.
assert doc('.data-toggle.review-files')[0].attrib['data-value'] == ''
assert doc('.data-toggle.review-tested')[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_test_data_value_attributes_admin(self):
AutoApprovalSummary.objects.create(
Expand Down Expand Up @@ -5300,7 +5305,6 @@ def test_test_data_value_attributes_admin(self):
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'
].split(' ') == [
Expand Down Expand Up @@ -5365,8 +5369,11 @@ def test_data_value_attributes_unlisted(self):
assert doc('.data-toggle.review-files')[0].attrib['data-value'] == ''
assert doc('.data-toggle.review-tested')[0].attrib['data-value'] == ''

# Regular reviewer won't see delayed rejection inputs, need an admin.
assert not doc('.data-toggle.review-delayed-rejection')
assert doc('.data-toggle.review-delayed-rejection')[0].attrib[
'data-value'
].split(' ') == [
'reject_multiple_versions',
]

def test_no_data_value_attributes_unlisted_for_viewer(self):
self.version.update(channel=amo.CHANNEL_UNLISTED)
Expand All @@ -5390,7 +5397,7 @@ def test_no_data_value_attributes_unlisted_for_viewer(self):
assert doc('.data-toggle.review-files')[0].attrib['data-value'] == ''
assert doc('.data-toggle.review-tested')[0].attrib['data-value'] == ''

# Regular reviewer won't see delayed rejection inputs, need an admin.
# Viewer won't see delayed rejection inputs, need a reviewer.
assert not doc('.data-toggle.review-delayed-rejection')

def test_data_value_attributes_unreviewed(self):
Expand Down
4 changes: 2 additions & 2 deletions src/olympia/reviewers/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -620,13 +620,13 @@ def get_actions(self):
'method': self.handler.reject_multiple_versions,
'label': 'Reject Multiple Versions',
'minimal': True,
'delayable': is_appropriate_admin_reviewer,
'delayable': True,
'multiple_versions': True,
'details': (
'This will reject the selected versions. '
'The comments will be sent to the developer.'
),
'available': (can_reject_multiple),
'available': can_reject_multiple,
'allows_reasons': True,
'resolves_cinder_jobs': True,
'requires_reasons': not is_static_theme,
Expand Down

0 comments on commit af29d9a

Please sign in to comment.