Skip to content

fix: booking timeslots#12195

Merged
CarinaWolli merged 10 commits intomainfrom
fix-booking-timeslots
Nov 3, 2023
Merged

fix: booking timeslots#12195
CarinaWolli merged 10 commits intomainfrom
fix-booking-timeslots

Conversation

@supalarry
Copy link
Copy Markdown
Contributor

@supalarry supalarry commented Nov 2, 2023

What does this PR do?

Fixes #12142

Fixes mergeOverlappingDateRanges not properly merging date ranges if first starts at day X and ends at day X + 1 but second date range starts and ends in X + 1 while starting after and ending before the first one.

How it was before

Lets say mergeOverlappingDateRanges took the given dateRanges below as argument. You see that first one starts November 2, 23:00 and ends November 3, 07:00 and all other date ranges are within its range.

  {
    "start": "2023-11-02T23:00:00.000Z",
    "end": "2023-11-03T07:00:00.000Z"
  },
  {
    "start": "2023-11-02T23:15:00.000Z",
    "end": "2023-11-03T00:00:00.000Z"
  },
  {
    "start": "2023-11-03T00:15:00.000Z",
    "end": "2023-11-03T01:00:00.000Z"
  },
  {
    "start": "2023-11-03T01:15:00.000Z",
    "end": "2023-11-03T02:00:00.000Z"
  },

then it didn't merge them and returned what's below. It should have only returned the first entry because it covers all other date ranges.

  {
    "start": "2023-11-02T23:00:00.000Z",
    "end": "2023-11-03T07:00:00.000Z"
  },
  {
    "start": "2023-11-03T00:15:00.000Z",
    "end": "2023-11-03T01:00:00.000Z"
  },
  {
    "start": "2023-11-03T01:15:00.000Z",
    "end": "2023-11-03T02:00:00.000Z"
  }

because within the function there is conditional

if (
    isSameDay(currentRange.start.toDate(), nextRange.start.toDate()) &&
    currentRange.end.valueOf() > nextRange.start.valueOf()
) {

where isSameDay is

function isSameDay(date1: Date, date2: Date) {
  return (
    date1.getUTCFullYear() === date2.getUTCFullYear() &&
    date1.getUTCMonth() === date2.getUTCMonth() &&
    date1.getUTCDate() === date2.getUTCDate()
  );
}

How it is now

We have a new isCurrentRangeOverlappingNext function for the if block that does not check the start based on the same day but based on timestamp value.

Given as input:

  {
    "start": "2023-11-02T23:00:00.000Z",
    "end": "2023-11-03T07:00:00.000Z"
  },
  {
    "start": "2023-11-02T23:15:00.000Z",
    "end": "2023-11-03T00:00:00.000Z"
  },
  {
    "start": "2023-11-03T00:15:00.000Z",
    "end": "2023-11-03T01:00:00.000Z"
  },
  {
    "start": "2023-11-03T01:15:00.000Z",
    "end": "2023-11-03T02:00:00.000Z"
  }

it returns

  {
    "start": "2023-11-02T23:00:00.000Z",
    "end": "2023-11-03T07:00:00.000Z"
  }

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

Test data above is from Australia/Brisbane example below (scenario 1)

Scenario 1

Create user 1 and user 2 with availabilities like below and Australia/Brisbane timezone. Then, create a round robin event where both of those users take part in. Check the booking page and see that there are no duplicate timetable entries and timetable entries are in ascending order.

Screenshot 2023-11-02 at 12 26 05
Screenshot 2023-11-02 at 12 26 02
Screenshot 2023-11-02 at 10 30 27

Scenario 2

Change availability timezones to Europe/Rome and check the round robin booking page again. Timetable entries are in order and there are no duplicates.

Mandatory Tasks

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

@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 2, 2023

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

Name Status Preview Comments Updated (UTC)
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 3, 2023 2:28pm
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 3, 2023 2:28pm
5 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ai ⬜️ Ignored (Inspect) Visit Preview Nov 3, 2023 2:28pm
cal ⬜️ Ignored (Inspect) Visit Preview Nov 3, 2023 2:28pm
cal-demo ⬜️ Ignored (Inspect) Visit Preview Nov 3, 2023 2:28pm
qa ⬜️ Ignored (Inspect) Visit Preview Nov 3, 2023 2:28pm
ui ⬜️ Ignored (Inspect) Visit Preview Nov 3, 2023 2:28pm

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 2, 2023

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes!

@github-actions github-actions Bot added bookings area: bookings, availability, timezones, double booking High priority Created by Linear-GitHub Sync 🐛 bug Something isn't working 👩‍🔬 needs investigation labels Nov 2, 2023
@zomars zomars added the core area: core, team members only label Nov 2, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 2, 2023

📦 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! 🙌

@deploysentinel
Copy link
Copy Markdown

deploysentinel Bot commented Nov 2, 2023

Current Playwright Test Results Summary

✅ 293 Passing - ⚠️ 7 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 11/03/2023 02:31:53pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 832f621

Started: 11/03/2023 02:24:20pm UTC

⚠️ Flakes

📄   packages/app-store/routing-forms/playwright/tests/basic.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Routing Forms Seeded Routing Form Test preview should return correct route
Retry 1Initial Attempt
0% (0) 0 / 282 runs
failed over last 7 days
4.26% (12) 12 / 282 runs
flaked over last 7 days

📄   apps/web/playwright/managed-event-types.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Managed Event Types tests Can create managed event type
Retry 2Retry 1Initial Attempt
9.51% (27) 27 / 284 runs
failed over last 7 days
14.08% (40) 40 / 284 runs
flaked over last 7 days

📄   apps/web/playwright/teams.e2e.ts • 2 Flakes

Top 1 Common Error Messages

null

2 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Teams - NonOrg Can create a booking for Collective EventType
Retry 1Initial Attempt
0% (0) 0 / 234 runs
failed over last 7 days
1.28% (3) 3 / 234 runs
flaked over last 7 days
Teams - NonOrg Can create a private team
Retry 1Initial Attempt
0% (0) 0 / 234 runs
failed over last 7 days
14.53% (34) 34 / 234 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/inline.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Inline Iframe Inline Iframe - Configured with Dark Theme
Retry 1Initial Attempt
2.11% (6) 6 / 284 runs
failed over last 7 days
25.70% (73) 73 / 284 runs
flaked over last 7 days

📄   packages/embeds/embed-core/playwright/tests/action-based.e2e.ts • 2 Flakes

Top 1 Common Error Messages

null

2 Test Cases Affected

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Popup Tests should be able to reschedule
Retry 1Initial Attempt
21.75% (62) 62 / 285 runs
failed over last 7 days
75.09% (214) 214 / 285 runs
flaked over last 7 days
Popup Tests should open Routing Forms embed on click
Retry 1Initial Attempt
0% (0) 0 / 285 runs
failed over last 7 days
29.82% (85) 85 / 285 runs
flaked over last 7 days

View Detailed Build Results


Copy link
Copy Markdown
Member

@CarinaWolli CarinaWolli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great fix! @supalarry could we add a test for that?

@supalarry
Copy link
Copy Markdown
Contributor Author

supalarry commented Nov 3, 2023

Looks good, great fix! @supalarry could we add a test for that?

Done.

  1. In commit f2b20c3 I split getAggregatedAvailability.ts into a folder with sub-folder where mergeOverlappingDateRanges is to make it easier to separate tests.

  2. In commit eb59496 I wrote tests for the mergeOverlappingDateRanges function that was updated in this PR.

Copy link
Copy Markdown
Member

@CarinaWolli CarinaWolli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good!

@CarinaWolli CarinaWolli merged commit fc716f5 into main Nov 3, 2023
@CarinaWolli CarinaWolli deleted the fix-booking-timeslots branch November 3, 2023 14:59
zomars pushed a commit that referenced this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working core area: core, team members only High priority Created by Linear-GitHub Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Booking time slots doubling up and not rendering in correct order

3 participants