-
Notifications
You must be signed in to change notification settings - Fork 11.7k
feat: extract core booking audit infrastructure from PR 25125 #25729
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
This PR contains only the core booking audit infrastructure changes from PR 25125, excluding integration changes with booking flows. Included: - All packages/features/booking-audit/* (core audit services, actions, repository) - packages/features/di/containers/BookingAuditViewerService.container.ts - packages/features/tasker/tasker.ts (audit task types) - packages/features/bookings/lib/types/actor.ts (actor types for audit) - packages/features/bookings/repositories/BookingRepository.ts (getFromRescheduleUid method) - apps/web/modules/booking/logs/views/booking-logs-view.tsx (UI for viewing audit logs) - apps/web/public/static/locales/en/common.json (translations) Excluded (integration changes): - packages/trpc/server/* (tRPC handlers) - packages/features/ee/round-robin/* (round-robin integration) - packages/features/bookings/lib/handleCancelBooking.ts - packages/features/bookings/lib/handleConfirmation.ts - packages/features/bookings/lib/onBookingEvents/BookingEventHandlerService.ts - packages/features/bookings/lib/service/RegularBookingService.ts - apps/api/v2/* (API v2 integration) Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| * Internal helper to queue audit task to Tasker | ||
| * @param params.action - Must be a valid BookingAuditAction value (TYPE from action services are string-typed) | ||
| */ | ||
| async queueAudit(bookingUid: string, actor: Actor, organizationId: number | null, actionData: BookingAuditTaskProducerActionData): Promise<void> { |
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.
Broke it down into specific fns so that actionData doesn't become heavy on TS with union of all action types
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.
Booking History UI improvement.
- JSON styling
- Reschedule Title with dynamic values and link support
- Other UI changes
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.
1 issue found across 32 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/booking-audit/lib/service/BookingAuditTaskConsumer.ts">
<violation number="1" location="packages/features/booking-audit/lib/service/BookingAuditTaskConsumer.ts:226">
P2: Rule violated: **Avoid Logging Sensitive Information**
The exhaustive check error could expose PII (email, name, phone) from the `actor` object if this branch is ever reached at runtime. Consider logging only the `identifiedBy` field to avoid potentially exposing sensitive guest information:
```typescript
throw new Error(`Unhandled actor type: ${(actor as { identifiedBy: string }).identifiedBy}`);
```</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
packages/features/booking-audit/lib/service/BookingAuditTaskConsumer.ts
Outdated
Show resolved
Hide resolved
- Add queueAudit method back to BookingAuditProducerService interface for backwards compatibility - Implement queueAudit method in BookingAuditTaskerProducerService - Make userTimeZone parameter optional in BookingAuditViewerService - Add BookingAuditTaskProducerActionData type for legacy queueAudit method - Use any generics in BookingAuditActionServiceRegistry (matching PR 25125) - Fix type assertions in BookingAuditTaskConsumer Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
packages/features/booking-audit/lib/service/BookingAuditViewerService.ts
Outdated
Show resolved
Hide resolved
7e9bfb8 to
44f973c
Compare
44f973c to
b61467b
Compare
…ments - Added ISimpleLogger dependency to BookingAuditViewerService for better error handling. - Updated actor type in enriched audit logs to use AuditActorType for improved type safety. - Replaced console.error with logger for error reporting when no rescheduled log is found.
b61467b to
079fb78
Compare
|
|
||
| constructor(private deps: BookingAuditActionServiceRegistryDeps) { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const services: Array<[BookingAuditAction, IAuditActionService<any, any>]> = [ |
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.
Have improved types in followup #25125
| const getActorRoleLabel = (actorType: AuditActorType): string | null => { | ||
| switch (actorType) { | ||
| case "GUEST": | ||
| return "Guest"; | ||
| case "ATTENDEE": | ||
| return "Attendee"; | ||
| case "SYSTEM": | ||
| return null; | ||
| case "USER": | ||
| return null; | ||
| default: | ||
| return null; | ||
| } | ||
| }; |
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.
Nit: Just a code style matter, but I always prefer a object/map instead of switches.
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.
Oh you are right, it certainly doesn't need switch. I will fix it in a followup PR
What does this PR do?
Extracts the core booking audit infrastructure from PR #25125 into a separate branch, excluding integration changes with booking flows. This allows the foundational audit system to be reviewed and merged independently.
Included:
packages/features/booking-audit/*(action services, repository, task consumer, viewer service)packages/features/di/containers/BookingAuditViewerService.container.ts(DI container)packages/features/tasker/tasker.ts(audit task type updates)packages/features/bookings/lib/types/actor.ts(actor types +makeAttendeeActor)packages/features/bookings/repositories/BookingRepository.ts(getFromRescheduleUidmethod)apps/web/modules/booking/logs/views/booking-logs-view.tsx(UI for viewing audit logs)common.jsonExcluded (integration changes):
packages/trpc/server/*(tRPC handlers)packages/features/ee/round-robin/*(round-robin integration)packages/features/bookings/lib/handleCancelBooking.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/onBookingEvents/BookingEventHandlerService.tspackages/features/bookings/lib/service/RegularBookingService.tsapps/api/v2/*(API v2 integration)Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
This PR contains infrastructure only - the audit system won't be triggered until integration changes from PR #25125 are merged separately. To verify:
yarn type-check:ci --force/booking/logs/[uid]should render (though no logs will appear without integration)Checklist for Human Review
BookingAuditTaskBaseSchemawithdata: z.unknown())BookingAuditViewerService.checkPermissions()- throws error in production (TODO comment)BookingAuditTaskConsumer.tsLink to Devin run: https://app.devin.ai/sessions/349411f11d51483494218c992d2b9220
Requested by: hariom@cal.com (@hariombalhara)