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

feat: display departure times and search time in CET, regardless of which timezone the users is located in #234

Merged
merged 7 commits into from
Mar 1, 2024

Conversation

jonasbrunvoll
Copy link
Contributor

Closes https://github.com/AtB-AS/kundevendt/issues/16880

Background

We have received feedback from users outside the Central European Timezone (CET), that when searching for trips, the departure times are displayed in their local time and not in CET. The trips should always be displayed in CET to avoid confusion.

Proposed solution

Added functionality to display departure times and the time in the search time selector in CTE, regardless of which timezone the user is located in.

Acceptance Criteria

  • Regardless of timezone, the search selector should display CET time.
  • Regardless of timezone, the departure times should be displayed with CET time when departure time is set to now.
  • Regardless of timezone, the departure times should be displayed with CET time when departure time is set to departure. Also attempt to change the search time.
  • Regardless of timezone, the departure times should be displayed with CET time when departure time is set to arrival. Also attempt to change the search time.

Test input

To test this feature, change the internal clock of the computer to the listed timezones. The departure times should and the search time selector should always be shown in CTE time.

  • Helsinki - Finland (+0200)
  • Oslo - Norway (+0100)
  • London - UK (+0000)
  • Los Angeles - US (-0800)

…arch time selectore in CTE, regardless of which time zone the user is located in.
Copy link

vercel bot commented Feb 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
planner-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 1, 2024 0:05am

src/utils/date.ts Outdated Show resolved Hide resolved
@mikaelbr
Copy link
Collaborator

You didn't require any changes in how the date is listed in search results? That seems odd to me as dates are provided in UTC so printing local time on the client when providing from the server intuitively would be wrong, no?

@adriansberg
Copy link
Contributor

Don't we always operate with UTC dates, or are they converted to local date when parsed from ISO?

@mikaelbr
Copy link
Collaborator

When formatting as string the local time is used. As input in the date selector parsing without timezone is done so that will probably also use local timezone 🤔

…ng from detailed view back to search results. + some minor updates in the code.
@jonasbrunvoll
Copy link
Contributor Author

setTimezoneIfNeeded is used in the search time selector to display in CET. When creating a tripQuery during a search in client/journey-planner, formatCETToLocal() formates the search time from CET to local time. Then, in trip() on the server side, the search time is again formated from local time to UTC before the search towards the db is performed.

I am not exactly sure where the search results are formated from UTC to local time, but it could be when parsed from ISO. Therefore, to formate from local to CET before displaying the trip departure times, the setTimezoneIfNeeded() is implemented into formateToClock().

@adriansberg adriansberg changed the title feat: display departure times and search time in CTE, regardless of which timezone the users is located in. feat: display departure times and search time in CET, regardless of which timezone the users is located in Feb 26, 2024
@adriansberg
Copy link
Contributor

  • date-fns's format does not take any options for the formatting to be in a certain time zone I assume?
  • Not sure if I like how e.g. formatCETToLocal modifies the time by adding or subtracting to the original time based on the timezone offset. Is there no way to keep to UTC times and only alter output in views (date/time selector might be an exception)? The journey planner API takes in UTC times I would guess?
  • Have this been testet by setting a different time zone on the machine? (Maybe that's my responsibility 🫣)

@jonasbrunvoll
Copy link
Contributor Author

  • No, date-fns's does not have an option to format to a certain timezone.
  • I can take a new look and investigate if it is possible to keep UTC times and only alter the views. As of now, the setTimezoneIfNeeded() in formatToClock() formates from local time to CET. I dont think how the timezone difference in the search time selector should be changed.
  • Yes, I have testet this with multiple different timezones on my machine locally and it seems to work. But, I do appreciate if others also would test this PR with different time zones😃

@mikaelbr
Copy link
Collaborator

In regards to testing with changed time zones, testing to vercel preview release is probably more realistic as locally it could be different with server time

@jonasbrunvoll
Copy link
Contributor Author

jonasbrunvoll commented Feb 27, 2024

Timezone testing in preview

These are the test results when searching after trips from different timezones in preview.
✅ means that the results from preview are the same as when searching frammr.no from a device using CTE.

CET (+0100) ✅

  • Departure times in trip results when search = 'now' ✅
  • Departure times in trip results when search = 'departure' and time = (current -1, current hour , current +1 hour) ✅
  • Departure times detailed view ✅
  • Departure times in trip result after navigating from detailed view ✅
  • Departure times in search time selector after navigating from detailed view ✅

EET (+0200) ✅

  • Departure times in trip results when search = 'now' ✅
  • Departure times in trip results when search = 'departure' and time = (current -1, current hour , current +1 hour) ✅
  • Departure times detailed view ✅
  • Departure times in trip result after navigating from detailed view ✅
  • Departure times in search time selector after navigating from detailed view ✅

UTC (+0000) ✅

  • Departure times in trip results when search = 'now' ✅
  • Departure times in trip results when search = 'departure' and time = (current -1, current hour , current +1 hour) ✅
  • Departure times detailed view ✅
  • Departure times in trip result after navigating from detailed view ✅
  • Departure times in search time selector after navigating from detailed view ✅

PST (-0800) ✅

  • Departure times in trip results when search = 'now' ✅
  • Departure times in trip results when search = 'departure' and time = (current -1, current hour, current +1 hour) ✅
  • Departure times detailed view ✅
  • Departure times in trip result after navigating from detailed view ✅
  • Departure times in search time selector after navigating from detailed view ✅

@mikaelbr
Copy link
Collaborator

Lets look into how to write playwright tests here @jonasbrunvoll

Copy link
Contributor

@adriansberg adriansberg left a comment

Choose a reason for hiding this comment

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

Let's add a couple test to make sure that we don't break anything.

@mikaelbr
Copy link
Collaborator

mikaelbr commented Mar 1, 2024

After the deployment action hook change for Playwright tests it takes longer for GH to discover playwright tests and run them. So they aren't run yet. Think it is good to wait for them to run

@adriansberg
Copy link
Contributor

Oh, that's right. I just glanced at the checks to see if all were green 🫣

@jonasbrunvoll
Copy link
Contributor Author

Is there some way to force GH to discover and run the playwright tests, or do we just wait until GH finds them?

@mikaelbr
Copy link
Collaborator

mikaelbr commented Mar 1, 2024

They are visible here: https://github.com/AtB-AS/planner-web/actions/workflows/playwright.yml

looks like it takes some time before it runs, so think we could look into if this can be improved somewhat

@jonasbrunvoll jonasbrunvoll merged commit 333ab75 into main Mar 1, 2024
6 checks passed
@jonasbrunvoll jonasbrunvoll deleted the jonas/ensure_central_european_timezone branch March 1, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants