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

Delegate date strings formatting to datetime.isoformat() #6413

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gonzalo-bulnes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes commented Apr 27, 2022

Status

Ready for review

Description of Changes

Closes #6257 (see Proposed plan section in the description).

Changes proposed in this pull request:

  • Remove the custom date string format (that used to be the only one supported by the SecureDrop Client)
  • Use ISO8061 via datetime.isoformat() instead (should be easier to use consistently too)

Testing

  • Visual review: do these changes make sense given the context recapped below?

Then test here:

  • Pay special attention to CI: do the tests changed here still pass?

And then check for surprises downstream:

  • Run make dev here and make regenerate-sdk-cassettes in freedomofpress/securedrop-client/client. It should pass.
  • Run make test in freedomofprses/securedrop-client/client. It should pass.

Deployment

Once this change has been released, the try/except logic in https://github.com/freedomofpress/securedrop-client/blob/7f7f60e67f7606708a6c9ef519c9657cd738be9e/client/securedrop_client/sdk/timestamps.py can be removed in favor of just datetime.fromisoformat().

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

@gonzalo-bulnes gonzalo-bulnes force-pushed the delegate-tz-string-formatting-to-datetime branch from 1eff662 to 1aaa7b3 Compare April 27, 2022 21:41
@gonzalo-bulnes
Copy link
Contributor Author

@legoktm I'm trying to gather again the context around this PR. It seems like one of the API responses contains timestamps without time zone information (specific error in CI). I believe that's a legit test suite error and that we want to fix the API response to conform to the new convention, does that seem right to you?

The SecureDrop SDK now supports date stings in ISO8061 format,
so we can stop relying on custom-defined formats.

The custom JSON formatter made this a one-line change!

Co-authored-by: Kunal Mehta <[email protected]>
@cfm cfm force-pushed the delegate-tz-string-formatting-to-datetime branch from 1aaa7b3 to da8f43f Compare December 5, 2024 20:23
@cfm cfm marked this pull request as ready for review December 5, 2024 20:37
@cfm cfm requested a review from a team as a code owner December 5, 2024 20:37
@cfm
Copy link
Member

cfm commented Dec 5, 2024

Tl;dr: This change does not have any side effects on time-zone support. It simply standardizes us to datetime.isoformat() and should be either approved or closed based whether we currently think that standardization is worthwhile.

We decided in today's backlog-pruning session that this was worth revisiting in a time-box to see if we could finalize it. Substantively all I've done is rebase this from develop. But to recap the context here:

  1. securedrop currently sends UTC timestamps in a custom API_DATETIME_FORMAT.
  2. With this change, securedrop will send UTC timestamps exclusively in datetime.isoformat().
  3. securedrop-client, specifically the SDK, will already parse either (1) or (2). (The test plan above confirms this.)
  4. After this change has been released, securedrop-client can drop the trial-and-error parsing in (3).

@cfm cfm requested a review from legoktm December 5, 2024 20:38
@legoktm legoktm self-assigned this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

API responses should include time-zone information, be valid ISO8061/RFC339
3 participants