Skip to content

feat: Render the document shepherd writeup templates at two new URLs#4225

Merged
rjsparks merged 8 commits into
ietf-tools:mainfrom
larseggert:docshep-render
Jul 22, 2022
Merged

feat: Render the document shepherd writeup templates at two new URLs#4225
rjsparks merged 8 commits into
ietf-tools:mainfrom
larseggert:docshep-render

Conversation

@larseggert

@larseggert larseggert commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

Those being /doc/shepherdwriteup-template/group and /doc/shepherdwriteup-template/individual.

CC @ghwood.

@codecov

codecov Bot commented Jul 18, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4225 (30db611) into main (6ba9114) will increase coverage by 0.20%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##             main    #4225      +/-   ##
==========================================
+ Coverage   88.20%   88.40%   +0.20%     
==========================================
  Files         295      294       -1     
  Lines       39279    39202      -77     
==========================================
+ Hits        34645    34656      +11     
+ Misses       4634     4546      -88     
Impacted Files Coverage Δ
ietf/doc/urls.py 100.00% <ø> (ø)
ietf/utils/markdown.py 100.00% <ø> (ø)
ietf/doc/views_draft.py 91.98% <93.75%> (ø)
ietf/doc/views_doc.py 90.52% <100.00%> (+0.02%) ⬆️
ietf/utils/text.py 85.97% <100.00%> (-0.99%) ⬇️
ietf/utils/draft.py 71.39% <0.00%> (-0.21%) ⬇️
ietf/group/dot.py
ietf/nomcom/utils.py 91.90% <0.00%> (+0.52%) ⬆️
ietf/ietfauth/utils.py 91.35% <0.00%> (+0.61%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bdbf7c...30db611. Read the comment docs.

@larseggert larseggert changed the title feat: Render the document shepherd writeup templates at two new URL. feat: Render the document shepherd writeup templates at two new URLs Jul 18, 2022

@jennifer-richards jennifer-richards left a comment

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.

A couple comments inline - the awkward Boolean logic in the template bugs me a bit. Maybe consider always passing type in the context instead of adding the complication of looking both at type and doc?

Also, I think this is the only place where the writeup is rendered with markdown - is that intentional? (I don't think it's rendered in the document_shepherd_writeup() view yet, but maybe I missed it?

These are just for consideration, no objection to running with it as-is.

Comment thread ietf/templates/doc/shepherd_writeup.txt Outdated
Comment thread ietf/doc/urls.py Outdated
@larseggert

Copy link
Copy Markdown
Collaborator Author

@jennifer-richards, thanks for the review. I think I addressed your comments in the latest push.

Yes, this is the only place where we render the template as Markdown - it only recently became Markdown, so I don't see a good way to do so when displaying submitted writeups.

Comment thread ietf/utils/markdown.py Outdated
@larseggert

Copy link
Copy Markdown
Collaborator Author

I restored the bleach step

@rjsparks

Copy link
Copy Markdown
Member

I wonder if it would make sense to bleach text before sending into markdown instead. I can merge this as is, but if you think this is a reasonable idea, maybe make another PR?

@larseggert

Copy link
Copy Markdown
Collaborator Author

Do you want it in another PR, or can I add it to this one?

@jennifer-richards

Copy link
Copy Markdown
Member

I'm worried about whether bleaching the input leads to a false sense of security. I haven't found recent discussions, but there seems to be a general sense that feeding markdown into bleach is not valid. I think we should probably stick to teaching bleach to allow the things we need or we're likely baking in vulnerabilities.

@rjsparks

Copy link
Copy Markdown
Member

I think adding it to this one is fine. Could you and Jennifer iterate on whether we're doing the right thing with bleach at the right place?

@jennifer-richards

Copy link
Copy Markdown
Member

@larseggert, Were there particular pieces of markdown formatting that bleach was still stripping?

@jennifer-richards

Copy link
Copy Markdown
Member

Just looked - I think the "id" attributes were being stripped from the rendered HTML:

$ diff bleached-individual.html unbleached-individual.html 
616c616
<     <h1>Document Shepherd Write-Up for Individual Documents</h1>
---
>     <h1 id="document-shepherd-write-up-for-individual-documents">Document Shepherd Write-Up for Individual Documents</h1>
626c626
< <h2>Document History</h2>
---
> <h2 id="document-history">Document History</h2>
650c650
< <h2>Additional Reviews</h2>
---
> <h2 id="additional-reviews">Additional Reviews</h2>
676c676
< <h2>Document Shepherd Checks</h2>
---
> <h2 id="document-shepherd-checks">Document Shepherd Checks</h2>

and similar for the group template. Adding the "id" attribute to the allowed attributes list in ietf/utils/text.py (attributes["*"] = ["id"], same as is done for the "start" attribute for "ol") fixes that and reduces the diff to null.

@larseggert

Copy link
Copy Markdown
Collaborator Author

Good catch. I'll see if I can cook up a test to see if there are others.

@larseggert

Copy link
Copy Markdown
Collaborator Author

There weren't any others, so I think this is good to go.

@rjsparks
rjsparks merged commit 1ba8789 into ietf-tools:main Jul 22, 2022
@larseggert
larseggert deleted the docshep-render branch July 22, 2022 19:06
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 26, 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.

3 participants