-
Notifications
You must be signed in to change notification settings - Fork 0
feat: convert InsightsBookingService to use Prisma.sql raw queries #7
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
base: insights-query-foundation
Are you sure you want to change the base?
feat: convert InsightsBookingService to use Prisma.sql raw queries #7
Conversation
…22345) * fix: use raw query at InsightsBookingService * feat: convert InsightsBookingService to use Prisma.sql raw queries - Convert auth conditions from Prisma object notation to Prisma.sql - Convert filter conditions from Prisma object notation to Prisma.sql - Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql - Fix type error in isOrgOwnerOrAdmin method - Follow same pattern as InsightsRoutingService conversion Co-Authored-By: [email protected] <[email protected]> * feat: convert InsightsBookingService to use Prisma.sql raw queries - Convert auth conditions from Prisma object notation to Prisma.sql - Convert filter conditions from Prisma object notation to Prisma.sql - Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql - Fix type error in isOrgOwnerOrAdmin method - Follow same pattern as InsightsRoutingService conversion Co-Authored-By: [email protected] <[email protected]> * fix: update InsightsBookingService integration tests for Prisma.sql format - Replace Prisma object notation expectations with Prisma.sql template literals - Add NOTHING_CONDITION constant for consistency with InsightsRoutingService - Update all test cases to use direct Prisma.sql comparisons - Use $queryRaw for actual database integration testing - Follow same testing patterns as InsightsRoutingService Co-Authored-By: [email protected] <[email protected]> * fix: exclude intentionally skipped jobs from required CI check failure - Remove 'skipped' from failure condition in pr.yml and all-checks.yml - Allow E2E jobs to be skipped without failing the required check - Only actual failures and cancelled jobs will cause required check to fail Co-Authored-By: [email protected] <[email protected]> * fix tests * Revert "fix: exclude intentionally skipped jobs from required CI check failure" This reverts commit 6ff44fc9a8f14ad657f7bba7c2e454e192b66c8f. * clean up tests * address feedback --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Greptile OverviewConfidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client
participant InsightsBookingService
participant TeamRepository
participant MembershipRepository
participant Database
Client->>InsightsBookingService: getBaseConditions()
InsightsBookingService->>InsightsBookingService: getAuthorizationConditions()
InsightsBookingService->>InsightsBookingService: buildAuthorizationConditions()
InsightsBookingService->>MembershipRepository: findUniqueByUserIdAndTeamId()
MembershipRepository->>Database: SELECT membership WHERE userId, teamId
Database-->>MembershipRepository: membership data
MembershipRepository-->>InsightsBookingService: isOwnerOrAdmin result
alt scope === "org"
InsightsBookingService->>TeamRepository: findAllByParentId(orgId)
TeamRepository->>Database: SELECT teams WHERE parentId
Database-->>TeamRepository: child teams
TeamRepository-->>InsightsBookingService: teamsFromOrg
InsightsBookingService->>MembershipRepository: findAllByTeamIds(teamIds)
MembershipRepository->>Database: SELECT memberships WHERE teamId IN (...)
Database-->>MembershipRepository: user memberships
MembershipRepository-->>InsightsBookingService: userIds
InsightsBookingService->>InsightsBookingService: Build Prisma.sql with ANY operator
end
InsightsBookingService->>InsightsBookingService: getFilterConditions()
InsightsBookingService->>InsightsBookingService: buildFilterConditions()
InsightsBookingService->>InsightsBookingService: Combine auth + filter SQL
InsightsBookingService-->>Client: Prisma.Sql conditions
Client->>Database: $queryRaw with conditions
Database-->>Client: Query results
|
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.
Additional Comments (1)
-
packages/lib/server/service/insightsBooking.ts, line 157-162 (link)logic: condition prevents fetching users when org has no child teams, but teamIds includes the org itself
The original code at line 157-162 fetches users from ALL teams including the org (line 154:
teamIds = [options.orgId, ...teamsFromOrg.map()]). But the checkteamsFromOrg.length > 0on line 158 skips fetching users when there are no child teams, even though the org itself (inteamIds) may have direct members.
2 files reviewed, 3 comments
| if (userIdsFromTeam.length > 0) { | ||
| conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`); | ||
| } |
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.
logic: missing empty array check before using ANY operator
When userIdsFromTeam is empty, ANY(${[]}) creates invalid SQL. The original code conditionally added this clause only when the array had elements. Add the same check:
| if (userIdsFromTeam.length > 0) { | |
| conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`); | |
| } | |
| if (userIdsFromTeam.length > 0) { | |
| conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/lib/server/service/insightsBooking.ts
Line: 200:202
Comment:
**logic:** missing empty array check before using ANY operator
When `userIdsFromTeam` is empty, `ANY(${[]})` creates invalid SQL. The original code conditionally added this clause only when the array had elements. Add the same check:
```suggestion
if (userIdsFromTeam.length > 0) {
conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`);
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (authConditions && filterConditions) { | ||
| return Prisma.sql`(${authConditions}) AND (${filterConditions})`; | ||
| } else if (authConditions) { |
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.
logic: incorrect null check for Prisma.Sql type
Prisma.Sql is an object type that cannot be null - checking authConditions && filterConditions will always be truthy even when they're NOTHING_CONDITION. This breaks the logic when filterConditions is intentionally null (no filters).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/lib/server/service/insightsBooking.ts
Line: 72:74
Comment:
**logic:** incorrect null check for `Prisma.Sql` type
`Prisma.Sql` is an object type that cannot be null - checking `authConditions && filterConditions` will always be truthy even when they're `NOTHING_CONDITION`. This breaks the logic when `filterConditions` is intentionally `null` (no filters).
How can I resolve this? If you propose a fix, please make it concise.
Test 5
Replicated from ai-code-review-evaluation/cal.com-greptile#5