Skip to content

Comments

chore: [Booking Flow Refactor - 5] Move post booking things to separate service BookingEventHandlerService starting with HashedLink usage handling#24025

Merged
hariombalhara merged 7 commits intomainfrom
hariom-pri-305-post-booking-handling
Oct 23, 2025
Merged

chore: [Booking Flow Refactor - 5] Move post booking things to separate service BookingEventHandlerService starting with HashedLink usage handling#24025
hariombalhara merged 7 commits intomainfrom
hariom-pri-305-post-booking-handling

Conversation

@hariombalhara
Copy link
Member

@hariombalhara hariombalhara commented Sep 24, 2025

What does this PR do?

This PR refactors post-booking logic by extracting it from the main booking flow into a dedicated BookingEventHandler service. This is the first step in improving the post booking architecture by separating concerns and making the codebase more maintainable.

Key Changes:

  • Extracts hashed link usage tracking from inline code into a new BookingEventHandler service
  • Introduces type-safe event payloads for booking creation and rescheduling events
  • Uses dependency injection everywhere
  • Adds comprehensive test coverage for post-booking event handling to ensure no regression

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?

Environment Setup

  • Standard Cal.com development environment
  • PostgreSQL database for hashed link functionality

Test Scenarios

  1. Hashed Link Creation: Create a booking with a hashed link and verify usage count increments
  2. Hashed Link Rescheduling: Reschedule a booking with a hashed link and verify usage tracking
  3. Dry Run Handling: Ensure dry runs don't increment usage counts
  4. Error Handling: Verify that hashed link service errors don't break booking creation

Expected Behavior

  • All existing booking functionality should work unchanged
  • Hashed link usage tracking should continue to work exactly as before
  • Post-booking errors should be logged but not break the booking flow
  • Tests in packages/features/bookings/lib/handleNewBooking/test/post-booking-handling.test.ts should pass

Checklist

⚠️ Critical Review Areas:

  • Verify BookingEventHandler Integration: Ensure the BookingEventHandler is properly instantiated and called in the booking flow (around line 2063 in RegularBookingService.ts)
  • Error Handling Strategy: Confirm that changing from throwing HttpError to logging errors is the intended behavior for post-booking failures
  • Dependency Injection Configuration: Validate that all DI modules are properly configured and dependencies are correctly wired
  • Service Location Changes: Check that moving HashedLinkService doesn't break any existing imports or functionality
  • Test Coverage vs Implementation: The tests simulate hashed link increments rather than fully mocking the service - verify this doesn't hide integration issues

Architectural Changes:

  • New BookingEventHandler class properly handles both creation and rescheduling events
  • Type definitions in onBookingEvents/types.d.ts are comprehensive and accurate
  • Dependency injection setup follows Cal.com patterns and conventions
  • Error boundaries prevent post-booking failures from affecting booking creation

Backward Compatibility:

  • All existing booking flows continue to work unchanged
  • Hashed link functionality maintains exact same behavior
  • No breaking changes to public APIs or interfaces

Link to Devin run: https://app.devin.ai/sessions/4465fc388ef1433d80ca534e6c4ec2dd
PR Description Requested by: @hariombalhara

Note: This refactoring provides a foundation for incrementally moving additional post-booking functionality (webhooks, notifications, analytics) into the event handler pattern in future PRs.

@linear
Copy link

linear bot commented Sep 24, 2025

PRI-305

@vercel
Copy link

vercel bot commented Sep 24, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
cal Ignored Ignored Oct 23, 2025 10:37am
cal-eu Ignored Ignored Oct 23, 2025 10:37am

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch hariom-pri-305-post-booking-handling

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@hariombalhara hariombalhara force-pushed the hariom/pri-305-bookingcreateservice-rename-handlers-to-services branch from de2c60a to 34dfdce Compare September 24, 2025 07:49
@hariombalhara hariombalhara force-pushed the hariom-pri-305-post-booking-handling branch from 6e09e8f to 62963b2 Compare September 24, 2025 07:49
@hariombalhara hariombalhara changed the title chore: Move post booking stuff to separate service chore: Move post booking things to separate service BookingEventHandler Sep 24, 2025
Comment on lines -2294 to -2299
if (error instanceof Error) {
throw new HttpError({ statusCode: 410, message: error.message });
}

// For unexpected errors, provide a generic message
throw new HttpError({ statusCode: 500, message: "Failed to process booking link" });
Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally removed these errors because Booking is actually successful and this is a Post Booking thing, which we should track for errors but booker shouldn't see any booking failure due to it.

private readonly hashedLinkRepository: HashedLinkRepository;
private readonly membershipService: MembershipService;

constructor(deps: HashedLinkServiceDeps) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to deps because we have a convention to use deps when there are more than one argument to constructor. This is required by bindModuleToClassOnToken implementation.

@hariombalhara hariombalhara force-pushed the hariom-pri-305-post-booking-handling branch 2 times, most recently from 148bab2 to 4cfb182 Compare September 24, 2025 08:21
@hariombalhara hariombalhara changed the title chore: Move post booking things to separate service BookingEventHandler chore: Move post booking things to separate service BookingEventHandlerService Sep 24, 2025
@hariombalhara hariombalhara force-pushed the hariom-pri-305-post-booking-handling branch from 4cfb182 to a01b098 Compare September 24, 2025 08:25
@hariombalhara hariombalhara changed the base branch from hariom/pri-305-bookingcreateservice-rename-handlers-to-services to graphite-base/24025 September 24, 2025 10:33
@hariombalhara hariombalhara force-pushed the hariom-pri-305-post-booking-handling branch from a01b098 to 1326af2 Compare September 24, 2025 11:45
@hariombalhara hariombalhara changed the base branch from graphite-base/24025 to hariom/pri-305-bookingcreateservice-rename-handlers-to-services September 24, 2025 11:45
volnei
volnei previously requested changes Sep 24, 2025
Copy link
Contributor

@volnei volnei left a comment

Choose a reason for hiding this comment

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

I see we're introducing a new kind of DI which is good but thinking forward I prefer to not go this way right now to maturate a better structure (and place) for DI since lib folder I don't think is the right place.

@hariombalhara
Copy link
Member Author

DI wise I am following the existing conventions, nothing new that I can see.

But yeah if you are talking about moving DI containers and modules to packages/features that we agreed on today, then yes that needs to be done.

@hariombalhara hariombalhara force-pushed the hariom/pri-305-bookingcreateservice-rename-handlers-to-services branch from 748408d to 3cbf712 Compare September 26, 2025 08:49
hariombalhara added a commit that referenced this pull request Oct 22, 2025
- Refactor updatePrivateLinkUsage to only accept hashedLink parameter instead of full payload
- Remove shouldProcess function and inline isDryRun check for better clarity
- Addresses feedback from PR #24025

Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
@hariombalhara hariombalhara force-pushed the hariom-pri-305-post-booking-handling branch from c3f6beb to ceb8311 Compare October 22, 2025 13:49
@hariombalhara hariombalhara requested a review from a team as a code owner October 23, 2025 04:20
@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2025

E2E results are ready!

- Add HashedLinkService to platform-libraries/private-links.ts
- Update API V2 to import from platform library instead of direct path
- Fixes MODULE_NOT_FOUND error in API V2 build

Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
Copy link
Member

@alishaz-polymath alishaz-polymath left a comment

Choose a reason for hiding this comment

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

LGTM 👏

@ThyMinimalDev
Copy link
Contributor

@volnei PR looks good to me

@hariombalhara hariombalhara enabled auto-merge (squash) October 23, 2025 12:04
@hariombalhara hariombalhara merged commit 82951cd into main Oct 23, 2025
39 checks passed
@hariombalhara hariombalhara deleted the hariom-pri-305-post-booking-handling branch October 23, 2025 12:05
KartikLabhshetwar pushed a commit to KartikLabhshetwar/cal.com that referenced this pull request Oct 25, 2025
…te service `BookingEventHandlerService` starting with HashedLink usage handling (calcom#24025)

* chore: Move post booking stuff to separate service

* refactor: improve ISP compliance in BookingEventHandlerService

- Refactor updatePrivateLinkUsage to only accept hashedLink parameter instead of full payload
- Remove shouldProcess function and inline isDryRun check for better clarity
- Addresses feedback from PR calcom#24025

Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>

* define only what is used

* Define hashed-link-service as well in api-v2

* fix: export HashedLinkService through platform-libraries

- Add HashedLinkService to platform-libraries/private-links.ts
- Update API V2 to import from platform library instead of direct path
- Fixes MODULE_NOT_FOUND error in API V2 build

Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Morgan <33722304+ThyMinimalDev@users.noreply.github.com>
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 enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants