Skip to content

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.

Sign up for GitHub

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

Open
Frawatson wants to merge 1 commit into
base: insights-query-foundation
Choose a base branch
from
+114 −231
Open

feat: convert InsightsBookingService to use Prisma.sql raw queries #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 41 additions & 149 deletions packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import type { Team, User, Membership } from "@prisma/client";

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR — Removed caching tests (confidence: 100%)

The entire 'Caching' describe block has been removed from the tests. While caching is still implemented in the source code (cachedAuthConditions and cachedFilterConditions), there are no tests verifying that the caching behavior works correctly with the new Prisma.Sql types. The intent spec mentions caching behavior as a concern.

Evidence:

  • The diff shows removal of 'describe("Caching", ...)' block with two test cases
  • Source code still has caching at lines 82-84 and 88-90
  • Intent spec edge case: 'Caching behavior: if options change between calls'

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (58 lines, 1 file) — review recommended)

The entire 'Caching' describe block has been removed from the tests. While caching is still implemented in the source code (cachedAuthConditions and cachedFilterConditions), there are no tests verifying that the caching behavior works correctly with the new Prisma.Sql types. The intent spec mentions caching behavior as a concern.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -426,6 +426,74 @@ describe("InsightsBookingService Integration Tests", () => {
       expect(conditions).toEqual(
         Prisma.sql`(("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})) AND ("userId" = ${testData.user.id})`
       );
 
       await testData.cleanup();
     });
   });
 
+  describe("Caching", () => {
+    it("should cache authorization conditions", async () => {
+      const testData = await createTestData({
+        teamRole: MembershipRole.OWNER,
+        orgRole: MembershipRole.OWNER,
+      });
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+      });
+
+      // First call should build conditions from scratch
+      const conditions1 = await service.getAuthorizationConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`
+      );
+
+      // Second call should return the exact same cached Prisma.Sql object (reference equality)
+      const conditions2 = await service.getAuthorizationConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+
+    it("should cache filter conditions", async () => {
+      const testData = await createTestData();
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+        filters: {
+          eventTypeId: testData.eventType.id,
+        },
+      });
+
+      // First call should build conditions from scratch
+      const conditions1 = await service.getFilterConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
+      );
+
+      // Second call should return the exact same cached Prisma.Sql object (reference equality)
+      const conditions2 = await service.getFilterConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+  });
+
   describe("getBaseConditions", () => {

🤖 Grapple PR auto-fix • minor • Review this diff before applying

import { Prisma } from "@prisma/client";
import { describe, expect, it } from "vitest";

import prisma from "@calcom/prisma";
import { BookingStatus, MembershipRole } from "@calcom/prisma/enums";

import { InsightsBookingService } from "../../service/insightsBooking";

const NOTHING_CONDITION = Prisma.sql`1=0`;

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR — Code Organization (confidence: 85%)

NOTHING_CONDITION is defined as a module-level constant in the test file, but it duplicates the definition in the source file. This creates a maintenance burden: if the constant changes in the source, tests will silently diverge.

Evidence:

  • Test file defines const NOTHING_CONDITION = Prisma.sql\1=0`` at line 9
  • Source file defines the same constant at insightsBooking.ts:44
  • Any future change to the source constant value will not automatically update test expectations

Agent: style

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ [Grapple PR] Auto-fix — style agent (Small fix (2 lines, 1 file))

NOTHING_CONDITION is defined as a module-level constant in the test file, but it duplicates the definition in the source file. This creates a maintenance burden: if the constant changes in the source, tests will silently diverge.

Suggested change
const NOTHING_CONDITION = Prisma.sql`1=0`;
export const NOTHING_CONDITION = Prisma.sql`1=0`;

🤖 Grapple PR auto-fix • minor • confidence: 85%

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR — Code Patterns (confidence: 100%)

Test file defines NOTHING_CONDITION as Prisma.sql1=0 at the top level but the service also defines NOTHING_CONDITION. Duplicated constant definition across test and implementation.

Evidence:

  • Line 9 defines NOTHING_CONDITION in test file
  • insightsBooking.ts line 44 defines the same constant in service
  • Test-implementation coupling: test hard-codes the value rather than importing it
  • The 'Caching' describe block with 2 tests was completely removed
  • cachedAuthConditions and cachedFilterConditions are still used in the service implementation (lines 83-84, 90-91)
  • The intent's risk areas mention: 'Caching behavior: if options change between calls... cachedAuthConditions could return stale results'

Agent: style

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ [Grapple PR] Auto-fix — style agent (Small fix (4 lines, 1 file))

Test file defines NOTHING_CONDITION as Prisma.sql1=0 at the top level but the service also defines NOTHING_CONDITION. Duplicated constant definition across test and implementation.

Suggested change
const NOTHING_CONDITION = Prisma.sql`1=0`;
import { InsightsBookingService, NOTHING_CONDITION } from "../../service/insightsBooking";

🤖 Grapple PR auto-fix • minor • confidence: 100%


// Helper function to create unique test data
async function createTestData({
teamRole = MembershipRole.MEMBER,
Expand Down Expand Up @@ -204,7 +207,7 @@ describe("InsightsBookingService Integration Tests", () => {
});

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 MAJOR — Prisma.sql deep equality in tests (confidence: 89%)

Tests use expect(conditions).toEqual(NOTHING_CONDITION) and expect(conditions).toEqual(Prisma.sql...) to compare Prisma.Sql objects. Prisma.Sql objects created by Prisma.sql template literals are instances of a class with strings, values, and text properties. Two Prisma.sql calls with identical template contents should produce structurally equal objects that toEqual (deep equality) can compare correctly. However, this depends on Prisma's internal implementation — if Prisma.Sql objects contain non-enumerable properties, symbols, or methods that affect equality, tests could pass or fail incorrectly. The intent spec explicitly calls this out as a risk area.

Evidence:

  • Line 207: expect(conditions).toEqual(NOTHING_CONDITION) where NOTHING_CONDITION = Prisma.sql1=0
  • Line 273: expect(conditions).toEqual(Prisma.sql("userId" = ${testData.user.id}) AND ("teamId" IS NULL))
  • Prisma.Sql objects use value-based properties (strings array and values array) that should be comparable with toEqual, but this is an implementation detail
  • Intent spec risk area: 'toEqual may not correctly compare Prisma.Sql objects by value'

Agent: logic

@grapple-pr grapple-pr Bot Apr 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (39 lines, 1 file) — review recommended)

Tests use expect(conditions).toEqual(NOTHING_CONDITION) and expect(conditions).toEqual(Prisma.sql...) to compare Prisma.Sql objects. Prisma.Sql objects created by Prisma.sql template literals are instances of a class with strings, values, and text properties. Two Prisma.sql calls with identical template contents should produce structurally equal objects that toEqual (deep equality) can compare correctly. However, this depends on Prisma's internal implementation — if Prisma.Sql objects contain non-enumerable properties, symbols, or methods that affect equality, tests could pass or fail incorrectly. The intent spec explicitly calls this out as a risk area.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -1,6 +1,22 @@
 import type { Team, User, Membership } from "@prisma/client";
 import { Prisma } from "@prisma/client";
 import { describe, expect, it } from "vitest";
 
 import prisma from "@calcom/prisma";
 import { BookingStatus, MembershipRole } from "@calcom/prisma/enums";
 
 import { InsightsBookingService } from "../../service/insightsBooking";
 
 const NOTHING_CONDITION = Prisma.sql`1=0`;
 
+/**
+ * Compare two Prisma.Sql objects by their semantic content rather than by
+ * reference or deep object equality. Prisma.Sql instances may contain
+ * non-enumerable properties or internal symbols that cause toEqual to behave
+ * unpredictably across Prisma versions. Comparing .sql (the interpolated SQL
+ * string) and .values (the bound parameters array) is the stable, public
+ * contract for Prisma.Sql equality.
+ */
+function expectSqlEqual(actual: Prisma.Sql, expected: Prisma.Sql) {
+  expect(actual.sql).toEqual(expected.sql);
+  expect(actual.values).toEqual(expected.values);
+}
+
 // Helper function to create unique test data
@@ -207,7 +223,7 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(NOTHING_CONDITION);
+      expectSqlEqual(conditions, NOTHING_CONDITION);
     });
 
     it("should return NOTHING for non-owner/admin user", async () => {
@@ -242,7 +258,7 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(NOTHING_CONDITION);
+      expectSqlEqual(conditions, NOTHING_CONDITION);
 
       // Clean up
       await prisma.membership.delete({
@@ -270,7 +286,9 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`);
+      expectSqlEqual(
+        conditions,
+        Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`
+      );
 
       await testData.cleanup();
     });
@@ -292,11 +310,13 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(
+      expectSqlEqual(
+        conditions,
         Prisma.sql`(("teamId" = ${testData.team.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[
           testData.user.id,
         ]})) AND ("isTeamBooking" = false))`
       );
 
       // Clean up
       await testData.cleanup();
@@ -329,18 +349,20 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getAuthorizationConditions();
 
-      expect(conditions).toEqual(
+      expectSqlEqual(
+        conditions,
         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))`
       );
 
       await testData.cleanup();
@@ -381,9 +403,11 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getFilterConditions();
-      expect(conditions).toEqual(
+      expectSqlEqual(
+        conditions,
         Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
       );
 
       await testData.cleanup();
@@ -404,7 +428,9 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getFilterConditions();
-      expect(conditions).toEqual(Prisma.sql`"userId" = ${testData.user.id}`);
+      expectSqlEqual(
+        conditions,
+        Prisma.sql`"userId" = ${testData.user.id}`
+      );
 
       await testData.cleanup();
@@ -426,9 +452,11 @@ describe("InsightsBookingService Integration Tests", () => {
       const conditions = await service.getFilterConditions();
-      expect(conditions).toEqual(
+      expectSqlEqual(
+        conditions,
         Prisma.sql`(("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})) AND ("userId" = ${testData.user.id})`
       );
 
       await testData.cleanup();

🤖 Grapple PR auto-fix • major • Review this diff before applying


const conditions = await service.getAuthorizationConditions();
expect(conditions).toEqual({ id: -1 });
expect(conditions).toEqual(NOTHING_CONDITION);
});

it("should return NOTHING for non-owner/admin user", async () => {
Expand Down Expand Up @@ -239,7 +242,7 @@ describe("InsightsBookingService Integration Tests", () => {
});

const conditions = await service.getAuthorizationConditions();
expect(conditions).toEqual({ id: -1 });
expect(conditions).toEqual(NOTHING_CONDITION);

// Clean up
await prisma.membership.delete({
Expand Down Expand Up @@ -267,14 +270,7 @@ describe("InsightsBookingService Integration Tests", () => {
});

const conditions = await service.getAuthorizationConditions();
expect(conditions).toEqual({
AND: [
{
userId: testData.user.id,
teamId: null,
},
],
});
expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`);

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 MAJOR — Test assertion correctness (confidence: 99%)

Tests use toEqual() to compare Prisma.sql template literal objects. Prisma.sql returns a Prisma.Sql instance whose internal structure (strings array + values array) depends on how the template is constructed. Two semantically identical SQL fragments built via different template literal compositions may not be structurally equal. For example, Prisma.sql("userId" = ${id}) AND ("teamId" IS NULL) constructed in the test may have different internal `strings` arrays than the one constructed inside the service via `Prisma.sql`("userId" = ${this.options.userId}) AND ("teamId" IS NULL) — they should match in this simple case, but for the nested reduce-based compositions (org/team scope), the structural equality is fragile because the service builds the SQL incrementally via reduce while the test builds it in a single template literal.

Evidence:

  • Service uses conditions.reduce((acc, condition, index) => Prisma.sql(${acc}) OR (${condition})) which nests Prisma.Sql objects inside each other
  • Tests construct the expected value as a single flat template: Prisma.sql(("teamId" = ${...}) AND ...) OR ...``
  • Prisma.Sql internally stores a strings array and values array; nested Sql objects in values are preserved as-is, not flattened
  • This means the reduce-built Sql and the flat-template Sql will have different internal structures even if they produce the same SQL string

Agent: architecture

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (20 lines, 1 file) — review recommended)

Tests use toEqual() to compare Prisma.sql template literal objects. Prisma.sql returns a Prisma.Sql instance whose internal structure (strings array + values array) depends on how the template is constructed. Two semantically identical SQL fragments built via different template literal compositions may not be structurally equal. For example, Prisma.sql("userId" = ${id}) AND ("teamId" IS NULL) constructed in the test may have different internal `strings` arrays than the one constructed inside the service via `Prisma.sql`("userId" = ${this.options.userId}) AND ("teamId" IS NULL) — they should match in this simple case, but for the nested reduce-based compositions (org/team scope), the structural equality is fragile because the service builds the SQL incrementally via reduce while the test builds it in a single template literal.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -1,6 +1,7 @@
 import type { Team, User, Membership } from "@prisma/client";
 import { Prisma } from "@prisma/client";
 import { describe, expect, it } from "vitest";
 
 import prisma from "@calcom/prisma";
 import { BookingStatus, MembershipRole } from "@calcom/prisma/enums";
 
 import { InsightsBookingService } from "../../service/insightsBooking";
 
+/**
+ * Extracts the SQL string and bound values from a Prisma.Sql instance for
+ * robust comparison. We cannot use toEqual() directly on Prisma.Sql objects
+ * because the service builds SQL incrementally via reduce() (nesting Sql
+ * objects inside each other), while test expectations are built as flat
+ * template literals — they produce the same SQL but have different internal
+ * `strings` array structures.
+ */
+function sqlToComparable(sql: Prisma.Sql): { sql: string; values: unknown[] } {
+  return { sql: sql.sql, values: sql.values };
+}
+
+/**
+ * Assertion helper: compares two Prisma.Sql instances by their rendered SQL
+ * string and bound parameter values rather than their internal AST structure.
+ */
+function expectSqlEqual(actual: Prisma.Sql, expected: Prisma.Sql): void {
+  expect(sqlToComparable(actual)).toEqual(sqlToComparable(expected));
+}
+
 const NOTHING_CONDITION = Prisma.sql`1=0`;

🤖 Grapple PR auto-fix • major • Review this diff before applying


await testData.cleanup();
});
Expand All @@ -296,24 +292,11 @@ describe("InsightsBookingService Integration Tests", () => {
});

const conditions = await service.getAuthorizationConditions();
expect(conditions).toEqual({
AND: [
{
OR: [
{
teamId: testData.team.id,
isTeamBooking: true,
},
{
userId: {
in: [testData.user.id],
},
isTeamBooking: false,
},
],
},
],
});
expect(conditions).toEqual(
Prisma.sql`(("teamId" = ${testData.team.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[
testData.user.id,
]})) AND ("isTeamBooking" = false))`

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR — Prisma.sql Test Equality (confidence: 88%)

Tests use toEqual() to compare Prisma.Sql objects produced by tagged template literals. Prisma.Sql equality via toEqual() depends on the internal structure (strings array + values array) being identical. The ANY(${[testData.user.id]}) syntax passes a JavaScript array as a parameter binding — this works with toEqual() only if both sides produce the same internal representation. If Prisma's internal Sql class uses non-enumerable properties or symbols, these tests could produce false positives or fragile failures across Prisma versions.

Evidence:

  • Multiple tests compare Prisma.sql tagged template results with .toEqual(Prisma.sql\...`)`
  • Prisma.Sql is a class with strings, values, and potentially other internal properties
  • Array bindings like ${[testData.user.id]} need to produce identical internal representation on both sides

Agent: architecture

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (69 lines, 1 file) — review recommended)

Tests use toEqual() to compare Prisma.Sql objects produced by tagged template literals. Prisma.Sql equality via toEqual() depends on the internal structure (strings array + values array) being identical. The ANY(${[testData.user.id]}) syntax passes a JavaScript array as a parameter binding — this works with toEqual() only if both sides produce the same internal representation. If Prisma's internal Sql class uses non-enumerable properties or symbols, these tests could produce false positives or fragile failures across Prisma versions.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -1,6 +1,6 @@
 import type { Team, User, Membership } from "@prisma/client";
 import { Prisma } from "@prisma/client";
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 
 import prisma from "@calcom/prisma";
 import { BookingStatus, MembershipRole } from "@calcom/prisma/enums";
@@ -10,6 +10,18 @@ import { InsightsBookingService } from "../../service/insightsBooking";
 
 const NOTHING_CONDITION = Prisma.sql`1=0`;
 
+/**
+ * Extracts the stable public properties of a Prisma.Sql object for reliable
+ * cross-version comparison. Comparing Prisma.Sql instances directly with
+ * toEqual() is fragile because the class may contain non-enumerable properties,
+ * symbols, or internal state that differs across Prisma versions. Instead, we
+ * compare only the documented public API: the compiled `sql` string and the
+ * bound `values` array, which are guaranteed to be stable.
+ *
+ * Edge case: nested Prisma.sql`` fragments are flattened into the top-level
+ * `sql` string by Prisma itself, so this captures the fully-resolved query.
+ */
+const sqlSnapshot = (s: Prisma.Sql) => ({ sql: s.sql, values: s.values });
+
 // Helper function to create unique test data
 async function createTestData({
   teamRole = MembershipRole.MEMBER,
@@ -207,7 +219,7 @@ describe("InsightsBookingService Integration Tests", () => {
       });
 
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(NOTHING_CONDITION);
+      expect(sqlSnapshot(conditions)).toEqual(sqlSnapshot(NOTHING_CONDITION));
     });
 
     it("should return NOTHING for non-owner/admin user", async () => {
@@ -242,7 +254,7 @@ describe("InsightsBookingService Integration Tests", () => {
       });
 
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(NOTHING_CONDITION);
+      expect(sqlSnapshot(conditions)).toEqual(sqlSnapshot(NOTHING_CONDITION));
 
       // Clean up
       await prisma.membership.delete({
@@ -270,7 +282,9 @@ describe("InsightsBookingService Integration Tests", () => {
       });
 
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`);
+      expect(sqlSnapshot(conditions)).toEqual(
+        sqlSnapshot(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`)
+      );
 
       await testData.cleanup();
     });
@@ -292,11 +306,13 @@ describe("InsightsBookingService Integration Tests", () => {
       });
 
       const conditions = await service.getAuthorizationConditions();
-      expect(conditions).toEqual(
-        Prisma.sql`(("teamId" = ${testData.team.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[
+      expect(sqlSnapshot(conditions)).toEqual(
+        sqlSnapshot(
+          Prisma.sql`(("teamId" = ${testData.team.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[
           testData.user.id,
-        ]})) AND ("isTeamBooking" = false))`
+          ]})) AND ("isTeamBooking" = false))`
+        )
       );
 
       // Clean up
@@ -329,18 +345,20 @@ describe("InsightsBookingService Integration Tests", () => {
 
       const conditions = await service.getAuthorizationConditions();
 
-      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))`
+      expect(sqlSnapshot(conditions)).toEqual(
+        sqlSnapshot(
+          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))`
+        )
       );
 
       await testData.cleanup();
@@ -381,7 +399,9 @@ describe("InsightsBookingService Integration Tests", () => {
       });
 
       const conditions = await service.getFilterConditions();
-      expect(conditions).toEqual(
-        Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
+      expect(sqlSnapshot(conditions)).toEqual(
+        sqlSnapshot(
+          Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
+        )
       );
 
       await testData.cleanup();
@@ -404,7 +424,7 @@ describe("InsightsBookingService Integration Tests", () => {
       });
 
       const conditions = await service.getFilterConditions();
-      expect(conditions).toEqual(Prisma.sql`"userId" = ${testData.user.id}`);
+      expect(sqlSnapshot(conditions)).toEqual(sqlSnapshot(Prisma.sql`"userId" = ${testData.user.id}`));
 
       await testData.cleanup();
     });
@@ -426,9 +446,11 @@ describe("InsightsBookingService Integration Tests", () => {
       });
 
       const conditions = await service.getFilterConditions();
-      expect(conditions).toEqual(
-        Prisma.sql`(("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})) AND ("userId" = ${testData.user.id})`
+      expect(sqlSnapshot(conditions)).toEqual(
+        sqlSnapshot(
+          Prisma.sql`(("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})) AND ("userId" = ${testData.user.id})`
+        )
       );
 
       await testData.cleanup();

🤖 Grapple PR auto-fix • minor • Review this diff before applying

);

// Clean up
await testData.cleanup();
Expand Down Expand Up @@ -346,26 +329,18 @@ describe("InsightsBookingService Integration Tests", () => {

const conditions = await service.getAuthorizationConditions();

expect(conditions).toEqual({
AND: [
{
OR: [
{
teamId: {
in: [testData.org.id, testData.team.id, team2.id, team3.id],
},
isTeamBooking: true,
},
{
userId: {
in: [testData.user.id, user2.id, user3.id],
},
isTeamBooking: false,
},
],
},
],
});
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))`
);

await testData.cleanup();
});
Expand Down Expand Up @@ -406,13 +381,9 @@ describe("InsightsBookingService Integration Tests", () => {
});

const conditions = await service.getFilterConditions();
expect(conditions).toEqual({
AND: [
{
OR: [{ eventTypeId: testData.eventType.id }, { eventParentId: testData.eventType.id }],
},
],
});
expect(conditions).toEqual(
Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
);

await testData.cleanup();
});
Expand All @@ -433,13 +404,7 @@ describe("InsightsBookingService Integration Tests", () => {
});

const conditions = await service.getFilterConditions();
expect(conditions).toEqual({
AND: [
{
userId: testData.user.id,
},
],
});
expect(conditions).toEqual(Prisma.sql`"userId" = ${testData.user.id}`);

await testData.cleanup();
});
Expand All @@ -461,90 +426,15 @@ describe("InsightsBookingService Integration Tests", () => {
});

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR — Test Coverage Regression (confidence: 100%)

The entire 'Caching' test section was removed (tests for caching of authorization and filter conditions). While the caching logic still exists in the implementation (cachedAuthConditions and cachedFilterConditions), removing the tests means there's no verification that the caching behavior works correctly after the refactor. This is a test coverage regression.

Evidence:

  • Lines removed include 'should cache authorization conditions' and 'should cache filter conditions' tests
  • Implementation still has caching at lines 83-86 and 90-93
  • No replacement caching tests were added
  • The diff shows removal of the 'Caching' describe block with two tests
  • The service still has cachedAuthConditions and cachedFilterConditions private fields
  • Caching behavior changes (e.g., accidentally clearing cache) would go undetected

Agent: logic

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (58 lines, 1 file) — review recommended)

The entire 'Caching' test section was removed (tests for caching of authorization and filter conditions). While the caching logic still exists in the implementation (cachedAuthConditions and cachedFilterConditions), removing the tests means there's no verification that the caching behavior works correctly after the refactor. This is a test coverage regression.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -435,6 +435,72 @@
       await testData.cleanup();
     });
   });
 
+  describe("Caching", () => {
+    it("should cache authorization conditions", async () => {
+      const testData = await createTestData({
+        teamRole: MembershipRole.OWNER,
+        orgRole: MembershipRole.OWNER,
+      });
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+      });
+
+      // First call should build conditions
+      const conditions1 = await service.getAuthorizationConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`
+      );
+
+      // Second call should return the cached conditions (same reference)
+      const conditions2 = await service.getAuthorizationConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+
+    it("should cache filter conditions", async () => {
+      const testData = await createTestData();
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+        filters: {
+          eventTypeId: testData.eventType.id,
+        },
+      });
+
+      // First call should build conditions
+      const conditions1 = await service.getFilterConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
+      );
+
+      // Second call should return the cached conditions (same reference)
+      const conditions2 = await service.getFilterConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+  });
+
   describe("getBaseConditions", () => {

🤖 Grapple PR auto-fix • minor • Review this diff before applying

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR — Caching test coverage removed (confidence: 90%)

The entire 'Caching' test suite has been removed (lines deleted around the old test). While the caching implementation still exists in the service (cachedAuthConditions, cachedFilterConditions), there are no longer any tests verifying that the caching behavior works correctly. This is a regression in test coverage for a feature that the intent specification lists as an acceptance criterion.

Evidence:

  • Old test had 'should cache authorization conditions' and 'should cache filter conditions' tests
  • Both tests are deleted in this diff
  • Acceptance criteria states: 'Cached conditions (cachedAuthConditions, cachedFilterConditions) still function correctly to avoid redundant DB calls'
  • No replacement caching tests are added

Agent: architecture

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (92 lines, 1 file) — review recommended)

The entire 'Caching' test suite has been removed (lines deleted around the old test). While the caching implementation still exists in the service (cachedAuthConditions, cachedFilterConditions), there are no longer any tests verifying that the caching behavior works correctly. This is a regression in test coverage for a feature that the intent specification lists as an acceptance criterion.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -434,6 +434,80 @@
       await testData.cleanup();
     });
   });
 
+  describe("Caching", () => {
+    it("should cache authorization conditions and avoid redundant DB calls", async () => {
+      const testData = await createTestData({
+        teamRole: MembershipRole.OWNER,
+        orgRole: MembershipRole.OWNER,
+      });
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+      });
+
+      // First call should build conditions from DB
+      const conditions1 = await service.getAuthorizationConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`
+      );
+
+      // Second call should return the exact same cached object (referential equality
+      // proves cachedAuthConditions is returned without re-querying the DB)
+      const conditions2 = await service.getAuthorizationConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+
+    it("should cache filter conditions and avoid redundant DB calls", async () => {
+      const testData = await createTestData();
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+        filters: {
+          eventTypeId: testData.eventType.id,
+        },
+      });
+
+      // First call should build conditions
+      const conditions1 = await service.getFilterConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
+      );
+
+      // Second call should return the exact same cached object (referential equality
+      // proves cachedFilterConditions is returned without re-building)
+      const conditions2 = await service.getFilterConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+
+    it("should cache combined authorization and filter conditions independently per service instance", async () => {
+      const testData = await createTestData({
+        teamRole: MembershipRole.OWNER,
+        orgRole: MembershipRole.OWNER,
+      });
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+        filters: {
+          memberUserId: testData.user.id,
+        },
+      });
+
+      // Both caches should be populated independently
+      const authConditions1 = await service.getAuthorizationConditions();
+      const filterConditions1 = await service.getFilterConditions();
+
+      const authConditions2 = await service.getAuthorizationConditions();
+      const filterConditions2 = await service.getFilterConditions();
+
+      // Referential equality confirms caching is working for both independently
+      expect(authConditions2).toBe(authConditions1);
+      expect(filterConditions2).toBe(filterConditions1);
+
+      await testData.cleanup();
+    });
+  });
+
   describe("getBaseConditions", () => {

🤖 Grapple PR auto-fix • minor • Review this diff before applying


const conditions = await service.getFilterConditions();
expect(conditions).toEqual({
AND: [
{
OR: [{ eventTypeId: testData.eventType.id }, { eventParentId: testData.eventType.id }],
},
{
userId: testData.user.id,
},
],
});

await testData.cleanup();
});
});

describe("Caching", () => {
it("should cache authorization conditions", async () => {
const testData = await createTestData({
teamRole: MembershipRole.OWNER,
orgRole: MembershipRole.OWNER,
});

const service = new InsightsBookingService({
prisma,
options: {
scope: "user",
userId: testData.user.id,
orgId: testData.org.id,
},
});

// First call should build conditions
const conditions1 = await service.getAuthorizationConditions();
expect(conditions1).toEqual({
AND: [
{
userId: testData.user.id,
teamId: null,
},
],
});

// Second call should use cached conditions
const conditions2 = await service.getAuthorizationConditions();
expect(conditions2).toEqual(conditions1);

// Clean up
await testData.cleanup();
});

it("should cache filter conditions", async () => {
const testData = await createTestData();

const service = new InsightsBookingService({
prisma,
options: {
scope: "user",
userId: testData.user.id,
orgId: testData.org.id,
},
filters: {
eventTypeId: testData.eventType.id,
},
});

// First call should build conditions
const conditions1 = await service.getFilterConditions();
expect(conditions1).toEqual({
AND: [
{
OR: [{ eventTypeId: testData.eventType.id }, { eventParentId: testData.eventType.id }],
},
],
});

// Second call should use cached conditions
const conditions2 = await service.getFilterConditions();
expect(conditions2).toEqual(conditions1);
expect(conditions).toEqual(

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 MAJOR — Test Coverage Regression — Caching tests removed (confidence: 100%)

The entire 'Caching' describe block (two test cases) was removed. The caching logic (cachedAuthConditions, cachedFilterConditions) still exists in the production code but is no longer tested. The intent specification explicitly states 'Cached conditions (cachedAuthConditions, cachedFilterConditions) still function correctly to avoid redundant DB calls' as an acceptance criterion, yet the tests validating this behavior have been deleted.

Evidence:

  • Lines removed: the 'should cache authorization conditions' and 'should cache filter conditions' tests
  • Acceptance criteria: 'Cached conditions (cachedAuthConditions, cachedFilterConditions) still function correctly to avoid redundant DB calls'
  • The caching code at lines 77-79 and 83-85 of insightsBooking.ts still exists but is untested

Agent: logic

@grapple-pr grapple-pr Bot Apr 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (60 lines, 1 file) — review recommended)

The entire 'Caching' describe block (two test cases) was removed. The caching logic (cachedAuthConditions, cachedFilterConditions) still exists in the production code but is no longer tested. The intent specification explicitly states 'Cached conditions (cachedAuthConditions, cachedFilterConditions) still function correctly to avoid redundant DB calls' as an acceptance criterion, yet the tests validating this behavior have been deleted.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -426,6 +426,84 @@
       await testData.cleanup();
     });
   });
+
+  describe("Caching", () => {
+    it("should cache authorization conditions", async () => {
+      const testData = await createTestData({
+        teamRole: MembershipRole.OWNER,
+        orgRole: MembershipRole.OWNER,
+      });
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+      });
+
+      // First call should build conditions from DB
+      const conditions1 = await service.getAuthorizationConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`
+      );
+
+      // Second call should return the exact same cached object (no recomputation)
+      // This validates cachedAuthConditions avoids redundant DB calls
+      const conditions2 = await service.getAuthorizationConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+
+    it("should cache filter conditions", async () => {
+      const testData = await createTestData();
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+        filters: {
+          eventTypeId: testData.eventType.id,
+        },
+      });
+
+      // First call should build conditions
+      const conditions1 = await service.getFilterConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
+      );
+
+      // Second call should return the exact same cached object (no recomputation)
+      // This validates cachedFilterConditions avoids redundant DB calls
+      const conditions2 = await service.getFilterConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+  });
 
   describe("getBaseConditions", () => {
     it("should combine authorization and filter conditions", async () => {

🤖 Grapple PR auto-fix • major • Review this diff before applying

Prisma.sql`(("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})) AND ("userId" = ${testData.user.id})`
);

await testData.cleanup();
});
});

describe("findMany", () => {
describe("getBaseConditions", () => {
it("should combine authorization and filter conditions", async () => {
const testData = await createTestData({
teamRole: MembershipRole.OWNER,
Expand Down Expand Up @@ -587,16 +477,18 @@ describe("InsightsBookingService Integration Tests", () => {
},
});

const results = await service.findMany({
select: {
id: true,
title: true,
},
});
const baseConditions = await service.getBaseConditions();
const results = await prisma.$queryRaw<{ id: number }[]>`
SELECT id FROM "BookingTimeStatusDenormalized"
WHERE ${baseConditions}
`;

@grapple-pr grapple-pr Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 INFO — Code Organization (confidence: 81%)

The test suite removed the 'Caching' test suite (lines 461-485 in original) and 'findMany' test suite, replacing them with a single 'getBaseConditions' test. The caching tests were meaningful coverage of the cached conditions logic—their removal reduces test coverage for an important implementation detail.

Evidence:

  • Diff shows 149 lines removed from the original test file
  • Caching tests verified that repeated calls return cached instances (same reference behavior)
  • New test only verifies getBaseConditions() combines conditions and executes a query, not that caching works

Agent: style

// Should return the user booking since it matches both conditions
expect(results).toHaveLength(1);
expect(results[0]?.id).toBe(userBooking.id);
expect(results).toEqual([
{
id: userBooking.id,
},

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MINOR — Pattern violations (confidence: 83%)

The caching tests for both authorization and filter conditions have been removed entirely. The caching behavior (cachedAuthConditions/cachedFilterConditions) still exists in the implementation and is called out in the acceptance criteria ('getAuthorizationConditions() caches its result after first call and does not re-query'). Removing these tests reduces coverage of an important behavioral contract.

Evidence:

  • The 'Caching' describe block with two tests was deleted (lines ~350-420 in old file)
  • The caching mechanism using cachedAuthConditions/cachedFilterConditions is still present in insightsBooking.ts
  • Acceptance criteria explicitly states: 'getAuthorizationConditions() caches its result after first call and does not re-query'

Agent: architecture

@grapple-pr grapple-pr Bot Apr 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (57 lines, 1 file) — review recommended)

The caching tests for both authorization and filter conditions have been removed entirely. The caching behavior (cachedAuthConditions/cachedFilterConditions) still exists in the implementation and is called out in the acceptance criteria ('getAuthorizationConditions() caches its result after first call and does not re-query'). Removing these tests reduces coverage of an important behavioral contract.

--- a/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
+++ b/packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
@@ -432,6 +432,72 @@ describe("InsightsBookingService Integration Tests", () => {
       await testData.cleanup();
     });
   });
 
+  describe("Caching", () => {
+    it("should cache authorization conditions after first call and does not re-query", async () => {
+      const testData = await createTestData({
+        teamRole: MembershipRole.OWNER,
+        orgRole: MembershipRole.OWNER,
+      });
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+      });
+
+      // First call should build conditions
+      const conditions1 = await service.getAuthorizationConditions();
+      expect(conditions1).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`);
+
+      // Second call should return the cached result (same reference)
+      const conditions2 = await service.getAuthorizationConditions();
+      expect(conditions2).toBe(conditions1);
+
+      // Clean up
+      await testData.cleanup();
+    });
+
+    it("should cache filter conditions after first call", async () => {
+      const testData = await createTestData();
+
+      const service = new InsightsBookingService({
+        prisma,
+        options: {
+          scope: "user",
+          userId: testData.user.id,
+          orgId: testData.org.id,
+        },
+        filters: {
+          eventTypeId: testData.eventType.id,
+        },
+      });
+
+      // First call should build conditions
+      const conditions1 = await service.getFilterConditions();
+      expect(conditions1).toEqual(
+        Prisma.sql`("eventTypeId" = ${testData.eventType.id}) OR ("eventParentId" = ${testData.eventType.id})`
+      );
+
+      // Second call should return the cached result (same reference)
+      const conditions2 = await service.getFilterConditions();
+      expect(conditions2).toBe(conditions1);
+
+      await testData.cleanup();
+    });
+  });
+
   describe("getBaseConditions", () => {

🤖 Grapple PR auto-fix • minor • Review this diff before applying

]);

// Clean up
await prisma.booking.delete({
Expand Down
Loading