Skip to content
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

fix: Multiple rr-hosts combine to create erroneous availability #18772

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

emrysal
Copy link
Contributor

@emrysal emrysal commented Jan 20, 2025

What does this PR do?

Copy link
Contributor

github-actions bot commented Jan 20, 2025

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

Unknown release type "bugfix" found in pull request title "bugfix: Multiple rr-hosts combine to create erroneous availability". 

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@keithwillcode keithwillcode added core area: core, team members only foundation labels Jan 20, 2025
@emrysal emrysal changed the title wip: Open draft PR with passing test (should fail) bugfix: Multiple rr-hosts combine to create erroneous availability Jan 20, 2025
Copy link

vercel bot commented Jan 21, 2025

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

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Jan 22, 2025 2:11am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Jan 22, 2025 2:11am

@emrysal emrysal changed the title bugfix: Multiple rr-hosts combine to create erroneous availability fix: Multiple rr-hosts combine to create erroneous availability Jan 21, 2025
expect(result).toHaveLength(6);
});

it("can merge multiple time slot groups when multiple date ranges are given that overlap", async () => {
Copy link
Contributor Author

@emrysal emrysal Jan 22, 2025

Choose a reason for hiding this comment

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

Overlapping time slots weren't possible before as date ranges were merged, even rr hosts. Now they are possible because different rr hosts may have overlapping date ranges but we still want to handle individually.

@@ -233,12 +237,12 @@ function buildSlotsWithDateRanges({
};
}

slots.push(slotData);
slots.set(slotData.time.toISOString(), slotData);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Key based on start time (same as rendering in Booker)

},
// 12:00-12:20,12:20-12:40
{
start: nextDay.hour(11).minute(20),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this resulted in 7 slots, due to 11:20/11:40 overlap with dateRanges[0]. This is now corrected to the correct (5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core area: core, team members only foundation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants