Skip to content

Comments

chore: [Booking Audit Stack -2] Add dependency injection for BookingAudit system and associated services#25123

Merged
hariombalhara merged 2 commits intomainfrom
booking-audit-di
Nov 28, 2025
Merged

chore: [Booking Audit Stack -2] Add dependency injection for BookingAudit system and associated services#25123
hariombalhara merged 2 commits intomainfrom
booking-audit-di

Conversation

@hariombalhara
Copy link
Member

@hariombalhara hariombalhara commented Nov 13, 2025

What does this PR do?

This PR establishes the Dependency Injection (DI) infrastructure for the BookingAudit system. It creates the necessary DI modules, services, and repositories to support audit logging of booking events, but does not yet integrate them into the actual booking flow (that will come in a follow-up PR).

Key changes:

  1. BookingAudit DI modules - Created DI modules for BookingAuditService, BookingAuditRepository, and ActorRepository with proper token-based dependency resolution
  2. BookingEventHandler DI - Created DI module for BookingEventHandlerService and wired it into RegularBookingService
  3. Logger refactor - Migrated logger service from factory-based to class-based tslog implementation for consistent DI injection
  4. Type improvements - Strengthened types in IBookingAuditRepository (using JsonValue instead of unknown, proper action typing)
  5. Action type consolidation - Simplified BookingAuditAction types by removing redundant reason-update actions and consolidating to REASSIGNMENT

Important notes:

  • BookingAuditService is wired into BookingEventHandlerService but marked as optional - actual audit recording will be implemented in a follow-up PR
  • The logger service was completely rewritten - existing consumers (watchlist, webhooks) were updated but reviewers should verify no other consumers are broken

Visual Demo

N/A - This is infrastructure/refactoring work with 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, internal refactoring only
  • I confirm automated tests are in place that prove my fix is effective or that my feature works - No tests added yet, this is foundation work

How should this be tested?

Environment setup:

  • Standard Cal.com development environment
  • No special environment variables needed

Testing approach:

  1. Verify the application builds successfully: yarn build
  2. Verify type checking passes: yarn type-check:ci --force
  3. Run existing tests to ensure no regressions: yarn test
  4. Start the dev server and verify basic booking flows still work: yarn dev

Expected behavior:

  • All existing functionality should work unchanged
  • No audit records will be created yet (that's expected - integration comes in follow-up PR)
  • Logger should work in both development (pretty format) and production (JSON format)

Human Review Checklist

Critical items to review:

  1. Logger service refactor - The logger was completely rewritten from a factory-based approach to a tslog class. Verify:

    • The new LoggerService class provides the same interface as the old logger
    • All consumers (watchlist, webhooks, and any others) are properly updated
    • The production/development formatting logic is correct
  2. BookingAuditAction type changes - Several action types were removed:

    • CANCELLATION_REASON_UPDATED
    • REJECTION_REASON_UPDATED
    • ASSIGNMENT_REASON_UPDATED
    • REASSIGNMENT_REASON_UPDATED

    These were consolidated into just REASSIGNMENT. Verify these removed types aren't referenced anywhere in the codebase.

  3. PrismaBookingAuditRepository null handling - The create() method has this line:

    data: bookingAudit.data === null ? undefined : bookingAudit.data,

    Why would data be null? Is this correct or should it throw an error instead?

  4. DI module loading order - Verify the dependency graph is correct:

    • BookingAuditService depends on BookingAuditRepository and ActorRepository
    • BookingEventHandlerService depends on BookingAuditService, HashedLinkService, and Logger
    • RegularBookingService depends on BookingEventHandlerService
  5. Optional dependency - BookingAuditService is marked as optional in BookingEventHandlerService. Confirm this is intentional for this PR and will be made required in the follow-up.


Link to Devin run: https://app.devin.ai/sessions/e771b4569f224cc199bd1cb11aa5e50d

Requested by: hariom@cal.com (@hariombalhara)

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
  • I have checked if my changes generate no new warnings

@vercel
Copy link

vercel bot commented Nov 13, 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 Nov 28, 2025 8:44am
cal-eu Ignored Ignored Nov 28, 2025 8:44am

@github-actions
Copy link
Contributor

github-actions bot commented Nov 13, 2025

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Add DI for BookingAudit". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

Copy link
Member Author

hariombalhara commented Nov 13, 2025

@devin-ai-integration devin-ai-integration bot changed the title Add DI for BookingAudit refactor: add dependency injection for BookingAudit system Nov 13, 2025
@hariombalhara hariombalhara changed the title refactor: add dependency injection for BookingAudit system chore: Add dependency injection for BookingAudit system Nov 13, 2025
@hariombalhara hariombalhara changed the title chore: Add dependency injection for BookingAudit system chore: [Booking Audit Stack -2] Add dependency injection for BookingAudit system [Ready For Review] Nov 13, 2025
@hariombalhara hariombalhara force-pushed the booking-audit-foundation branch from a2dddaf to a2b4be1 Compare November 13, 2025 13:46
@hariombalhara hariombalhara force-pushed the booking-audit-foundation branch from a2b4be1 to 476b5e8 Compare November 14, 2025 12:28
@hariombalhara hariombalhara force-pushed the booking-audit-foundation branch from 476b5e8 to 25820e9 Compare November 14, 2025 12:39
@hariombalhara hariombalhara force-pushed the booking-audit-foundation branch from 25820e9 to d475433 Compare November 15, 2025 04:41
@hariombalhara hariombalhara force-pushed the booking-audit-di branch 2 times, most recently from 05ed8eb to 7e73880 Compare November 19, 2025 13:02
@hariombalhara hariombalhara requested a review from a team November 19, 2025 13:02
@hariombalhara hariombalhara force-pushed the booking-audit-di branch 2 times, most recently from 330d235 to 4df835a Compare November 19, 2025 13:15
@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2025

E2E results are ready!

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.

1 issue found across 26 files

Prompt for AI agents (all 1 issues)

Understand the root cause of the following 1 issues and fix them.


<file name="packages/features/booking-audit/lib/service/BookingAuditService.ts">

<violation number="1" location="packages/features/booking-audit/lib/service/BookingAuditService.ts:50">
Rule violated: **Avoid Logging Sensitive Information**

The new audit log statement emits `bookingUid` and `actorId`, leaking stable booking and user identifiers into application logs, which violates the &quot;Avoid Logging Sensitive Information&quot; rule. Remove these identifiers or replace them with non-sensitive aggregates before logging.</violation>
</file>

Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR

@hariombalhara hariombalhara force-pushed the booking-audit-di branch 2 times, most recently from 5733315 to 2bb803a Compare November 19, 2025 15:08
{
provide: Logger,
useFactory: () => {
return new Logger();
Copy link
Member Author

Choose a reason for hiding this comment

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

Need this because Logger has an optional dependency in constructor that isn't injected. Following convention similar to used in prisma.module.ts to solve this.

Udit-takkar
Udit-takkar previously approved these changes Nov 21, 2025
Copy link
Contributor

@Udit-takkar Udit-takkar left a comment

Choose a reason for hiding this comment

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

LGTM.

Just file name change is required #25123 which was created in earlier PR and can be done in a follow up PR

@Udit-takkar
Copy link
Contributor

@hariombalhara can you fix the conflicts?

@hariombalhara hariombalhara merged commit cbc8d3d into main Nov 28, 2025
37 checks passed
@hariombalhara hariombalhara deleted the booking-audit-di branch November 28, 2025 09:14
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/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants