Allow review team secretaries to adjust the completion date of a review when closing it

[ietf-svn-bot]

owner:sasha@dashcare.nl resolution_fixed type_defect | by rjsparks@nostrum.com

---

Sometimes review team members send reviews directly to a list or two, but don't tell the datatracker that they've completed it. The review team secretary can complete it on their behalf later, giving the datatracker the link to the review on the list.

Right now, the review is marked completed on the date the secretary takes this action. It would reflect reality better if the secretary could say what date the review was actually completed. It would be nice if the datatracker suggested that date by looking at the message the link points to.

Of course, the timestamp for the event that the secretary marked the review competed should be when the secretary actually did that.

---

Issue migrated from trac:2590 at 2022-03-04 06:52:04 +0000

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

#2591 will be a prerequisite for this

[ietf-svn-bot]

@sasha@dashcare.nl changed status from new to assigned

[ietf-svn-bot]

@sasha@dashcare.nl changed owner from `` to sasha@dashcare.nl

[ietf-svn-bot]

@sasha@dashcare.nl commented

---

Related to this change: when a review is revised, the review completion date can be changed already. When this happens, currently, the date of the ReviewAssignmentDocEvent is set to the newly entered review completion date.

As the plan is:

Of course, the timestamp for the event that the secretary marked the review competed should be when the secretary actually did that.

I'd suggest that this causes an inconsistency, and rather the ReviewAssignmentDocEvent should be set to the current date when submitting a revision, i.e. the ReviewAssignmentDocEvent should have the date the revision was submitted, not the date the original review happened. If we retain the current behaviour for revisions, the completion and event date are linked in case of revisions, but not in case of initially submitting the review by a secretary, which seems inconsistent. Thoughts?

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

I'm not following the suggestion yet.

The use case driving the request is this (and apologies if this was already clear from earlier text in the ticket).

A reviewer sends email to a list on, say, 1 Jun 2019. The reviewer did not enter anything into the datatracker at all. Later, say 5 Aug 2019, the secretary notices the reviewer did this thing and goes to tell the datatracker about the message that was sent to the list, letting the datatracker know the review was completed.

The datatracker should capture that the secretary did this on 5Aug2019 (which I think would be done by having that date be the date of the ReviewAssignmentDocEvent that marks the review complete), and the ReviewAssignment completed_on field would be set to 1 Jun 2019.

That completed_on field was added after Tero opened this ticket, satisfying part of the request. The part that's left is his request that the completed_on field automatically set the field to 1 Jun 2019 by looking at the email message he selected rather than making him look at the message and fill in the date himself.

Does that help resolve the feeling of inconsistency your suggestion is based on?

[ietf-svn-bot]

@rjsparks@nostrum.com changed _comment0 which not transferred by tractive

[ietf-svn-bot]

@henrik@levkowetz.com commented

---

We've tried to keep event timestamps always being set to current time as an invariant; i.e., DocEvent.time==datetime.datetime.now(), so I'd expect that any DocEvent should have as its time the actual time the event took place, and I don't think we should ever modify a DocEvent time. If we're violating this today, we should look at how to avoid it. For the DocEvent that indicates that the secretariat has marked a review as completed, there should be an event which indicates that the secretariat has retroactively marked the review as done, with event.time set to datetime.datetime.now() on creation, i.e., the time the secretariat took action.

However, one could argue that when the secretariat marks the review as completed at an earlier time, there is a missing DocEvent for that earlier time. Retroactively inserting a DocEvent with that timestamp could make sense. In that case, it may also make sense to include in the DocEvent description that this is the time the review was posted to the mailing list.

[ietf-svn-bot]

@sasha@dashcare.nl commented

---

How that part should work is clear, and makes sense to me. However, when a review is revised, after it was entered in the datatracker previously, the behaviour is different - I'm talking about using the "Correct review" button here. In that case, a secretary can already set a custom completion date, and in that case the ReviewAssignmentDocEvent date is set to the completion date, rather than the date this revision was actually submitted. That sounds inconsistent.

So in the case you describe, if the secretary later corrects the review on 10Aug2019 (say, the Result field was set wrong by them), and keeps the (correct) completion date of 1Jun2019, suddenly the ReviewAssignmentDocEvent also moves to 1Jun2019. This is existing behaviour in the code now, as setting/updating the completion date when revising already is possible. However, it sounds like the date on the event should be 10Aug2019.

But, I don't know what the typical use case for correcting a review is, so maybe it makes sense.

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

I think you found a bug, and agree that the behavior you are looking at is inconsistent with what we want Event times to be. I probably introduced it blush. Yes, the date on the event should be 10Aug.
Please correct it.

[ietf-svn-bot]

@henrik@levkowetz.com commented

---

Replying to ietf-svn-conversion/datatracker#2590 (comment:5):

How that part should work is clear, and makes sense to me. However,
when a review is revised, after it was entered in the datatracker
previously, the behaviour is different - I'm talking about using the
"Correct review" button here. In that case, a secretary can already
set a custom completion date, and in that case the
ReviewAssignmentDocEvent date is set to the completion date, rather
than the date this revision was actually submitted. That sounds
inconsistent.

Yes. It sounds to me as if the right thing would be to have two DocEvents, one for the time the secretariat made the change, another (retrofitted) for the corrected completion date. That may leave a third DocEvent for the original completion date in place, which also seems right.

Again, unless doing data migrations to fix incorrect data (DocEvents which have had a actually incorrect date set due to a code bug, something which has happened) we should never change the time of an existing DocEvent.

[ietf-svn-bot]

@sasha@dashcare.nl changed status from assigned to closed

[ietf-svn-bot]

@sasha@dashcare.nl changed resolution from `` to fixed

[ietf-svn-bot]

@sasha@dashcare.nl commented

---

Fixed in 3942f9a:

Fix #2590 - Allow secretary to adjust date when completing a review.

This also fixes other issues identified in #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.

[ietf-svn-bot]

@sasha@dashcare.nl commented

---

Not sure why the diff on commit 16670 is such a mess - the actually change is much smaller. It's ready for merge as far as I'm concerned, forgot to note that in the message.