search

ietf-tools / datatracker

The day-to-day front-end to the IETF database for people who work on IETF standards.
https://datatracker.ietf.org
BSD 3-Clause "New" or "Revised" License
608 stars 375 forks source link

Summary of reviews on document's main page doesn't say enough when a team is not going to review a version #2685

Open ietf-svn-bot opened 5 years ago

ietf-svn-bot commented 5 years ago

keyword_sprint type_defect | by rjsparks@nostrum.com

If a review team has ever responded to a review request with "team will not review version", the summary text for reviews done on a document currently contains a line similar to "BLAHDIR, FOODIR will not review this version".

"this version" is problematic in this context. New versions happen. Older versions might have been reviewed already. There is no way to know which version "this" refers to in the sentence.

Instead, the line needs to say "will not review version xx" for the appropriate xx.

See the attachment for a real example.

Issue migrated from trac:2685 at 2022-03-04 07:07:44 +0000

ietf-svn-bot commented 5 years ago

@rjsparks@nostrum.com uploaded file Screenshot 2019-02-13 08.50.40.png (134.9 KiB)

Screenshot of review summary of draft-ietf-dots-requirements Screenshot 2019-02-13 08.50.40.png

ietf-svn-bot commented 5 years ago

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

ietf-svn-bot commented 5 years ago

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

ietf-svn-bot commented 5 years ago

@sasha@dashcare.nl commented

Currently, this data does not seem to be recorded for most cases. When a review request is made without requesting a specific version, and then the review request is closed (for any reason), no revision is recorded along with "will not review this version". Reviews that do not request a specific version are the majority, about 97% from 2018 and newer.

I also think a bug has been introduced since your screenshot, or an obscure feature: the "will not review" parts are not visible anymore for your example. All review requests for draft-ietf-dots-requirements did not request a specific version. Reading the code, it makes sense that they're not shown - that data is filled from https://trac.tools.ietf.org/tools/ietfdb/browser/trunk/ietf/doc/views_doc.py#L399 which runs the query in https://trac.tools.ietf.org/tools/ietfdb/browser/trunk/ietf/review/utils.py#L84. As doc.rev is never empty (right?), that query will not return any groups which (only) have review requests without a specific requested version.

Conclusion: for almost all documents, decisions to close a review are not visible on the summary page at all, with or without revision, which seems to have been introduced in 8726e74709a19496c436d6cfeb40ff0fded3fc00, as that's when the reviewrequest__requested_rev=rev was added in the query review/utils.py. I can fix this bug (unless it's on purpose) as part of this ticket, though it leaves the initial problem of "in many cases we have no data on which revision the team decided not to review".

The best option I'm thinking of now for recording that data is to add a field to the close request form, to say for which revision the decision applies, and extending the ReviewRequest to record this information. However, I'm not too familiar with the human processes behind deciding not to review a version, so I'm not sure whether there's a better option? It does also mean this will only work going forward - historical data can not be amended.

ietf-svn-bot commented 5 years ago

@rjsparks@nostrum.com commented

Thanks for the thorough analysis. 8726e74709a19496c436d6cfeb40ff0fded3fc00 was a fairly major refactor, and the concept this ticket is talking about certainly got caught up in it.

The decision to not review is made against a review request. That we have a reality where the request usually doesn't provide a version means "will not review this version" makes less sense than "will not provide a review for this request", and where we show that we can talk about whether the request mentioned a revision and what the revision was when the decision to not provide a review was made. Btw, my inclination is to keep that last bit of data in an extension of ReviewRequestDocEvent.

There is now some design tension around where to show this data.

I do think the code at https://trac.tools.ietf.org/tools/ietfdb/browser/trunk/ietf/review/utils.py#L84 is unintentionally buggy, but I don't think the correction will be trivial. What to display needs to be rethought, and the objects that utility function returns are, I anticipate, not going to be the right objects to satisfy the need once that thinking is complete.

I think the right thing to do for now is to take this particular ticket out of the current project and address it separately later.

ietf-svn-bot commented 3 years ago

@rjsparks@nostrum.com changed status from assigned to accepted

ietf-svn-bot commented 3 years ago

@rjsparks@nostrum.com changed owner from sasha@dashcare.nl to ``