Skip to content

Comments

fix: use injected bookingEventHandler for proper DI#25273

Merged
hariombalhara merged 1 commit intobooking-audit-difrom
devin/fix-booking-audit-ci-1763560747
Nov 19, 2025
Merged

fix: use injected bookingEventHandler for proper DI#25273
hariombalhara merged 1 commit intobooking-audit-difrom
devin/fix-booking-audit-ci-1763560747

Conversation

@hariombalhara
Copy link
Member

What does this PR do?

This PR fixes CI test failures in the booking-audit-di branch by using the injected bookingEventHandler dependency instead of creating a new instance locally in RegularBookingService.

Context: This is a follow-up fix for PR #25123 which added DI infrastructure for the BookingAudit system. CI was failing with "Unhandled Errors" about emails not being sent in reschedule.test.ts and roundRobinManualReassignment.test.ts, even though tests passed locally.

Root cause: The code was creating a new BookingEventHandlerService instance instead of using the injected dependency from deps.bookingEventHandler. This prevented tests from properly stubbing/observing the handler, causing CI failures.

The fix: Changed line 2261 in RegularBookingService.ts to use deps.bookingEventHandler directly, aligning with the DI pattern introduced in the parent PR.

Visual Demo

N/A - This is an internal refactoring to fix test failures. No user-facing changes.

Mandatory Tasks

  • I have self-reviewed the code
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. N/A - No documentation changes needed for this internal refactoring.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works. Existing unit tests pass locally (3921 tests)

How should this be tested?

Prerequisites:

  • This PR targets the booking-audit-di branch, not main
  • No special environment variables needed

Testing steps:

  1. Checkout this branch
  2. Run TZ=UTC yarn test to verify all unit tests pass
  3. Wait for CI to complete and verify that the previously failing tests now pass:
    • Tests / Unit (reschedule.test.ts)
    • Tests / Unit (roundRobinManualReassignment.test.ts)

Expected behavior:

  • All unit tests should pass both locally and in CI
  • No "Unhandled Errors" about emails not being sent

Checklist

  • I have read the contributing guide
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas N/A - Simple one-line change
  • I have checked if my changes generate no new warnings

Important Notes for Reviewers

⚠️ Critical items to verify:

  1. DI wiring completeness: Please verify that all call sites instantiating RegularBookingService provide the bookingEventHandler dependency. Check:

    • packages/features/bookings/di/RegularBookingService.module.ts
    • Any other places where RegularBookingService is constructed
  2. CI validation: This fix is based on analysis of CI failures that didn't reproduce locally. The actual validation will happen when CI runs on this PR.

  3. Base branch: This PR targets booking-audit-di, not main. It should be merged to booking-audit-di first, then that branch can be merged to main.


Devin session: https://app.devin.ai/sessions/343c8e5dcda94d85b77a3eb65a30a7c6
Requested by: @hariombalhara (hariom@cal.com)

Use the injected bookingEventHandler from dependencies instead of creating a new instance locally. This enables proper dependency injection and allows tests to stub/observe the handler, which should fix CI test failures related to email notifications in reschedule and round-robin reassignment tests.

Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@keithwillcode keithwillcode added core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO labels Nov 19, 2025
@hariombalhara hariombalhara marked this pull request as ready for review November 19, 2025 14:41
@hariombalhara hariombalhara requested a review from a team as a code owner November 19, 2025 14:41
@graphite-app graphite-app bot requested a review from a team November 19, 2025 14:41
@hariombalhara hariombalhara merged commit d9c8558 into booking-audit-di Nov 19, 2025
13 of 14 checks passed
@hariombalhara hariombalhara deleted the devin/fix-booking-audit-ci-1763560747 branch November 19, 2025 14:42
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

hariombalhara added a commit that referenced this pull request Nov 19, 2025
Use the injected bookingEventHandler from dependencies instead of creating a new instance locally. This enables proper dependency injection and allows tests to stub/observe the handler, which should fix CI test failures related to email notifications in reschedule and round-robin reassignment tests.

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@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 size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants