Fix recovery date off by one day bug #230
Conversation
src/util/date.ts
Outdated
| /** | ||
| * Returns the date portion of a ISO formatted date string (YYYY-MM-DDT...), but | ||
| * makes sure to return the date local to the current timezone. | ||
| * | ||
| * It might be the 15th in the local timezone but the 16th in UTC. So if one | ||
| * were to use `toISOString()` then the date would be 1 day off from the local | ||
| * timezone. | ||
| * | ||
| * @param date A JS Date Object | ||
| * @returns ISO formatted date string (YYYY-MM-DD) but in the local timezone. | ||
| */ |
There was a problem hiding this comment.
I don't think this utility function should exist. Nobody else should be using this, and keeping it around is super-dangerous.
Basically, both JavaScript Date objects and the ISO format operate in seconds-since-the-epoch. This metric is simple, unambiguous, and free of human timezone concerns. When we talk about time in computer systems, this is the way to go, because it's precise.
The recovery questions, on the other hand, do not want a specific point in time. They want string, which we can later check for equality using ===. The string is date-ish, sure, but it's NOT a an "ISO formatted date sting" by any definition of the term. You can't take the recovery answer and say for sure when they got married or whatever the question is - to answer that, you would need to know the timezone, which we don't have. This is because humans, unlike computers, are really sloppy with the way we think about dates. Everything depends on context and culture for us.
As soon as you mix human and computer ways of thinking, you start to get into trouble. You end up with situations where all the values in the database are off by 8 hours, because the server happened to be in GMT+8 or something. The solution is to always do everything in computer time, since that keeps the data free from corrupting influences.
If you need to deal with local time, that's an input/output issue, and should really be using Date.toLocaleString in 99% of the cases. If we are going to to talk to a human, let's talk to the human in their preferred format!
This brings us to the recovery questions, which are a weird case. We want the value to use "human dates", but we don't want the value to use the culture's preferred format - we want YYYY-MM-DD, so === is more likely to work. Note that this is NOT the ISO format, since it's in local time, and yet doesn't include timezone specifier on the end like GMT+8. It's a weird hybrid franken-format used only by the recovery system.
So, we cannot leave this utility sitting around. It implements a dangerous hybrid format that looks like something it's not, and is used only by the recovery system. We need to give this a better name, like toRecoveryDateString, and tuck it away in the recovery system were nobody else is going to find it.
There was a problem hiding this comment.
I agree with all this said. I updated the PR to reflect your comment.
samholmes
force-pushed
the
sam/recovery-dates
branch
2 times, most recently
from
be69216 to
80b2282
Compare
src/components/modals/DateModal.tsx
Outdated
| @@ -116,3 +116,17 @@ export function DateModalAndroid(props: Props) { | |||
|
|
|||
| export const DateModal = | |||
| Platform.OS === 'android' ? DateModalAndroid : withTheme(DateModalIos) | |||
|
|
|||
| /** | |||
| * Returns teh date portion of a date object, formatted YYYY-MM-DD. | |||
src/components/modals/DateModal.tsx
Outdated
| * Returns teh date portion of a date object, formatted YYYY-MM-DD. | ||
| * It makes sure to return the date local to the current timezone. | ||
| * | ||
| * It's only use-case is to convert a date object from the date picker model |
The date picker would return the date picked at the current local time based on the device's locale (timezone). Because `toISOString` was used, the resulting date my be different then the picked date because the day in UTC may be a different day then what the current device is in. To solve this, use `getDate/Month/FullYear` methods which respect locale to format the ISO formatted date string. This has been abstracted into a utility that is only scoped to the DateModel component.
samholmes
force-pushed
the
sam/recovery-dates
branch
from
80b2282 to
7f09cc9
Compare
The date picker would return the date picked at the current local time
based on the device's locale (timezone). Because
toISOStringwas used,the resulting date my be different then the picked date because the day
in UTC may be a different day then what the current device is in.
To solve this, use
getDate/Month/FullYearmethods which respect localeto format the ISO formatted date string. This has been abstracted into
a utility.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
none