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: accomodate input timezone while create user without on-boarding and default timezone #18288

Conversation

vijayraghav-io
Copy link
Contributor

@vijayraghav-io vijayraghav-io commented Dec 20, 2024

What does this PR do?

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • -N/A - I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

Copy link

vercel bot commented Dec 20, 2024

@vijayraghav-io is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@graphite-app graphite-app bot requested a review from a team December 20, 2024 05:52
@graphite-app graphite-app bot added the community Created by Linear-GitHub Sync label Dec 20, 2024
@github-actions github-actions bot added the ❗️ migrations contains migration files label Dec 20, 2024
@dosubot dosubot bot added the bookings area: bookings, availability, timezones, double booking label Dec 20, 2024
Copy link

graphite-app bot commented Dec 20, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (12/20/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add community label" took an action on this PR • (12/20/24)

1 label was added to this PR based on Keith Williams's automation.

"Add ready-for-e2e label" took an action on this PR • (12/21/24)

1 label was added to this PR based on Keith Williams's automation.

@vijayraghav-io vijayraghav-io changed the title fix: accomodate input timezone while create user without on-boarding and explicitly save default timezone in schedule table fix: accomodate input timezone while create user without on-boarding and default timezone Dec 20, 2024
@Praashh Praashh self-assigned this Dec 20, 2024
@@ -683,7 +683,7 @@ model Schedule {
eventType EventType[]
instantMeetingEvents EventType[] @relation("InstantMeetingSchedule")
name String
timeZone String?
timeZone String? @default("Europe/London")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we having "Europe/London" as default timeZone 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Europe/London" timezone aligns with UTC or has UTC +0, probably that's why its chosen as default timezone even for users

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we directly use UTC timeZone, I think that would be better 🤔 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be in sync with default value for timezone in other tables - User, Team and other places in code, i think it's better to go with Europe/London.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool 🆒

Copy link
Member

Choose a reason for hiding this comment

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

@emrysal any thoughts in here? Are we comfortable setting this default at DB level?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why not, but I don't really adore the idea of default schedules as a whole.

Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

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

LGTM!!

Copy link
Contributor

E2E results are ready!

@vijayraghav-io
Copy link
Contributor Author

@Praashh , Reminding for merge 🙏

@Praashh
Copy link
Contributor

Praashh commented Jan 1, 2025

tagging @anikdhabal for this 🙏

Copy link
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

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

not sure if we want this hardcoded as a migration

@vijayraghav-io
Copy link
Contributor Author

not sure if we want this hardcoded as a migration

I remember checking this scenario - for existing records, if 'timezone' column already has a different value, this migration won't update that row with this new default value.
Anyway will check and verify once again

@emrysal
Copy link
Contributor

emrysal commented Jan 7, 2025

I think for this one we'd strongly prefer defaulting in application code, as if we default on the DB to Europe/London we have no way of knowing a schedule is a default schedule without a tz specified or the actual timezone of the user, as specified by the user themselves.

That's a big reason for keeping it nullable.

@emrysal
Copy link
Contributor

emrysal commented Jan 7, 2025

Closing after internal consensus on the above, thanks for the contribution though @vijayraghav-io

@emrysal emrysal closed this Jan 7, 2025
@vijayraghav-io
Copy link
Contributor Author

Closing after internal consensus on the above, thanks for the contribution though @vijayraghav-io

🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bookings area: bookings, availability, timezones, double booking community Created by Linear-GitHub Sync ❗️ migrations contains migration files ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants