Buffer limits fix by sean-brydon · Pull Request #5248 · calcom/cal.diy

sean-brydon · 2022-10-27T14:36:32Z

This PR fixes the buffer limit logic for before + after limits.

I will record a Loom for this tomorrow if needed. Please checkout and test this branch on preview to ensure the correct behaviour is indented.

vercel · 2022-10-27T14:36:34Z

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

Name	Status	Preview	Updated
cal	✅ Ready (Inspect)	Visit Preview	Nov 2, 2022 at 9:35AM (UTC)

CarinaWolli

I did not fully follow the conversation about buffers but I think there is something still not right yet 🤔 I have a before buffer of 15 minutes, so I don't think I should be able to make my bookings like that:

CarinaWolli · 2022-10-28T13:34:47Z

-    end: dayjs(a.end)
-      .add(currentUser.bufferTime + (afterEventBuffer || 0), "minute")
+    start: dayjs(a.start)
+      .subtract(afterEventBuffer || 0, "minutes")


I think we need to add the beforeEventBuffer here too. I was able to book this with a beforeEventBuffer of 15 minutes:

I will investigate this on Monday :) Although this is different behaviour from what myself and @Jaibles have experienced.

Confirmed this is indeed a bug @CarinaWolli - nice spot. It happens when you book 2:30 before you book 2:15. beforeBuffer needs to be taken into account in the busyTime

ciaranha · 2022-10-31T11:56:34Z

I did not fully follow the conversation about buffers but I think there is something still not right yet 🤔 I have a before buffer of 15 minutes, so I don't think I should be able to make my bookings like that:

Yea the conversation was largely about ensuring you cannot end up with back to back meetings (and preventing it actually requires adding a buffer before and after even if you've just set before)

alishaz-polymath · 2022-10-31T12:50:17Z

Pretty sure in this logic we can still end up with back to back bookings by simply booking through an event type which doesn't have buffer. @sean-brydon @Jaibles

CarinaWolli

I left you one comment. However, I found another bug that has nothing to do with your PR that makes it unable to even test this. I tried to book 2 different event types (one has only a before buffer and one has before and after), but I realized that busyTimes only includes the bookings from the event type that is currently booked. I think we need to remove the eventTypeId when querying the busyTimes, so we also include all other bookings from different event types in the availabilities

CarinaWolli · 2022-10-31T17:50:19Z

+      bookings.map(({ startTime, endTime, title, id, eventType }) => ({
        start: startTime,
+        end: dayjs(endTime)
+          .add((eventType?.afterEventBuffer || afterEventBuffer || 0) + (beforeEventBuffer || 0), "minute")


I think we only need to add eventType.afterEventBuffer here. If there is none, we don't need to add the afterEventBuffer from the booking.

sean-brydon · 2022-10-31T22:54:08Z

    .findMany({
      where: {
        userId,
-        eventTypeId,


Nice spot @CarinaWolli - Looks like we were only getting busy events for this particular event type and not all event types. Potentially allowing a double booking (same time) as each other

sean-brydon · 2022-11-01T11:59:56Z


 MockDate.set("2021-06-20T11:59:59Z");

-it("can fit 24 hourly slots for an empty day", async () => {


Remove tests on code that isn't used in product anymore

sean-brydon · 2022-11-01T12:01:05Z

-        start: startTime,
+      bookings.map(({ startTime, endTime, title, id, eventType }) => ({
+        start: dayjs(startTime)
+          .subtract((eventType?.beforeEventBuffer || 0) + (afterEventBuffer || 0), "minute")


Thanks @CarinaWolli for this fix :)

Always subtract the beforeEventBuffer for this specific booking eventType - then add afterEventBuffer for the buffer you are trying to book.

Preventing different event types from having a clash

sean-brydon · 2022-11-01T12:02:05Z

+    busyTimes.push(
+      ...calendarBusyTimes.map((value) => ({
+        ...value,
+        end: dayjs(value.end)


Always add beforeEventBuffer to the end time so we get separation on calendar events

Without this we could book a meeting straight after a normal calendar event.

sean-brydon · 2022-11-01T12:02:35Z

+          .add(beforeEventBuffer || 0, "minute")
+          .toDate(),
+        start: dayjs(value.start)
+          .subtract(afterEventBuffer || 0, "minute")


Always subtract afterEventBuffer if it exists

sean-brydon · 2022-11-01T12:02:58Z

  const bufferedBusyTimes: EventBusyDetails[] = busyTimes.map((a) => ({
    ...a,
-    start: dayjs(a.start).subtract(currentUser.bufferTime, "minute").toISOString(),
-    end: dayjs(a.end)


This logic is handled in getBusyTimes now as bookings/calendar events require different logic

sean-brydon · 2022-11-01T12:03:15Z

  currentSeats,
 }: {
  time: Dayjs;
-  busy: (TimeRange | { start: string; end: string } | EventBusyDate)[];


These types just simply are EventBusyDate - not sure why we replicated this

sean-brydon · 2022-11-01T12:03:25Z


  return busy.every((busyTime) => {
-    const startTime = dayjs.utc(busyTime.start).subtract(beforeBufferTime, "minutes").utc();
+    const startTime = dayjs.utc(busyTime.start).utc();


Handled in getBusyTimes

alishaz-polymath

Looks good to me. Buffer specific testing went well
Awesome job @sean-brydon

CarinaWolli

Tested and works as expected 👏🏻

* Buffer limits + remove redundant tests * Fixing buffer * Compound * Afterbuffer fix for no event afterbuffer set * Bug fixes * Buffer includes eventType before

Buffer limits + remove redundant tests

122c9db

vercel Bot deployed to Preview October 27, 2022 14:40 View deployment

CarinaWolli requested changes Oct 28, 2022

View reviewed changes

Fixing buffer

c5091a8

vercel Bot had a problem deploying to Preview October 31, 2022 09:17 Failure

sean-brydon added 2 commits October 31, 2022 14:24

Compound

1ae11c3

Afterbuffer fix for no event afterbuffer set

fcf060e

vercel Bot deployed to Preview October 31, 2022 14:42 View deployment

emrysal requested a review from CarinaWolli October 31, 2022 17:15

CarinaWolli reviewed Oct 31, 2022

View reviewed changes

Bug fixes

b31e720

sean-brydon commented Oct 31, 2022

View reviewed changes

Merge branch 'main' into bug/buffer_limits

c964d10

vercel Bot had a problem deploying to Preview October 31, 2022 23:01 Failure

Buffer includes eventType before

68505ed

vercel Bot had a problem deploying to Preview November 1, 2022 10:56 Failure

sean-brydon commented Nov 1, 2022

View reviewed changes

Merge branch 'main' into bug/buffer_limits

d3a71c0

vercel Bot had a problem deploying to Preview November 1, 2022 12:08 Failure

alishaz-polymath approved these changes Nov 1, 2022

View reviewed changes

alishaz-polymath requested a review from CarinaWolli November 1, 2022 12:13

CarinaWolli approved these changes Nov 2, 2022

View reviewed changes

Merge branch 'main' into bug/buffer_limits

de771cd

sean-brydon added ♻️ autoupdate tells kodiak to keep this branch up-to-date automerge labels Nov 2, 2022

vercel Bot deployed to Preview November 2, 2022 09:35 View deployment

kodiakhq Bot merged commit f34d6e3 into main Nov 2, 2022

kodiakhq Bot deleted the bug/buffer_limits branch November 2, 2022 09:40

haffla pushed a commit to tourlane/cal.com that referenced this pull request Nov 22, 2022

Buffer limits fix (calcom#5248)

296c018

* Buffer limits + remove redundant tests * Fixing buffer * Compound * Afterbuffer fix for no event afterbuffer set * Bug fixes * Buffer includes eventType before


		MockDate.set("2021-06-20T11:59:59Z");

		it("can fit 24 hourly slots for an empty day", async () => {

Conversation

sean-brydon commented Oct 27, 2022

Uh oh!

vercel Bot commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarinaWolli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

ciaranha commented Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alishaz-polymath commented Oct 31, 2022

Uh oh!

CarinaWolli 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!

alishaz-polymath left a comment

Choose a reason for hiding this comment

Uh oh!

CarinaWolli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vercel Bot commented Oct 27, 2022 •

edited

Loading

ciaranha commented Oct 31, 2022 •

edited

Loading