Skip to content

fix: Use related docs pages count when no page count - #3686

Merged
rjsparks merged 1 commit into
ietf-tools:feat/bs5from
evyncke:fix-3503-related-page-count
Mar 22, 2022
Merged

fix: Use related docs pages count when no page count#3686
rjsparks merged 1 commit into
ietf-tools:feat/bs5from
evyncke:fix-3503-related-page-count

Conversation

@evyncke

@evyncke evyncke commented Mar 19, 2022

Copy link
Copy Markdown
Collaborator

Fixing issue #3503

This is also my first PR for datatracker, so, please review it carefully even if it is only a dozen of lines ;-)

@evyncke evyncke changed the title Use related docs pages count when no page count Fix: Use related docs pages count when no page count Mar 19, 2022
@NGPixel NGPixel changed the title Fix: Use related docs pages count when no page count fix: Use related docs pages count when no page count Mar 19, 2022
@evyncke evyncke closed this Mar 19, 2022
@evyncke
evyncke deleted the fix-3503-related-page-count branch March 19, 2022 21:04
@evyncke
evyncke restored the fix-3503-related-page-count branch March 20, 2022 07:48
@evyncke evyncke reopened this Mar 20, 2022
@rjsparks

Copy link
Copy Markdown
Member

This is an improvement, and it will help IESGs, and I plan to merge it as-is.

I worry, though, about changing the meaning doc.pages this way - that will be surprising to future coders working on the templates, and it requires cultural training for the community (particularly new IESG members) to know that what the number means on the output page is different for conflict review rows than it is for any other kind of documents.

We should consider a small project where we formally and carefully change the display element on the search result rows that currently holds a page count (which is what this PR affects) to explicitly hold document-specific metrics that have document-specific labels/layout in the search result row template.

It hasn't happened very often but it is possible to get a Conflict Review that by itself is very long, or requires a lot of reading of some _other_thing, even if the document being reviewed is short.

Comment thread ietf/doc/utils_search.py

for d in docs:
if d.type_id == 'conflrev':
if len(d.related_that_doc('conflrev')) != 1:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, the other parts of the datatracker that set up conflict reviews will not let a conflict review be created that has anything other than exactly one reviewed document, so this line is a safety check against misconfiguration or changes to that assumption in the future. That's good.

The code could instead have looped over the reviewed documents and summed their pages, but that just makes another assumption about what having multiple related documents might mean might mean in the future.

@rjsparks
rjsparks merged commit 65d4c99 into ietf-tools:feat/bs5 Mar 22, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 16, 2022
@evyncke
evyncke deleted the fix-3503-related-page-count branch November 2, 2023 14:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants