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

    Remove some pre-ReviewAssignment refactor logic, and simplify what remains while still allowing a group secretary to not assign a row on the unassigned requests form after touching the control to assign a reviewer. Fixes ietf-tools#2812.
 - Legacy-Id: 16900
Note: SVN reference [16854] has been migrated to Git commit b158807

Author: levkowetz

ietf/doc/views_review.py

@@ -281,7 +281,7 @@ def close_request(request, name, request_id):
 
 
 class AssignReviewerForm(forms.Form):
-    reviewer = PersonEmailChoiceField(label="Assign Additional Reviewer", empty_label="(None)", required=False)
+    reviewer = PersonEmailChoiceField(label="Assign Additional Reviewer", empty_label="(None)")
     add_skip = forms.BooleanField(label='Skip next time', required=False)
 
     def __init__(self, review_req, *args, **kwargs):

ietf/group/views.py

@@ -1525,16 +1525,15 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status=
                 if action=="close":
                     close_review_request(request, review_req, review_req.form.cleaned_data["close"],
                                          review_req.form.cleaned_data["close_comment"])
-                elif action=="assign":
+                elif action=="assign" and review_req.form.cleaned_data["reviewer"]:
                     reqs_to_assign.append(review_req)
 
             assignments_by_person = dict()
             for r in reqs_to_assign:
-                if r.form.cleaned_data["reviewer"]:
-                    person = r.form.cleaned_data["reviewer"].person
-                    if not person in assignments_by_person:
-                        assignments_by_person[person] = []
-                    assignments_by_person[person].append(r)
+                person = r.form.cleaned_data["reviewer"].person
+                if not person in assignments_by_person:
+                    assignments_by_person[person] = []
+                assignments_by_person[person].append(r)
 
             # Make sure the any assignments to the person at the head
             # of the rotation queue are processed first so that the queue

ietf/review/utils.py

@@ -31,6 +31,7 @@
                                 ReviewSecretarySettings, ReviewTeamSettings)
 from ietf.utils.mail import send_mail
 from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs
+from ietf.utils import log
 
 # The origin date is used to have a single reference date for "every X days".
 # This date is arbitrarily chosen and has no special meaning, but should be consistent.
@@ -407,6 +408,10 @@ def email_reviewer_availability_change(request, team, reviewer_role, msg, by):
 
 def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=False):
     assert review_req.state_id in ("requested", "assigned")
+    # In the original implementation, review unassignments could be made on formsets by setting reviewers to None.
+    # After refactoring to explicitly model ReviewAssignments, this no longer makes sense. Unassignment is now done
+    # with a different view on a ReviewAssignment.
+    log.assertion('reviewer is not None')
 
     if review_req.reviewassignment_set.filter(reviewer=reviewer).exists():
         return
@@ -419,8 +424,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
 
     assignment = review_req.reviewassignment_set.create(state_id='assigned', reviewer = reviewer, assigned_on = datetime.datetime.now())
 
-    if reviewer:
-        possibly_advance_next_reviewer_for_team(review_req.team, reviewer.person_id, add_skip)
+    possibly_advance_next_reviewer_for_team(review_req.team, reviewer.person_id, add_skip)
 
     ReviewRequestDocEvent.objects.create(
         type="assigned_review_request",
@@ -430,7 +434,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
         desc="Request for {} review by {} is assigned to {}".format(
             review_req.type.name,
             review_req.team.acronym.upper(),
-            reviewer.person if reviewer else "(None)",
+            reviewer.person,
         ),
         review_request=review_req,
         state_id='assigned',
@@ -444,7 +448,7 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
         desc="Request for {} review by {} is assigned to {}".format(
             review_req.type.name,
             review_req.team.acronym.upper(),
-            reviewer.person if reviewer else "(None)",
+            reviewer.person,
         ),
         review_assignment=assignment,
         state_id='assigned',