fix: all booking and duration limits by nicktrn · Pull Request #10480 · calcom/cal.diy

nicktrn · 2023-07-31T14:01:55Z

What does this PR do?

Fixes #10361 🙏

This builds on #10435 to also fix the following issues:

yearly duration limits
yearly booking limits if event booked in multiple years
yearly limits if the checked date range is in multiple years (31 Dec or 1 Jan in view with diff TZ)
performance: re-checking of known busy periods
some unexpected type issues

Further performance improvements are possible if required. The groundwork is already there.

Current unit and e2e tests are happy so hopefully I didn't break anything. New e2e tests in #10518

Type of change

Bug fix (non-breaking change which fixes an issue)
Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

Yearly Duration Limits

Set yearly duration limit
Make some bookings
Issue: year NOT busy
Fix: year busy

Yearly Booking Limits

Set yearly booking limit to 1
Make one booking this year
See that the whole year is now busy
Make one booking next year
Issue: BOTH years are now available for bookings
Fix: both years busy

Multiple Years

Can currently not be tested as there seems to be a separate bug hiding these slots from view. Set availability to 24h for all days, then check start/end of month (or year). When TZ differs, the slots that cross into the host's next month (or year) will not be shown.

Misc

All duration and booking limits should be tested extensively. None of them were working that well beforehand. New E2E tests just for limits in #10518

Mandatory Tasks

Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

~~- I haven't added tests that prove my fix is effective or that my feature works~~

vercel · 2023-07-31T14:02:05Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 9, 2023 10:49pm

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
cal	⬜️ Ignored (Inspect)	Visit Preview		Aug 9, 2023 10:49pm

vercel · 2023-07-31T14:02:06Z

@nicktrn is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

github-actions · 2023-07-31T14:02:13Z

Thank you for following the naming conventions! 🙏

github-actions · 2023-07-31T14:10:06Z

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

nicktrn

Ready for review and suggestions!

nicktrn · 2023-07-31T17:59:26Z

-  label: `Per ${key.split("_")[1].toLocaleLowerCase()}`,
+  label: `Per ${intervalLimitKeyToUnit(key)}`,


this was used in a lot of different places, so extracted to lib

Can we avoid transformation here and just use plain array with ["day", "week", "month", "year"]? Or there is a reason for this?

Sure, just thought it might be easier to extend in the future, e. g. adding "quarter". Looking back should probably localise those strings.

We should be good with plains on low number options.

nicktrn · 2023-07-31T17:59:52Z

-                    intervalOrderKeys.indexOf(limitKeyA as IntervalLimitsKey) -
-                    intervalOrderKeys.indexOf(limitKeyB as IntervalLimitsKey)
+                    ascendingLimitKeys.indexOf(limitKeyA as IntervalLimitsKey) -
+                    ascendingLimitKeys.indexOf(limitKeyB as IntervalLimitsKey)


so was this

nicktrn · 2023-07-31T18:01:36Z

+export async function getBusyTimesForLimitChecks(params: {
+  userId: number;
+  eventTypeId: number;
+  startDate: Date;
+  endDate: Date;
+}) {


This allows us to only fetch the absolute minimum info required to check limits

nicktrn · 2023-07-31T18:05:27Z

+  const bookings = await prisma.booking.findMany({
+    where: {
+      userId,
+      eventTypeId,
+      status: BookingStatus.ACCEPTED,
+      // FIXME: bookings that overlap on one side will never be counted
+      startTime: {
+        gte: startDate,
+      },
+      endTime: {
+        lte: endDate,
+      },
+    },


Should probably get all bookings that start in that period, not also end. Need some input here.

True, an event end time typically does not matter unless it's greater than startDate - this probably makes the most sense.

Suggested change

const bookings = await prisma.booking.findMany({

where: {

userId,

eventTypeId,

status: BookingStatus.ACCEPTED,

// FIXME: bookings that overlap on one side will never be counted

startTime: {

gte: startDate,

},

endTime: {

lte: endDate,

},

},

const bookings = await prisma.booking.findMany({

where: {

userId,

eventTypeId,

status: BookingStatus.ACCEPTED,

// FIXME: bookings that overlap on one side will never be counted

startTime: {

gte: startDate,

},

endTime: {

gte: startDate,

},

},

nicktrn · 2023-07-31T18:08:27Z

-    // needed to correctly apply limits (weeks can be part of two months)
-    startTime: dateFrom.startOf("week").toISOString(),
-    endTime: dateTo.endOf("week").toISOString(),
+    startTime: getBusyTimesStart,
+    endTime: getBusyTimesEnd,


No longer required as we get the booking info to apply limits separately now

nicktrn · 2023-07-31T18:27:27Z

+  // convoluted but typesafe
+  const limitCalculations = ascendingLimitKeys
+    .filter((key) => parsedBookingLimits[key])
+    .map((key) =>
+      checkBookingLimit({ key, limitingNumber: parsedBookingLimits[key], eventStartDate, eventId })
    );


Correct types were previously not preserved and there was an issue further down the chain with this. The entries() helper would have prevented using a simple map function here.

nicktrn · 2023-07-31T18:31:48Z

        },
      },
    });
-    if (bookingsInPeriod >= limitingNumber) {


This used to hide that limitingNumber could sometimes be undefined, as greater and lesser than always evaluate to false in that case

nicktrn · 2023-07-31T18:34:01Z

    const bookingsInPeriod = await prisma.booking.count({
      where: {
-        status: "ACCEPTED",
-        eventType: {
-          AND: [
-            {
-              id: eventId,
-              bookings: {
-                every: {
-                  startTime: {
-                    gte: startDate,
-                  },
-                  endTime: {
-                    lte: endDate,
-                  },


This is a strange recursion that would not count bookings if not ALL bookings for that event were in the same period

nicktrn · 2023-07-31T18:36:08Z

+  key: keyof IntervalLimit;
+  limitingNumber: number | undefined;


type safety

nicktrn · 2023-07-31T18:39:42Z

+  // FIXME: bookings that overlap on one side will never be counted
  const [totalBookingTime] = (await prisma.$queryRaw`
    SELECT SUM(EXTRACT(EPOCH FROM ("endTime" - "startTime")) / 60) as "totalMinutes"
    FROM "Booking"
    WHERE "status" = 'accepted'
-      AND "id" = ${eventId}
+      AND "eventTypeId" = ${eventId}
      AND "startTime" >= ${startDate}
      AND "endTime" <= ${endDate};


Using the wrong key caused this to return 0 most of the time, and a random booking duration at best. Need some input here re counting overlapping bookings, e.g. starts in one week and ends in the next. Currently, those bookings are never considered when checking weekly limits. Same naturally applies to other periods.

nicktrn

Added some more comments for the recent push

nicktrn · 2023-08-02T09:37:00Z

-      if (returnBusyTimes) {
-        return {
-          start: startDate,
-          end: endDate,
-        };


now unused, caller already has this

nicktrn · 2023-08-02T09:39:06Z


-  const limitCalculations = Object.entries(parsedDurationLimits).map(
-    async ([key, limitingNumber]) =>
-      await checkDurationLimit({ key, limitingNumber, eventStartDate, eventId })
+  // not iterating entries to preserve types
+  const limitCalculations = ascendingLimitKeys.map((key) =>
+    checkDurationLimit({ key, limitingNumber: parsedDurationLimits[key], eventStartDate, eventId })
  );


not preserving types caused a hidden undefined comparison in the callee

nicktrn · 2023-08-02T09:41:47Z

-  const [totalBookingTime] = (await prisma.$queryRaw`
+  // FIXME: bookings that overlap on one side will never be counted
+  const [totalBookingTime] = await prisma.$queryRaw<[{ totalMinutes: number | null }]>`


previous type assertion missed this could be null which caused issues when comparing to numbers

nicktrn · 2023-08-02T09:44:55Z

-export interface IntervalLimit {
-  PER_DAY?: number | undefined;
-  PER_WEEK?: number | undefined;
-  PER_MONTH?: number | undefined;
-  PER_YEAR?: number | undefined;
-}
+export type IntervalLimitUnit = "day" | "week" | "month" | "year";
+
+export type IntervalLimit = Partial<Record<`PER_${Uppercase<IntervalLimitUnit>}`, number | undefined>>;


abstracted limit units as they were repeatedly asserted in many different places

nicktrn · 2023-08-02T10:17:27Z

@emrysal @CarinaWolli hope it's okay to ping you here as this builds on #10435 and probably easier to review together. E2E tests are WIP #10518

Quick recap:

weekly limit issues were fixed by extending the date range of bookings we fetch fix: weekly limits #10404
monthly limit issues were pointed out for dates in the past, but simply extending the date range further would have been quite bad for performance
looked into yearly limits and found more issues

nicktrn · 2023-08-06T12:10:50Z

      })
    ).rejects.toThrowError();
  });
-  it("Should return busyTimes when set", async () => {


no need to return busyTimes anymore after refactor

nicktrn · 2023-08-06T12:11:02Z

      })
    ).resolves.toBeUndefined();
  });
-  it("Should return busyTimes when set and limit is reached", async () => {


no need to return busyTimes anymore after refactor

nicktrn · 2023-08-06T12:12:45Z

  bookingLimits: IntervalLimit,
  eventStartDate: Date,
-  eventId: number,
-  returnBusyTimes?: boolean


not needed anymore

nicktrn · 2023-08-06T12:13:55Z

-  if (parsedBookingLimits) {
-    const limitCalculations = Object.entries(parsedBookingLimits).map(
-      async ([key, limitingNumber]) =>
-        await checkBookingLimit({ key, limitingNumber, eventStartDate, eventId, returnBusyTimes })
-    );
-    await Promise.all(limitCalculations)
-      .then((res) => {
-        if (returnBusyTimes) {
-          return res;
-        }
-      })
-      .catch((error) => {
-        throw new HttpError({ message: error.message, statusCode: 401 });
-      });
-    return true;
+  if (!parsedBookingLimits) return false;
+
+  // not iterating entries to preserve types
+  const limitCalculations = ascendingLimitKeys.map((key) =>
+    checkBookingLimit({ key, limitingNumber: parsedBookingLimits[key], eventStartDate, eventId })
+  );
+
+  try {
+    return !!(await Promise.all(limitCalculations));
+  } catch (error) {
+    throw new HttpError({ message: getErrorFromUnknown(error).message, statusCode: 401 });
  }
-  return false;


refactor, same behaviour except removal of busyTimes, added type safety

nicktrn · 2023-08-06T12:14:41Z

-  key: string;
-  limitingNumber: number;
-  returnBusyTimes?: boolean;
+  key: keyof IntervalLimit;
+  limitingNumber: number | undefined;


type safety

nicktrn · 2023-08-06T12:16:58Z

-      // This is used when getting availability
-      if (returnBusyTimes) {
-        return {
-          start: startDate,
-          end: endDate,
-        };
-      }


Now unused, as we already have that data when checking availability for specific periods

nicktrn · 2023-08-06T12:21:15Z

    FROM "Booking"
    WHERE "status" = 'accepted'
-      AND "id" = ${eventId}
+      AND "eventTypeId" = ${eventId}


mixup between booking and event id, caused booking limits to not be enforced here and lead to unnecessary calls to getUserAvailability where those limits were checked again

zomars

Impressive work so far @nicktrn 🙏🏽 I love myself a good refactor. Will test locally and get back to you.

nicktrn · 2023-08-09T22:28:18Z

This is what happens when the surgical approach fails. Apologies if the gutting was too extensive. Feel free to keep the knife if you find one in there!

zomars

Loving the refactor so far. My only concern are the possible git conflicts for other PRs. Not a blocker on my side. Thank you for your contribution 🙏

nicktrn · 2023-08-09T22:54:19Z

Wow, that was quick! I was getting concerned with merges too, glad it was spotted.

emrysal · 2023-08-09T23:15:39Z

+      id: true,
+      startTime: true,
+      endTime: true,
+      eventType: {


I believe what you can do as well is eventTypeId: true - for future reference

emrysal · 2023-08-09T23:16:41Z


 /** This should be called getUsersWorkingHoursAndBusySlots (...and remaining seats, and final timezone) */
-export async function getUserAvailability(
+export const getUserAvailability = async function getUsersWorkingHoursLifeTheUniverseAndEverythingElse(


welp this got merged 😆

emrysal · 2023-08-09T23:17:22Z

  const busyTimes = await getBusyTimes({
    credentials: user.credentials,
-    // needed to correctly apply limits (weeks can be part of two months)
-    startTime: dateFrom.startOf("week").toISOString(),


🙌 - disliked this very much

emrysal · 2023-08-09T23:22:37Z

+  try {
+    return !!(await Promise.all(limitCalculations));
+  } catch (error) {
+    throw new HttpError({ message: getErrorFromUnknown(error).message, statusCode: 401 });


401 Unauthorized?

emrysal · 2023-08-09T23:26:35Z

Amazing work done here! 👍

nicktrn · 2023-08-09T23:37:00Z

Thanks to everyone for the thorough review, this really helps!

PeerRich · 2023-08-10T08:37:55Z

/tip 50 thank you for this awesome PR!

algora-pbc · 2023-08-10T08:38:06Z

@nicktrn: You just got a $50 tip! 👉 Complete your Algora onboarding to collect your payment.

algora-pbc · 2023-08-11T10:21:31Z

🎉🎈 @nicktrn has been awarded $50! 🎈🎊

github-actions Bot added bookings area: bookings, availability, timezones, double booking High priority Created by Linear-GitHub Sync linear Sync Github Issue from community members to Linear.app 🐛 bug Something isn't working labels Jul 31, 2023

vercel Bot deployed to Preview – ui July 31, 2023 14:05 View deployment

nicktrn commented Jul 31, 2023

View reviewed changes

nicktrn marked this pull request as ready for review July 31, 2023 18:45

nicktrn force-pushed the fix-availability-limits branch from 511fa48 to 24fee7b Compare July 31, 2023 18:48

vercel Bot deployed to Preview – ui July 31, 2023 18:51 View deployment

nicktrn mentioned this pull request Aug 2, 2023

test: booking and frequency limit e2e #10518

Closed

2 tasks

nicktrn force-pushed the fix-availability-limits branch from 24fee7b to 9bd9917 Compare August 2, 2023 09:31

vercel Bot deployed to Preview – ui August 2, 2023 09:35 View deployment

nicktrn commented Aug 2, 2023

View reviewed changes

rkreddy99 and others added 12 commits August 6, 2023 11:39

fix: limit booking frequency

ccc1220

fix: add per day for limit check

dbf5469

fix: queries used for yearly limits

7ae7b9d

chore: add interval limit helpers and missing type

1a230e2

chore: refactor limit check libs

230f43e

chore: refactor busy times getter for limits

eb84e35

fix: availability limit perf and multiple years

ec4da17

fix: availability limit types and unit tests

dac89ab

fix: total booking duration type

dbc5a70

chore: remove excess filter

df8831f

chore: remove unused parameters

19c3570

chore: simplify limit manager

b50fbbb

vercel Bot deployed to Preview – ui August 6, 2023 12:05 View deployment

nicktrn commented Aug 6, 2023

View reviewed changes

Merge branch 'main' into fix-availability-limits

594ab57

alannnc requested review from a team August 9, 2023 21:36

vercel Bot deployed to Preview – ui August 9, 2023 21:37 View deployment

zomars reviewed Aug 9, 2023

View reviewed changes

zomars approved these changes Aug 9, 2023

View reviewed changes

zomars merged commit 37ce886 into calcom:main Aug 9, 2023

zomars mentioned this pull request Aug 9, 2023

fix: Limit Booking Frequency #10435

Closed

2 tasks

emrysal reviewed Aug 9, 2023

View reviewed changes

algora-pbc Bot added the 💰 Rewarded Rewarded bounties on Algora.io label Aug 11, 2023

nicktrn mentioned this pull request Aug 25, 2023

test: booking and duration limits e2e #10968

Merged

2 tasks

hariombalhara mentioned this pull request Dec 8, 2023

test: booking and duration limits e2e #10968 #12705

Closed

2 tasks

		label: `Per ${key.split("_")[1].toLocaleLowerCase()}`,
		label: `Per ${intervalLimitKeyToUnit(key)}`,

		key: keyof IntervalLimit;
		limitingNumber: number \| undefined;

Conversation

nicktrn commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Type of change

How should this be tested?

Yearly Duration Limits

Yearly Booking Limits

Multiple Years

Misc

Mandatory Tasks

Checklist

Uh oh!

vercel Bot commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Jul 31, 2023

Uh oh!

github-actions Bot commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Next.js Bundle Analysis for @calcom/web

Uh oh!

nicktrn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicktrn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicktrn commented Aug 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nicktrn commented Jul 31, 2023 •

edited

Loading

vercel Bot commented Jul 31, 2023 •

edited

Loading

github-actions Bot commented Jul 31, 2023 •

edited

Loading

github-actions Bot commented Jul 31, 2023 •

edited

Loading