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

Rotations: Deprecated timezone leading to text rendering issues #38

Closed
arurao opened this issue Jul 2, 2019 · 6 comments · Fixed by #3278
Closed

Rotations: Deprecated timezone leading to text rendering issues #38

arurao opened this issue Jul 2, 2019 · 6 comments · Fixed by #3278
Labels
bug Something isn't working stale This is inactive

Comments

@arurao
Copy link
Contributor

arurao commented Jul 2, 2019

Describe the Bug:
Text rendering issues have been observed on some browsers (e.g. Chrome) for deprecated timezones.
This is happening because of some timezones (like 'US/Central') being deprecated and hence, not being supported by all browsers, but the backend database (Postgres) still considers them as valid.

Steps to Reproduce:

  1. Go to 'Rotations'
  2. Click on any one rotation to go to its details page.
  3. Scroll down to the description text for the rotation, most often starts with 'Hands off....'
  4. See error like 'Invalid DateTime US/Central...'

Expected Behavior:
A proper text description for the rotation handoff date and time is expected.

Observed Behavior:
A rendering error is observed in that text with 'null' handoff day of the week and/or 'Invalid DateTime' for timestamp.

Screenshots/Stack Traces:

Screen Shot 2019-07-02 at 11 20 48 AM

Application Version:
v0.22.0

Desktop:

  • OS: MacOS
  • Browser: Chrome

Additional Context:
This issue is only limited to the UI. Scheduling still works as intended.

A possible workaround for this issue is to replace the deprecated timezone with it's equivalent non-deprecated one.
For example: Replace US/Central with America/Chicago

For more information on which timezones are deprecated and their equivalent zones, please refer to https://en.wikipedia.org/wiki/List_of_tz_database_time_zones.

@arurao arurao added the bug Something isn't working label Jul 2, 2019
@mastercactapus
Copy link
Member

We could probably look at embedding tzdata in the backend and using that. Postgres is only used because we can list them easily. However, all TZ-type calculations happen in GoAlert itself, which then uses whatever is available on the system.

In the official container, which is based on Alpine, the package uses the IANA db:
https://www.iana.org/time-zones

But if we embed it, we can guarantee GoAlert will have the same TZ data for a particular version/binary on all systems & environments. It also has a list of linked non-canonical names we can use.

We might still have issues on windows systems unless Chrome & friends have their own TZ data.
https://golang.org/src/time/zoneinfo_windows.go#L20


If we go forward with that, then next steps would be:

  • Work out a way to embed TZ data
  • It should fallback to current state (can be done from util/loadlocation.go)
  • Provide the list of TZ, without asking Postgres
  • Validate the new list of TZ works in major browsers
  • migrate old TZ aliases to their current ones (e.g. US/Central becomes America/Chicago)

@m17ch
Copy link
Contributor

m17ch commented Jul 29, 2019

Reminder: When we do implement a fix for this, include a migration that maps deprecated timezones as appropriate (in the case there is existing deprecated timezone data).

@stale
Copy link

stale bot commented Dec 18, 2020

This issue has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the stale This is inactive label Dec 18, 2020
@dctalbot
Copy link
Contributor

dctalbot commented Sep 10, 2021

Migration queries have been generated here: https://github.com/target/goalert/compare/tzmigrate
Note - still need to validate correctness. Also, they don't cover all cases e.g. CST6CDT (#1824)

@stale stale bot removed the stale This is inactive label Sep 10, 2021
@stale
Copy link

stale bot commented Mar 10, 2022

This issue has been automatically marked as stale because it has not had recent activity.

@stale stale bot added the stale This is inactive label Mar 10, 2022
@dctalbot dctalbot removed the stale This is inactive label Mar 10, 2022
@stale
Copy link

stale bot commented Sep 20, 2022

This issue has been automatically marked as stale because it has not had recent activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale This is inactive
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants