feat: add support for apple travel time#10660
Conversation
|
@nicktrn 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! 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📦 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! 🙌 |
|
Hey @nicktrn Thank you for the PR. |
|
Hi @alishaz-polymath Thanks for having a look this quickly, but this is only a draft for now. As you know there are still things to discuss. By the way, ical.js is a pain to work with. |
Oh for sure. And yes, iCal.js is painful 😅 |
nicktrn
left a comment
There was a problem hiding this comment.
Ready for review by a Certified iCal.js Expert ™️
| }; | ||
|
|
||
| const applyTravelDuration = (event: ICAL.Event, seconds: number) => { | ||
| if (seconds <= 0) return event; |
There was a problem hiding this comment.
Negative travel times should be impossible, but better to be safe than sorry
|
|
||
| // for Apple's Travel Time feature only (for now) | ||
| const getTravelDurationInSeconds = (vevent: ICAL.Component, log: typeof logger) => { | ||
| const travelDuration: ICAL.Duration = vevent.getFirstPropertyValue("x-apple-travel-duration"); |
There was a problem hiding this comment.
I did consider checking if this is indeed an ICAL.Duration, but their helpers are incredibly useless here. Will let any string with P as first or second character pass. See:
| const travelSeconds = travelDuration.toSeconds(); | ||
| // integer validation as we can never be sure with ical.js | ||
| if (!Number.isInteger(travelSeconds)) return 0; |
There was a problem hiding this comment.
This is also rather incredible. There are no type checks in the function and we could be getting a very long random string back instead. I think isInteger() is the best bet here.
| return travelSeconds; | ||
| } catch (e) { | ||
| log.error("[ERROR] invalid travelDuration?", e); | ||
| return 0; |
There was a problem hiding this comment.
Just in case anything fails. Will actually change the error message quickly, only just realised I replaced it with the proper logger in the end.
| public isNegative: boolean; | ||
| public icalclass: string; | ||
| public icaltype: string; | ||
| public toSeconds(): number; |
There was a problem hiding this comment.
I did consider using number | string or even any type here to be accurate, but the rest of the ambient def doesn't do this so I chose not to.
|
I'm wondering if it would be better to have travel time as separate events attached to the primary event, or extend the duration of the event instead (as done in this case) 🤔
How does Apple Calendar handle this exactly? |
|
@alishaz-polymath Thanks for reviewing! I think mutating the ICAL.Event startDate directly is safe and the simplest solution in this case, as we are just checking availability and returning The data is only used for those availability checks and discarded when the function returns, so this should not have any unintended side-effects. In summary, this will only affect availability of users with linked Apple Calendars who set travel times for specific events. Apologies if my comments in the related issue confused matters. This is what it looks like in Calendar on Mac, and this is exactly what we will be blocking:
|
What does this PR do?
Adds support for Apple's Travel Time feature
See JSFiddle for example payload and parsing.
Fixes #10474
/claim #10474
Requirement/Documentation
Type of change
How should this be tested?
Mandatory Tasks
Checklist