fix: Include missing related drafts in IPR searches #7836

microamp · 2024-08-19T08:28:36Z

Draft PR to get some feedback.

Fixes #7824 as well as two UI bugs (see below).

Currently, the first item in the search results in https://datatracker.ietf.org/ipr/search/?submit=draft&id=rfc8955 is

Results for RFC 5575 ("Dissemination of Flow Specification Rules")

(note no "was replaced by" or "was obsoleted by")

It should have been

Results for RFC 5575 ("Dissemination of Flow Specification Rules"), which was obsoleted by RFC 8955 ("Dissemination of Flow Specification Rules")
https://datatracker.ietf.org/ipr/search/?submit=draft&id=rfc8955

currently lists

Results for draft-ietf-idr-rfc5575bis ("Dissemination of Flow Specification Rules"), which was became rfc draft-ietf-idr-rfc5575bis ("Dissemination of Flow Specification Rules")

(note "<draft>, which was became rfc <draft>")

It should have been

Results for draft-ietf-idr-rfc5575bis ("Dissemination of Flow Specification Rules"), which became rfc RFC 8955 ("Dissemination of Flow Specification Rules")

In the case of RFC 8955, it should now list 3 IPR disclosures

2009-02-26, 1106, Juniper's Statement of IPR related to draft-ietf-idr-flow-spec-05
2009-02-09, 1088, Juniper's Statement of IPR related to draft-ietf-idr-flow-spec-03
2008-12-23, 1052, Juniper Networks Inc.'s Statement of IPR claimed in draft-ietf-idr-flow-spec-03 (Removed)

and 6 documents

Results for RFC 5575 ("Dissemination of Flow Specification Rules"), which was obsoleted by RFC 8955 ("Dissemination of Flow Specification Rules")
- no direct IPR disclosures
Results for draft-ietf-idr-flow-spec ("Dissemination of Flow Specification Rules"), which became rfc RFC 5575 ("Dissemination of Flow Specification Rules")
- the same three IPR disclosures as above
Results for RFC 8955 ("Dissemination of Flow Specification Rules")
- no direct IPR disclosures
Results for draft-ietf-idr-rfc5575bis ("Dissemination of Flow Specification Rules"), which became rfc RFC 8955 ("Dissemination of Flow Specification Rules")
- no direct IPR disclosures
Results for draft-ietf-idr-flowspec-redirect-rt-bis ("Clarification of the Flowspec Redirect Extended Community"), which became rfc RFC 7674 ("Clarification of the Flowspec Redirect Extended Community")
- no direct IPR disclosures
Results for RFC 7674 ("Clarification of the Flowspec Redirect Extended Community"), which was obsoleted by RFC 8955 ("Dissemination of Flow Specification Rules")
- no direct IPR disclosures

microamp · 2024-08-19T08:55:08Z

Re: the first bug above, the current solution is to replace

{% if not forloop.first %}

with

{% if d != doc %}

in the template.

Another option is to preserve the order of items in the backend code, so that the RFC selected will always be the first item in the doc list. That way, {% if not forloop.first %} will work as expected.

Code like

list(set(results))  # items in a set are unordered

from the related_docs function can be replaced with

sorted(set(results), key=results.index)  # use set to discard duplicates, but preserve the order of items

for instance.

codecov · 2024-08-19T09:03:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.82%. Comparing base (c7f6bde) to head (a4fc313).
Report is 61 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7836      +/-   ##
==========================================
+ Coverage   88.78%   88.82%   +0.04%     
==========================================
  Files         296      304       +8     
  Lines       41320    41495     +175     
==========================================
+ Hits        36687    36860     +173     
- Misses       4633     4635       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

microamp · 2024-08-19T09:07:09Z

Re: the second bug above, the current solution is to introduce an additional if-else statements in the template to handle both relationship and reverse relationship. (Currently, it's not handling the latter correctly.)

{% if d != d.related.source %}, which was ...
{% else %}, which ...
{% endif %}

So, for a draft that has become one of the related RFCs, it will correctly say

<draft>, which became rfc <rfc> instead of
<draft>, which was became rfc <draft>

However, I'm looking for a cleaner solution.

rjsparks · 2024-08-29T15:11:37Z

I pushed a significant refactor that gets the same documents, but (I think) reads better for future us. I also added a sort of the related documents. If this looks good to @microamp, please take this out of draft and request a review from @jennifer-richards

microamp

LGTM. I'll take the PR out of draft.

microamp · 2024-08-30T02:40:57Z

ietf/ipr/views.py

+                                for draft in drafts
+                            ]
+                        )
+                    )


No strong preference here, but I personally find it easier to read when fors are kept flat like below.

docs.update( set( draft for d in docs for draft in related_docs(d, ...) ) )

Let it be a team decision though.

It should be whatever Black style says, which I think is what you suggest but not certain

Ah, now that I'm at a keyboard I see that this is more about the brackets than about the styling per se so appealing to Black isn't quite enough...

well, what's there is what black produced.

Ah, now that I'm at a keyboard I see that this is more about the brackets than about the styling per se

Correct. I wasn't referring to formatting style - sorry for the confusion. It was more about whether the new code is actually more readable despite having one more for in the expression. My opinion is that it isn't.

Anyway, I'm okay with either as long as you're happy with the logic itself.

microamp · 2024-08-30T02:41:06Z

ietf/ipr/views.py

+                            d.rfc_number if d.rfc_number is not None else 0,
+                            d.became_rfc().rfc_number if d.became_rfc() else 0,
+                        ),
+                        reverse=True,


Nice. Food for thought (not requesting a change here) - we can even do something like

key=lambda d: ( d == first, # to ensure that the doc selected will always be first (untested) # the rest... )

The expression will return a bool value which is just an int in Python. i.e.

assert isinstance(True, int)

I think 0 if d == first else 1 would be safer since it doesn't rely on knowing about the Python internals (and because I think it'd have to be d != first because

>>> False < True True

(and because I think it'd have to be d != first because

>>> False < True True

We have reverse=True as an argument to the sorted function above. So, I believe d == first is the correct one.

assert sorted([False, True], reverse=True) == [True, False]

Anyway, I'll leave the existing code as-is, leaving the template to handle that logic for now.

microamp · 2024-08-30T02:41:10Z

ietf/ipr/views.py

+                    updated_docs = related_docs(first, ("updates",))
+                    related_iprs = list(
+                        set(iprs_from_docs(updated_docs, states=states)) - set(iprs)
+                    )


Nitpick: Formatting changes can be done in a separate PR.

I ran black on the unit. I could have done it in a separate commit.

jennifer-richards

Inline, I have one non-essential (but I would encourage) reformatting suggestion for template readability.

The "request changes" is really just to ask whether the gathering of docs deals well enough with a list that contains a document that is related to more than one document in the set. At a quick read, I think it'll wind up with an unpredictable related value "winning" in the list that actually gets rendered and cause incorrect output. However, I'm not sure whether the data are such that this is a real concern. It'd be good to check.

(Not opposed to merging this as an improvement and handling that as a separate issue if it actually is one)

jennifer-richards · 2024-08-30T17:05:52Z

ietf/ipr/views.py

+                            d.rfc_number if d.rfc_number is not None else 0,
+                            d.became_rfc().rfc_number if d.became_rfc() else 0,
+                        ),
+                        reverse=True,


I think 0 if d == first else 1 would be safer since it doesn't rely on knowing about the Python internals (and because I think it'd have to be d != first because

>>> False < True True

jennifer-richards · 2024-08-30T17:27:44Z

ietf/templates/ipr/search_doc_result.html

            <tbody>
                <tr>
                    <th scope="col" class="table-info" colspan="3">
-                        Results for {{ doc.name|prettystdname|urlize_ietf_docs }} ("{{ doc.title }}"){% if not forloop.first %}{% if doc.related %}, which was {{ doc.relation|lower }} {{ doc.related.source|prettystdname|urlize_ietf_docs  }} ("{{ doc.related.source.title }}"){% endif %}{% endif %}
+                        Results for {{ d.name|prettystdname|urlize_ietf_docs }} ("{{ d.title }}"){% if d != doc %}{% if d.related %}{% if d != d.related.source %}, which was {{ d.relation|lower }} {{ d.related.source|prettystdname|urlize_ietf_docs }} ("{{ d.related.source.title }}"){% else %}, which {{ d.relation|lower}} {{ d.related.target|prettystdname|urlize_ietf_docs }} ("{{ d.related.target.title }}"){% endif %}{% endif %}{% endif %}


shudder

I think maybe the following refactor reduces the 🍝 factor a little?

Results for {{ d.name|prettystdname|urlize_ietf_docs }} ("{{ d.title }}"){% if d != doc and d.related %}, which {% if d == d.related.source %} {{ d.relation|lower }} {{ d.related.target|prettystdname|urlize_ietf_docs }} ("{{ d.related.target.title }}") {% else %} was {{ d.relation|lower }} {{ d.related.source|prettystdname|urlize_ietf_docs }} ("{{ d.related.source.title }}") {% endif %} {% endif %}

It introduces a lot of whitespace in the resulting html - something Lars was pushing back against pretty hard.

Readable template seem more valuable to me. Could also flatten the indentation if it's about file size since a CR doesn't actually consume any more bits than a space...

(I'm sympathetic to itching due to awful html whitespace, but not convinced it's a driving concern)

The refactor suggested above is in. We can either keep it or revert it.

6e4c803 shows how many whitespace characters were introduced by the previous commit.

Seems ok to me. I don't want to pick a fight with Lars though. :-)

microamp · 2024-09-10T04:57:05Z

The "request changes" is really just to ask whether the gathering of docs deals well enough with a list that contains a document that is related to more than one document in the set. At a quick read, I think it'll wind up with an unpredictable related value "winning" in the list that actually gets rendered and cause incorrect output. However, I'm not sure whether the data are such that this is a real concern. It'd be good to check.

I don't think that behaviour is possible, but there may be some edge cases that I'm not aware of. Do you have a specific example that I can verify locally?

microamp · 2024-09-10T08:49:31Z

The unit test failing could be related to #7918.

jennifer-richards · 2024-09-10T15:31:46Z

I don't think that behaviour is possible, but there may be some edge cases that I'm not aware of. Do you have a specific example that I can verify locally?

No, I was just worried because the new structure of the code collects some docs, then adds docs related to those docs, then adds docs related to those docs. However, I looked more closely and in pseudocode I believe it's gathering:

DraftsThatBecameThisRFC(doc + ReplacedBy(doc) + ObsoletedBy(doc))

so a collision would mean something like an RFC came from two drafts, one of which replaced/obsoleted the other, and that can't happen.

rjsparks · 2024-09-10T22:19:06Z

ietf/ipr/tests.py

+        self.assertContains(
+            r,
+            f"""Results for <a href="/doc/{rfc_new.name}/">{prettify_std_name(rfc_new.name)}</a>
+                        ("{rfc_new.title}")""",
+        )
+        self.assertContains(
+            r,
+            f"""Results for <a href="/doc/{rfc.name}/">{prettify_std_name(rfc.name)}</a>
+                        ("{rfc.title}"), which
+
+                                was obsoleted by
+                                <a href="/doc/{rfc_new.name}/">{prettify_std_name(rfc_new.name)}</a>
+                                ("{rfc_new.title}")""",
+        )
+        self.assertContains(
+            r,
+            f"""Results for <a href="/doc/{draft.name}/">{prettify_std_name(draft.name)}</a>
+                        ("{draft.title}"), which
+
+                                became rfc
+                                <a href="/doc/{rfc.name}/">{prettify_std_name(rfc.name)}</a>
+                                ("{rfc.title}")""",
+        )
+


Having a test live at risk of failing because of whitespace change in the html isn't good.
I'll let this in, but we should change this soon to something that is testing for the important content and not the layout details. Looking just to see that the right drafts and RFCs are mentioned should be enough.

fix: Include missing related drafts in IPR searches

1147c4f

microamp requested a review from rjsparks August 19, 2024 08:28

rjsparks added 2 commits August 29, 2024 08:39

Merge branch 'main' into 7824-ipr-search

f643b1c

refactor: extract drafts, sort docs

62a52ee

microamp commented Aug 30, 2024

View reviewed changes

microamp requested a review from jennifer-richards August 30, 2024 04:14

microamp marked this pull request as ready for review August 30, 2024 04:14

jennifer-richards requested changes Aug 30, 2024

View reviewed changes

microamp added 2 commits September 10, 2024 13:07

chore: indent loop and conditionals to improve readability

195f8cb

test: handle whitespaces added to IPR search result page

6e4c803

Merge branch 'main' into 7824-ipr-search

a4fc313

jennifer-richards approved these changes Sep 10, 2024

View reviewed changes

rjsparks approved these changes Sep 10, 2024

View reviewed changes

rjsparks merged commit 80599f2 into ietf-tools:main Sep 10, 2024
9 checks passed

microamp deleted the 7824-ipr-search branch September 10, 2024 22:50

microamp mentioned this pull request Sep 11, 2024

test: check IPR search results in HTML with whitespace ignored #7921

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Include missing related drafts in IPR searches #7836

fix: Include missing related drafts in IPR searches #7836

microamp commented Aug 19, 2024

microamp commented Aug 19, 2024

codecov bot commented Aug 19, 2024 •

edited

Loading

microamp commented Aug 19, 2024 •

edited

Loading

rjsparks commented Aug 29, 2024

microamp left a comment

microamp Aug 30, 2024

jennifer-richards Aug 30, 2024

jennifer-richards Aug 30, 2024

rjsparks Sep 3, 2024

microamp Sep 10, 2024 •

edited

Loading

microamp Aug 30, 2024

jennifer-richards Aug 30, 2024

microamp Sep 10, 2024

microamp Aug 30, 2024

rjsparks Sep 3, 2024

jennifer-richards left a comment

jennifer-richards Aug 30, 2024

jennifer-richards Aug 30, 2024

rjsparks Sep 3, 2024

jennifer-richards Sep 4, 2024

microamp Sep 10, 2024

microamp Sep 10, 2024

jennifer-richards Sep 10, 2024

microamp commented Sep 10, 2024 •

edited

Loading

microamp commented Sep 10, 2024

jennifer-richards commented Sep 10, 2024 •

edited

Loading

rjsparks Sep 10, 2024

fix: Include missing related drafts in IPR searches #7836

fix: Include missing related drafts in IPR searches #7836

Conversation

microamp commented Aug 19, 2024

microamp commented Aug 19, 2024

codecov bot commented Aug 19, 2024 • edited Loading

Codecov Report

microamp commented Aug 19, 2024 • edited Loading

rjsparks commented Aug 29, 2024

microamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microamp Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennifer-richards left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microamp commented Sep 10, 2024 • edited Loading

microamp commented Sep 10, 2024

jennifer-richards commented Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 19, 2024 •

edited

Loading

microamp commented Aug 19, 2024 •

edited

Loading

microamp Sep 10, 2024 •

edited

Loading

microamp commented Sep 10, 2024 •

edited

Loading

jennifer-richards commented Sep 10, 2024 •

edited

Loading