From 518f7a1d6583faa97880ed679276a5a4feb6ded0 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Thu, 19 Jan 2017 19:17:29 +0000 Subject: [PATCH] Restrict editing ReviewSettings.skip_next to team secretaries and the secretariat. Improve validation of skip_next value. Fixes #2149. Commit ready for merge. - Legacy-Id: 12706 --- ietf/group/tests_review.py | 31 +++++++++++++++++++++++++++++-- ietf/group/views_review.py | 20 ++++++++++++++++++-- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 7cda95a12..79316b2e9 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -373,14 +373,13 @@ class ReviewTests(TestCase): "action": "change_settings", "min_interval": "7", "filter_re": "test-[regexp]", - "skip_next": "2", "remind_days_before_deadline": "6" }) self.assertEqual(r.status_code, 302) settings = ReviewerSettings.objects.get(person=reviewer, team=review_req.team) self.assertEqual(settings.min_interval, 7) self.assertEqual(settings.filter_re, "test-[regexp]") - self.assertEqual(settings.skip_next, 2) + self.assertEqual(settings.skip_next, 0) self.assertEqual(settings.remind_days_before_deadline, 6) self.assertEqual(len(outbox), 1) self.assertTrue("reviewer availability" in outbox[0]["subject"].lower()) @@ -388,6 +387,18 @@ class ReviewTests(TestCase): self.assertTrue("frequency changed", msg_content) self.assertTrue("skip next", msg_content) + # Normal reviewer should not be able to change skip_next + r = self.client.post(url, { + "action": "change_settings", + "min_interval": "7", + "filter_re": "test-[regexp]", + "remind_days_before_deadline": "6", + "skip_next" : "2", + }) + self.assertEqual(r.status_code, 302) + settings = ReviewerSettings.objects.get(person=reviewer, team=review_req.team) + self.assertEqual(settings.skip_next, 0) + # add unavailable period start_date = datetime.date.today() + datetime.timedelta(days=10) empty_outbox() @@ -435,6 +446,22 @@ class ReviewTests(TestCase): self.assertTrue(start_date.isoformat(), msg_content) self.assertTrue(end_date.isoformat(), msg_content) + # secretaries and the secretariat should be able to change skip_next + for username in ["secretary","reviewsecretary"]: + skip_next_val = {'secretary':'3','reviewsecretary':'4'}[username] + self.client.login(username=username,password=username+"+password") + r = self.client.post(url, { + "action": "change_settings", + "min_interval": "7", + "filter_re": "test-[regexp]", + "remind_days_before_deadline": "6", + "skip_next" : skip_next_val, + }) + self.assertEqual(r.status_code, 302) + settings = ReviewerSettings.objects.get(person=reviewer, team=review_req.team) + self.assertEqual(settings.skip_next, int(skip_next_val)) + + def test_change_review_secretary_settings(self): doc = make_test_data() diff --git a/ietf/group/views_review.py b/ietf/group/views_review.py index ec319c1a0..bdd600e37 100644 --- a/ietf/group/views_review.py +++ b/ietf/group/views_review.py @@ -448,6 +448,18 @@ class ReviewerSettingsForm(forms.ModelForm): model = ReviewerSettings fields = ['min_interval', 'filter_re', 'skip_next', 'remind_days_before_deadline'] + def __init__(self, *args, **kwargs): + exclude_fields = kwargs.pop('exclude_fields', []) + super(ReviewerSettingsForm, self).__init__(*args, **kwargs) + for field_name in exclude_fields: + self.fields.pop(field_name) + + def clean_skip_next(self): + skip_next = self.cleaned_data.get('skip_next') + if skip_next < 0: + raise forms.ValidationError("Skip next must not be negative") + return skip_next + class AddUnavailablePeriodForm(forms.ModelForm): class Meta: model = UnavailablePeriod @@ -496,6 +508,10 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None): or can_manage_review_requests_for_team(request.user, group)): return HttpResponseForbidden("You do not have permission to perform this action") + exclude_fields = [] + if not can_manage_review_requests_for_team(request.user, group): + exclude_fields.append('skip_next') + settings = ReviewerSettings.objects.filter(person=reviewer, team=group).first() if not settings: settings = ReviewerSettings(person=reviewer, team=group) @@ -513,7 +529,7 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None): if request.method == "POST" and request.POST.get("action") == "change_settings": prev_min_interval = settings.get_min_interval_display() prev_skip_next = settings.skip_next - settings_form = ReviewerSettingsForm(request.POST, instance=settings) + settings_form = ReviewerSettingsForm(request.POST, instance=settings, exclude_fields=exclude_fields) if settings_form.is_valid(): settings = settings_form.save() @@ -528,7 +544,7 @@ def change_reviewer_settings(request, acronym, reviewer_email, group_type=None): return HttpResponseRedirect(back_url) else: - settings_form = ReviewerSettingsForm(instance=settings) + settings_form = ReviewerSettingsForm(instance=settings,exclude_fields=exclude_fields) # periods unavailable_periods = unavailable_periods_to_list().filter(person=reviewer, team=group)