-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: insights-performance-optimization #370
Automated Test: insights-performance-optimization #370
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: eunjae@cal.com <hey@eunjae.dev> * 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: eunjae@cal.com <hey@eunjae.dev> * 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: eunjae@cal.com <hey@eunjae.dev> * 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: eunjae@cal.com <hey@eunjae.dev> * 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>
📝 WalkthroughWalkthroughThis pull request refactors the InsightsBookingService from using structured Prisma where-input objects to raw SQL fragments (Prisma.Sql) for building authorization and filter conditions. The corresponding integration tests are updated to assert on SQL fragments and use raw queries instead of typed findMany operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/lib/server/service/insightsBooking.ts`:
- Around line 164-174: The SQL array inputs must be made deterministic: sort and
dedupe the ID arrays before embedding them in Prisma.sql to stabilize generated
SQL fragments. Update the logic that builds conditions (the teamIds and
userIdsFromOrg inputs used in the conditions array and the uniqueUserIds
computation) to first create stable arrays (e.g., Array.from(new Set(...)) then
.sort()) and use those sorted unique arrays when constructing the Prisma.sql
snippets; apply the same change to the other similar block that constructs SQL
arrays later in this file (the block that mirrors this conditions/build
pattern).
🧹 Nitpick comments (1)
packages/lib/server/service/insightsBooking.ts (1)
29-34: Tighten public options type to avoid silent runtime fallback.The public type allows
scope: "team"withoutteamId, which then fails validation and returnsNOTHING_CONDITION, masking caller mistakes. Prefer a discriminated union (or reuse the Zod-inferred type) so invalid combinations fail at compile time.♻️ Proposed type tightening
-export type InsightsBookingServicePublicOptions = { - scope: "user" | "org" | "team"; - userId: number; - orgId: number; - teamId?: number; -}; +export type InsightsBookingServicePublicOptions = + | { scope: "user"; userId: number; orgId: number } + | { scope: "org"; userId: number; orgId: number } + | { scope: "team"; userId: number; orgId: number; teamId: number };Also applies to: 58-58
| const conditions: Prisma.Sql[] = [Prisma.sql`("teamId" = ANY(${teamIds})) AND ("isTeamBooking" = true)`]; | ||
|
|
||
| if (userIdsFromOrg.length > 0) { | ||
| const uniqueUserIds = Array.from(new Set(userIdsFromOrg)); | ||
| conditions.push(Prisma.sql`("userId" = ANY(${uniqueUserIds})) AND ("isTeamBooking" = false)`); | ||
| } | ||
|
|
||
| return conditions.reduce((acc, condition, index) => { | ||
| if (index === 0) return condition; | ||
| return Prisma.sql`(${acc}) OR (${condition})`; | ||
| }); |
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.
Stabilize ID ordering before building SQL arrays.
Team/user ID lists come from DB queries with no guaranteed ordering, so SQL fragments can vary across runs. Sorting (and deduping) makes conditions deterministic and keeps SQL-fragment comparisons stable.
🧭 Proposed deterministic ordering
- const teamIds = [options.orgId, ...teamsFromOrg.map((t) => t.id)];
+ const teamIds = [options.orgId, ...teamsFromOrg.map((t) => t.id)].sort((a, b) => a - b);
- if (userIdsFromOrg.length > 0) {
- const uniqueUserIds = Array.from(new Set(userIdsFromOrg));
- conditions.push(Prisma.sql`("userId" = ANY(${uniqueUserIds})) AND ("isTeamBooking" = false)`);
- }
+ if (userIdsFromOrg.length > 0) {
+ const uniqueUserIds = Array.from(new Set(userIdsFromOrg)).sort((a, b) => a - b);
+ conditions.push(Prisma.sql`("userId" = ANY(${uniqueUserIds})) AND ("isTeamBooking" = false)`);
+ }- const userIdsFromTeam = usersFromTeam.map((u) => u.userId);
+ const userIdsFromTeam = usersFromTeam.map((u) => u.userId);
+ const uniqueUserIds = Array.from(new Set(userIdsFromTeam)).sort((a, b) => a - b);
- if (userIdsFromTeam.length > 0) {
- conditions.push(Prisma.sql`("userId" = ANY(${userIdsFromTeam})) AND ("isTeamBooking" = false)`);
- }
+ if (uniqueUserIds.length > 0) {
+ conditions.push(Prisma.sql`("userId" = ANY(${uniqueUserIds})) AND ("isTeamBooking" = false)`);
+ }Also applies to: 196-207
🤖 Prompt for AI Agents
In `@packages/lib/server/service/insightsBooking.ts` around lines 164 - 174, The
SQL array inputs must be made deterministic: sort and dedupe the ID arrays
before embedding them in Prisma.sql to stabilize generated SQL fragments. Update
the logic that builds conditions (the teamIds and userIdsFromOrg inputs used in
the conditions array and the uniqueUserIds computation) to first create stable
arrays (e.g., Array.from(new Set(...)) then .sort()) and use those sorted unique
arrays when constructing the Prisma.sql snippets; apply the same change to the
other similar block that constructs SQL arrays later in this file (the block
that mirrors this conditions/build pattern).
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit