Merged in [12706] from rjsparks@nostrum.com:

Restrict editing ReviewSettings.skip_next to team secretaries and the secretariat. Improve validation of skip_next value. Fixes #2149.
 - Legacy-Id: 12710
Note: SVN reference [12706] has been migrated to Git commit 518f7a1d65

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()
 

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)