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

Make <time datetime= into valid HTML by using a custom jinja tag #6058

Closed
wants to merge 0 commits into from
Closed

Conversation

anorthall
Copy link
Contributor

@anorthall anorthall commented Aug 10, 2021

Status

Ready for review

Description of Changes

Fixes #6057

Change the direct call to __str__() into a call to strftime() to meet the requirement of not having more than 3 decimal places for seconds on HTML. Now, no decimal places are displayed. I decided to implement this instead of the truncate filter as you cannot truncate the datetime object without first converting it to a string.

Testing

Open the website and validate the HTML.

Deployment

No special measures required.

Checklist

  • These changes do not require documentation

@anorthall anorthall requested a review from a team as a code owner August 10, 2021 16:36
@cfm
Copy link
Member

cfm commented Aug 10, 2021

Thank you, @anorthall! Just a heads-up that this will likely require a rebase after #6041, which moves around the time elements you fix here.

@anorthall
Copy link
Contributor Author

Thank you, @anorthall! Just a heads-up that this will likely require a rebase after #6041, which moves around the time elements you fix here.

No problem.

@zenmonkeykstop
Copy link
Contributor

Hey, @anorthall, the fix using strftime() looks good - one thing it doesn't account for (and this is an existing problem, not one you added) is formatting of the time values to different locales. If you want to try to support that, the approach I'd suggest would be to:

a) use flask_babel to do the date formatting
b) do the formatting in a custom jinja filter, to make it reusable across the application if necessary (I think there's only one other instance of <time />

In either case, thanks for the PR!

@anorthall
Copy link
Contributor Author

Hey, @anorthall, the fix using strftime() looks good - one thing it doesn't account for (and this is an existing problem, not one you added) is formatting of the time values to different locales.

Thanks for the tip/info - I will work on this but not until later tomorrow/Thursday at the latest.

@zenmonkeykstop
Copy link
Contributor

Thanks for the tip/info - I will work on this but not until later tomorrow/Thursday at the latest.

No problem, and no rush!

@anorthall
Copy link
Contributor Author

anorthall commented Aug 12, 2021

Hopefully that latest PR will be a better solution. As you can see I added the template tag (html_datetime_format), and I also updated the other location that <time> was used, in the journalist portal.

All tested and working, and the HTML validates.

@anorthall anorthall changed the title Make <time datetime= into valid HTML by using strftime() Make <time datetime= into valid HTML by using a custom jinja tag Aug 14, 2021
@cfm cfm self-assigned this Aug 18, 2021
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

Test plan

  • Open the website and validate the HTML.

In addition:

  • Specifically, there are no more Bad value errors on the time[datetime] attributes.
  • All instances of the pattern %Y-%m-%d %H:%M:%S%Z have been replaced by the html_format_datetime filter.

Looks good to me, @anorthall! I've left a comment about the localization question, which I've clarified with @zenmonkeykstop; fortunately the solution is just a deletion here and another issue I'll open separately. :-)

Given that there are a couple of iterations recorded in your branch, it would be ideal if you could squash your commits before merge. Or, if you prefer, @zenmonkeykstop can take care of that as the maintainer who will ultimately approve and merge.

securedrop/template_filters.py Outdated Show resolved Hide resolved
@anorthall
Copy link
Contributor Author

anorthall commented Aug 24, 2021

@cfm Changes made and squashed

I note the updater-gui-tests fail is nothing to do with my PR.

@anorthall anorthall requested review from cfm and removed request for a team August 24, 2021 10:37
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

Thanks, @anorthall! I confirm that updater-gui-tests is failing without #6069, but since it's irrelevant to your changes here I agree it's not worth rebasing for it.

Tagging @zenmonkeykstop for final approval and merge.

@cfm cfm removed their assignment Aug 24, 2021
@zenmonkeykstop
Copy link
Contributor

Apologies @anorthall, a rebase gone awry reverted the changes in the PR - I'll fix it momentarily!

@zenmonkeykstop
Copy link
Contributor

Hey @anorthall - I've recovered your changes and resubmitted them in PR #6075 rebased against the origin develop (so CI should pass). Sorry for the mix-up - it looks like the rebase simply overwrote your develop branch with the origin one, the simplest path to recovery was to grab your commit from the reflog and apply them to a new (non-develop) branch on origin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid datetime attributes
3 participants