Checkpoint. ReviewTests pass, but others still fail of course. Next step is to address stats.

- Legacy-Id: 16021

ietf/doc/tests_review.py

@@ -152,6 +152,7 @@ class ReviewTests(TestCase):
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
         content = unicontent(r)
+        #debug.show('unicontent(r)')
         self.assertTrue("{} Review".format(review_req.type.name) in content)
 
     def test_review_request(self):
@@ -181,8 +182,8 @@ class ReviewTests(TestCase):
         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',name_id='reviewer')
         RoleFactory(group=review_team,person__user__username='reviewsecretary',person__user__email='reviewsecretary@example.com',name_id='secr')
-        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))
-
+        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))
+        ReviewAssignmentFactory(review_request=review_req, state_id='accepted', reviewer=rev_role.person.email_set.first())
         close_url = urlreverse('ietf.doc.views_review.close_request', kwargs={ "name": doc.name, "request_id": review_req.pk })
 
 
@@ -290,7 +291,8 @@ class ReviewTests(TestCase):
         settings, _ = ReviewerSettings.objects.get_or_create(team=team, person=reviewers[2])
         settings.min_interval = 30
         settings.save()
-        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())
+        req = ReviewRequest.objects.create(team=team, doc=doc, type_id="early", state_id="assigned", deadline=today, requested_by=reviewers[0])
+        ReviewAssignmentFactory(review_request=req, state_id="accepted", reviewer = reviewers[2].email_set.first(),assigned_on = req.time)
         possibly_advance_next_reviewer_for_team(team, assigned_review_to_person_id=reviewers[3].pk)
         self.assertEqual(NextReviewerInTeam.objects.get(team=team).next_reviewer, reviewers[4])
         self.assertEqual(get_skip_next(reviewers[0]), 0)
@@ -317,17 +319,24 @@ class ReviewTests(TestCase):
         doc.save_with_history([DocEvent.objects.create(doc=doc, rev=doc.rev, type="changed_document", by=Person.objects.get(user__username="secretary"), desc="Test")])
 
         # previous review
-        ReviewRequest.objects.create(
+        req = ReviewRequestFactory(
             time=datetime.datetime.now() - datetime.timedelta(days=100),
             requested_by=Person.objects.get(name="(System)"),
             doc=doc,
-            type=ReviewTypeName.objects.get(slug="early"),
+            type_id='early',
             team=review_req.team,
-            state=ReviewRequestStateName.objects.get(slug="completed"),
-            result_id='serious-issues',
-            reviewed_rev="01",
+            state_id='assigned',
+            requested_rev="01",
             deadline=datetime.date.today() - datetime.timedelta(days=80),
+        )
+        ReviewAssignmentFactory(
+            review_request = req,
+            state_id='completed',
+            result_id='serious-issues',
             reviewer=reviewer_email,
+            reviewed_rev="01",
+            assigned_on=req.time,
+            completed_on=req.time + datetime.timedelta(days=10),
         )
 
         reviewer_settings = ReviewerSettings.objects.get(person__email=reviewer_email, team=review_req.team)
@@ -390,37 +399,25 @@ class ReviewTests(TestCase):
         self.assertEqual(r.status_code, 302)
 
         review_req = reload_db_objects(review_req)
-        self.assertEqual(review_req.state_id, "requested")
-        self.assertEqual(review_req.reviewer, reviewer)
+        self.assertEqual(review_req.state_id, "assigned")
+        self.assertEqual(review_req.reviewassignment_set.count(),1)
+        assignment = review_req.reviewassignment_set.first()
+        self.assertEqual(assignment.reviewer, reviewer)
+        self.assertEqual(assignment.state_id, "requested")
         self.assertEqual(len(outbox), 1)
         self.assertTrue("assigned" in outbox[0].get_payload(decode=True).decode("utf-8"))
         self.assertEqual(NextReviewerInTeam.objects.get(team=review_req.team).next_reviewer, rotation_list[1])
 
-        # re-assign
-        empty_outbox()
-        review_req.state = ReviewRequestStateName.objects.get(slug="accepted")
-        review_req.save()
-        reviewer = Email.objects.filter(role__name="reviewer", role__group=review_req.team, person=rotation_list[1]).first()
-        r = self.client.post(assign_url, { "action": "assign", "reviewer": reviewer.pk, "add_skip": 1 })
-        self.assertEqual(r.status_code, 302)
-
-        review_req = reload_db_objects(review_req)
-        self.assertEqual(review_req.state_id, "requested") # check that state is reset
-        self.assertEqual(review_req.reviewer, reviewer)
-        self.assertEqual(len(outbox), 2)
-        self.assertTrue("cancelled your assignment" in outbox[0].get_payload(decode=True).decode("utf-8"))
-        self.assertTrue("assigned" in outbox[1].get_payload(decode=True).decode("utf-8"))
-        self.assertEqual(ReviewerSettings.objects.get(person=reviewer.person).skip_next, 1)
-
     def test_accept_reviewer_assignment(self):
 
         doc = WgDraftFactory(group__acronym='mars',rev='01')
         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',name_id='reviewer')
-        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))
+        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))
+        assignment = ReviewAssignmentFactory(review_request=review_req, state_id='requested', 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 = review_req.reviewer.person.user.username
+        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)
@@ -431,8 +428,8 @@ class ReviewTests(TestCase):
         r = self.client.post(url, { "action": "accept" })
         self.assertEqual(r.status_code, 302)
 
-        review_req = reload_db_objects(review_req)
-        self.assertEqual(review_req.state_id, "accepted")
+        assignment = reload_db_objects(assignment)
+        self.assertEqual(assignment.state_id, "accepted")
 
     def test_reject_reviewer_assignment(self):
         doc = WgDraftFactory(group__acronym='mars',rev='01')
@@ -682,6 +679,7 @@ class ReviewTests(TestCase):
 
         assignment = reload_db_objects(assignment)
         self.assertEqual(assignment.state_id, "completed")
+        self.assertNotEqual(assignment.completed_on, None)
 
         with open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
             self.assertEqual(f.read(), "This is a review\nwith two lines")

ietf/doc/views_doc.py

@@ -64,8 +64,8 @@ from ietf.doc.mails import email_comment
 from ietf.mailtrigger.utils import gather_relevant_expansions
 from ietf.meeting.models import Session
 from ietf.meeting.utils import group_sessions, get_upcoming_manageable_sessions, sort_sessions
-from ietf.review.models import ReviewRequest
-from ietf.review.utils import can_request_review_of_doc, review_requests_to_list_for_docs
+from ietf.review.models import ReviewRequest, ReviewAssignment
+from ietf.review.utils import can_request_review_of_doc, review_requests_to_list_for_docs, review_assignments_to_list_for_docs
 from ietf.review.utils import no_review_from_teams_on_doc
 from ietf.utils import markup_txt, log
 from ietf.utils.text import maybe_split
@@ -382,7 +382,7 @@ def document_main(request, name, rev=None):
         published = doc.latest_event(type="published_rfc")
         started_iesg_process = doc.latest_event(type="started_iesg_process")
 
-        review_requests = review_requests_to_list_for_docs([doc]).get(doc.pk, [])
+        review_assignments = review_assignments_to_list_for_docs([doc]).get(doc.pk, [])
         no_review_from_teams = no_review_from_teams_on_doc(doc, rev or doc.rev)
 
         return render(request, "doc/document_draft.html",
@@ -446,7 +446,7 @@ def document_main(request, name, rev=None):
                                        search_archive=search_archive,
                                        actions=actions,
                                        presentations=presentations,
-                                       review_requests=review_requests,
+                                       review_assignments=review_assignments,
                                        no_review_from_teams=no_review_from_teams,
                                        ))
 
@@ -598,12 +598,11 @@ def document_main(request, name, rev=None):
         # If we want to go back to using markup_txt.markup_unicode, call it explicitly here like this:
         # content = markup_txt.markup_unicode(content, split=False, width=80)
        
-        # TODO - verify if this shows other reviews from the same team - I bet it doesn't
-        review_req = ReviewRequest.objects.filter(reviewassignment__review=doc.name).first()
+        review_assignment = ReviewAssignment.objects.filter(review=doc.name).first()
 
         other_reviews = []
-        if review_req:
-            other_reviews = [r for r in review_requests_to_list_for_docs([review_req.doc]).get(doc.pk, []) if r != review_req]
+        if review_assignment:
+            other_reviews = [r for r in review_assignments_to_list_for_docs([review_assignment.review_request.doc]).get(doc.pk, []) if r != review_assignment]
 
         return render(request, "doc/document_review.html",
                       dict(doc=doc,
@@ -612,7 +611,7 @@ def document_main(request, name, rev=None):
                            revisions=revisions,
                            latest_rev=latest_rev,
                            snapshot=snapshot,
-                           review_req=review_req,
+                           review_req=review_assignment.review_request,
                            other_reviews=other_reviews,
                       ))
 

ietf/doc/views_review.py

@@ -186,11 +186,11 @@ def review_request(request, name, request_id):
 
     can_manage_request = can_manage_review_requests_for_team(request.user, review_req.team)
 
-    can_close_request = (review_req.state_id in ["requested", "accepted"]
+    can_close_request = (review_req.state_id in ["requested", "assigned"]
                          and (is_authorized_in_doc_stream(request.user, doc)
                               or can_manage_request))
 
-    can_assign_reviewer = (review_req.state_id in ["requested", "accepted"]
+    can_assign_reviewer = (review_req.state_id in ["requested", "assigned"]
                            and can_manage_request)
 
     can_edit_comment = can_request_review_of_doc(request.user, doc)
@@ -210,6 +210,9 @@ def review_request(request, name, request_id):
         assignment.can_complete_review = (assignment.state_id in ["requested", "accepted", "overtaken", "no-response", "part-completed", "completed"]
                                           and (assignment.is_reviewer or can_manage_request))
 
+    # This implementation means if a reviewer accepts one assignment for a review_request, he accepts all assigned to him (for that request)
+    # This problematic - it's a bug (probably) for the same person to have more than one assignment for the same request.
+    # It is, however unintuitive, and acceptance should be refactored to be something that works on assignments, not requests
     if request.method == "POST" and request.POST.get("action") == "accept":
         for assignment in assignments:
             if assignment.can_accept_reviewer_assignment:
@@ -245,7 +248,7 @@ class CloseReviewRequestForm(forms.Form):
 @login_required
 def close_request(request, name, request_id):
     doc = get_object_or_404(Document, name=name)
-    review_req = get_object_or_404(ReviewRequest, pk=request_id, state__in=["requested", "accepted"])
+    review_req = get_object_or_404(ReviewRequest, pk=request_id, state__in=["requested", "assigned"])
 
     can_request = is_authorized_in_doc_stream(request.user, doc)
     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):
     return render(request, 'doc/review/close_request.html', {
         'doc': doc,
         'review_req': review_req,
+        'assignments': review_req.reviewassignment_set.all(),
         'form': form,
     })
 
@@ -745,7 +749,7 @@ def edit_deadline(request, name, request_id):
         if form.is_valid():
             if form.cleaned_data['deadline'] != old_deadline:
                 form.save()
-                subject = "Deadline changed: {} {} review of {}-{}".format(review_req.team.acronym.capitalize(),review_req.type.name.lower(), review_req.doc.name, review_req.reviewed_rev)
+                subject = "Deadline changed: {} {} review of {}-{}".format(review_req.team.acronym.capitalize(),review_req.type.name.lower(), review_req.doc.name, review_req.requested_rev)
                 msg = render_to_string("review/deadline_changed.txt", {
                     "review_req": review_req,
                     "old_deadline": old_deadline,

ietf/iesg/agenda.py

@@ -156,7 +156,7 @@ def fill_in_agenda_docs(date, sections, docs=None):
         docs = docs.select_related("stream", "group").distinct()
         fill_in_telechat_date(docs)
 
-    review_requests_for_docs = review_requests_to_list_for_docs(docs)
+    review_assignments_for_docs = review_assignments_to_list_for_docs(docs)
 
     for doc in docs:
         if doc.telechat_date() != date:
@@ -182,7 +182,7 @@ def fill_in_agenda_docs(date, sections, docs=None):
                 if e and (e.consensus != None):
                     doc.consensus = "Yes" if e.consensus else "No"
 
-            doc.review_requests = review_requests_for_docs.get(doc.pk, [])
+            doc.review_assignments = review_assignments_for_docs.get(doc.pk, [])
         elif doc.type_id == "conflrev":
             doc.conflictdoc = doc.relateddocument_set.get(relationship__slug='conflrev').target.document
         elif doc.type_id == "charter":

ietf/review/factories.py

@@ -46,6 +46,7 @@ class ReviewAssignmentFactory(factory.DjangoModelFactory):
     review_request = factory.SubFactory('ietf.review.factories.ReviewRequestFactory')
     state_id = 'requested'
     reviewer = factory.SubFactory('ietf.person.factories.EmailFactory')
+    assigned_on = datetime.datetime.now()
 
 class ReviewerSettingsFactory(factory.DjangoModelFactory):
     class Meta:

ietf/review/utils.py

@@ -8,7 +8,7 @@ from django.contrib.sites.models import Site
 import debug                            # pyflakes:ignore
 
 from ietf.group.models import Group, Role
-from ietf.doc.models import (Document, ReviewRequestDocEvent, State,
+from ietf.doc.models import (Document, ReviewRequestDocEvent, ReviewAssignmentDocEvent, State,
                              LastCallDocEvent, TelechatDocEvent,
                              DocumentAuthor, DocAlias)
 from ietf.iesg.models import TelechatDate
@@ -24,7 +24,7 @@ def active_review_teams():
     return Group.objects.filter(reviewteamsettings__isnull=False,state="active")
 
 def close_review_request_states():
-    return ReviewRequestStateName.objects.filter(used=True).exclude(slug__in=["requested", "accepted", "rejected", "part-completed", "completed"])
+    return ReviewRequestStateName.objects.filter(used=True).exclude(slug__in=["requested", "assigned"])
 
 def can_request_review_of_doc(user, doc):
     if not user.is_authenticated:
@@ -50,6 +50,16 @@ def can_access_review_stats_for_team(user, team):
     return (Role.objects.filter(name__in=("secr", "reviewer"), person__user=user, group=team).exists()
             or has_role(user, ["Secretariat", "Area Director"]))
 
+def review_assignments_to_list_for_docs(docs):
+    assignment_qs = ReviewAssignment.objects.filter(
+        state__in=["requested", "accepted", "part-completed", "completed"],
+    ).prefetch_related("result")
+
+    doc_names = [d.name for d in docs]
+
+    return extract_revision_ordered_review_assignments_for_documents_and_replaced(assignment_qs, doc_names)
+
+# TODO : remove this when there are no callers
 def review_requests_to_list_for_docs(docs):
     request_qs = ReviewRequest.objects.filter(
         state__in=["requested", "accepted", "part-completed", "completed"],
@@ -67,7 +77,7 @@ def augment_review_requests_with_events(review_reqs):
 def no_review_from_teams_on_doc(doc, rev):
     return Group.objects.filter(
         reviewrequest__doc__name=doc.name,
-        reviewrequest__reviewed_rev=rev,
+        reviewrequest__requested_rev=rev,
         reviewrequest__state__slug="no-review-version",
     ).distinct()
 
@@ -140,9 +150,9 @@ def days_needed_to_fulfill_min_interval_for_reviewers(team):
     """Returns person_id -> days needed until min_interval is fulfilled
     for reviewer (in case it is necessary to wait, otherwise reviewer
     is absent in result)."""
-    latest_assignments = dict(ReviewRequest.objects.filter(
-        team=team,
-    ).values_list("reviewer__person").annotate(Max("time")))
+    latest_assignments = dict(ReviewAssignment.objects.filter(
+        review_request__team=team,
+    ).values_list("reviewer__person").annotate(Max("assigned_on")))
 
     min_intervals = dict(ReviewerSettings.objects.filter(team=team).values_list("person_id", "min_interval"))
 
@@ -161,12 +171,78 @@ def days_needed_to_fulfill_min_interval_for_reviewers(team):
 
     return res
 
+ReviewAssignmentData = namedtuple("ReviewAssignmentData", [
+    "assignment_pk", "doc", "doc_pages", "req_time", "state", "assigned_time", "deadline", "reviewed_rev", "result", "team", "reviewer",
+    "late_days",
+    "request_to_assignment_days", "assignment_to_closure_days", "request_to_closure_days"])
+
+# TODO - see if this becomes dead
 ReviewRequestData = namedtuple("ReviewRequestData", [
     "req_pk", "doc", "doc_pages", "req_time", "state", "assigned_time", "deadline", "reviewed_rev", "result", "team", "reviewer",
     "late_days",
     "request_to_assignment_days", "assignment_to_closure_days", "request_to_closure_days"])
 
+def extract_review_assignment_data(teams=None, reviewers=None, time_from=None, time_to=None, ordering=[]):
+    """Yield data on each review assignment, sorted by (*ordering, assigned_on)
+    for easy use with itertools.groupby. Valid entries in *ordering are "team" and "reviewer"."""
 
+    filters = Q()
+
+    if teams:
+        filters &= Q(review_request__team__in=teams)
+
+    if reviewers:
+        filters &= Q(reviewer__person__in=reviewers)
+
+    if time_from:
+        filters &= Q(assigned_on__gte=time_from)
+
+    if time_to:
+        filters &= Q(assigned_on__lte=time_to)
+
+    # This doesn't do the left-outer join on docevent that the previous code did. These variables could be renamed
+    event_qs = ReviewAssignment.objects.filter(filters)
+
+    event_qs = event_qs.values_list(
+        "pk", "review_request__doc", "review_request__doc__pages", "review_request__time", "state", "review_request__deadline", "reviewed_rev", "result", "review_request__team",
+        "reviewer__person", "assigned_on", "completed_on"
+    )
+
+    event_qs = event_qs.order_by(*[o.replace("reviewer", "reviewer__person") for o in ordering] + ["assigned_on", "pk", "completed_on"])
+
+    def positive_days(time_from, time_to):
+        if time_from is None or time_to is None:
+            return None
+
+        delta = time_to - time_from
+        seconds = delta.total_seconds()
+        if seconds > 0:
+            return seconds / float(24 * 60 * 60)
+        else:
+            return 0.0
+
+    requested_time = assigned_time = closed_time = None
+
+    for assignment in event_qs:
+
+        assignment_pk, doc, doc_pages, req_time, state, deadline, reviewed_rev, result, team, reviewer, assigned_on, completed_on = assignment
+
+        requested_time = req_time
+        assigned_time = assigned_on
+        closed_time = completed_on
+
+        late_days = positive_days(datetime.datetime.combine(deadline, datetime.time.max), closed_time)
+        request_to_assignment_days = positive_days(requested_time, assigned_time)
+        assignment_to_closure_days = positive_days(assigned_time, closed_time)
+        request_to_closure_days = positive_days(requested_time, closed_time)
+
+        d = ReviewAssignmentData(assignment_pk, doc, doc_pages, req_time, state, assigned_time, deadline, reviewed_rev, result, team, reviewer,
+                              late_days, request_to_assignment_days, assignment_to_closure_days,
+                              request_to_closure_days)
+
+        yield d
+
+# TODO - see if this is dead code
 def extract_review_request_data(teams=None, reviewers=None, time_from=None, time_to=None, ordering=[]):
     """Yield data on each review request, sorted by (*ordering, time)
     for easy use with itertools.groupby. Valid entries in *ordering are "team" and "reviewer"."""
@@ -190,11 +266,11 @@ def extract_review_request_data(teams=None, reviewers=None, time_from=None, time
 
     # left outer join with RequestRequestDocEvent for request/assign/close time
     event_qs = event_qs.values_list(
-        "pk", "doc", "doc__pages", "time", "state", "deadline", "reviewed_rev", "result", "team",
-        "reviewer__person", "reviewrequestdocevent__time", "reviewrequestdocevent__type"
+        "pk", "doc", "doc__pages", "time", "state", "deadline", "reviewassignment__reviewed_rev", "reviewassignment__result", "team",
+        "reviewassignment__reviewer__person", "reviewrequestdocevent__time", "reviewrequestdocevent__type"
     )
 
-    event_qs = event_qs.order_by(*[o.replace("reviewer", "reviewer__person") for o in ordering] + ["time", "pk", "-reviewrequestdocevent__time"])
+    event_qs = event_qs.order_by(*[o.replace("reviewer", "reviewassignment__reviewer__person") for o in ordering] + ["time", "pk", "-reviewrequestdocevent__time"])
 
     def positive_days(time_from, time_to):
         if time_from is None or time_to is None:
@@ -304,6 +380,24 @@ def sum_raw_review_request_aggregations(raw_aggregations):
 
     return state_dict, late_state_dict, result_dict, assignment_to_closure_days_list, assignment_to_closure_days_count
 
+def latest_review_assignments_for_reviewers(team, days_back=365):
+    """Collect and return stats for reviewers on latest assignments, in
+    extract_review_assignment_data format."""
+
+    extracted_data = extract_review_assignment_data(
+        teams=[team],
+        time_from=datetime.date.today() - datetime.timedelta(days=days_back),
+        ordering=["reviewer"],
+    )
+
+    assignment_data_for_reviewers = {
+        reviewer: list(reversed(list(req_data_items)))
+        for reviewer, req_data_items in itertools.groupby(extracted_data, key=lambda data: data.reviewer)
+    }
+
+    return assignment_data_for_reviewers
+
+# TODO - see if this is dead code
 def latest_review_requests_for_reviewers(team, days_back=365):
     """Collect and return stats for reviewers on latest requests, in
     extract_review_request_data format."""
@@ -397,7 +491,8 @@ def email_review_request_change(request, review_req, subject, msg, by, notify_se
         else:
             extract_email_addresses(Role.objects.filter(name="secr", group=review_req.team).distinct())
     if notify_reviewer:
-        extract_email_addresses([review_req.reviewer])
+        for assignment in review_req.reviewassignment_set.all():
+            extract_email_addresses([assignment.reviewer])
     if notify_requested_by:
         extract_email_addresses([review_req.requested_by.email()])
         
@@ -462,19 +557,17 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
     if review_req.reviewassignment_set.filter(reviewer=reviewer).exists():
         return
 
-    # Unassignment now has to be explicit
-    #if review_req.reviewer:
-    #    email_review_request_change(
-    #        request, review_req,
-    #        "Unassigned from review of %s" % review_req.doc.name,
-    #        "%s has cancelled your assignment to the review." % request.user.person,
-    #        by=request.user.person, notify_secretary=False, notify_reviewer=True, notify_requested_by=False)
+    # Note that assigning a review no longer unassigns other reviews
 
     review_req.reviewassignment_set.create(state_id='requested', reviewer = reviewer, assigned_on = datetime.datetime.now())
+    if review_req.state_id != 'assigned':
+        review_req.state_id = 'assigned'
+        review_req.save()
 
     if reviewer:
         possibly_advance_next_reviewer_for_team(review_req.team, reviewer.person_id, add_skip)
 
+    # TODO - should these be ReviewAssignmentDocEvents?
     ReviewRequestDocEvent.objects.create(
         type="assigned_review_request",
         doc=review_req.doc,
@@ -483,7 +576,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(),
-            review_req.reviewer.person if review_req.reviewer else "(None)",
+            reviewer.person if reviewer else "(None)",
         ),
         review_request=review_req,
         state=None,
@@ -491,9 +584,9 @@ def assign_review_request_to_reviewer(request, review_req, reviewer, add_skip=Fa
 
     msg = "%s has assigned you as a reviewer for this document." % request.user.person.ascii
     prev_team_reviews = ReviewAssignment.objects.filter(
-        doc=review_req.doc,
+        review_request__doc=review_req.doc,
         state="completed",
-        team=review_req.team,
+        review_request__team=review_req.team,
     )
     if prev_team_reviews.exists():
         msg = msg + '\n\nThis team has completed other reviews of this document:\n'
@@ -575,6 +668,19 @@ def close_review_request(request, review_req, close_state):
             state=review_req.state,
         )
 
+        for assignment in review_req.reviewassignment_set.filter(state_id__in=['requested','accepted']):
+            assignment.state_id = 'withdrawn'
+            assignment.save()
+            ReviewAssignmentDocEvent.objects.create(
+                type='closed_review_request',
+                doc=review_req.doc,
+                rev=review_req.doc.rev,
+                by=request.user.person,
+                desc="Request closed, assignment withdrawn: {} {} {} review".format(assignment.reviewer.person.plain_name, assignment.review_request.type.name, assignment.review_request.team.acronym.upper()),
+                review_assignment=assignment,
+                state=assignment.state,
+            )
+
         email_review_request_change(
             request, review_req,
             "Closed review request for {}: {}".format(review_req.doc.name, close_state.name),
@@ -693,6 +799,53 @@ def suggested_review_requests_for_team(team):
     res.sort(key=lambda r: (r.deadline, r.doc_id), reverse=True)
     return res
 
+def extract_revision_ordered_review_assignments_for_documents_and_replaced(review_assignment_queryset, names):
+    """Extracts all review assignments for document names (including replaced ancestors), return them neatly sorted."""
+
+    names = set(names)
+
+    replaces = extract_complete_replaces_ancestor_mapping_for_docs(names)
+
+    assignments_for_each_doc = defaultdict(list)
+    for r in review_assignment_queryset.filter(review_request__doc__in=set(e for l in replaces.itervalues() for e in l) | names).order_by("-reviewed_rev","-assigned_on", "-id").iterator():
+        assignments_for_each_doc[r.review_request.doc_id].append(r)
+
+    # now collect in breadth-first order to keep the revision order intact
+    res = defaultdict(list)
+    for name in names:
+        front = replaces.get(name, [])
+        res[name].extend(assignments_for_each_doc.get(name, []))
+
+        seen = set()
+
+        while front:
+            replaces_assignments = []
+            next_front = []
+            for replaces_name in front:
+                if replaces_name in seen:
+                    continue
+
+                seen.add(replaces_name)
+
+                assignments = assignments_for_each_doc.get(replaces_name, [])
+                if assignments:
+                    replaces_assignments.append(assignments)
+
+                next_front.extend(replaces.get(replaces_name, []))
+
+            # in case there are multiple replaces, move the ones with
+            # the latest reviews up front
+            replaces_assignments.sort(key=lambda l: l[0].assigned_on, reverse=True)
+
+            for assignments in replaces_assignments:
+                res[name].extend(assignments)
+
+            # move one level down
+            front = next_front
+
+    return res
+
+# TODO - remove when there are no remaining callers
 def extract_revision_ordered_review_requests_for_documents_and_replaced(review_request_queryset, names):
     """Extracts all review requests for document names (including replaced ancestors), return them neatly sorted."""
 
@@ -740,10 +893,12 @@ def extract_revision_ordered_review_requests_for_documents_and_replaced(review_r
 
     return res
 
+# TODO : Change this field to deal with multiple already assigned reviewers
 def setup_reviewer_field(field, review_req):
     field.queryset = field.queryset.filter(role__name="reviewer", role__group=review_req.team)
-    if review_req.reviewer:
-        field.initial = review_req.reviewer_id
+    one_assignment = review_req.reviewassignment_set.first()
+    if one_assignment:
+        field.initial = one_assignment.reviewer_id
 
     choices = make_assignment_choices(field.queryset, review_req)
     if not field.required:
@@ -788,15 +943,15 @@ def make_assignment_choices(email_queryset, review_req):
     # previous review of document
     has_reviewed_previous = ReviewRequest.objects.filter(
         doc=doc,
-        reviewer__person__in=possible_person_ids,
-        state="completed",
+        reviewassignment__reviewer__person__in=possible_person_ids,
+        reviewassignment__state="completed",
         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("reviewer__person", flat=True))
+    has_reviewed_previous = set(has_reviewed_previous.values_list("reviewassignment__reviewer__person", flat=True))
 
     # review wishes
     wish_to_review = set(ReviewWish.objects.filter(team=team, person__in=possible_person_ids, doc=doc).values_list("person", flat=True))
@@ -816,7 +971,7 @@ def make_assignment_choices(email_queryset, review_req):
     unavailable_periods = current_unavailable_periods_for_reviewers(team)
 
     # reviewers statistics
-    req_data_for_reviewers = latest_review_requests_for_reviewers(team)
+    assignment_data_for_reviewers = latest_review_assignments_for_reviewers(team)
 
     ranking = []
     for e in possible_emails:
@@ -871,21 +1026,21 @@ def make_assignment_choices(email_queryset, review_req):
 
         # stats
         stats = []
-        req_data = req_data_for_reviewers.get(e.person_id, [])
+        assignment_data = assignment_data_for_reviewers.get(e.person_id, [])
 
-        currently_open = sum(1 for d in req_data if d.state in ["requested", "accepted"])
-        pages = sum(rd.doc_pages for rd in req_data if rd.state in ["requested", "accepted"])
+        currently_open = sum(1 for d in assignment_data if d.state in ["requested", "accepted"])
+        pages = sum(rd.doc_pages for rd in assignment_data if rd.state in ["requested", "accepted"])
         if currently_open > 0:
             stats.append("currently {count} open, {pages} pages".format(count=currently_open, pages=pages))
-        could_have_completed = [d for d in req_data if d.state in ["part-completed", "completed", "no-response"]]
+        could_have_completed = [d for d in assignment_data if d.state in ["part-completed", "completed", "no-response"]]
         if could_have_completed:
-            no_response     = len([d for d in req_data if d.state == 'no-response'])
+            no_response     = len([d for d in assignment_data if d.state == 'no-response'])
             if no_response:
                 stats.append("%s no response" % no_response)
-            part_completed  = len([d for d in req_data if d.state == 'part-completed'])
+            part_completed  = len([d for d in assignment_data if d.state == 'part-completed'])
             if part_completed:
                 stats.append("%s partially complete" % part_completed)
-            completed       = len([d for d in req_data if d.state == 'completed'])
+            completed       = len([d for d in assignment_data if d.state == 'completed'])
             if completed:
                 stats.append("%s fully completed" % completed)
 

ietf/templates/doc/document_draft.html

@@ -204,14 +204,14 @@
 	{% endif %}
       {% endfor %}
 
-      {% if review_requests or can_request_review %}
+      {% if review_assignments or can_request_review %}
       <tr>
         <th></th>
         <th>Reviews</th>
 	<td class="edit"></td>
         <td>
-          {% for review_request in review_requests %}
-            {% include "doc/review_request_summary.html" with current_doc_name=doc.name current_rev=doc.rev %}
+          {% for review_assignment in review_assignments %}
+            {% include "doc/review_assignment_summary.html" with current_doc_name=doc.name current_rev=doc.rev %}
           {% endfor %}
 
           {% if no_review_from_teams %}

ietf/templates/doc/document_review.html

@@ -43,8 +43,8 @@
         <th>Other reviews</th>
         <td class="edit"></td>
         <td>
-          {% for review_request in other_reviews %}
-            {% include "doc/review_request_summary.html" with current_doc_name=review_req.doc_id current_rev=review_req.reviewed_rev %}
+          {% for review_assignment in other_reviews %}
+            {% include "doc/review_assignment_summary.html" with current_doc_name=review_assignemnt.review_request.doc_id current_rev=review_assignment.reviewed_rev %}
           {% endfor %}
         </td>
       </tr>

ietf/templates/doc/review/request_info.html

@@ -1,5 +1,4 @@
 {# Copyright The IETF Trust 2017, All Rights Reserved #}{% load origin bootstrap3 %}{% origin %}
-  <p>I got included</p>
   <table class="table table-condensed">
     <tbody class="meta">
       <tr>

ietf/templates/doc/review_assignment_summary.html

@@ -0,0 +1,12 @@
+<div class="review-assignment-summary">
+  {% if review_assignment.state_id == "completed" or review_assignment.state_id == "part-completed" %}
+    <a href="{% if review_assignment.review %}{% url "ietf.doc.views_doc.document_main" review_assignment.review.name %}{% else %}{% url "ietf.doc.views_review.review_request" review_assignment.review_request.doc_id review_assignment.review_request.pk %}{% endif %}">
+      {{ review_assignment.review_request.team.acronym|upper }} {{ review_assignment.review_request.type.name }} Review{% if review_assignment.reviewed_rev and review_assignment.reviewed_rev != current_rev or review_assignment.review_request.doc_id != current_doc_name %} (of {% if review_assignment.review_request.doc_id != current_doc_name %}{{ review_assignment.review_request.doc_id }}{% endif %}-{{ review_assignment.reviewed_rev }}){% endif %}{% if review_assignment.result %}:
+      {{ review_assignment.result.name }}{% endif %} {% if review_assignment.state_id == "part-completed" %}(partially completed){% endif %}
+    </a>
+  {% else %}
+    <i>
+    <a href="{% url "ietf.doc.views_review.review_request" review_assignment.review_request.doc_id review_assignment.review_request.pk %}">{{ review_assignment.review_request.team.acronym|upper }} {{ review_assignment.review_request.type.name }} Review
+      - due: {{ review_assignment.review_request.deadline|date:"Y-m-d" }}</a></i>
+  {% endif %}
+</div>

ietf/templates/iesg/agenda_doc.html

@@ -50,8 +50,8 @@
   {% if doc.review_requests %}
     <dt>Reviews</dt>
     <dd>
-      {% for review_request in doc.review_requests %}
-        {% include "doc/review_request_summary.html" with current_doc_name=doc.name current_rev=doc.rev %}
+      {% for review_assignment in doc.review_assignments %}
+        {% include "doc/review_assignment_summary.html" with current_doc_name=doc.name current_rev=doc.rev %}
       {% endfor %}
     </dd>
   {% endif %}