Merged in [16670] from sasha@dashcare.nl:

    Fix ietf-tools#2590 - Allow secretary to adjust date when completing a review.
This also fixes other issues identified in ietf-tools#2590, around the
modification of historical document events. The behaviour is now:
- When the assigned reviewer posts a review, a single event is
  created, set to current date/time.
- When the secretary records a review in the datatracker, they may
  set a different completion date, which is autofilled if an email
  is selected. One event is generated for the original completion
  date, and one for the secretary's action.
- Each revision generates a new event, rather than updating previous
  existing events.
 - Legacy-Id: 16791
Note: SVN reference [16670] has been migrated to Git commit 3942f9a

Author: levkowetz

ietf/doc/tests_review.py

@@ -688,21 +688,67 @@ def test_complete_review_enter_content(self):
             "review_content": "This is a review\nwith two lines",
             "review_url": "",
             "review_file": "",
+            # Custom completion should be ignored - review posted by assignee is always set to now
+            "completion_date": "2012-12-24",
+            "completion_time": "12:13:14",
         })
         self.assertEqual(r.status_code, 302)
 
         assignment = reload_db_objects(assignment)
         self.assertEqual(assignment.state_id, "completed")
-        self.assertNotEqual(assignment.completed_on, None)
+        # Completed time should be close to now, but will not be exactly, so check within 10s margin
+        completed_time_diff = datetime.datetime.now() - assignment.completed_on
+        self.assertLess(completed_time_diff, datetime.timedelta(seconds=10))
+
+        with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
+            self.assertEqual(f.read(), "This is a review\nwith two lines")
+
+        self.assertEqual(len(outbox), 1)
+        self.assertIn(assignment.review_request.team.list_email, outbox[0]["To"])
+        self.assertIn("This is a review", outbox[0].get_payload(decode=True).decode("utf-8"))
+
+        self.assertIn(settings.MAILING_LIST_ARCHIVE_URL, assignment.review.external_url)
+
+    def test_complete_review_enter_content_by_secretary(self):
+        assignment, url = self.setup_complete_review_test()
+        login_testing_unauthorized(self, 'reviewsecretary', url)
+
+        empty_outbox()
+
+        r = self.client.post(url, data={
+            "result": ReviewResultName.objects.get(reviewteamsettings_review_results_set__group=assignment.review_request.team, slug="ready").pk,
+            "state": ReviewAssignmentStateName.objects.get(slug="completed").pk,
+            "reviewed_rev": assignment.review_request.doc.rev,
+            "review_submission": "enter",
+            "review_content": "This is a review\nwith two lines",
+            "review_url": "",
+            "review_file": "",
+            "completion_date": "2012-12-24",
+            "completion_time": "12:13:14",
+        })
+        self.assertEqual(r.status_code, 302)
 
+        # The secretary is allowed to set a custom completion date (#2590)
+        assignment = reload_db_objects(assignment)
+        self.assertEqual(assignment.state_id, "completed")
+        self.assertEqual(assignment.completed_on, datetime.datetime(2012, 12, 24, 12, 13, 14))
+
+        # There should be two events:
+        # - the event logging when the change when it was entered, i.e. very close to now.
+        # - the completion of the review, set to the provided date/time
+        events = ReviewAssignmentDocEvent.objects.filter(doc=assignment.review_request.doc).order_by('-time')
+        event0_time_diff = datetime.datetime.now() - events[0].time
+        self.assertLess(event0_time_diff, datetime.timedelta(seconds=10))
+        self.assertEqual(events[1].time, datetime.datetime(2012, 12, 24, 12, 13, 14))
+        
         with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
             self.assertEqual(f.read(), "This is a review\nwith two lines")
 
         self.assertEqual(len(outbox), 1)
-        self.assertTrue(assignment.review_request.team.list_email in outbox[0]["To"])
-        self.assertTrue("This is a review" in outbox[0].get_payload(decode=True).decode("utf-8"))
+        self.assertIn(assignment.review_request.team.list_email, outbox[0]["To"])
+        self.assertIn("This is a review", outbox[0].get_payload(decode=True).decode("utf-8"))
 
-        self.assertTrue(settings.MAILING_LIST_ARCHIVE_URL in assignment.review.external_url)
+        self.assertIn(settings.MAILING_LIST_ARCHIVE_URL, assignment.review.external_url)
 
     def test_complete_notify_ad_because_team_settings(self):
         assignment, url = self.setup_complete_review_test()
@@ -879,8 +925,11 @@ def test_revise_review_enter_content(self):
 
         assignment = reload_db_objects(assignment)
         self.assertEqual(assignment.state_id, "completed")
-        event = ReviewAssignmentDocEvent.objects.get(type="closed_review_assignment", review_assignment=assignment)
-        self.assertEqual(event.time, datetime.datetime(2012, 12, 24, 12, 13, 14))
+        # The revision event time should be the date the revision was submitted, i.e. not backdated
+        event1 = assignment.review_request.doc.latest_event(ReviewAssignmentDocEvent)
+        event_time_diff = datetime.datetime.now() - event1.time
+        self.assertLess(event_time_diff, datetime.timedelta(seconds=10))
+        self.assertTrue('revised' in event1.desc.lower())
 
         with io.open(os.path.join(self.review_subdir, assignment.review.name + ".txt")) as f:
             self.assertEqual(f.read(), "This is a review\nwith two lines")
@@ -904,8 +953,11 @@ def test_revise_review_enter_content(self):
 
         assignment = reload_db_objects(assignment)
         self.assertEqual(assignment.review.rev, "01")
-        event = ReviewAssignmentDocEvent.objects.get(type="closed_review_assignment", review_assignment=assignment)
-        self.assertEqual(event.time, datetime.datetime(2013, 12, 24, 11, 11, 11))
+        event2 = assignment.review_request.doc.latest_event(ReviewAssignmentDocEvent)
+        event_time_diff = datetime.datetime.now() - event2.time
+        self.assertLess(event_time_diff, datetime.timedelta(seconds=10))
+        # Ensure that a new event was created for the new revision (#2590)
+        self.assertNotEqual(event1.id, event2.id)
 
         self.assertEqual(len(outbox), 0)
 

ietf/doc/views_review.py

@@ -522,7 +522,7 @@ def format_submission_choice(label):
 
         if revising_review:
             del self.fields["cc"]
-        else:
+        elif is_reviewer:
             del self.fields["completion_date"]
             del self.fields["completion_time"]
 
@@ -649,6 +649,7 @@ def complete_review(request, name, assignment_id):
 
             need_to_email_review = review_submission != "link" and assignment.review_request.team.list_email and not revising_review
 
+            submitted_on_different_date = completion_datetime.date() != datetime.date.today()
             desc = "Request for {} review by {} {}: {}. Reviewer: {}.".format(
                 assignment.review_request.type.name,
                 assignment.review_request.team.acronym.upper(),
@@ -658,18 +659,38 @@ def complete_review(request, name, assignment_id):
             )
             if need_to_email_review:
                 desc += " " + "Sent review to list."
-
-            close_event = ReviewAssignmentDocEvent.objects.filter(type="closed_review_assignment", review_assignment=assignment).first()
-            if not close_event:
-                close_event = ReviewAssignmentDocEvent(type="closed_review_assignment", review_assignment=assignment)
-
+            if revising_review:
+                desc += " Review has been revised by {}.".format(request.user.person)
+            elif submitted_on_different_date:
+                desc += " Submission of review completed at an earlier date."
+            close_event = ReviewAssignmentDocEvent(type="closed_review_assignment", review_assignment=assignment)
             close_event.doc = assignment.review_request.doc
             close_event.rev = assignment.review_request.doc.rev
             close_event.by = request.user.person
             close_event.desc = desc
             close_event.state = assignment.state
-            close_event.time = completion_datetime
+            close_event.time = datetime.datetime.now()
             close_event.save()
+            
+            # If the completion date is different, record when the initial review was made too.
+            if not revising_review and submitted_on_different_date:
+                desc = "Request for {} review by {} {}: {}. Reviewer: {}.".format(
+                    assignment.review_request.type.name,
+                    assignment.review_request.team.acronym.upper(),
+                    assignment.state.name,
+                    assignment.result.name,
+                    assignment.reviewer.person,
+                )
+
+                initial_close_event = ReviewAssignmentDocEvent(type="closed_review_assignment",
+                                                               review_assignment=assignment)
+                initial_close_event.doc = assignment.review_request.doc
+                initial_close_event.rev = assignment.review_request.doc.rev
+                initial_close_event.by = request.user.person
+                initial_close_event.desc = desc
+                initial_close_event.state = assignment.state
+                initial_close_event.time = completion_datetime
+                initial_close_event.save()
 
             if assignment.state_id == "part-completed" and not revising_review: 
                 existing_assignments = ReviewAssignment.objects.filter(review_request__doc=assignment.review_request.doc, review_request__team=assignment.review_request.team, state__in=("assigned", "accepted", "completed"))