-
Notifications
You must be signed in to change notification settings - Fork 7.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: Implement global booking limits #14243
base: main
Are you sure you want to change the base?
Conversation
@asadath1395 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. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (03/28/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (03/28/24)1 label was added to this PR based on Keith Williams's automation. "Add foundation team as reviewer" took an action on this PR • (04/16/24)1 reviewer was added to this PR based on Keith Williams's automation. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 New Page AddedThe following page was added to the bundle from the code in this PR:
|
…pecified for individual events
…values are copied
… set individually
…5/cal.com into booking-global-limits
This PR is being closed due to a lack of activity. Please reopen if work is to continue. |
@CarinaWolli Since you are working on global booking limits for teams #16557, 2nd point here #14243 (review) is not applicable for this PR, right? |
@keithwillcode Could you please re-open this PR? I would like to continue the work here |
Yes you are right, we changed that. We only want to count the user's bookings including bookings from managed event types, but not bookings from other team event types (collective, round-robin) |
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.
Didn't test this yet, but I found some issues in the code that need to be addressed first. We also need to add tests for this, probably best inside global-booking-limits.test.ts
@@ -30,6 +30,7 @@ const tabs: VerticalTabItemProps[] = [ | |||
{ name: "general", href: "/settings/my-account/general" }, | |||
{ name: "calendars", href: "/settings/my-account/calendars" }, | |||
{ name: "conferencing", href: "/settings/my-account/conferencing" }, | |||
{ name: "bookings", href: "/settings/my-account/bookings" }, |
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.
Should be my-account/bookingLimits
and tab also should be shown as 'Booking Limits'
undefined, | ||
rescheduleUid, | ||
eventType.schedule?.timeZone, | ||
userId |
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.
userId isn't a parameter of checkBookingLimits
title={t("bookings")} | ||
description={t("bookings_settings_description", { appName: APP_NAME })} |
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.
title: Booking Limits
description: Set global limits for when and how often you can be booked
} else { | ||
bookingsLimitFormMethods.setValue("bookingLimits", {}); | ||
} | ||
handleSubmit(bookingsLimitFormMethods.getValues()); |
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.
toast is missing when enabling/disabling toggle
packages/core/getBusyTimes.ts
Outdated
rescheduleUid, | ||
bookingLimits, | ||
durationLimits, | ||
globalBookingLimits, |
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.
globalBookingLimits
isn't in of params
packages/core/getBusyTimes.ts
Outdated
let eventTypeIdFilter: Prisma.BookingWhereInput = { eventTypeId }; | ||
if (globalBookingLimits) { | ||
if (bookingLimits && Object.keys(bookingLimits).length > 0) { | ||
eventTypeIdFilter = { eventTypeId: { not: eventTypeId } }; |
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.
what's that filter for?
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.
This was added earlier as i had a different implementation in place, removed it now as implementation has changed.
@CarinaWolli Added tests and addressed all the comments from above. Could you please review this PR again? Thanks!! |
85553a3
to
f109a4a
Compare
f109a4a
to
6cf0a97
Compare
@CarinaWolli Please review this PR once you are free. Thanks! |
What does this PR do?
Fixes #8985
Type of change
Mandatory Tasks