-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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: minimum notice for events with seats #15343
base: main
Are you sure you want to change the base?
feat: minimum notice for events with seats #15343
Conversation
@balthazur 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. |
@CarinaWolli done! There was some auto-formatting happening in 2 files, I think in a pre-commit hook but I am not sure. I marked the files with a comment. Maybe I have something misconfigured locally? |
This PR is being marked as stale due to inactivity. |
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.
// not using ternary's here to help TS narrow down the type of effectiveMinimumBookingNotice correctly | ||
if ( | ||
eventType.seatsPerTimeSlot && | ||
eventType.seatsMinimumBookingNotice !== null && |
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.
eventType.seatsMinimumBookingNotice !== null && | |
!isNaN(eventType.seatsMinimumBookingNotice) && |
Possibly for clarity; undefined
is used below as `undefined < {minimumBookingNoticeValue} which will always be false, but it's quite unclear this works this way.
eventType.seatsMinimumBookingNotice < eventType.minimumBookingNotice | ||
) { | ||
seatsMinimumBookingNoticeActive = true; | ||
effectiveMinimumBookingNotice = eventType.seatsMinimumBookingNotice; |
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.
Why not simplify and go for something like this? Untested pseudocode.
const effectiveMinimumBookingNotice = !isNaN(seatsMinimumBookingNotice) && seatsCount > 0
? Math.min(seatsMinimumBookingNotice, minimumBookingNotice)
: minimumBookingNotice;
@CarinaWolli and I also had a discussion just now that may be simplifying further also; we see no reason to continue applying a minimum booking notice once a booking has been booked for further seats. So the seatsMinimumBookingNotice is possibly default-able to 0
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.
So the seatsMinimumBookingNotice is possibly default-able to 0
Also in the input field we should default to 0 for seatsMinimumBookingNotice
, so it's clear to the user
This PR is being marked as stale due to inactivity. |
What does this PR do?
This PR adds a new advanced setting for event types with seats enabled. The goal is to allow a booking with at least one existing attendee to be booked later than the minimumBookingNotice would allow.
Here is short Loom, demonstrating the feature.
The PR essentially does three things:
Changes made to slot logic
These were explained to Zomars during our call. Basically, it does not change the getSlot logic.
The changes were to remove a redundant check for minimumBookingNotice at the top of the getAvailableSlots function; otherwise, we would lose all potential slots that would be enabled by the new setting. We then decide if the feature is active and pass minimumBookingNotice = 0 to getSlots, so we get all available slots.
Further down the getAvailableSlots function, there is a previously redundant "safety check" that filters out out-of-bounds slots based on minimumBookingNotice. We extended this check to watch for seatsMinimumBookingNotice and minimumBookingNotice, so in that special case when the setting is active, we will filter out the correct slots at last.
Settings, 3 cases:
Default, empty seatsMinimumBookingNotice:
shorter minimum booking notice for seats with existing booking:
"invalid case", handled by allowing it because it's not breaking but we arenotifiying the user that I will have no effect. Use case would also be that user changes the default minimumBookingNotice, so the seatsMinimumBookingNotice becomes valid again.
This is what seemed the most reasonable to me. Maybe need to be check with product if this solution is fine.
Full Example:
E.g. minimumBookingNotice is 7 days. There is an existing event with only 1 out of 4 seats booked tomorrow, which cannot be booked. By setting the seatsMinimumBookingNotice to e.g. 1 hour., users are displayed the existing booking with 1/4 seats, and can book it up to 1 hour before the event.
Calendar:
Advanced Settings. Info: Hint message was not implemented yet when I took this screenshot.
Limit Settings:
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
To test in the app:
Tests can be run with:
TZ=UTC yarn test getSchedule.test.ts