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

Be explicit about using UTC #2781

Closed

Conversation

richardTowers
Copy link
Contributor

Draft PR, because the more I think about this the less confident I am that it's the right thing to do.

This PR makes publishing API be explicit about using UTC when its turning dates into strings.

This is to allow us to be consistent in the timezone in GOV.UK apps - they should all use London, and publishing-api should be consistent with that.

However, now that I've implemented this, I'm not sure it really does make sense for publishing-api to have a London timezone. Because it's not user facing, it's never got a reason to turn a time into a string in the user's timezone. And because it's an API, it usually wants to render times in UTC. If there's never a case where we want to render things in the London timezone, maybe it would be better for us to say "publishing apps and frontends (which have human users) should use London as the timezone, but API-only apps should use UTC". Otherwise there's a bit of a risk of getting it wrong (e.g. doing datetime.iso8601 instead of datetime.utc.iso8601). 🤔 🤔 🤔

If we do want to have some UTC apps and some London apps, it might be that alphagov/govuk_app_config#381 is not such a good idea after all. We could introduce something like config.govuk_time_zone = "UTC" to allow apps to be explicit, but maybe we'd be better off without the weirdness in the gem and just setting London explicitly instead...

Discussion very welcome 😅

public_timestamp comes from activerecord, so it will be UTC. If we compare it with a time in the current timezone (London), it won't be equal.
new_first_published_at is already explicitly UTC in this test, before it's turned into a string with iso8601. If we compare a time in the current timezone (London) with that, it won't be equal.
And add a test for this behaviour
... this probably doesn't matter in practice, but to stop the change in default timezone from having an observable effect it's better to be explicit.
These are the last few cases where we print dates/times with `.iso8601`, but weren't first ensuring they were in UTC.
@theseanything
Copy link
Contributor

On quick glance, it makes sense for the API's to store dates and present dates in a single Timezone, as it simplifies logic around them (and I dunno if we have a use case for needing to preserve Timezone information).

Does the fact that we have users outside the London Timezone make more sense to use UTC for our APIs? For example, if we introduced localised content, then things aren't going to be consistent anyway.

@richardTowers
Copy link
Contributor Author

We definitely only want to store things as UTC (but that's already the case everywhere, because active record has its own, separate time zone, which is UTC by default).

When we show users times in publishing apps, they should be in the London timezone (or if we ever get around to handling users in other time zones, like embassy workers in travel advice publisher, in that user's timezone).

When we communicate dates between publishing apps and apis / apis and frontends, it doesn't feel like it's useful to have them in any format other than UTC. It's not really meaningful to say "this was last updated at 2024-06-25T17:40 Cairo time" in an API payload. Much more convenient to have everything in UTC.

So I'm thinking the APIs should all be in UTC, and the apps with UIs should all be in London (and they've got the option to override things to their users' timezones if that's considered worthwhile).

@richardTowers
Copy link
Contributor Author

Another alternative which Kevin and I worked out would be to explicitly set the time_zone back to UTC in publishing-api. This is a bit inconvenient, because of the approach I've taken in the govuk_app_config gem, but it's still possible - #2786

@richardTowers
Copy link
Contributor Author

Closing in favour of #2786

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.

2 participants