From ee4bc0cb0773f70de7f2631ce07b0d52f51f49ec Mon Sep 17 00:00:00 2001 From: Sasha Romijn Date: Mon, 28 Oct 2019 11:43:48 +0000 Subject: [PATCH] Fix #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 --- ietf/group/forms.py | 5 + ietf/group/tests_review.py | 99 ++++++++++++++++++- ietf/group/views.py | 8 +- ietf/review/utils.py | 25 ++--- .../group/manage_review_requests.html | 6 +- 5 files changed, 125 insertions(+), 18 deletions(-) diff --git a/ietf/group/forms.py b/ietf/group/forms.py index b0c0410af..f95f5f94c 100644 --- a/ietf/group/forms.py +++ b/ietf/group/forms.py @@ -16,6 +16,7 @@ from django.utils.html import mark_safe # 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 @@ class ManageReviewRequestForm(forms.Form): 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 diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py index 7e7c55cae..d729ca2ff 100644 --- a/ietf/group/tests_review.py +++ b/ietf/group/tests_review.py @@ -13,7 +13,7 @@ from pyquery import PyQuery 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 @@ from ietf.review.utils import ( 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): 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 @@ class ReviewTests(TestCase): "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 @@ class ReviewTests(TestCase): 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) diff --git a/ietf/group/views.py b/ietf/group/views.py index 1d5f1f090..f8f1a5d66 100644 --- a/ietf/group/views.py +++ b/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() diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 705b166f0..818656429 100644 --- a/ietf/review/utils.py +++ b/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 diff --git a/ietf/templates/group/manage_review_requests.html b/ietf/templates/group/manage_review_requests.html index 1a2cd83c6..facf1a460 100644 --- a/ietf/templates/group/manage_review_requests.html +++ b/ietf/templates/group/manage_review_requests.html @@ -37,7 +37,7 @@

- {{ 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 %}{{ r.due }} day{{ r.due|pluralize }}{% endif %} @@ -134,6 +134,10 @@ {{ r.form.action }} + {% if r.form.review_type %} + + {{ r.form.review_type }} + {% endif %} {{ r.form.reviewer }} {{ r.form.add_skip }}