Cal.com / PR #4

feat: convert InsightsBookingService to use Prisma.sql raw queries

calcom/cal.com#22345
Golden recall: 50% Extra findings: 2

Golden Comments (0/2 found, 1 partial)

# Comment Severity Status FriendlyReviewer Detail
#1 Dead code branches in getBaseConditions(): getAuthorizationConditions() always returns non-null Prisma.Sql, so authConditions is always truthy → the else if (filterConditions) and else branches never reach LOW ✗ Missed FriendlyReviewer noted the missing test coverage for these branches but didn't recognize that the branches themselves are unreachable code. That's a significant gap: recognizing that code cannot execute requires understanding control flow beyond what line-level analysis provides.
#2 userIdsFromOrg conditioned on teamsFromOrg.length > 0 → may exclude org-level members for orgs without child teams MEDIUM ● Partial FriendlyReviewer: buildOrgAuthorizationCondition collects user IDs only from team memberships. If the requesting org admin/owner is not a member of any specific team, their personal (non-team) bookings will be excluded. Same underlying bug, different manifestation.

Supplementary Findings (4 findings)

Additional issues found by FriendlyReviewer that are not part of the golden (reference) set.

Finding File Severity Legitimate?
Caching tests completely removed — caching logic exists but untested insightsBooking.integration-test.ts:437 MEDIUM ✓ Test regression
teamId optional in TypeScript but Zod requires it for scope='team' → silent runtime error insightsBooking.ts:35 MEDIUM ✓ Type mismatch
Edge cases not tested for getBaseConditions (auth-only, NOTHING_CONDITION, etc.) insightsBooking.integration-test.ts:470 LOW ✓ Test coverage
Authorization gap: non-team org admins have their personal bookings excluded insightsBooking.ts:146 LOW ✓ Related to golden #2

Honest Analysis

This is FriendlyReviewer's weakest Cal.com result. 0 of 2 golden bugs were directly found, and only 1 partial match. The dead code branches in getBaseConditions() (Golden #1) were completely missed — FriendlyReviewer noted the missing test coverage for these branches but didn't recognize that the branches themselves are unreachable code. That's a significant gap: recognizing that code cannot execute requires understanding control flow beyond what line-level analysis provides.

The pre-existing authorization gap finding is the most valuable supplementary finding. FriendlyReviewer correctly identifies that org admins without team memberships have their personal bookings excluded. But calling this LOW understates its functional impact — it's a correctness bug in authorization logic.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

⛅ Tech
2 medium, 2 low

This PR converts the InsightsBookingService to use Prisma.sql raw queries, removing the previous Prisma ORM-based query construction. The refactoring itself is sound and the raw SQL approach is appropriate for the complex aggregation queries required by the insights feature. However, it introduces test coverage regressions (caching tests were dropped entirely), a type safety gap between the Zod schema and the TypeScript types for teamId, and insufficient edge-case coverage for the new getBaseConditions control flow. The pre-existing authorization logic for org-level queries also has a correctness issue where org admins without team memberships may have their personal bookings excluded.

Worth checking

  • 🟡 packages/trpc/server/routers/viewer/insights/insightsBooking.integration-test.ts (L437)
    Test regression: The previous test suite included coverage for caching behavior (verify that cached results are returned without re-executing the query). These tests have been completely removed in the refactored version, yet the caching logic still exists in the production code. Without tests, a change to the caching layer would go undetected. Restore or rewrite caching tests for the new implementation.
  • 🟡 packages/trpc/server/routers/viewer/insights/insightsBooking.ts (L35)
    Type mismatch: teamId is declared as optional in the TypeScript input type, but the Zod schema validation requires it when scope === 'team'. A caller that passes scope='team' without teamId will pass TypeScript compilation but fail at runtime with a Zod validation error. Either make teamId required in the TypeScript type when scope='team' (discriminated union), or handle the missing case gracefully.

Small things (take or leave)

  • 🔵 packages/trpc/server/routers/viewer/insights/insightsBooking.integration-test.ts (L470)
    Test coverage: getBaseConditions has several code paths (auth-only conditions, NOTHING_CONDITION, combined auth+filter) that are not exercised by the existing tests. Adding parameterized tests for these edge cases would improve confidence in the refactored query builder.
  • 🔵 packages/trpc/server/routers/viewer/insights/insightsBooking.ts (L146)
    Authorization logic: buildOrgAuthorizationCondition collects user IDs only from team memberships (teamMembers). If the requesting org admin/owner is not a member of any specific team within the org, their personal (non-team) bookings will be excluded from the results. This is a pre-existing issue in the authorization logic that predates this PR, but the refactoring is an opportunity to address it.