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

Fix date type update #9700

Merged
merged 9 commits into from
Jan 17, 2025
Merged

Fix date type update #9700

merged 9 commits into from
Jan 17, 2025

Conversation

lucasbordeau
Copy link
Contributor

@lucasbordeau lucasbordeau commented Jan 16, 2025

This PR fixes a problem with how TypeORM handles date without time.

A date without time that is stored in PostgreSQL database as date type gets returned as an ISO string date with a timezone that can shift its date part in an unwanted way.

In short DB stores 2025-01-01, TypeORM query builder returns 2024-12-31T23:00:00Z which gets parsed as 2024-12-31 on the front end field.

We don't want to handle timezone here because we are manipulating a date without its time part, so this PR adds a step that counteracts what TypeORM does and returns 2025-01-01T00:00:00.000Z so that the front can parse it correctly.

@Weiko We might want to check other places of the backend where date types are returned by TypeORM, we might have the same problem, this PR only fixes it for updateOne resolver return.

  • Fixed date persist on frontend which was shifting the date to a different day due to timezone issue
  • Fixed date returned by the backend update logic, which was shifting the date by the timezone offset (so this PR adds back the offset so that it stays at 00:00:00Z time)

@lucasbordeau lucasbordeau requested a review from Weiko January 16, 2025 17:37
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR addresses timezone-related issues in date handling across the frontend and backend, ensuring consistent date persistence at 00:00:00Z time.

  • Modified DateFieldInput to manually construct YYYY-MM-DD strings instead of using toISOString() to prevent timezone shifts
  • Added timezone offset compensation in format-result.util.ts to maintain dates at 00:00:00Z time
  • Renamed InternalDatePicker to DateTimePicker and improved timezone handling using Luxon's DateTime
  • Added proper timezone handling in FormDateTimeFieldInput using user's timezone context
  • Should remove debug console.log statements before merging to production

9 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +35 to 40
const newDateWithoutTime = `${newDate?.getFullYear()}-${(
newDate?.getMonth() + 1
)
.toString()
.padStart(2, '0')}-${newDate?.getDate().toString().padStart(2, '0')}`;

Copy link
Contributor

Choose a reason for hiding this comment

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

style: getMonth() returns 0-11, so adding 1 is correct, but this date construction could be simplified using toISOString().split('T')[0] which would be more reliable

Comment on lines 28 to 30
console.log({
fieldValue,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Remove debugging console.log before merging to production

@@ -104,16 +122,16 @@ export const DateInput = ({

return (
<div ref={wrapperRef}>
<InternalDatePicker
<DateTimePicker
date={internalValue ?? new Date()}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Using new Date() as fallback could cause timezone issues. Consider using a timezone-aware default or UTC date.

Comment on lines 102 to 104
console.log({
records,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Debug console.log statement should be removed before merging to production

Comment on lines 343 to 346
console.log('handleClose', {
newDate,
});
};
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Remove console.log statements before merging to production

Comment on lines 389 to 392
console.log('handleDateChange', {
dateParsed,
});

Copy link
Contributor

Choose a reason for hiding this comment

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

style: Remove console.log statements before merging to production

Comment on lines 407 to 410
console.log('handleDateSelect', {
dateParsed,
});

Copy link
Contributor

Choose a reason for hiding this comment

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

style: Remove console.log statements before merging to production

Copy link
Contributor

@prastoin prastoin left a comment

Choose a reason for hiding this comment

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

Hello there ! I'm not 100% sure to understand the initial issue
Left few questions
Please let me know !

for (const dateFieldMetadata of dateFieldMetadataCollection) {
const currentDate = new Date(newData[dateFieldMetadata.name]);

const offsetInMilliseconds = -(new Date().getTimezoneOffset() * 60000);
Copy link
Contributor

@prastoin prastoin Jan 17, 2025

Choose a reason for hiding this comment

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

Question: Unless I'm mistaken the API's instance timezone might not be than the same the client that sent or will display the data initially ?

Copy link
Contributor Author

@lucasbordeau lucasbordeau Jan 17, 2025

Choose a reason for hiding this comment

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

Ok I'll update the naming to reflect the fact that we are countering a problem with TypeORM which automatically converts a date only string '2025-01-01' to a Date ISO string and that it adds the timezone of the server to then convert it back to UTC, thus subtracting the timezone offset of the raw date and ending up with a different day.

In short 2025-01-01 in DB, is returned as 2024-12-31 because of the returning() function of TypeORM, which seems to be an opinionated design decision.

Copy link
Contributor

@prastoin prastoin Jan 17, 2025

Choose a reason for hiding this comment

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

Ok I got it ! Thanks a lot for these explanations makes sense !

TypeORM newbie here, feels bit weird to have to make that mapping manually. I'm wondering if there're no native solution to store date along their timezone ?

Again sorry for nitpicking, but unless I'm mistaken there're might be a world where the API instance that inserted the date is located in a different timezone than the one extracting them ?
As from my understanding, we keep in db dates values that needs to be contextualized each time we wanna retrieve them ?

Copy link
Contributor Author

@lucasbordeau lucasbordeau Jan 17, 2025

Choose a reason for hiding this comment

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

For reference, here is how PostgreSQL stores a date type (which is PostgreSQL-specific, not TypeORM-specific) :

image

And here is how PostgreSQL stores a timestamptz type, which corresponds to what you talk about from what I understand :

image

What is necessary to understand for datetime handling in Twenty is that we decided to store a date+time as a point in time, unique, the same for all timezone, and this is done with timestamptz which stores the timezone, but the important thing to understand is that no matter what timezone get stored, the point in time is the same whether expressed in UTC or CET.

The timestamp type on the contrary doesn't store this timezone information, thus we cannot rely on it to know which precise point in time it is, because the timezone information is lost, and there's no way to reliably know it except by taking assumptions that have to be documented somewhere, plus libraries, client computer / server computer that have different timezone, the native JavaScript Date object being nasty with timezone (you cannot change its timezone) plus difficulty to wrap your head around all of that on a bad day, and this is the beginning of huge problems when working with multiple timezones or daylight save time changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@lucasbordeau lucasbordeau Jan 17, 2025

Choose a reason for hiding this comment

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

For our date type, that would be interesting to think about it, for now I think that the consensus is that we only store the date expressed as a simple day in the calendar for someone locally, which is the equivalent of losing the timezone information but here we don't really care because we don't want to know where exactly in time is someone's birthday but rather what day of the year it is.

I think that if someone would want to define a meeting date for people in different timezones, that person would necessary have to use a DateTime type in Twenty, which would display the same point in time for different people in their respective timezone. Because anyway people would ask, ok but when during that day in my timezone ? Because we have 10 hours of difference.

(field) => field.type === FieldMetadataType.DATE,
);

// This is a temporary fix to handle a bug in the frontend where the date gets returned in the wrong timezone,
Copy link
Contributor

Choose a reason for hiding this comment

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

Praise: 🚀

@lucasbordeau lucasbordeau merged commit 6bd0244 into main Jan 17, 2025
47 checks passed
@lucasbordeau lucasbordeau deleted the fix/date-input branch January 17, 2025 15:19
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.

3 participants