-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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: add utcOffset in webhook payload #12709
Conversation
@jkcs is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing 🙏 Left you one comment, everything else looks good
packages/lib/date-fns/index.ts
Outdated
if (!timeZone) return null; | ||
|
||
if (timeZoneWithDST(timeZone)) { | ||
return getUTCOffsetInDST(timeZone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this now always return DST offset of this timezone, even when we are not in DST like now in December?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, you reminded me so well. I was overthinking it. This is not necessary.
packages/lib/date-fns/index.ts
Outdated
export function getUTCOffsetByTimezone(timeZone: string) { | ||
if (!timeZone) return null; | ||
|
||
return dayjs().tz(timeZone).utcOffset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we still do need to take care of DST here.
This should work if we add another parameter to the function for the date:
dayjs(date).tz(timeZone).utcOffset()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Hello, @Shpadoinkle . I suggest that you open a new issue. The changes in this pull request only include the modification mentioned in #12415 "Add the attendee UTC offset as a field, rather than the string of the timezone." As for the issue you mentioned, I think it's worth addressing as well. |
@jkcs ah ignore me all together, the timeZone string was already on the webhook |
This PR is being marked as stale due to inactivity. |
data: Omit<WebhookDataType, "createdAt" | "triggerEvent"> | ||
): WithUTCOffsetType<WebhookDataType> { | ||
if (data.organizer?.timeZone) { | ||
(data.organizer as Person & UTCOffset).utcOffset = getUTCOffsetByTimezone(data.organizer.timeZone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are missing the date
parameter here (data.startTime
), otherwise, we will always get the utc offset from the current date (important for DST)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
if (data?.attendees?.length) { | ||
(data.attendees as (Person & UTCOffset)[]).forEach((attendee) => { | ||
attendee.utcOffset = getUTCOffsetByTimezone(attendee.timeZone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now 👍 Thank you for your contribution 🙏
What does this PR do?
Fixes #12415
/claim #12415
Requirement/Documentation
Type of change
Mandatory Tasks