feat: convert InsightsBookingService to use Prisma.sql raw queries#3
feat: convert InsightsBookingService to use Prisma.sql raw queries#3ShashankFC wants to merge 1 commit into
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>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe InsightsBookingService refactoring migrates from Prisma ORM object-based conditions to SQL fragments (Prisma.sql). The core method Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/lib/server/service/__tests__/insightsBooking.integration-test.ts:
- Around line 332-343: The assertion is flaky because the arrays inserted into
the Prisma.sql template can be in any order; fix by making the arrays
deterministic before building the SQL: sort the teamIds and userIds arrays used
to construct the Prisma.sql (the arrays currently produced from teamsFromOrg /
userIdsFromOrg and used where `expect(conditions).toEqual(Prisma.sql\`...\`)` is
asserted), e.g. convert the sets/arrays to arrays then call .sort() (numeric or
lexicographic consistent with your id types) so the generated SQL always has a
stable ordering; alternatively add explicit orderBy in the repository methods
`findAllByParentId` and `findAllByTeamIds` to guarantee deterministic query
results—pick one and apply the change where the test builds the `teamIds` and
`userIds` used in the Prisma.sql template.
🧹 Nitpick comments (4)
packages/lib/server/service/insightsBooking.ts (4)
29-34: Consider deriving public options type from schema.The
InsightsBookingServicePublicOptionstype manually duplicates what could be derived frominsightsBookingServiceOptionsSchema. Usingz.input<typeof insightsBookingServiceOptionsSchema>would ensure the type stays in sync with the schema automatically.♻️ Suggested refactor
-export type InsightsBookingServicePublicOptions = { - scope: "user" | "org" | "team"; - userId: number; - orgId: number; - teamId?: number; -}; +export type InsightsBookingServicePublicOptions = z.input<typeof insightsBookingServiceOptionsSchema>;
68-81: Simplify condition logic – some branches are unreachable.Since
getAuthorizationConditions()always returns aPrisma.Sql(includingNOTHING_CONDITION), the checkif (authConditions)on line 74 is always truthy. This makes lines 76-77 (else if (filterConditions)) unreachable dead code.♻️ Simplified implementation
async getBaseConditions(): Promise<Prisma.Sql> { const authConditions = await this.getAuthorizationConditions(); const filterConditions = await this.getFilterConditions(); - if (authConditions && filterConditions) { - return Prisma.sql`(${authConditions}) AND (${filterConditions})`; - } else if (authConditions) { - return authConditions; - } else if (filterConditions) { - return filterConditions; - } else { - return NOTHING_CONDITION; - } + if (filterConditions) { + return Prisma.sql`(${authConditions}) AND (${filterConditions})`; + } + return authConditions; }
210-219: Minor: Redundant role truthiness check.The check
membership.role &&on line 216 is redundant since the subsequent equality checks (=== MembershipRole.OWNER || === MembershipRole.ADMIN) will return false for null/undefined values anyway.♻️ Simplified check
return Boolean( membership && membership.accepted && - membership.role && (membership.role === MembershipRole.OWNER || membership.role === MembershipRole.ADMIN) );
52-66: Consider logging validation failures for debugging.When
insightsBookingServiceOptionsSchema.safeParse(options)fails (line 62-63),this.optionsis silently set tonull, which causesNOTHING_CONDITIONto be returned. While this is fail-safe, it may make debugging difficult if callers pass invalid options unknowingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/lib/server/service/__tests__/insightsBooking.integration-test.tspackages/lib/server/service/insightsBooking.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/lib/server/service/insightsBooking.ts (3)
packages/prisma/index.ts (1)
readonlyPrisma(71-75)packages/lib/server/repository/team.ts (1)
TeamRepository(169-336)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)
🔇 Additional comments (6)
packages/lib/server/service/__tests__/insightsBooking.integration-test.ts (3)
2-2: LGTM: NOTHING_CONDITION constant aligns with service implementation.The constant definition mirrors the one in
insightsBooking.ts, ensuring test assertions match the actual return values.Also applies to: 10-11
384-386: LGTM: Filter condition assertions correctly reflect SQL template generation.The tests properly verify the SQL fragment structure for individual filters and their combination with AND logic.
Also applies to: 407-407, 429-431
481-491: The test is correct as written.BookingTimeStatusDenormalizedis a database view that automatically reflects data from the underlyingBookingDenormalizedtable, which is populated automatically via database triggers. When a booking is inserted viaprisma.booking.create(), a trigger fires and populatesBookingDenormalized, making the data immediately available to the query.packages/lib/server/service/insightsBooking.ts (3)
97-123: LGTM: Filter condition building is correctly implemented.The early return for empty conditions array (lines 114-116) protects the
reducecall from being invoked on an empty array.
145-175: LGTM: Org authorization condition building handles edge cases well.The deduplication of user IDs (line 167) and the guaranteed minimum team condition (orgId) ensure the reduce operation is always safe.
177-208: LGTM: Team authorization properly validates team-org relationship.The early return with
NOTHING_CONDITIONwhen the team doesn't belong to the org (lines 186-188) is a good security measure preventing unauthorized access.
| expect(conditions).toEqual( | ||
| Prisma.sql`(("teamId" = ANY(${[ | ||
| testData.org.id, | ||
| testData.team.id, | ||
| team2.id, | ||
| team3.id, | ||
| ]})) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[ | ||
| testData.user.id, | ||
| user2.id, | ||
| user3.id, | ||
| ]})) AND ("isTeamBooking" = false))` | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the test file
fd -n "insightsBooking.integration-test.ts" -type fRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 317
🏁 Script executed:
# Look at the specific test section around lines 332-343
cat -n packages/lib/server/service/__tests__/insightsBooking.integration-test.ts | sed -n '320,355p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1263
🏁 Script executed:
# Search for findAllByParentId and findAllByTeamIds implementations
rg -n "findAllByParentId|findAllByTeamIds" --type ts -B 2 -A 10Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 7403
🏁 Script executed:
# Check for any ORDER BY clauses in the service implementation
rg -n "ORDER BY|orderBy" packages/lib/server/service --type ts -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
🏁 Script executed:
# Get the complete findAllByTeamIds implementation
cat -n packages/lib/server/repository/membership.ts | sed -n '402,420p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 567
🏁 Script executed:
# Check the complete findAllByParentId implementation
cat -n packages/lib/server/repository/team.ts | sed -n '185,210p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 751
🏁 Script executed:
# Look at the test setup to see how test data is created
cat -n packages/lib/server/service/__tests__/insightsBooking.integration-test.ts | sed -n '1,100p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 3551
🏁 Script executed:
# Search for where team2 and team3 are created in the test
rg -n "team2\|team3" packages/lib/server/service/__tests__/insightsBooking.integration-test.ts -B 3 -A 3Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
🏁 Script executed:
# Look for team2 and team3 creation in the test file
cat -n packages/lib/server/service/__tests__/insightsBooking.integration-test.ts | sed -n '100,250p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 5623
🏁 Script executed:
# Search for where org scope test is set up
rg -n "scope.*org" packages/lib/server/service/__tests__/insightsBooking.integration-test.ts -B 20 | head -80Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1778
🏁 Script executed:
# Let me check how the service actually constructs and returns the conditions
cat -n packages/lib/server/service/insightsBooking.ts | sed -n '140,175p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1575
🏁 Script executed:
# Also check if there's any sorting applied to uniqueUserIds
cat -n packages/lib/server/service/insightsBooking.ts | sed -n '160,170p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 539
🏁 Script executed:
# Check the actual usage of findAllByParentId - does the full select matter?
cat -n packages/lib/server/repository/team.ts | sed -n '1,30p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1187
🏁 Script executed:
# Check if there's any implicit ordering by looking at database schema or Prisma schema
cat -n packages/lib/server/repository/team.ts | sed -n '180,220p'Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1049
🏁 Script executed:
# Look for the teamSelect default to see if it includes any ordering
rg -n "const teamSelect" packages/lib/server/repository/team.ts -A 15Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 497
Test may be flaky due to unordered array comparisons in SQL assertions.
The test assumes teamIds and userIds arrays will be in a specific order. However, both findAllByParentId (team.ts:185-197) and findAllByTeamIds (membership.ts:402-419) use findMany without an orderBy clause, so database query results have no guaranteed order. Additionally, line 167 converts userIdsFromOrg to a Set and back to an array, which doesn't guarantee the order matches the test's hardcoded expectation.
If database query execution order changes (due to indexes, query planner optimization, or different database systems), the test will fail despite semantically correct SQL.
Options:
- Sort arrays before building the SQL:
const teamIds = [options.orgId, ...teamsFromOrg.map((t) => t.id).sort((a, b) => a - b)]; - Use a custom matcher that compares arrays ignoring order
- Add explicit
orderByto the repository methods to ensure deterministic results
🤖 Prompt for AI Agents
In @packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
around lines 332 - 343, The assertion is flaky because the arrays inserted into
the Prisma.sql template can be in any order; fix by making the arrays
deterministic before building the SQL: sort the teamIds and userIds arrays used
to construct the Prisma.sql (the arrays currently produced from teamsFromOrg /
userIdsFromOrg and used where `expect(conditions).toEqual(Prisma.sql\`...\`)` is
asserted), e.g. convert the sets/arrays to arrays then call .sort() (numeric or
lexicographic consistent with your id types) so the generated SQL always has a
stable ordering; alternatively add explicit orderBy in the repository methods
`findAllByParentId` and `findAllByTeamIds` to guarantee deterministic query
results—pick one and apply the change where the test builds the `teamIds` and
`userIds` used in the Prisma.sql template.
2 participants
Test 5nn
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#5](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/5)*