Skip to content

fix: Use the doc name matched by fuzzy_find_documents when rendering#4862

Closed
larseggert wants to merge 5 commits intoietf-tools:mainfrom
larseggert:fix-4855
Closed

fix: Use the doc name matched by fuzzy_find_documents when rendering#4862
larseggert wants to merge 5 commits intoietf-tools:mainfrom
larseggert:fix-4855

Conversation

@larseggert
Copy link
Copy Markdown
Collaborator

Fixes #4855

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 9, 2022

Codecov Report

Merging #4862 (4efa1d9) into main (bc1cba1) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4862      +/-   ##
==========================================
- Coverage   88.47%   88.46%   -0.01%     
==========================================
  Files         296      296              
  Lines       39795    39796       +1     
==========================================
- Hits        35208    35206       -2     
- Misses       4587     4590       +3     
Impacted Files Coverage Δ
ietf/doc/views_doc.py 90.79% <100.00%> (+<0.01%) ⬆️
ietf/utils/pipe.py 82.60% <0.00%> (-4.35%) ⬇️
ietf/utils/draft.py 71.44% <0.00%> (-0.31%) ⬇️
ietf/nomcom/utils.py 91.30% <0.00%> (-0.25%) ⬇️
ietf/doc/views_search.py 89.64% <0.00%> (ø)
ietf/person/utils.py 86.98% <0.00%> (+0.59%) ⬆️
ietf/utils/text.py 87.19% <0.00%> (+0.60%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment thread ietf/doc/views_doc.py Outdated
@larseggert larseggert requested a review from rjsparks December 12, 2022 16:42
@larseggert
Copy link
Copy Markdown
Collaborator Author

@rjsparks you mentioned in Slack that this PS is wrong. I tried to do what you said before, i.e.,

Why aren't you changing the conditional at L812 instead? That would cause the URL to get updated in the browser.

But I guess I misunderstood?

@larseggert
Copy link
Copy Markdown
Collaborator Author

OBE, see #4897.

@larseggert larseggert closed this Dec 16, 2022
@larseggert larseggert deleted the fix-4855 branch December 16, 2022 12:27
@rjsparks
Copy link
Copy Markdown
Member

I see I have some pending comments on this - failed a submit button somewhere along the way. I'll try to let them come through for completeness, but treat them as FYI only.

Comment thread ietf/doc/views_doc.py

if found.matched_name.startswith('rfc') and name != found.matched_name:
return redirect('ietf.doc.views_doc.document_html', name=found.matched_name)
name = found.matched_name
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.

This does nothing - name is never used after this assignment.

Comment thread ietf/doc/views_doc.py
@@ -810,7 +810,8 @@ def document_html(request, name, rev=None):
raise Http404("Multiple documents matched: %s" % name)

if found.matched_name.startswith('rfc') and name != found.matched_name:
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.

The problem you are working on is dealing with URLs like https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-07. And the issue is that the URL dispatcher is handing

    [name, rev]: '['draft-ietf-oauth-v2', '1-07']'

To the view.
What's in the PR right now won't help since (beyond it being a no-op) the change is in the logic branch gated on finding something that starts with 'rfc'.

Your original commit was closer, and the property I was hoping to get out of the redirect isn't going to happen since the globbing is getting things so wrong to begin with.

I'll take the PR back to that commit, but we need to add tests to make sure that this isn't just pushing the general problem around and breaking other things.

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.

Our real problem is that we have

   "rev": r"(?P<rev>[0-9]{1,2}(-[0-9]{2})?)",

in URL_REGEXPS so that we can match charter document versions that look like -00-01, which was a terrible thing to have let happen, and maybe we should refactor the URL parsers to match charters first with that rev pattern and everything else with just [0-9]{2}.

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.

For instance, this is 404ing, and it shouldn't:

https://datatracker.ietf.org/doc/html/draft-ietf-tsvwg-ieee-802-11

It should be showing RFC8325.

Your earlier fix causes it to show draft-ietf-tsvwg-ieee-802-11-11, so there's more to fix than just this name change.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 20, 2022
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.

OAuth 2.1 links show OAuth 2.0 (Problem with drafts with version numbers in the name)

2 participants