From c718fedb6c2d10ce3160226d53ac4267934b3252 Mon Sep 17 00:00:00 2001
From: Paul Selkirk <paul@painless-security.com>
Date: Wed, 27 Sep 2023 09:57:34 -0400
Subject: [PATCH] feat: Allow review rejections to be undone (#6312)
* feat: Allow reviewer to accept a review they're previously rejected
* feat: Add a reviewer who has previously rejected a review to the list of suggested reviewers.
This largely un-does d105f8b, at the request of at least one team secretary.
* fix: Went a little overboard on the previous commit
one_assignment still has to exclude reviewers who rejected the assignment,
or they could end up being the suggested reviewer.
* fix: Actually do the assignment
* fix: If there's an existing assignment, don't create a new one
* style: Restructure conditional for clarity
* test: Add test cases for accepting or assigning a review assignment after rejecting it
---
ietf/doc/tests_review.py | 42 ++++++++++++++++++++-
ietf/doc/views_review.py | 2 +-
ietf/group/tests_review.py | 18 ++++++++-
ietf/review/policies.py | 22 +++++++----
ietf/review/tests_policies.py | 5 +--
ietf/review/utils.py | 6 ++-
ietf/templates/doc/review/request_info.html | 2 +
7 files changed, 80 insertions(+), 17 deletions(-)
diff --git a/ietf/doc/tests_review.py b/ietf/doc/tests_review.py
index 6dd661055..33a53178a 100644
--- a/ietf/doc/tests_review.py
+++ b/ietf/doc/tests_review.py
@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2016-2020, All Rights Reserved
+# Copyright The IETF Trust 2016-2023, All Rights Reserved
# -*- coding: utf-8 -*-
@@ -355,6 +355,23 @@ class ReviewTests(TestCase):
request_events = review_req.reviewrequestdocevent_set.all()
self.assertEqual(request_events.count(), 0)
+ def test_assign_reviewer_after_reject(self):
+ doc = WgDraftFactory()
+ review_team = ReviewTeamFactory()
+ rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',name_id='reviewer')
+ reviewer_email = Email.objects.get(person__user__username="reviewer")
+ RoleFactory(group=review_team,person__user__username='reviewsecretary',name_id='secr')
+ review_req = ReviewRequestFactory(team=review_team,doc=doc)
+ ReviewAssignmentFactory(review_request=review_req, state_id='rejected', reviewer=rev_role.person.email_set.first())
+
+ url = urlreverse('ietf.doc.views_review.assign_reviewer', kwargs={ "name": doc.name, "request_id": review_req.pk })
+ login_testing_unauthorized(self, "reviewsecretary", url)
+ r = self.client.get(url)
+ self.assertEqual(r.status_code, 200)
+ q = PyQuery(r.content)
+ reviewer_label = q("option[value=\"{}\"]".format(reviewer_email.address)).text().lower()
+ self.assertIn("rejected review of document before", reviewer_label)
+
def test_previously_reviewed_replaced_doc(self):
review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',person__name='Some Reviewer',name_id='reviewer')
@@ -569,6 +586,29 @@ class ReviewTests(TestCase):
self.assertContains(r, '<button type="submit"')
+ def test_accept_reviewer_assignment_after_reject(self):
+ doc = WgDraftFactory()
+ review_team = ReviewTeamFactory()
+ rev_role = RoleFactory(group=review_team,name_id='reviewer')
+ review_req = ReviewRequestFactory(doc=doc,team=review_team)
+ assignment = ReviewAssignmentFactory(review_request=review_req, state_id='rejected', reviewer=rev_role.person.email_set.first())
+
+ url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
+ username = assignment.reviewer.person.user.username
+ self.client.login(username=username, password=username + "+password")
+ r = self.client.get(url)
+ self.assertEqual(r.status_code, 200)
+ q = PyQuery(r.content)
+ d = q('.reviewer-assignment-not-accepted')
+ self.assertTrue(d("[name=action][value=accept]"))
+
+ # accept
+ r = self.client.post(url, { "action": "accept" })
+ self.assertEqual(r.status_code, 302)
+
+ assignment = reload_db_objects(assignment)
+ self.assertEqual(assignment.state_id, "accepted")
+
def make_test_mbox_tarball(self, review_req):
mbox_path = os.path.join(self.review_dir, "testmbox.tar.gz")
with tarfile.open(mbox_path, "w:gz") as tar:
diff --git a/ietf/doc/views_review.py b/ietf/doc/views_review.py
index 18ec70fa0..e267d9a46 100644
--- a/ietf/doc/views_review.py
+++ b/ietf/doc/views_review.py
@@ -223,7 +223,7 @@ def review_request(request, name, request_id):
for assignment in assignments:
assignment.is_reviewer = user_is_person(request.user, assignment.reviewer.person)
- assignment.can_accept_reviewer_assignment = (assignment.state_id == "assigned"
+ assignment.can_accept_reviewer_assignment = (assignment.state_id in ["assigned", "rejected"]
and (assignment.is_reviewer or can_manage_request))
assignment.can_reject_reviewer_assignment = (assignment.state_id in ["assigned", "accepted"]
diff --git a/ietf/group/tests_review.py b/ietf/group/tests_review.py
index 60c11689b..a03b806f8 100644
--- a/ietf/group/tests_review.py
+++ b/ietf/group/tests_review.py
@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2016-2020, All Rights Reserved
+# Copyright The IETF Trust 2016-2023, All Rights Reserved
# -*- coding: utf-8 -*-
@@ -711,6 +711,22 @@ class ReviewTests(TestCase):
self.assertEqual(settings.max_items_to_show_in_reviewer_list, 10)
self.assertEqual(settings.days_to_show_in_reviewer_list, 365)
+ def test_assign_reviewer_after_reject(self):
+ team = ReviewTeamFactory()
+ reviewer = RoleFactory(name_id='reviewer', group=team).person
+ ReviewerSettingsFactory(person=reviewer, team=team)
+ review_req = ReviewRequestFactory(team=team)
+ ReviewAssignmentFactory(review_request=review_req, state_id='rejected', reviewer=reviewer.email())
+
+ 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)
+
+ r = self.client.get(unassigned_url)
+ self.assertEqual(r.status_code, 200)
+ q = PyQuery(r.content)
+ reviewer_label = q("option[value=\"{}\"]".format(reviewer.email())).text().lower()
+ self.assertIn("rejected review of document before", reviewer_label)
+
class BulkAssignmentTests(TestCase):
diff --git a/ietf/review/policies.py b/ietf/review/policies.py
index ac3cca283..2b97fda14 100644
--- a/ietf/review/policies.py
+++ b/ietf/review/policies.py
@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2019-2021, All Rights Reserved
+# Copyright The IETF Trust 2019-2023, All Rights Reserved
import re
@@ -10,6 +10,7 @@ from simple_history.utils import bulk_update_with_history
from ietf.doc.models import DocumentAuthor, DocAlias
from ietf.doc.utils import extract_complete_replaces_ancestor_mapping_for_docs
from ietf.group.models import Role
+from ietf.name.models import ReviewAssignmentStateName
from ietf.person.models import Person
import debug # pyflakes:ignore
from ietf.review.models import NextReviewerInTeam, ReviewerSettings, ReviewWish, ReviewRequest, \
@@ -55,8 +56,6 @@ def persons_with_previous_review(team, review_req, possible_person_ids, state_id
reviewassignment__state=state_id,
team=team,
).distinct()
- if review_req.pk is not None:
- has_reviewed_previous = has_reviewed_previous.exclude(pk=review_req.pk)
has_reviewed_previous = set(
has_reviewed_previous.values_list("reviewassignment__reviewer__person", flat=True))
return has_reviewed_previous
@@ -70,7 +69,14 @@ class AbstractReviewerQueuePolicy:
"""Assign a reviewer to a request and update policy state accordingly"""
# Update policy state first - needed by LRU policy to correctly compute whether assignment was in-order
self.update_policy_state_for_assignment(review_req, reviewer.person, add_skip)
- return review_req.reviewassignment_set.create(state_id='assigned', reviewer=reviewer, assigned_on=timezone.now())
+ assignment = review_req.reviewassignment_set.filter(reviewer=reviewer).first()
+ if assignment:
+ assignment.state = ReviewAssignmentStateName.objects.get(slug='assigned', used=True)
+ assignment.assigned_on = timezone.now()
+ assignment.save()
+ return assignment
+ else:
+ return review_req.reviewassignment_set.create(state_id='assigned', reviewer=reviewer, assigned_on=timezone.now())
def default_reviewer_rotation_list(self, include_unavailable=False):
""" Return a list of reviewers (Person objects) in the default reviewer rotation for a policy.
@@ -168,15 +174,15 @@ class AbstractReviewerQueuePolicy:
PersonEmailChoiceField(label="Assign Reviewer", empty_label="(None)")
"""
- # Collect a set of person IDs for people who have either not responded
- # to or outright rejected reviewing this document in the past
+ # Collect a set of person IDs for people who have not responded
+ # to this document in the past
rejecting_reviewer_ids = review_req.doc.reviewrequest_set.filter(
- reviewassignment__state__slug__in=('rejected', 'no-response')
+ reviewassignment__state__slug='no-response'
).values_list(
'reviewassignment__reviewer__person_id', flat=True
)
- # Query the Email objects for reviewers who haven't rejected or
+ # Query the Email objects for reviewers who haven't
# not responded to this document in the past
field.queryset = field.queryset.filter(
role__name="reviewer",
diff --git a/ietf/review/tests_policies.py b/ietf/review/tests_policies.py
index ca687ff86..1eb57a8be 100644
--- a/ietf/review/tests_policies.py
+++ b/ietf/review/tests_policies.py
@@ -1,4 +1,4 @@
-# Copyright The IETF Trust 2016-2021, All Rights Reserved
+# Copyright The IETF Trust 2016-2023, All Rights Reserved
import debug # pyflakes:ignore
import datetime
@@ -471,9 +471,6 @@ class _Wrapper(TestCase):
addresses = list( map( lambda choice: choice[0], field.choices ) )
- self.assertNotIn(
- str(rejected_reviewer.email()), addresses,
- "Reviews should not suggest people who have rejected this request in the past")
self.assertNotIn(
str(no_response_reviewer.email()), addresses,
"Reviews should not suggest people who have not responded to this request in the past.")
diff --git a/ietf/review/utils.py b/ietf/review/utils.py
index 058e8cee9..a91bcbd62 100644
--- a/ietf/review/utils.py
+++ b/ietf/review/utils.py
@@ -388,8 +388,10 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
log.assertion('reviewer is not None')
# cannot reference reviewassignment_set relation until pk exists
- if review_req.pk is not None and review_req.reviewassignment_set.filter(reviewer=reviewer).exists():
- return
+ if review_req.pk is not None:
+ reviewassignment_set = review_req.reviewassignment_set.filter(reviewer=reviewer)
+ if reviewassignment_set.exists() and not reviewassignment_set.filter(state_id='rejected').exists():
+ return
# Note that assigning a review no longer unassigns other reviews
diff --git a/ietf/templates/doc/review/request_info.html b/ietf/templates/doc/review/request_info.html
index 31578f010..5c7ca8907 100644
--- a/ietf/templates/doc/review/request_info.html
+++ b/ietf/templates/doc/review/request_info.html
@@ -177,6 +177,8 @@
<div class="reviewer-assignment-not-accepted">
{% if assignment.state_id == "assigned" %}
Assignment not accepted yet
+ {% elif assignment.state_id == "rejected" %}
+ <span class="text-danger">Assignment rejected</span>
{% else %}
<span class="text-success">Assignment accepted</span>
{% endif %}