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: Include time zone information in v1 api datetimes #5851

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

jennifer-richards
Copy link
Member

This customizes the Tastypie serializer to change datetimes in the v1 API JSON to be of the format "2023-03-24T09:36:43+00:00". That is, it adds the time zone offset and suppresses fractional seconds.

However, to my reading of draft-ietf-sedate-datetime-extended, a Z suffix is appropriate when UTC is used incidentally rather than because it is meaningful as a local time. I think that means that a Z would be better as our default, and in (any?) cases where we're serializing times that have a significant local time, we can override the default. I don't know whether there are any where we'd care.

To do this, we just change the format_datetime() method to return

        return data.astimezone(datetime.timezone.utc).replace(tzinfo=None).isoformat(timespec="seconds") + "Z"

@cabo
Copy link
Collaborator

cabo commented Jun 19, 2023

This customizes the Tastypie serializer to change datetimes in the v1 API JSON to be of the format "2023-03-24T09:36:43+00:00". That is, it adds the time zone offset and suppresses fractional seconds.

However, to my reading of draft-ietf-sedate-datetime-extended, a Z suffix is appropriate when UTC is used incidentally rather than because it is meaningful as a local time. I think that means that a Z would be better as our default,

Yes.

and in (any?) cases where we're serializing times that have a significant local time, we can override the default. I don't know whether there are any where we'd care.

Since RFC 3339 timestamps are mostly seen by humans only during debugging, I'm having a hard time imagining such a case (unless there is programmatic access to timezone information -- is there any?).
(But this is proof by lack of imagination, so I'm happy to read:)

To do this, we just change the format_datetime() method to return

        return data.astimezone(datetime.timezone.utc).replace(tzinfo=None).isoformat(timespec="seconds") + "Z"

@jennifer-richards
Copy link
Member Author

(But this is proof by lack of imagination, so I'm happy to read:)

I can't think of any likely cases where we'd use this. I'm wary of the lack of imagination factor myself, though, so am inclined to maintain the threat that we might some day use other formats allowed by RFC 3339. That is probably more likely for fractional seconds than for getting tricky with time zones, though.

@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #5851 (9376d4e) into main (62d6891) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5851      +/-   ##
==========================================
+ Coverage   88.66%   88.74%   +0.07%     
==========================================
  Files         288      288              
  Lines       39949    39965      +16     
==========================================
+ Hits        35421    35465      +44     
+ Misses       4528     4500      -28     
Impacted Files Coverage Δ
ietf/api/__init__.py 69.69% <100.00%> (+0.62%) ⬆️
ietf/doc/feeds.py 99.24% <100.00%> (+23.61%) ⬆️
ietf/group/views.py 90.53% <100.00%> (+<0.01%) ⬆️
ietf/meeting/models.py 85.15% <100.00%> (ø)
ietf/nomcom/forms.py 95.39% <100.00%> (ø)

... and 3 files with indirect coverage changes

@rjsparks rjsparks merged commit 0144ee9 into ietf-tools:main Jun 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2023
@jennifer-richards jennifer-richards deleted the api-timestamps branch June 26, 2023 15:42
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.

4 participants