Skip to content

fix: Fix "Could not book the meeting" issue (fix-bookingIssue)#12252

Merged
keithwillcode merged 4 commits intomainfrom
fix-bookingIssue
Nov 7, 2023
Merged

fix: Fix "Could not book the meeting" issue (fix-bookingIssue)#12252
keithwillcode merged 4 commits intomainfrom
fix-bookingIssue

Conversation

@gitstart-app
Copy link
Copy Markdown
Contributor

@gitstart-app gitstart-app Bot commented Nov 6, 2023

@vercel
Copy link
Copy Markdown

vercel Bot commented Nov 6, 2023

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

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 6, 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
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 6, 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 6, 2023

Current Playwright Test Results Summary

✅ 313 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/07/2023 02:35:30pm UTC)

Run Details

Running Workflow PR Update on Github Actions

Commit: 47a37a4

Started: 11/07/2023 02:24:27pm UTC

⚠️ Flakes

📄   apps/web/playwright/login.2fa.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
2FA Tests should allow a user to enable 2FA and login using 2FA
Retry 1Initial Attempt
0.45% (1) 1 / 224 run
failed over last 7 days
30.36% (68) 68 / 224 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
20.60% (48) 48 / 233 runs
failed over last 7 days
73.82% (172) 172 / 233 runs
flaked over last 7 days
Popup Tests should open embed iframe on click - Configured with light theme
Retry 2Retry 1Initial Attempt
0.43% (1) 1 / 233 run
failed over last 7 days
60.09% (140) 140 / 233 runs
flaked over last 7 days

📄   apps/web/playwright/insights.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Insights should be able to switch between memberUsers
Retry 1Initial Attempt
0% (0) 0 / 227 runs
failed over last 7 days
1.32% (3) 3 / 227 runs
flaked over last 7 days

📄   apps/web/playwright/teams.e2e.ts • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Teams - NonOrg Non admin team members cannot create team in org
Retry 1Initial Attempt
0% (0) 0 / 214 runs
failed over last 7 days
40.19% (86) 86 / 214 runs
flaked over last 7 days

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

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Event Types tests user Different Locations Tests Can add Link Meeting as location and book with it
Retry 1Initial Attempt
0% (0) 0 / 226 runs
failed over last 7 days
10.18% (23) 23 / 226 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
10.82% (25) 25 / 231 runs
failed over last 7 days
12.99% (30) 30 / 231 runs
flaked over last 7 days

View Detailed Build Results


Copy link
Copy Markdown
Contributor

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

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

Nice find. Can we update the tests in date-ranges.test.ts to include tests for this change?

@gitstart-calcom
Copy link
Copy Markdown
Contributor

gitstart-calcom commented Nov 7, 2023

@keithwillcode We have made sure that these changes do not affect any test, so these current tests will also cover this new change 😄.

@keithwillcode
Copy link
Copy Markdown
Contributor

keithwillcode commented Nov 7, 2023

@gitstart-calcom My point is that if there were a unit test for the issue that is fixed in this PR, that unit test would have been failing this whole time. If logic like this changes and no tests fail, that's great, but it doesn't mean that we actually have a test that ensures this change in logic works and continues working.

Even if we have E2E tests to cover this, I prefer adding unit tests so we have verification at the root of the code.

@gitstart-calcom
Copy link
Copy Markdown
Contributor

@keithwillcode Makes sense! We will add a unit test for that now!

it("should skip event if it ends before it starts (same day but different hours)", () => {
const item = {
days: [1],
startTime: new Date(new Date().setUTCHours(8, 0, 0, 0)), // 8 AM
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@gitstart-calcom Thanks for adding these tests. Do we know how this happens? I'm just wondering how an end time would get set to be before a start time?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be honest we got confused about how this can happen too. Also, the issue occurring just in the first available time-slot was really weird. This hypothetical situation may be the result of a data entry error or a bug in the application code. It could be a mistake in the way the data is being provided or manipulated before it reaches the processWorkingHours function. Maybe setting the endTime/dateTo before the startTime/dateFrom can cause this issue, but we don't see a scenario in which that happens

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But now, with the specific tests and the new logic, we fixed the issue and we can identify a possible issue faster 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool yeah that's what I suspected myself. Seems like this function is being passed bad data for some reason.

@gitstart-calcom
Copy link
Copy Markdown
Contributor

@keithwillcode We added 2 test lines for this case and added a screenshot in the description 😄

@keithwillcode keithwillcode merged commit d28e20e into main Nov 7, 2023
@keithwillcode keithwillcode deleted the fix-bookingIssue branch November 7, 2023 15:33
zomars pushed a commit that referenced this pull request Jan 29, 2024
Co-authored-by: gitstart-calcom <gitstart-calcom@users.noreply.github.com>
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 High priority Created by Linear-GitHub Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants