Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
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. Comment |
keithwillcode
added
core
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
| export type BookingAuditData = { | ||
| version?: number; | ||
| actor?: { | ||
| type: "User" | "System" | "Attendee"; | ||
| }; | ||
| booking?: { | ||
| meetingTime?: string; | ||
| totalReschedules?: number; | ||
| attendeeCountChange?: number; | ||
| cancellationReason?: string; | ||
| rejectionReason?: string; | ||
| assignmentReason?: string; | ||
| reassignmentReason?: string; | ||
| }; | ||
| attendee?: { | ||
| id?: string; | ||
| }; | ||
| meeting?: { | ||
| provider?: string; | ||
| meetingId?: string; | ||
| meetingUrl?: string; | ||
| }; | ||
| location?: { | ||
| type?: string; | ||
| address?: string; | ||
| details?: Record<string, unknown>; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
@emrysal Does this make sense as the structure? Anything I'm missing out on, or extra/unnecessary? Did you have the same vision about the versioning?
| @@ -0,0 +1,8 @@ | |||
| import type { BookingAudit, Prisma } from "@prisma/client"; | |||
|
|
|||
| export interface IBookingAuditRepository { | |||
There was a problem hiding this comment.
The repository interface should be lightly decoupled from Prisma - use a DTO
e.g.
import type { BookingAuditCreateInput, BookingAudit } from "./dto/bookingAudit";
type BookingAuditCreateInput = Prisma.BookingAuditCreateInput;
...| }) | ||
| .optional(); | ||
|
|
||
| export type BookingAuditData = z.infer<typeof BookingAuditDataSchema>; |
There was a problem hiding this comment.
Try not to use z.infer if possible
There was a problem hiding this comment.
z.infer seems like the idiomatic way to infer types from the zod schema. Is there a reason why we should avoid it? 👀
There was a problem hiding this comment.
It's heavy on TS, but it is idiomatic.
There was a problem hiding this comment.
What would be the recommended alternative?
| data?: BookingAuditData | null; | ||
| }; | ||
|
|
||
| const CURRENT_AUDIT_DATA_VERSION = 1; |
There was a problem hiding this comment.
Versioning should be done on the action, not on the entire table.
There was a problem hiding this comment.
Could you expand on this? I'm not sure I follow 🤔
Currently we version the data's structure to then be able to parse correctly in the future when any further changes comes in. Was that not the purpose/intention?
There was a problem hiding this comment.
yes, but this is per payload type, not the entire table 👍
|
This PR is being marked as stale due to inactivity. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 5 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="packages/prisma/schema.prisma">
<violation number="1" location="packages/prisma/schema.prisma:2651">
`bookingId` is declared as `String`, but `Booking.id` is an `Int`. This mismatch will block a proper relation and require incorrect casting. Please align the field type with the booking primary key (and add the relation).</violation>
<violation number="2" location="packages/prisma/schema.prisma:2652">
`userId` is typed as `String?`, but `User.id` is an `Int`. This blocks FK enforcement and will cause type mismatches when writing audits. Please switch to the correct integer type (and wire up the relation).</violation>
<violation number="3" location="packages/prisma/schema.prisma:2655">
Rule violated: **Prevent Direct NOW() Usage in Database Queries**
`@default(now())` relies on the database server timezone, which violates the Prevent Direct NOW() Usage rule and risks non-UTC audit timestamps. Please use an explicit UTC expression instead so future server timezone changes cannot skew audit history.</violation>
<violation number="4" location="packages/prisma/schema.prisma:2656">
`BookingAudit` is missing its closing brace. Without it the Prisma schema will not compile.</violation>
</file>
<file name="packages/lib/server/service/bookingAuditService.ts">
<violation number="1" location="packages/lib/server/service/bookingAuditService.ts:97">
The host no-show helper requires a userId even though the actor is recorded as an attendee; make the userId optional so attendee-triggered updates can be logged without fabricating a user record.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| userId String? | ||
| type BookingAuditType | ||
| action BookingAuditAction? | ||
| timestamp DateTime @default(now()) |
There was a problem hiding this comment.
Rule violated: Prevent Direct NOW() Usage in Database Queries
@default(now()) relies on the database server timezone, which violates the Prevent Direct NOW() Usage rule and risks non-UTC audit timestamps. Please use an explicit UTC expression instead so future server timezone changes cannot skew audit history.
Prompt for AI agents
Address the following comment on packages/prisma/schema.prisma at line 2655:
<comment>`@default(now())` relies on the database server timezone, which violates the Prevent Direct NOW() Usage rule and risks non-UTC audit timestamps. Please use an explicit UTC expression instead so future server timezone changes cannot skew audit history.</comment>
<file context>
@@ -2609,6 +2609,52 @@ model RolePermission {
+ userId String?
+ type BookingAuditType
+ action BookingAuditAction?
+ timestamp DateTime @default(now())
+ data Json?
+
</file context>
| timestamp DateTime @default(now()) | |
| timestamp DateTime @default(dbgenerated("CURRENT_TIMESTAMP AT TIME ZONE 'UTC'")) |
| model BookingAudit { | ||
| id String @id @default(uuid()) //TODO: Use uuid7() for time-sortable ID once Prisma updates to 7. | ||
| bookingId String | ||
| userId String? |
There was a problem hiding this comment.
userId is typed as String?, but User.id is an Int. This blocks FK enforcement and will cause type mismatches when writing audits. Please switch to the correct integer type (and wire up the relation).
Prompt for AI agents
Address the following comment on packages/prisma/schema.prisma at line 2652:
<comment>`userId` is typed as `String?`, but `User.id` is an `Int`. This blocks FK enforcement and will cause type mismatches when writing audits. Please switch to the correct integer type (and wire up the relation).</comment>
<file context>
@@ -2609,6 +2609,52 @@ model RolePermission {
+model BookingAudit {
+ id String @id @default(uuid()) //TODO: Use uuid7() for time-sortable ID once Prisma updates to 7.
+ bookingId String
+ userId String?
+ type BookingAuditType
+ action BookingAuditAction?
</file context>
| type BookingAuditType | ||
| action BookingAuditAction? | ||
| timestamp DateTime @default(now()) | ||
| data Json? |
There was a problem hiding this comment.
BookingAudit is missing its closing brace. Without it the Prisma schema will not compile.
Prompt for AI agents
Address the following comment on packages/prisma/schema.prisma at line 2656:
<comment>`BookingAudit` is missing its closing brace. Without it the Prisma schema will not compile.</comment>
<file context>
@@ -2609,6 +2609,52 @@ model RolePermission {
+ type BookingAuditType
+ action BookingAuditAction?
+ timestamp DateTime @default(now())
+ data Json?
+
enum PhoneNumberSubscriptionStatus {
</file context>
|
|
||
| model BookingAudit { | ||
| id String @id @default(uuid()) //TODO: Use uuid7() for time-sortable ID once Prisma updates to 7. | ||
| bookingId String |
There was a problem hiding this comment.
bookingId is declared as String, but Booking.id is an Int. This mismatch will block a proper relation and require incorrect casting. Please align the field type with the booking primary key (and add the relation).
Prompt for AI agents
Address the following comment on packages/prisma/schema.prisma at line 2651:
<comment>`bookingId` is declared as `String`, but `Booking.id` is an `Int`. This mismatch will block a proper relation and require incorrect casting. Please align the field type with the booking primary key (and add the relation).</comment>
<file context>
@@ -2609,6 +2609,52 @@ model RolePermission {
+
+model BookingAudit {
+ id String @id @default(uuid()) //TODO: Use uuid7() for time-sortable ID once Prisma updates to 7.
+ bookingId String
+ userId String?
+ type BookingAuditType
</file context>
| */ | ||
| async onBookingCreated( | ||
| bookingId: string, | ||
| userId: string, |
There was a problem hiding this comment.
The host no-show helper requires a userId even though the actor is recorded as an attendee; make the userId optional so attendee-triggered updates can be logged without fabricating a user record.
Prompt for AI agents
Address the following comment on packages/lib/server/service/bookingAuditService.ts at line 97:
<comment>The host no-show helper requires a userId even though the actor is recorded as an attendee; make the userId optional so attendee-triggered updates can be logged without fabricating a user record.</comment>
<file context>
@@ -0,0 +1,500 @@
+ */
+ async onBookingCreated(
+ bookingId: string,
+ userId: string,
+ data?: Partial<BookingAuditData>
+ ): Promise<BookingAudit> {
</file context>
| userId: string, | |
| userId?: string, |
|
Closing in favor of #25125 |
4 participants
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist