Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Document type badges #7760

Merged
merged 14 commits into from
Aug 28, 2024
Merged

Conversation

holloway
Copy link
Contributor

See #7475

ietf/doc/templatetags/document_type_badge.py Outdated Show resolved Hide resolved
ietf/doc/templatetags/document_type_badge.py Outdated Show resolved Hide resolved
ietf/templates/doc/badge/doc-badge-rfc.html Outdated Show resolved Hide resolved
{# Non-RFC #}

{% if doc.became_rfc %}
<div{% if document_html %} class="alert alert-warning small"{% endif %} style="border: dashed 3px green">This is an older version of an Internet-Draft that was ultimately published as <a href="{% if document_html %}{% url 'ietf.doc.views_doc.document_html' name=doc.became_rfc.name %}{% else %}{% url 'ietf.doc.views_doc.document_main' name=doc.became_rfc.name %}{% endif %}">{{doc.became_rfc.name|prettystdname}}</a>.</div>
Copy link
Member

Choose a reason for hiding this comment

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

why introduce new styling inline? Can this be moved to the css?

Copy link
Member

Choose a reason for hiding this comment

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

I think we still have inline styling in this branch and that it's being applied in cases that aren't intended - right now the tests are failing complaining about the inline styling, and because instead of this on main:

image

we're seeing this on the PR branch:

image

This is an active draft, so why the dashed lines, and what happened to the (stir WG) link?

Copy link
Member

Choose a reason for hiding this comment

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

That (stir WG) link is what's passed in as submission

ietf/templates/doc/document_info.html Show resolved Hide resolved
@rjsparks
Copy link
Member

@holloway : You've addressed my comments so far. What remains before this PR moves out draft status?

@holloway holloway marked this pull request as ready for review August 13, 2024 18:07
rjsparks
rjsparks previously approved these changes Aug 14, 2024
@rjsparks
Copy link
Member

@holloway (or anyone else puzzled by the test failures earlier): at 1c3fd00 - the issue that caused test fails reporting an invalid document type of "Draft" was the compare and render using type vs type_id:

>>> from ietf.doc.models import Document
>>> d = Document.objects.filter(type_id="draft").last()
>>> type(d.type)
<class 'ietf.name.models.DocTypeName'>
>>> d.type=="draft"
False
>>> str(d.type)
'Draft'

One bit of django queryset syntactic "assistance" to be aware of. The following are equivalent, because the ORM does helpful work - assume dt = d.type from above (that is, dt is <DocTypeName: Draft>)

Document.objects.filter(type_id="draft")
Document.objects.filter(type="draft")
Document.objects.filter(type=dt)

All of which turn into this SQL

>>> str(Document.objects.filter(type=dt).query)
'SELECT "doc_document"."id", "doc_document"."time", "doc_document"."type_id", "doc_document"."title", 
"doc_document"."stream_id", "doc_document"."group_id", "doc_document"."abstract", "doc_document"."rev", 
"doc_document"."pages", "doc_document"."words", "doc_document"."intended_std_level_id", 
"doc_document"."std_level_id", "doc_document"."ad_id", "doc_document"."shepherd_id", "doc_document"."expires", 
"doc_document"."notify", "doc_document"."external_url", "doc_document"."uploaded_filename", "doc_document"."note", 
"doc_document"."internal_comments", "doc_document"."rfc_number", "doc_document"."name" FROM "doc_document" 
WHERE "doc_document"."type_id" = draft'



@register.simple_tag
def document_type_badge(doc, snapshot):
Copy link
Member

Choose a reason for hiding this comment

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

submission from the context is being lost here I think

Internet-Draft
{% endif %}
</span>
{% document_type_badge doc snapshot%}
Copy link
Member

Choose a reason for hiding this comment

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

this also needs the context?

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.78%. Comparing base (c7f6bde) to head (42f3fa2).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
ietf/doc/templatetags/document_type_badge.py 70.58% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7760      +/-   ##
==========================================
- Coverage   88.78%   88.78%   -0.01%     
==========================================
  Files         296      304       +8     
  Lines       41320    41439     +119     
==========================================
+ Hits        36687    36792     +105     
- Misses       4633     4647      +14     

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

rjsparks
rjsparks previously approved these changes Aug 26, 2024
@rjsparks
Copy link
Member

Asking for a second review given the twisty passages

Copy link
Member

@jennifer-richards jennifer-richards left a comment

Choose a reason for hiding this comment

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

Just a couple minor things

ietf/doc/templatetags/document_type_badge.py Outdated Show resolved Hide resolved
ietf/doc/templatetags/document_type_badge.py Outdated Show resolved Hide resolved
@rjsparks rjsparks merged commit 17bd312 into ietf-tools:main Aug 28, 2024
9 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2024
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