Fix ietf-tools#2119 - Allow specifying review type for suggested reviews in LC and telechat

If a review is suggested on the "manage unassigned reviews" page, and
the document is in both last call and telechat, the assign form now asks
for the type of review that should be assigned.

This commit also fixes two bugs in this process:
- Comparisons in some cases between strings and integers
  (group/views.py:1485/1487)
- Rejections when assigning suggested reviews, as they could be
  considered a newly opened request due to not having a pk
  (group/views.py:1508)
  
Commit ready for merge.
 - Legacy-Id: 16933

Author: Sasha Romijn

ietf/group/forms.py

@@ -16,6 +16,7 @@
 
 # IETF imports
 from ietf.group.models import Group, GroupHistory, GroupStateName
+from ietf.name.models import ReviewTypeName
 from ietf.person.fields import SearchableEmailsField, PersonEmailChoiceField
 from ietf.person.models import Person
 from ietf.review.models import ReviewerSettings, UnavailablePeriod, ReviewSecretarySettings
@@ -234,6 +235,7 @@ class ManageReviewRequestForm(forms.Form):
     close = forms.ModelChoiceField(queryset=close_review_request_states(), required=False)
     close_comment = forms.CharField(max_length=255, required=False)
     reviewer = PersonEmailChoiceField(empty_label="(None)", required=False, label_with="person")
+    review_type = forms.ModelChoiceField(queryset=ReviewTypeName.objects.filter(slug__in=['telechat', 'lc']), required=True)
     add_skip = forms.BooleanField(required=False)
 
     def __init__(self, review_req, *args, **kwargs):
@@ -262,6 +264,9 @@ def __init__(self, review_req, *args, **kwargs):
         setup_reviewer_field(self.fields["reviewer"], review_req)
         self.fields["reviewer"].widget.attrs["class"] = "form-control input-sm"
 
+        if not getattr(review_req, 'in_lc_and_telechat', False):
+            del self.fields["review_type"]
+
         if self.is_bound:
             if self.data.get("action") == "close":
                 self.fields["close"].required = True

ietf/group/tests_review.py

@@ -13,7 +13,7 @@
 from django.urls import reverse as urlreverse
 
 from ietf.utils.test_utils import login_testing_unauthorized, TestCase, reload_db_objects
-from ietf.doc.models import TelechatDocEvent
+from ietf.doc.models import TelechatDocEvent, LastCallDocEvent, State
 from ietf.group.models import Role
 from ietf.iesg.models import TelechatDate
 from ietf.person.models import Person
@@ -26,7 +26,8 @@
     reviewer_rotation_list,
     email_unavaibility_period_ending_reminder, email_reminder_all_open_reviews,
     email_review_reminder_overdue_assignment, email_reminder_unconfirmed_assignments)
-from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName
+from ietf.name.models import ReviewResultName, ReviewRequestStateName, ReviewAssignmentStateName, \
+    ReviewTypeName
 import ietf.group.views
 from ietf.utils.mail import outbox, empty_outbox
 from ietf.dbtemplate.factories import DBTemplateFactory
@@ -91,6 +92,7 @@ def test_suggested_review_requests(self):
         self.assertEqual(len(suggestions), 1)
         self.assertEqual(suggestions[0].doc, doc)
         self.assertEqual(suggestions[0].team, team)
+        self.assertFalse(getattr(suggestions[0], 'in_lc_and_telechat', None))
 
         # blocked by non-versioned refusal
         review_req.requested_rev = ""
@@ -120,6 +122,35 @@ def test_suggested_review_requests(self):
 
         self.assertEqual(len(suggested_review_requests_for_team(team)), 1)
 
+    def test_suggested_review_requests_on_lc_and_telechat(self):
+        review_req = ReviewRequestFactory(state_id='assigned')
+        doc = review_req.doc
+        team = review_req.team
+
+        # put on telechat
+        TelechatDocEvent.objects.create(
+            type="scheduled_for_telechat",
+            by=Person.objects.get(name="(System)"),
+            doc=doc,
+            rev=doc.rev,
+            telechat_date=TelechatDate.objects.all().first().date,
+        )
+
+        # Put on last call as well
+        doc.states.add(State.objects.get(type="draft-iesg", slug="lc", used=True))
+        LastCallDocEvent.objects.create(
+            doc=doc,
+            expires=datetime.datetime.now() + datetime.timedelta(days=365),
+            by=Person.objects.get(name="(System)"),
+            rev=doc.rev
+        )
+
+        suggestions = suggested_review_requests_for_team(team)
+        self.assertEqual(len(suggestions), 1)
+        self.assertEqual(suggestions[0].doc, doc)
+        self.assertEqual(suggestions[0].team, team)
+        self.assertTrue(suggestions[0].in_lc_and_telechat)
+
     def test_reviewer_overview(self):
         team = ReviewTeamFactory()
         reviewer = RoleFactory(name_id='reviewer',group=team,person__user__username='reviewer').person
@@ -228,7 +259,9 @@ def test_manage_review_requests(self):
             "r{}-action".format(review_req3.pk): "assign",
             "r{}-reviewer".format(review_req3.pk): another_reviewer.email_set.first().pk,
             "r{}-add_skip".format(review_req3.pk): 1,
-            
+            # Should be ignored, setting review_type only applies to suggested reviews
+            "r{}-review_type".format(review_req3.pk): ReviewTypeName.objects.get(slug='telechat').pk,
+
             "action": "save",
         })
         self.assertEqual(r.status_code, 302)
@@ -237,7 +270,67 @@ def test_manage_review_requests(self):
         settings = ReviewerSettings.objects.filter(team=review_req3.team, person=another_reviewer).first()
         self.assertEqual(settings.skip_next,1)
         self.assertEqual(review_req3.state_id, "assigned")
+        self.assertEqual(review_req3.type_id, "lc")
+
+    def test_manage_review_requests_assign_suggested_reviews(self):
+        doc = DocumentFactory()
+        team = ReviewTeamFactory()
 
+        # put on telechat
+        TelechatDocEvent.objects.create(
+            type="scheduled_for_telechat",
+            by=Person.objects.get(name="(System)"),
+            doc=doc,
+            rev=doc.rev,
+            telechat_date=TelechatDate.objects.all().first().date,
+        )
+
+        # Put on last call as well
+        doc.states.add(State.objects.get(type="draft-iesg", slug="lc", used=True))
+        LastCallDocEvent.objects.create(
+            doc=doc,
+            expires=datetime.datetime.now() + datetime.timedelta(days=365),
+            by=Person.objects.get(name="(System)"),
+            rev=doc.rev
+        )
+
+        reviewer = RoleFactory(name_id='reviewer', group=team, person__user__username='reviewer')
+        unassigned_url = urlreverse(ietf.group.views.manage_review_requests, kwargs={ 'acronym': team.acronym, 'group_type': team.type_id, "assignment_status": "unassigned" })
+
+        login_testing_unauthorized(self, "secretary", unassigned_url)
+
+        # get
+        r = self.client.get(unassigned_url)
+        self.assertEqual(r.status_code, 200)
+        self.assertContains(r, doc.name)
+
+        # Submit as lc review
+        r = self.client.post(unassigned_url, {
+            "rlc-{}-action".format(doc.name): "assign",
+            "rlc-{}-review_type".format(doc.name): ReviewTypeName.objects.get(slug='lc').pk,
+            "rlc-{}-reviewer".format(doc.name): reviewer.person.email_set.first().pk,
+
+            "action": "save",
+        })
+        self.assertEqual(r.status_code, 302)
+        review_request = doc.reviewrequest_set.get()
+        self.assertEqual(review_request.type_id, 'lc')
+        
+        # Clean up and try again as telechat
+        review_request.reviewassignment_set.all().delete()
+        review_request.delete()
+        
+        r = self.client.post(unassigned_url, {
+            "rlc-{}-action".format(doc.name): "assign",
+            "rlc-{}-review_type".format(doc.name): ReviewTypeName.objects.get(slug='telechat').pk,
+            "rlc-{}-reviewer".format(doc.name): reviewer.person.email_set.first().pk,
+
+            "action": "save",
+        })
+        self.assertEqual(r.status_code, 302)
+        review_request = doc.reviewrequest_set.get()
+        self.assertEqual(review_request.type_id, 'telechat')
+        
     def test_email_open_review_assignments(self):
         review_req1 = ReviewRequestFactory()
         review_assignment_completed = ReviewAssignmentFactory(review_request=review_req1,reviewer=EmailFactory(person__user__username='marschairman'), state_id='completed', reviewed_rev=0)

ietf/group/views.py

@@ -1482,9 +1482,9 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status=
             for a in r.reviewassignment_set.all():
                 if l and rev:
                     if r.doc_id == l[0].doc_id and a.reviewed_rev:
-                        if int(a.reviewed_rev) > rev:
+                        if int(a.reviewed_rev) > int(rev):
                             l = [r]
-                        elif int(a.reviewed_rev) == rev:
+                        elif int(a.reviewed_rev) == int(rev):
                             l.append(r)
                 else:
                     l = [r]
@@ -1505,7 +1505,7 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status=
         saving = form_action.startswith("save")
 
         # check for conflicts
-        review_requests_dict = { six.text_type(r.pk): r for r in review_requests }
+        review_requests_dict = { six.text_type(r.pk): r for r in review_requests if r.pk}
         posted_reqs = set(request.POST.getlist("reviewrequest", []))
         current_reqs = set(review_requests_dict.keys())
 
@@ -1530,6 +1530,8 @@ def manage_review_requests(request, acronym, group_type=None, assignment_status=
                     close_review_request(request, review_req, review_req.form.cleaned_data["close"],
                                          review_req.form.cleaned_data["close_comment"])
                 elif action=="assign":
+                    if review_req.form.cleaned_data.get("review_type"):
+                        review_req.type = review_req.form.cleaned_data.get("review_type")
                     reqs_to_assign.append(review_req)
 
             assignments_by_person = dict()

ietf/review/utils.py

@@ -605,7 +605,6 @@ def suggested_review_requests_for_team(team):
                 requested_by=system_person,
                 state=requested_state,
             )
-
             seen_deadlines[doc.pk] = deadline
 
 
@@ -639,16 +638,20 @@ def suggested_review_requests_for_team(team):
 
             if not deadline or deadline > seen_deadlines.get(doc_pk, datetime.date.max):
                 continue
-
-            requests[doc_pk] = ReviewRequest(
-                time=event_time,
-                type=telechat_type,
-                doc_id=doc_pk,
-                team=team,
-                deadline=deadline,
-                requested_by=system_person,
-                state=requested_state,
-            )
+                
+            if doc_pk in requests:
+                # Document was already added in last call, i.e. it is both in last call and telechat
+                requests[doc_pk].in_lc_and_telechat = True
+            else:
+                requests[doc_pk] = ReviewRequest(
+                    time=event_time,
+                    type=telechat_type,
+                    doc_id=doc_pk,
+                    team=team,
+                    deadline=deadline,
+                    requested_by=system_person,
+                    state=requested_state,
+                )
 
             seen_deadlines[doc_pk] = deadline
 

ietf/templates/group/manage_review_requests.html

@@ -37,7 +37,7 @@ <h1>Manage {{ assignment_status }} open review requests for {{ group.acronym }}<
 
             <h3 class="panel-title">
               <span class="pull-right">
-                {{ r.type.name }}
+                {% if r.in_lc_and_telechat %}Last Call and telechat{% else %}{{ r.type.name }}{% endif %}
                 - deadline {{ r.deadline|date:"Y-m-d" }}
                 {% if r.due %}<span class="label label-warning">{{ r.due }} day{{ r.due|pluralize }}</span>{% endif %}
               </span>
@@ -134,6 +134,10 @@ <h3 class="panel-title">
             {{ r.form.action }}
 
             <span class="reviewer-controls form-inline">
+              {% if r.form.review_type %}
+                <label for="{{ r.form.review_type.id_for_label }}">Review Type:</label>
+                {{ r.form.review_type }}
+              {% endif %}
               <label for="{{ r.form.reviewer.id_for_label }}">Assign:</label>
               {{ r.form.reviewer }} 
               {{ r.form.add_skip }} <label for="{{ r.form.add_skip.id_for_label }}">Skip next time</label>