Skip to content

refactor: handleNewBooking #4#15673

Merged
CarinaWolli merged 20 commits intomainfrom
refactor/handleNewBooking-4
Oct 2, 2024
Merged

refactor: handleNewBooking #4#15673
CarinaWolli merged 20 commits intomainfrom
refactor/handleNewBooking-4

Conversation

@Udit-takkar
Copy link
Copy Markdown
Contributor

@Udit-takkar Udit-takkar commented Jul 5, 2024

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Continuing refactor of handleNewBooking

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • N/A I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

@keithwillcode keithwillcode added consumer core area: core, team members only labels Jul 5, 2024
@vercel
Copy link
Copy Markdown

vercel bot commented Jul 5, 2024

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 Oct 2, 2024 1:49pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 1:49pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 1:49pm

reqBodyRescheduleUid?: string;
};

export const checkBookingAndDurationLimits = async ({
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

check both booking and duration limits.

safeStringify({
users: users.map(getPiiFreeUser),
})
await validateBookingTimeIsNotOutOfBounds<typeof eventType>(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

moved validation code

Comment on lines +172 to +178
const eventType = await getEventType({
eventTypeId: req.body.eventTypeId,
eventTypeSlug: req.body.eventTypeSlug,
});
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's just more simpler to read.

Comment on lines +39 to +48
export function getCustomInputsResponses(
reqBody: RequestBody,
eventTypeCustomInputs: getEventTypeResponse["customInputs"]
): NonNullable<CalendarEvent["customInputs"]> {
if (reqBody.customInputs && reqBody.customInputs.length > 0) {
return mapCustomInputs(reqBody.customInputs);
}

const responses = reqBody.responses || {};
return mapResponsesToCustomInputs(responses, eventTypeCustomInputs);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

refactored this function

Comment on lines +30 to +37
return Object.entries(responses).reduce((acc, [fieldName, fieldValue]) => {
const foundInput = eventTypeCustomInputs.find((input) => slugify(input.label) === fieldName);
if (foundInput) {
acc[foundInput.label] = fieldValue;
}
return acc;
}, {} as NonNullable<CalendarEvent["customInputs"]>);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

use .reduce method instead of for loop

throw new HttpError({ statusCode: 403, message: ErrorCode.CancelledBookingsCannotBeRescheduled });
}
}
let originalRescheduledBooking = originalBooking;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have used let here because it originalRescheduledBooking is re assigned

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.

It looks like this function is doing two things either getting the rescheduled booking or getting the seated booking. I think if we're going to abstract this logic it should be split into two functions that each havea single purpose.

@Udit-takkar Udit-takkar marked this pull request as ready for review July 10, 2024 20:48
@graphite-app graphite-app bot requested a review from a team July 10, 2024 20:49
@dosubot dosubot bot added bookings area: bookings, availability, timezones, double booking 💻 refactor labels Jul 10, 2024
@graphite-app
Copy link
Copy Markdown

graphite-app bot commented Jul 10, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (07/10/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (09/19/24)

1 reviewer was added to this PR based on Keith Williams's automation.

@Udit-takkar
Copy link
Copy Markdown
Contributor Author

To be merged after "booking with phone number"

joeauyeung
joeauyeung previously approved these changes Sep 25, 2024
Copy link
Copy Markdown
Contributor

@joeauyeung joeauyeung left a comment

Choose a reason for hiding this comment

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

Feedback is addressed. Great refactoring @Udit-takkar

@Udit-takkar
Copy link
Copy Markdown
Contributor Author

@joeauyeung fixed merge conflicts

Copy link
Copy Markdown
Contributor

@joeauyeung joeauyeung left a comment

Choose a reason for hiding this comment

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

Approving again after fixing merge conflicts

@Udit-takkar Udit-takkar mentioned this pull request Nov 18, 2024
3 tasks
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 consumer core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e 💻 refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants