feat: Add support to audit and view audit log with Booking CREATED action#25468
feat: Add support to audit and view audit log with Booking CREATED action#25468hariombalhara merged 8 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
keithwillcode
added
core
This stack of pull requests is managed by Graphite. Learn more about stacking. |
hariombalhara
force-pushed
the
booking-audit-viewer-demo
branch
2 times, most recently
from
06bfd36 to
2cb8518
Compare
hariombalhara
force-pushed
the
booking-audit-viewer-demo
branch
from
2cb8518 to
4f2fba2
Compare
hariombalhara
force-pushed
the
booking-audit-viewer-demo
branch
from
4f2fba2 to
27ea1e1
Compare
hariombalhara
changed the title
hariombalhara
force-pushed
the
booking-audit-viewer-demo
branch
from
27ea1e1 to
7b350fd
Compare
… features - Introduced FilterSearchField and Select components for better search and filtering options in booking logs. - Updated state management to handle multiple expanded log entries and JSON data visibility. - Refactored action icon rendering to use string identifiers for icons instead of JSX elements. - Improved layout and styling for audit log entries, including a timeline view and enhanced detail display. These changes improve the user experience by providing more intuitive filtering and a clearer presentation of booking audit logs.
- Added TaskerService to encapsulate tasker functionality and provide a consistent interface for retrieving taskers. - Updated BookingAuditProducerService to utilize TaskerService for tasker retrieval, enhancing dependency injection. - Refactored imports in regular-booking.module.ts and bookings.ts to include the new TaskerService. These changes streamline task management and improve the modularity of the booking audit system.
hariombalhara
force-pushed
the
booking-audit-viewer-demo
branch
from
5d5fc8b to
c252e53
Compare
…mproved structure - Introduced BookingLogsFilters and BookingLogsTimeline components to separate concerns and improve readability. - Updated state management for search and filter functionality, allowing for better user interaction. - Refactored the rendering of audit logs to enhance layout and detail presentation. - Added type definitions for audit logs and filters to improve type safety. These changes streamline the booking logs view, making it more modular and user-friendly.
hariombalhara
force-pushed
the
booking-audit-viewer-demo
branch
from
c252e53 to
983d7c5
Compare
volnei
left a comment
volnei
left a comment
There was a problem hiding this comment.
Let's avoid static methods for repositories 🙏🏼
joeauyeung
left a comment
joeauyeung
left a comment
There was a problem hiding this comment.
Love the architecture here. This consumer and producer pattern, is this something we want to adopt throughout the whole codebase?
All my comments are non-blocking but can be addressed in a follow up.
There was a problem hiding this comment.
Something to consider while we're working on this is to add a permission check before logging more sensitive data.
There was a problem hiding this comment.
Yeah, I have been thinking about it. So right now I haven;t implemented permission check at all.
I believe audit logs is initially for admins only and they need to see all the data.
Going forward we might need to show audit logs for regular members too, and there we might need to hide certain details.
| } | ||
|
|
||
| // First try to find by email if email exists | ||
| if (normalizedEmail) { |
There was a problem hiding this comment.
We should handle the scenario if an actor has both an email and phone values. Right now it'll make 4 possible DB calls because of the two if statements
There was a problem hiding this comment.
Let me think about how to handle it well.
| * data: { startTime, endTime, status } | ||
| * }); | ||
| */ | ||
| async queueAudit(bookingUid: string, actor: Actor, organizationId: number | null, actionData: BookingAuditTaskProducerActionData): Promise<void> { |
There was a problem hiding this comment.
Should we also check to see if the feature flag is enabled here before pushing to the tasker? Would help prevent creating unnecessary tasks
There was a problem hiding this comment.
I discussed this with Morgan. My suggestion was to have no impact on booking flow's performance at all. Checking feature flag for audit conflicts with that intention.
I was thinking that in future we anyway we would have audit logs enabled for all organizations, so it should be fine.
| }; | ||
| } | ||
|
|
||
| function buildBookingCreatedPayload({ |
There was a problem hiding this comment.
I'm hesitant about having this here in RegularBookingService it feels like it should live as a method in another service.
There was a problem hiding this comment.
So, this is building all the data that is needed to fire an event(bookingCreated). This is usually the responsibility of the calling site itself because it knows where to get the data from and then only thing left is to properly place it as required by event handler signature. Type safety ensures they are passed correctly.
| } else { | ||
| await bookingEventHandler.onBookingCreated(bookingCreatedPayload); | ||
| // TODO: We need to check session in booking flow and accordingly create USER actor if applicable. | ||
| const auditActor = makeGuestActor({ email: bookerEmail, name: fullName }); |
There was a problem hiding this comment.
Likewise should this be handled in another service? Maybe the one on the line below? I want to avoid needing people to call these specific functions in different features.
There was a problem hiding this comment.
I am not so sure about this as well. If you look at this definition, it isn't doing much at all.
My reasoning was that everywhere one would need to define these actors, and it would be a lot of places. Either we pass the as a POJO or we have a well defined utility fn to create it, so that it is difficult to make mistakes
- Renamed Task class to TaskRepository - Converted all static methods to instance methods - Added Dependencies type with prismaClient injection - Exported singleton 'Task' for backward compatibility - All existing call sites continue to work unchanged Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 53 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/app/(use-page-wrapper)/(main-nav)/booking/[uid]/logs/page.tsx">
<violation number="1" location="apps/web/app/(use-page-wrapper)/(main-nav)/booking/[uid]/logs/page.tsx:3">
P3: Typo in comment: "TOOD" should be "TODO".</violation>
</file>
<file name="packages/features/booking-audit/lib/repository/PrismaAuditActorRepository.ts">
<violation number="1" location="packages/features/booking-audit/lib/repository/PrismaAuditActorRepository.ts:65">
P1: Potential unique constraint violation when email and phone belong to different existing records. The method finds by email first and updates with phone (or vice versa), but doesn't verify if the new phone/email is already used by a different record. Consider checking for conflicts before updating, or using a transaction to merge records when both identifiers exist on separate actors.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| @@ -0,0 +1,48 @@ | |||
| // Added as a separate route for now to ease the testing of the audit logs feature | |||
| // It partially matches the figma design - https://www.figma.com/design/wleA2SR6rn60EK7ORxAfMy/Cal.com-New-Features?node-id=5641-6732&p=f | |||
| // TOOD: Move it to the booking page side bar later | |||
There was a problem hiding this comment.
P3: Typo in comment: "TOOD" should be "TODO".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/app/(use-page-wrapper)/(main-nav)/booking/[uid]/logs/page.tsx, line 3:
<comment>Typo in comment: "TOOD" should be "TODO".</comment>
<file context>
@@ -0,0 +1,48 @@
+// Added as a separate route for now to ease the testing of the audit logs feature
+// It partially matches the figma design - https://www.figma.com/design/wleA2SR6rn60EK7ORxAfMy/Cal.com-New-Features?node-id=5641-6732&p=f
+// TOOD: Move it to the booking page side bar later
+import { ShellMainAppDir } from "app/(use-page-wrapper)/(main-nav)/ShellMainAppDir";
+import type { PageProps } from "app/_types";
</file context>
| // TOOD: Move it to the booking page side bar later | |
| // TODO: Move it to the booking page side bar later |
|
|
||
| if (existingByEmail) { | ||
| // Update existing record found by email | ||
| return this.deps.prismaClient.auditActor.update({ |
There was a problem hiding this comment.
P1: Potential unique constraint violation when email and phone belong to different existing records. The method finds by email first and updates with phone (or vice versa), but doesn't verify if the new phone/email is already used by a different record. Consider checking for conflicts before updating, or using a transaction to merge records when both identifiers exist on separate actors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/booking-audit/lib/repository/PrismaAuditActorRepository.ts, line 65:
<comment>Potential unique constraint violation when email and phone belong to different existing records. The method finds by email first and updates with phone (or vice versa), but doesn't verify if the new phone/email is already used by a different record. Consider checking for conflicts before updating, or using a transaction to merge records when both identifiers exist on separate actors.</comment>
<file context>
@@ -25,5 +25,87 @@ export class PrismaAuditActorRepository implements IAuditActorRepository {
+
+ if (existingByEmail) {
+ // Update existing record found by email
+ return this.deps.prismaClient.auditActor.update({
+ where: { email: normalizedEmail },
+ data: {
</file context>
ThyMinimalDev
left a comment
ThyMinimalDev
left a comment
There was a problem hiding this comment.
API v2 side looks good
6 participants
What does this PR do?
This PR introduces the booking audit infrastructure with a viewer service to fetch, enrich, and format audit logs. This is the first iteration that includes only the CREATED action with versioned schema
Note: Additional audit actions, UI improvements to match with Figma will be taken care of in followup PRs, to ensure the PR size remains as small as possible(as it is large enough already)
Demo - Loom - https://www.loom.com/share/22c2f38b052b480b8be3716e1608eaf6
Key Changes
New Booking Audit Package (
packages/features/booking-audit/):BookingAuditViewerService- Fetches, enriches, and formats audit logs for displayBookingAuditTaskConsumer- Processes audit tasks asynchronously via TaskerCreatedAuditActionService(v1) - Handles RECORD_CREATED action with versioned schemaRepository Layer:
BookingAuditRepository- CRUD operations for audit recordsAuditActorRepository- Actor management (users, guests, system)UserRepository.findByUuid()- User lookup for actor enrichmentUI Components:
/booking/logs/[bookinguid]for viewing audit historyInfrastructure:
bookingAuditTasker task type for async processingbooking-auditfeature flag (disabled by default)viewer.bookings.getAuditLogsUpdates since last revision
Taskclass toTaskRepository: Converted all static methods to instance methods following the repository pattern. A singletonTaskis exported for backward compatibility, so existing call sites continue to work unchanged.Important Notes for Reviewers
checkPermissions()inBookingAuditViewerServiceis not yet implementedbooking-auditflag, disabled by defaultbooking-logs-view.tsxmay need translationHuman Review Checklist
BookingAuditTaskConsumeris appropriate for retry scenariosTaskRepositoryrefactoring maintains backward compatibility (singleton exportTaskshould work for all existing call sites)Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Run the integration tests:
To test the UI:
booking-auditfeature flag in the database/booking/logs/{bookingUid}to view the audit trailChecklist