fix: date-picker bug for negative UTC timezones attempt 3 #6261

LinHuiqing · 2023-05-04T01:59:54Z

Problem

(taken from #6096)

(taken from #6025)
Date picker was using the midnight of the date picked and taking into account the timezone where the user is filling in the form. However, it does not make sense to do so as users will be picking particular dates rather than timings, and the understanding of the different timezones should be established as part of the question.

However, bug fix in PR #6025 almost caused an incident as the fix removed converting to UTC on the date picker itself, which was causing the selection offset bug.

Unfortunately, datepicker bug fix attempt #2 (#6096) caused an actual incident due to incorrect date validation for the pastOnlyValidator (post-mortem).

Solution

For bug caused by #6096 - Restore original date validators.

Breaking Changes

No - this PR is backwards compatible. Custom dates were already saved in midnight UTC in the DB.

Improvements:

Cast time to UTC only right before submitting custom date so the conversion will be completely invisible to users
Cast time to local timezone before being shown on the frontend so the frontend timezone will be consistent

Bug Fixes:

Users with negative UTC timezones will now have their date fields correctly reflecting the dates selected on their date pickers
zonedTimeToUTC was converting the timings to UTC and thus the actual date being referred to will not be preserved for respondents in negative UTC timezones. Implemented normalizeDateToUtc to ensure the actual date numbers are are preserved.
- We now cast time from local timezone to UTC timezone using Date.UTC instead of converting time using zonedTimeToUTC (which we were using wrong anyway, as pointed out by @darrelhong in fix: date picker calendar select correct date in negative UTC offset timezones #5731)

Before & After Screenshots

BEFORE:

Screen.Recording.2023-03-31.at.3.18.31.AM.mov

AFTER:

Disallow past dates:

Screen.Recording.2023-04-13.at.2.48.49.PM.mov

Disallow future dates:

Screen.Recording.2023-04-13.at.5.08.17.PM.mov

Custom date range:

Screen.Recording.2023-04-13.at.5.11.57.PM.mov

Tests

To ensure that all our bases are covered, try to test the most extreme timezones. Here are the suggested timezones:

Most common use case: Singapore - Singapore (UTC +08)
Negative: Alofi - Niue (UTC -11)
Positive: Wellington - New Zealand (UTC +12)

Preparation:

Create a form with a date field and open the form.

For the tests below, timezones A and B refer to the positive and negative timezones mentioned above, in any order.

i) Ensure that date picker for date field with "Disallow past dates" option exhibits expected behaviour:

- Set your timezone as A.
- On the admin panel, refresh, select and save the "Disallow past dates" option for the date field.
- On the respondent form, refresh and open the date picker. Only the current and future dates of the timezone should be shown.
- Select a date. The appropriate date should be reflected in the date field.
- Change your device's timezone to B.
- Repeat tasks 3-4.
- Repeat tasks 2-4.
- Change your device's timezone to A.
- Repeat tasks 3-4.

ii) Ensure that date picker for date field with "Disallow future dates" option exhibits expected behaviour:

- Set your timezone as A.
- On the admin panel, refresh, select and save the "Disallow future dates" option for the date field.
- On the respondent form, refresh and open the date picker. Only the current and future dates of the timezone should be shown.
- Select a date. The appropriate date should be reflected in the date field.
- Change your device's timezone to B.
- Repeat tasks 3-4.
- Repeat tasks 2-4.
- Change your device's timezone to A.
- Repeat tasks 3-4.

iii) Ensure that date picker for date field with "Custom date range" option exhibits expected behaviour:

- Set your timezone as A.
- On the admin panel, refresh and select the "Custom date range" option for the date field.
- Select a start date.
- Open the end date field date picker. Only the dates on or after the selected start date should be selectable.
- Select an end date.
- Save the selected date range. Optionally, check that the dates saved in the DB are in UTC.
- On the respondent form, refresh and open the date picker. Only the dates in the selected date range should be selectable.
- Select a date. The respective date should be reflected in the date field.
- Change your device's timezone to B.
- Repeat tasks 7-8.
- Repeat task 2. Ensure that the start and end dates remain the same.
- Update the start and end dates for the date field. Save it.
- Repeat tasks 7-8.
- Change your device's timezone to A.
- Repeat tasks 7-8.

iv) Ensure that email mode forms with date field still work:

Repeat test set (i), making sure to select the current date using the date picker and submitting.
Repeat test set (ii), making sure to select the current date using the date picker and submitting.
Repeat test set (iii), making sure to:
- Select the start date using the date picker and submitting.
- Select the end date using the date picker and submitting.

Note: Here's how to set your Mac's timezone.

Design/ product question: Should "Disallow past dates" and "Disallow future dates" be taking the past / future based on the admins' timezone, specified timezone or the users' timezone? Also, even when we have decided on the previous question, does it even make sense if the concept of time is not considered? E.g. if the recurring event I want to organise is at 10am, our date picker won't be able to know that so if we have a timezone in mind, it won't "filter" properly. Currently, it's simply implemented based on the users' timezone so its not disorienting.

LinHuiqing · 2023-05-04T05:37:17Z

For pastOnlyValidator and futureOnlyValidator, I wonder if the conversions done are even warranted actually, especially considering that answer from response is a DD MMM YYYY string with no time in it. For now, the code change in this PR does not make any changes for this, but just something for discussion for correctness!

karrui · 2023-05-08T05:14:29Z

Looking at the main problem, I would warrant that the client should not need to know about any server implementation.
The following inputs -> outputs should be handled on the client:

no past dates: prevent past dates according to client time
no future dates: prevent future dates according to client time
date range: accept as long as client date selected in range.

All client should do is to set the date object to start of day selected before passing to the client, no need to convert to UTC. The futureOnlyValidator and pastOnlyValidator should be able to handle any day range as long as the time is a start of day.

LinHuiqing · 2023-05-08T07:07:51Z

Looking at the main problem, I would warrant that the client should not need to know about any server implementation. The following inputs -> outputs should be handled on the client:

no past dates: prevent past dates according to client time no future dates: prevent future dates according to client time date range: accept as long as client date selected in range.

All client should do is to set the date object to start of day selected before passing to the client, no need to convert to UTC. The futureOnlyValidator and pastOnlyValidator should be able to handle any day range as long as the time is a start of day.

So regarding this, should we stick to the implementation in this PR as of now and open a new issue for handling date 'normalisation' on backend?

FYI:

Did some investigation on the custom date range saving - the dates are saved in UTC midnight in production.
Date field responses for both storage mode and email mode forms are in strings (DD MMM YYYY).

karrui

seems correct, lgtm. Can we have some unit tests on the date util functions to ensure that the two functions work as expected?

LinHuiqing · 2023-05-11T02:10:06Z

seems correct, lgtm. Can we have some unit tests on the date util functions to ensure that the two functions work as expected?

ah yes, we should have that in place before pushing! thanks @karrui will add it in 👍 🌮

LinHuiqing · 2023-05-16T06:07:53Z

seems correct, lgtm. Can we have some unit tests on the date util functions to ensure that the two functions work as expected?

ah yes, we should have that in place before pushing! thanks @karrui will add it in 👍 🌮

Regarding unit tests for date utils, I'm finding it particularly hard to test normalizeDateToUtc and loadDateFromNormalizedDate because there is no timezone information stored in Date so it implicitly uses local time when methods like getFullYear, getMonth and getDate are used, but it is what we're passing in as input... Migrating to use strings is getting more and more tempting 😭

karrui · 2023-05-16T07:32:02Z

because there is no timezone information stored in Date

Can try mocking with timezone-mock

LinHuiqing · 2023-05-16T08:50:22Z

because there is no timezone information stored in Date

Can try mocking with timezone-mock

Thanks for the suggestion! It works

- (FE) normalize dates to UTC before storing in DB - (FE) load normalized dates to local timezone - (BE) remove adjustments due to timezone

LinHuiqing · 2023-05-16T08:53:17Z

Rebased to develop and force pushed so that Playwright Tests can pass. New commits since last review are "fix: normalizeDateToUtc to return Date object" and after.

karrui · 2023-05-16T14:22:01Z

frontend/src/utils/date.test.ts

+      const localDate = new Date(Date.parse(dateString))
+      const utcDate = new Date(Date.parse(`${dateString}+00:00`))
+
+      const result = DateUtils.normalizeDateToUtc(localDate)


Might want to assert before and after the util is called to ensure dates are different first

Hm I don't think this is especially valuable as an assertion as ideally the test should still pass when the local time is UTC. But I get it that there is no value in the test if the local time and UTC time is the same, I guess. Will it be better that this test is removed or that the local time here is set as SGT?

The rationale is to assert that the mock date library is also working, just in case :P

haha okay, I'll add the assertion for the positive/negative utc tests then! yea when i tried the library I did print the dates to make sure hahaha

checks added here: d232fad

LinHuiqing · 2023-05-17T07:29:48Z

Note:
Playwright tests failed twice for commit d232fad due to being unable to install Playwright browsers: actions/runner-images#7048

karrui

lgtm~

* fix: date-picker bug for negative UTC timezones attempt 3 (#6261) * fix: remove timezone from datepicker * fix: datepicker bugs due to timezones - (FE) normalize dates to UTC before storing in DB - (FE) load normalized dates to local timezone - (BE) remove adjustments due to timezone * docs: futureOnly and pastOnly validators * refactor: allow null inputs for new date util fns * fix: date normalization fns to use date as numbers * refactor: remove unnecessary var assignment * fix: unset date validation * fix: compare to dates in earliest & latest tzs * fix: date validation bug in release v6.41.0 * fix: restore original date validators * fix: normalizeDateToUtc to return Date object * test: frontend date utils * build(deps): add timezone-mock to mock local time * test: date utils in the most extreme timezones * test: add check to ensure that timezone-mock works * chore: bump version to v6.53.0

LinHuiqing changed the title ~~Fix/date picker timezone~~ fix: date-picker bug for negative UTC timezones attempt #3 May 4, 2023

LinHuiqing marked this pull request as ready for review May 4, 2023 02:52

LinHuiqing requested review from foochifa and tshuli May 4, 2023 02:54

LinHuiqing mentioned this pull request May 4, 2023

fix: remove timezone from datepicker opengovsg/camp-design-system#309

Merged

1 task

LinHuiqing changed the title ~~fix: date-picker bug for negative UTC timezones attempt #3~~ fix: date-picker bug for negative UTC timezones attempt 3 May 4, 2023

karrui reviewed May 11, 2023

View reviewed changes

LinHuiqing added 14 commits May 16, 2023 20:51

fix: remove timezone from datepicker

97a524d

fix: datepicker bugs due to timezones

419080a

- (FE) normalize dates to UTC before storing in DB - (FE) load normalized dates to local timezone - (BE) remove adjustments due to timezone

docs: futureOnly and pastOnly validators

eb902dc

refactor: allow null inputs for new date util fns

f99cae7

fix: date normalization fns to use date as numbers

d2d8795

refactor: remove unnecessary var assignment

e2d6a87

fix: unset date validation

7098421

fix: compare to dates in earliest & latest tzs

1033816

fix: date validation bug in release v6.41.0

942ab6a

fix: restore original date validators

c369b8e

fix: normalizeDateToUtc to return Date object

3d14e1a

test: frontend date utils

7102c32

build(deps): add timezone-mock to mock local time

f8f6f93

test: date utils in the most extreme timezones

51a8169

LinHuiqing force-pushed the fix/date-picker-timezone branch from 5eaa50a to 51a8169 Compare May 16, 2023 08:51

LinHuiqing requested a review from karrui May 16, 2023 09:27

karrui reviewed May 16, 2023

View reviewed changes

test: add check to ensure that timezone-mock works

d232fad

LinHuiqing requested a review from karrui May 17, 2023 07:41

karrui approved these changes May 18, 2023

View reviewed changes

LinHuiqing merged commit a21f0e1 into develop May 24, 2023

LinHuiqing deleted the fix/date-picker-timezone branch May 24, 2023 05:28

LinHuiqing mentioned this pull request May 24, 2023

build: release v6.53.0 #6382

Merged

39 tasks

LinHuiqing mentioned this pull request May 25, 2023

build: release v6.54.0 #6385

Merged

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: date-picker bug for negative UTC timezones attempt 3 #6261

fix: date-picker bug for negative UTC timezones attempt 3 #6261

LinHuiqing commented May 4, 2023 •

edited

Loading

LinHuiqing commented May 4, 2023

karrui commented May 8, 2023

LinHuiqing commented May 8, 2023 •

edited

Loading

karrui left a comment

LinHuiqing commented May 11, 2023

LinHuiqing commented May 16, 2023

karrui commented May 16, 2023

LinHuiqing commented May 16, 2023

LinHuiqing commented May 16, 2023 •

edited

Loading

karrui May 16, 2023

LinHuiqing May 17, 2023

karrui May 17, 2023

LinHuiqing May 17, 2023

LinHuiqing May 17, 2023

LinHuiqing commented May 17, 2023

karrui left a comment

fix: date-picker bug for negative UTC timezones attempt 3 #6261

fix: date-picker bug for negative UTC timezones attempt 3 #6261

Conversation

LinHuiqing commented May 4, 2023 • edited Loading

Problem

Solution

Before & After Screenshots

Tests

LinHuiqing commented May 4, 2023

karrui commented May 8, 2023

LinHuiqing commented May 8, 2023 • edited Loading

karrui left a comment

Choose a reason for hiding this comment

LinHuiqing commented May 11, 2023

LinHuiqing commented May 16, 2023

karrui commented May 16, 2023

LinHuiqing commented May 16, 2023

LinHuiqing commented May 16, 2023 • edited Loading

karrui May 16, 2023

Choose a reason for hiding this comment

LinHuiqing May 17, 2023

Choose a reason for hiding this comment

karrui May 17, 2023

Choose a reason for hiding this comment

LinHuiqing May 17, 2023

Choose a reason for hiding this comment

LinHuiqing May 17, 2023

Choose a reason for hiding this comment

LinHuiqing commented May 17, 2023

karrui left a comment

Choose a reason for hiding this comment

LinHuiqing commented May 4, 2023 •

edited

Loading

LinHuiqing commented May 8, 2023 •

edited

Loading

LinHuiqing commented May 16, 2023 •

edited

Loading