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: Fairness when routing #18550

Open
wants to merge 69 commits into
base: main
Choose a base branch
from
Open

Conversation

emrysal
Copy link
Contributor

@emrysal emrysal commented Jan 9, 2025

What does this PR do?

Rewrites the handling of pre-qualification due to various filters, correctly and adds fallback logic to whenever no users are returned. We intend to prevent no availability at no cost.

WIP: Pre-push of filters for partial review.

@@ -27,18 +26,10 @@ afterEach(() => {
});

describe("filterHostByLeadThreshold", () => {
it("skips filter if host is fixed", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed users are now type guarded.

@keithwillcode keithwillcode added core area: core, team members only foundation labels Jan 9, 2025
it("skips filter if lead threshold is null", async () => {
const hosts = [{ isFixed: false, createdAt: new Date(), user: { id: 1 } }];
const hosts = [
{ isFixed: false as const, createdAt: new Date(), user: { id: 1, email: "[email protected]" } },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to mark isFixed as const so it's not type hinted boolean - passing a boolean type errors.

Copy link

vercel bot commented Jan 9, 2025

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

Name Status Preview Comments Updated (UTC)
cal-com-ui-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 7, 2025 4:32pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Feb 7, 2025 4:32pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Feb 7, 2025 4:32pm

if (contactOwnerEmail && aggregatedAvailability.length > 0) {
// Fairness and Contact Owner disqualification reasons need Availability within 2 weeks
// if not, we attempt to re-open.
if (fallbackHosts && fallbackHosts.length > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joeauyeung rewritten this check, now any time there's fallbackHosts found I run the 2-weeks check; if there's not enough availability within the qualified users we use the fallbackHosts instead.

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want that?

If reschedule with same host is enabled, we would do that too then. I think we shouldn't make that change in this PR

@@ -886,29 +875,6 @@ const calculateHostsAndAvailabilities = async ({
bypassBusyCalendarTimes: boolean;
shouldServeCache?: boolean;
}) => {
const routedTeamMemberIds = input.routedTeamMemberIds ?? null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to filterHostsBySameRoundRobinHost check.

Copy link
Contributor

github-actions bot commented Jan 9, 2025

E2E results are ready!


return {
qualifiedHosts: [...fixedHosts, ...hostsAfterContactOwnerMatching],
fallbackHosts,
Copy link
Member

Choose a reason for hiding this comment

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

I think the fallbackHosts should never be the event type hosts.

Fallback hosts should ideally be hostsAfterFairnessMatching. In case the contact owne has no availability for 2 weeks we would ignore all other logic (attribute routing, fairness,..)

Copy link
Member

Choose a reason for hiding this comment

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

@emrysal should be fixed with ae12518

});

// We filter out users but ensure allHostUsers remain same.
let users = allHostUsers;
let users = [...qualifiedRRUsers, ...fallbackRRUsers, ...fixedUsers];
Copy link
Member

Choose a reason for hiding this comment

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

change naming of this.

these fallback users are different than the once returned from getQualified users. They are filtered don't don't include the qualified users

@@ -657,26 +710,6 @@ async function handler(

Copy link
Member

Choose a reason for hiding this comment

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

this is already handled in findQualifedUsers

@@ -1267,7 +1300,7 @@ async function handler(
endTime: reqBody.end,
contactOwnerFromReq,
contactOwnerEmail,
allHostUsers,
allHostUsers: [...qualifiedRRUsers, ...fallbackRRUsers, ...fixedUsers],
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we need fallbackRRUsers here, take a look at what buildDryRunBooking exactly does

availableUsers,
fixedUserPool,
});
const luckyUsers: typeof users = [];
Copy link
Member

Choose a reason for hiding this comment

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

luckyUserPool already includes only the contact owner (if available)


return {
qualifiedRRUsers,
fallbackRRUsers, // without qualified
Copy link
Member

Choose a reason for hiding this comment

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

Rename to make clear this is different from the fallback users returned form findQualifiedUsers

],
eventType,
allRRHosts: hosts,
routingFormResponse,
Copy link
Member

Choose a reason for hiding this comment

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

routingFormResponse is added here

contactOwnerEmail,
loggerWithEventDetails,
startTime,
// adjust start time so we can check for available slots in the first two weeks
startTime: startTime.isBefore(twoWeeksFromNow)
Copy link
Member

Choose a reason for hiding this comment

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

if the start time is within the first two weeks, make sure to load the whole two weeks starting today. Otherwise we have issues if it's end of a month and users already loads next month

bypassBusyCalendarTimes,
shouldServeCache,
});
if (!firstTwoWeeksAvailabilities.aggregatedAvailability.length) {
Copy link
Member

Choose a reason for hiding this comment

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

If a whole month (that's not within the first two weeks) isn't available, then check if the first two week are available. If they are also not available then fallback

Todo: Add more tests for this fallback logic

shouldServeCache,
}));
}
// Merge qualified availability with fallback availabilities
Copy link
Member

Choose a reason for hiding this comment

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

we still want to be qualified hosts to be bookable, they might be available after the two weeks

Copy link
Member

Choose a reason for hiding this comment

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

i might don't need this merge

@@ -1,90 +0,0 @@
import { describe, expect, it } from "vitest";
Copy link
Member

Choose a reason for hiding this comment

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

getUsersWithCredentialsConsideringContactOwner was removed because all this logic is already handled in findQualifiedUsers

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 core area: core, team members only foundation High priority Created by Linear-GitHub Sync high-risk Requires approval by Foundation team ready-for-e2e routing-forms area: routing forms, routing, forms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants