-
Notifications
You must be signed in to change notification settings - Fork 11.6k
refactor: handleNewBooking #4 #15673
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
adf661c to
00b43a7
Compare
| reqBodyRescheduleUid?: string; | ||
| }; | ||
|
|
||
| export const checkBookingAndDurationLimits = async ({ |
There was a problem hiding this comment.
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>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved validation code
| const eventType = await getEventType({ | ||
| eventTypeId: req.body.eventTypeId, | ||
| eventTypeSlug: req.body.eventTypeSlug, | ||
| }); |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored this function
| 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"]>); | ||
| } |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. |
|
To be merged after "booking with phone number" |
8be4088 to
074b0b2
Compare
|
@joeauyeung fixed merge conflicts |
joeauyeung
left a comment
There was a problem hiding this 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
What does this PR do?
Continuing refactor of handleNewBooking
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?