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

chore: Create BookingCreatedService CAL-[5026] #18516

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

Conversation

joeauyeung
Copy link
Contributor

@joeauyeung joeauyeung commented Jan 8, 2025

What does this PR do?

  • Aims to abstract post booking created code from handleNewBooking into a new class BookingListener

  • This PR starts with creating a .create() method on the BookingListener

  • The follow post booking actions are added to the method

    • Video link creation
    • Calendar event creation
    • Writing to CRM
    • Sending/scheduling webhooks
    • Sending/scheduling workflows
  • Fixes #XXXX (GitHub issue number)

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

Mandatory Tasks (DO NOT REMOVE)

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

How should this be tested?

  • Create a booking that does the following
    • Has a dynamic video link
    • Connected to a calendar
    • Sends data to a CRM
    • Sends a webhook
    • Sends a workflow

referencesToCreate = createManager.referencesToCreate;
videoCallUrl = evt.videoCallData && evt.videoCallData.url ? evt.videoCallData.url : null;
if (!isDryRun) {
await BookingListener.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

BookingListener.create() reads weird. If it’s a listener, why is it creating anything?

Seems like we need to switch to the pub/sub naming convention.

BookingPublisher/BookingSubscriber

Copy link
Contributor

Choose a reason for hiding this comment

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

Or BookingService

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the confusion. .create() refers to when a booking is created. I'm hoping to add BookingListener.reschedule(), BookingListener.cancel().

I was debating on BookingListener.bookingCreated(), would that be clearer?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. It's the fact that it's called BookingListener. Typically classes with name Listener don't have functions like .create() or .reschedule(). They are typically just .listen()

Copy link
Contributor

Choose a reason for hiding this comment

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

If you plan to add those functions, BookingService operating as an application service makes a lot more sense.

Copy link
Contributor

@keithwillcode keithwillcode Jan 8, 2025

Choose a reason for hiding this comment

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

In fact, in many big systems like ours with complex code, a service per operation is sometimes preferred. e.g. BookingCreationService, BookingRescheduleService, etc.

results = createManager.results;
referencesToCreate = createManager.referencesToCreate;
videoCallUrl = evt.videoCallData && evt.videoCallData.url ? evt.videoCallData.url : null;
if (!isDryRun) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A benefit is we don't have to wrap (!isDryRun) around multiple functions.

);
}
}
return {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once the post booking actions are taken we can return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note some post booking functions we need to keep in handleNewBooking as they're being used for rescheduled bookings, payments, and setting up bookings that require confirmation.

};

loggerWithEventDetails.error(
`EventManager.create failure in some of the integrations ${organizerUser.username}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the following removed code can be moved to the BookingListener

@@ -1832,67 +1710,8 @@ async function handler(

loggerWithEventDetails.debug(`Booking ${organizerUser.username} completed`);

// We are here so, booking doesn't require payment and booking is also created in DB already, through createBooking call
if (isConfirmedByDefault) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These calls are moved to the booking listener

Comment on lines +57 to +58
noEmail?: NoEmail;
reqAppsStatus?: ReqAppsStatus;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These properties only exist in handleNewBooking although we'll need to access these in the tasker.

return calculateAggregatedAppsStatus(reqAppsStatus, resultStatus);
}

return resultStatus;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In updateBookingWithStatus the resultStatus is returned

@@ -108,6 +110,13 @@ const handleSeats = async (newSeatedBookingObject: NewSeatedBookingObject) => {
...reqBodyMetadata,
};
try {
const workflows = await getAllWorkflowsFromEventType(
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 getAllWorkflowsFromEventType from being called in handleNewBooking to either being called in handleSeats or BookingListener.create()

referencesToCreate = createManager.referencesToCreate;
videoCallUrl = evt.videoCallData && evt.videoCallData.url ? evt.videoCallData.url : null;
if (!isDryRun) {
await BookingListener.create({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the confusion. .create() refers to when a booking is created. I'm hoping to add BookingListener.reschedule(), BookingListener.cancel().

I was debating on BookingListener.bookingCreated(), would that be clearer?

Copy link

vercel bot commented Jan 8, 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 13, 2025 3:55am
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Jan 13, 2025 3:55am

@joeauyeung joeauyeung changed the title chore: Create BookingListener and .create method chore: Create BookingCreatedService Jan 9, 2025
@joeauyeung joeauyeung changed the title chore: Create BookingCreatedService chore: Create BookingCreatedService CAL-[5026] Jan 15, 2025
@joeauyeung joeauyeung linked an issue Jan 15, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consumer core area: core, team members only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-5026] Create BookingCreatedService
2 participants