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: display document expiration notification in UTC #6600

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

meadmaker
Copy link
Collaborator

The document expiration mail displays the date of the upcoming document expiration, but different timezones makes that date ambituous. Instead, clearly show that as a timestamp in UTC.

Fixes #1825

The document expiration mail displays the date of the upcoming document expiration, but different timezones makes that date ambituous.  Instead, clearly show that as a timestamp in UTC.

Fixes ietf-tools#1825
Two templates seem related to the expiration notifications, but they aren't actually used.  Get rid of them.
Copy link

codecov bot commented Nov 5, 2023

Codecov Report

Merging #6600 (5dfbaab) into main (d246879) will increase coverage by 0.01%.
Report is 19 commits behind head on main.
The diff coverage is 96.00%.

❗ Current head 5dfbaab differs from pull request most recent head 602515c. Consider uploading reports for the commit 602515c to get more accurate results

@@            Coverage Diff             @@
##             main    #6600      +/-   ##
==========================================
+ Coverage   88.85%   88.86%   +0.01%     
==========================================
  Files         284      284              
  Lines       40253    40275      +22     
==========================================
+ Hits        35765    35791      +26     
+ Misses       4488     4484       -4     
Files Coverage Δ
ietf/doc/expire.py 95.31% <100.00%> (ø)
ietf/doc/urls_review.py 100.00% <ø> (ø)
ietf/review/models.py 92.85% <100.00%> (+0.88%) ⬆️
ietf/doc/views_review.py 95.14% <94.73%> (-0.02%) ⬇️

... and 3 files with indirect coverage changes

@rjsparks rjsparks changed the title Display document expiration notification in UTC fix: display document expiration notification in UTC Nov 5, 2023
@rjsparks
Copy link
Member

rjsparks commented Nov 5, 2023

@meadmaker - please start using this for your commit messages: https://www.conventionalcommits.org/en/v1.0.0/

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.

Requesting a minor code cleanup as noted inline.

I also want to point out to @rjsparks that this will change the deadline from a tz-naive date to the UTC datetime describing midnight Pacific time. That was what I suggested as my best guess as the way to do things.

Thinking about it now, I think it'll be midnight at the start of the doc.expires date, which might be incorrect. Maybe it needs to be +datetime.timedelta(days=1).

DEADLINE_TZINFO
).replace(
hour=0, minute=0, second=0, microsecond=0
).astimezone(
Copy link
Member

Choose a reason for hiding this comment

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

Don't actually need to convert the timezone to UTC - the |utc in the template will do the right thing (also, just as a non-obvious fyi, we're trying to use datetime.timezone.utc as the UTC zone rather than ZoneInfo("UTC") or any of the various other synonyms)

@rjsparks
Copy link
Member

rjsparks commented Nov 6, 2023

@larseggert - Can we by fiat declare draft expiration happens at UTC midnight, or do we need more process first?

@larseggert
Copy link
Collaborator

larseggert commented Nov 6, 2023

I think IESG can declare.

How easy would it be to do "anywhere on Earth"?

@rjsparks
Copy link
Member

rjsparks commented Nov 6, 2023

Anywhere on earth essentially means "international date line" instead of "Greenwich Meridian". It is easy to do, but it runs counter to the previous expressed goal to do everything in UTC.

@cabo
Copy link
Collaborator

cabo commented Nov 6, 2023 via email

@meadmaker
Copy link
Collaborator Author

My takeaways here are:

I'm going to go work on that, and submit it to this issue. I'm going to look for opportunities to make the expiration of documents more explicit, though - we've already found at least six reasonable interpretations for when that could be (beginning of date and end of date, crossed with PDT, UTC, and International Dateline). I wonder whether it would be better to convert the date into a datestamp, in order to make that explicit.

@meadmaker
Copy link
Collaborator Author

I wonder whether it would be better to convert the date into a datestamp, in order to make that explicit.

I should have said timestamp instead of datestamp. Oops.

I just examined the doc_document table, and the expires column is already a timestamp data type. It looks like that is set in ietf/submit/utils.py for regular submission and ietf/doc/views_draft.py for document resurrection by the Secretariat. I expect that if I can direct those two code paths through a single function to compute the expiration timestamp correctly, then all the other calculations can be converted to simple value display instead.

@rjsparks
Copy link
Member

rjsparks commented Sep 4, 2024

This lost attention during the transition period. Lets see if it can either be finished or turned back into a clarified issue.
cc: @jennifer-richards

@jennifer-richards
Copy link
Member

I believe the main hold-up is deciding what is the desired functionality.

Two parts:

  1. what time of day is the expiration
  2. where on Earth do you specify that time

I think 1) is "end of the expiration date" - at least, @cabo and I seem to agree.

For 2), UTC or "anywhere on earth" have been suggested.

The current implementation, if I understand it, will expire a document at the stroke of midnight UTC at the start of the expiration date. I think that leaves everyone unhappy, because we either want 12 hours later (for the end of day "anywhere on earth" standard, neglecting the Millenium Islands time zone) or 24 hours later (for end of day UTC)

@rjsparks
Copy link
Member

rjsparks commented Sep 4, 2024

I agree with 1 and 2. We should build a reusable component to do that calculation. Is this PR a sufficient basis to work with to shift to that point, or should we approach the codebase again more generally?

@jennifer-richards
Copy link
Member

I'd say this is a good place to start.

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.

Draft reminder emails need to show UTC times, and show draft submission tool closures if relevant
5 participants