Skip to content

Commit 073ce55

Browse files
rjsparksrjsparks
committed
Checkpoint. ReviewTests pass, but others still fail of course. Next step is to address stats.
 - Legacy-Id: 16021
1 parent b85052f commit 073ce55

File tree

11 files changed

+255
-87
lines changed

11 files changed

+255
-87
lines changed

‎ietf/doc/tests_review.py‎

Lines changed: 28 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ def test_doc_page(self):
152152
r = self.client.get(url)
153153
self.assertEqual(r.status_code, 200)
154154
content = unicontent(r)
155+
#debug.show('unicontent(r)')
155156
self.assertTrue("{} Review".format(review_req.type.name) in content)
156157

157158
def test_review_request(self):
@@ -181,8 +182,8 @@ def test_close_request(self):
181182
review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
182183
rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',name_id='reviewer')
183184
RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr')
184-
review_req = ReviewRequestFactory(doc=doc,team=review_team,type_id='early',state_id='accepted',requested_by=rev_role.person,reviewer=rev_role.person.email_set.first(),deadline=datetime.datetime.now()+datetime.timedelta(days=20))
185-
185+
review_req = ReviewRequestFactory(doc=doc,team=review_team,type_id='early',state_id='assigned',requested_by=rev_role.person,deadline=datetime.datetime.now()+datetime.timedelta(days=20))
186+
ReviewAssignmentFactory(review_request=review_req, state_id='accepted', reviewer=rev_role.person.email_set.first())
186187
close_url = urlreverse('ietf.doc.views_review.close_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
187188

188189

@@ -290,7 +291,8 @@ def get_skip_next(person):
290291
settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2])
291292
settings.min_interval = 30
292293
settings.save()
293-
ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="accepted", deadline=today, requested_by=reviewers[0], reviewer=reviewers[2].email_set.first())
294+
req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0])
295+
ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time)
294296
possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk)
295297
self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
296298
self.assertEqual(get_skip_next(reviewers[0]), 0)
@@ -317,17 +319,24 @@ def test_assign_reviewer(self):
317319
doc.save_with_history([DocEvent.objects.create(doc=doc, rev=doc.rev, type="changed_document", by=Person.objects.get(user__username="secretary"), desc="Test")])
318320

319321
# previous review
320-
ReviewRequest.objects.create(
322+
req = ReviewRequestFactory(
321323
time=datetime.datetime.now() - datetime.timedelta(days=100),
322324
requested_by=Person.objects.get(name="(System)"),
323325
doc=doc,
324-
type=ReviewTypeName.objects.get(slug="early"),
326+
type_id='early',
325327
team=review_req.team,
326-
state=ReviewRequestStateName.objects.get(slug="completed"),
327-
result_id='serious-issues',
328-
reviewed_rev="01",
328+
state_id='assigned',
329+
requested_rev="01",
329330
deadline=datetime.date.today() - datetime.timedelta(days=80),
331+
)
332+
ReviewAssignmentFactory(
333+
review_request = req,
334+
state_id='completed',
335+
result_id='serious-issues',
330336
reviewer=reviewer_email,
337+
reviewed_rev="01",
338+
assigned_on=req.time,
339+
completed_on=req.time + datetime.timedelta(days=10),
331340
)
332341

333342
reviewer_settings = ReviewerSettings.objects.get(person__email=reviewer_email, team=review_req.team)
@@ -390,37 +399,25 @@ def test_assign_reviewer(self):
390399
self.assertEqual(r.status_code, 302)
391400

392401
review_req = reload_db_objects(review_req)
393-
self.assertEqual(review_req.state_id, "requested")
394-
self.assertEqual(review_req.reviewer, reviewer)
402+
self.assertEqual(review_req.state_id, "assigned")
403+
self.assertEqual(review_req.reviewassignment_set.count(),1)
404+
assignment = review_req.reviewassignment_set.first()
405+
self.assertEqual(assignment.reviewer, reviewer)
406+
self.assertEqual(assignment.state_id, "requested")
395407
self.assertEqual(len(outbox), 1)
396408
self.assertTrue("assigned" in outbox[0].get_payload(decode=True).decode("utf-8"))
397409
self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
398410

399-
# re-assign
400-
empty_outbox()
401-
review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
402-
review_req.save()
403-
reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[1]).first()
404-
r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk, "add_skip": 1 })
405-
self.assertEqual(r.status_code, 302)
406-
407-
review_req = reload_db_objects(review_req)
408-
self.assertEqual(review_req.state_id, "requested") # check that state is reset
409-
self.assertEqual(review_req.reviewer, reviewer)
410-
self.assertEqual(len(outbox), 2)
411-
self.assertTrue("cancelled your assignment" in outbox[0].get_payload(decode=True).decode("utf-8"))
412-
self.assertTrue("assigned" in outbox[1].get_payload(decode=True).decode("utf-8"))
413-
self.assertEqual(ReviewerSettings.objects.get(person=reviewer.person).skip_next, 1)
414-
415411
def test_accept_reviewer_assignment(self):
416412

417413
doc = WgDraftFactory(group__acronym='mars',rev='01')
418414
review_team = ReviewTeamFactory(acronym="reviewteam", name="Review Team", type_id="review", list_email="reviewteam@ietf.org", parent=Group.objects.get(acronym="farfut"))
419415
rev_role = RoleFactory(group=review_team,person__user__username='reviewer',person__user__email='reviewer@example.com',name_id='reviewer')
420-
review_req = ReviewRequestFactory(doc=doc,team=review_team,type_id='early',state_id='requested',requested_by=rev_role.person,reviewer=rev_role.person.email_set.first(),deadline=datetime.datetime.now()+datetime.timedelta(days=20))
416+
review_req = ReviewRequestFactory(doc=doc,team=review_team,type_id='early',state_id='assigned',requested_by=rev_role.person,deadline=datetime.datetime.now()+datetime.timedelta(days=20))
417+
assignment = ReviewAssignmentFactory(review_request=review_req, state_id='requested', reviewer=rev_role.person.email_set.first())
421418

422419
url = urlreverse('ietf.doc.views_review.review_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
423-
username = review_req.reviewer.person.user.username
420+
username = assignment.reviewer.person.user.username
424421
self.client.login(username=username, password=username + "+password")
425422
r = self.client.get(url)
426423
self.assertEqual(r.status_code, 200)
@@ -431,8 +428,8 @@ def test_accept_reviewer_assignment(self):
431428
r = self.client.post(url, { "action": "accept" })
432429
self.assertEqual(r.status_code, 302)
433430

434-
review_req = reload_db_objects(review_req)
435-
self.assertEqual(review_req.state_id, "accepted")
431+
assignment = reload_db_objects(assignment)
432+
self.assertEqual(assignment.state_id, "accepted")
436433

437434
def test_reject_reviewer_assignment(self):
438435
doc = WgDraftFactory(group__acronym='mars',rev='01')
@@ -682,6 +679,7 @@ def test_complete_review_enter_content(self):
682679

683680
assignment = reload_db_objects(assignment)
684681
self.assertEqual(assignment.state_id, "completed")
682+
self.assertNotEqual(assignment.completed_on, None)
685683

686684
with open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
687685
self.assertEqual(f.read(), "This is a review\nwith two lines")

‎ietf/doc/views_doc.py‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@
6464
from ietf.mailtrigger.utils import gather_relevant_expansions
6565
from ietf.meeting.models import Session
6666
from ietf.meeting.utils import group_sessions, get_upcoming_manageable_sessions, sort_sessions
67-
from ietf.review.models import ReviewRequest
68-
from ietf.review.utils import can_request_review_of_doc, review_requests_to_list_for_docs
67+
from ietf.review.models import ReviewRequest, ReviewAssignment
68+
from ietf.review.utils import can_request_review_of_doc, review_requests_to_list_for_docs, review_assignments_to_list_for_docs
6969
from ietf.review.utils import no_review_from_teams_on_doc
7070
from ietf.utils import markup_txt, log
7171
from ietf.utils.text import maybe_split
@@ -382,7 +382,7 @@ def document_main(request, name, rev=None):
382382
published = doc.latest_event(type="published_rfc")
383383
started_iesg_process = doc.latest_event(type="started_iesg_process")
384384

385-
review_requests = review_requests_to_list_for_docs([doc]).get(doc.pk, [])
385+
review_assignments = review_assignments_to_list_for_docs([doc]).get(doc.pk, [])
386386
no_review_from_teams = no_review_from_teams_on_doc(doc, rev or doc.rev)
387387

388388
return render(request, "doc/document_draft.html",
@@ -446,7 +446,7 @@ def document_main(request, name, rev=None):
446446
search_archive=search_archive,
447447
actions=actions,
448448
presentations=presentations,
449-
review_requests=review_requests,
449+
review_assignments=review_assignments,
450450
no_review_from_teams=no_review_from_teams,
451451
))
452452

@@ -598,12 +598,11 @@ def document_main(request, name, rev=None):
598598
# If we want to go back to using markup_txt.markup_unicode, call it explicitly here like this:
599599
# content = markup_txt.markup_unicode(content, split=False, width=80)
600600

601-
# TODO - verify if this shows other reviews from the same team - I bet it doesn't
602-
review_req = ReviewRequest.objects.filter(reviewassignment__review=doc.name).first()
601+
review_assignment = ReviewAssignment.objects.filter(review=doc.name).first()
603602

604603
other_reviews = []
605-
if review_req:
606-
other_reviews = [r for r in review_requests_to_list_for_docs([review_req.doc]).get(doc.pk, []) if r != review_req]
604+
if review_assignment:
605+
other_reviews = [r for r in review_assignments_to_list_for_docs([review_assignment.review_request.doc]).get(doc.pk, []) if r != review_assignment]
607606

608607
return render(request, "doc/document_review.html",
609608
dict(doc=doc,
@@ -612,7 +611,7 @@ def document_main(request, name, rev=None):
612611
revisions=revisions,
613612
latest_rev=latest_rev,
614613
snapshot=snapshot,
615-
review_req=review_req,
614+
review_req=review_assignment.review_request,
616615
other_reviews=other_reviews,
617616
))
618617

‎ietf/doc/views_review.py‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -186,11 +186,11 @@ def review_request(request, name, request_id):
186186

187187
can_manage_request = can_manage_review_requests_for_team(request.user, review_req.team)
188188

189-
can_close_request = (review_req.state_id in ["requested", "accepted"]
189+
can_close_request = (review_req.state_id in ["requested", "assigned"]
190190
and (is_authorized_in_doc_stream(request.user, doc)
191191
or can_manage_request))
192192

193-
can_assign_reviewer = (review_req.state_id in ["requested", "accepted"]
193+
can_assign_reviewer = (review_req.state_id in ["requested", "assigned"]
194194
and can_manage_request)
195195

196196
can_edit_comment = can_request_review_of_doc(request.user, doc)
@@ -210,6 +210,9 @@ def review_request(request, name, request_id):
210210
assignment.can_complete_review = (assignment.state_id in ["requested", "accepted", "overtaken", "no-response", "part-completed", "completed"]
211211
and (assignment.is_reviewer or can_manage_request))
212212

213+
# This implementation means if a reviewer accepts one assignment for a review_request, he accepts all assigned to him (for that request)
214+
# This problematic - it's a bug (probably) for the same person to have more than one assignment for the same request.
215+
# It is, however unintuitive, and acceptance should be refactored to be something that works on assignments, not requests
213216
if request.method == "POST" and request.POST.get("action") == "accept":
214217
for assignment in assignments:
215218
if assignment.can_accept_reviewer_assignment:
@@ -245,7 +248,7 @@ def __init__(self, can_manage_request, *args, **kwargs):
245248
@login_required
246249
def close_request(request, name, request_id):
247250
doc = get_object_or_404(Document, name=name)
248-
review_req = get_object_or_404(ReviewRequest, pk=request_id, state__in=["requested", "accepted"])
251+
review_req = get_object_or_404(ReviewRequest, pk=request_id, state__in=["requested", "assigned"])
249252

250253
can_request = is_authorized_in_doc_stream(request.user, doc)
251254
can_manage_request = can_manage_review_requests_for_team(request.user, review_req.team)
@@ -265,6 +268,7 @@ def close_request(request, name, request_id):
265268
return render(request, 'doc/review/close_request.html', {
266269
'doc': doc,
267270
'review_req': review_req,
271+
'assignments': review_req.reviewassignment_set.all(),
268272
'form': form,
269273
})
270274

@@ -745,7 +749,7 @@ def edit_deadline(request, name, request_id):
745749
if form.is_valid():
746750
if form.cleaned_data['deadline'] != old_deadline:
747751
form.save()
748-
subject = "Deadline changed: {} {} review of {}-{}".format(review_req.team.acronym.capitalize(),review_req.type.name.lower(), review_req.doc.name, review_req.reviewed_rev)
752+
subject = "Deadline changed: {} {} review of {}-{}".format(review_req.team.acronym.capitalize(),review_req.type.name.lower(), review_req.doc.name, review_req.requested_rev)
749753
msg = render_to_string("review/deadline_changed.txt", {
750754
"review_req": review_req,
751755
"old_deadline": old_deadline,

‎ietf/iesg/agenda.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ def fill_in_agenda_docs(date, sections, docs=None):
156156
docs = docs.select_related("stream", "group").distinct()
157157
fill_in_telechat_date(docs)
158158

159-
review_requests_for_docs = review_requests_to_list_for_docs(docs)
159+
review_assignments_for_docs = review_assignments_to_list_for_docs(docs)
160160

161161
for doc in docs:
162162
if doc.telechat_date() != date:
@@ -182,7 +182,7 @@ def fill_in_agenda_docs(date, sections, docs=None):
182182
if e and (e.consensus != None):
183183
doc.consensus = "Yes" if e.consensus else "No"
184184

185-
doc.review_requests = review_requests_for_docs.get(doc.pk, [])
185+
doc.review_assignments = review_assignments_for_docs.get(doc.pk, [])
186186
elif doc.type_id == "conflrev":
187187
doc.conflictdoc = doc.relateddocument_set.get(relationship__slug='conflrev').target.document
188188
elif doc.type_id == "charter":

‎ietf/review/factories.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ class Meta:
4646
review_request = factory.SubFactory('ietf.review.factories.ReviewRequestFactory')
4747
state_id = 'requested'
4848
reviewer = factory.SubFactory('ietf.person.factories.EmailFactory')
49+
assigned_on = datetime.datetime.now()
4950

5051
class ReviewerSettingsFactory(factory.DjangoModelFactory):
5152
class Meta:

0 commit comments

Comments
 (0)