feat: make orgId optional for user and team scopes in /insights and /insights/routing#23912
Merged
eunjae-lee merged 11 commits intomainfrom Sep 19, 2025
Merged
Conversation
…BaseService - User scope authorization only checks formUserId and formTeamId IS NULL - Team scope now supports standalone teams without organizations - Add validation logic to return NOTHING_CONDITION if team belongs to org but no orgId provided - Add comprehensive test coverage for null/undefined orgId scenarios in both scopes - Aligns schema with actual usage patterns and supports teams without organizations Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Make orgId optional for user and team scopes in InsightsBookingBaseService - Update InsightsBookingServicePublicOptions type to allow orgId: number | null - Add validation logic for team scope to handle missing orgId - Add comprehensive test coverage for null/undefined orgId scenarios - Fix type casting issues in test file - Maintains backward compatibility while supporting teams without organizations Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
E2E results are ready! |
- Skip isOwnerOrAdmin check for team scope when orgId is null (standalone teams) - Maintain security for org scope and team scope with orgId - Fixes integration test failures for null orgId test cases Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Properly handle both null and undefined orgId values in authorization logic - Fix integration test failures where null orgId was incorrectly triggering isOwnerOrAdmin check - Ensure team scope with null orgId skips ownership validation for standalone teams Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Remove orgId condition from isOwnerOrAdmin check for team scope - Ensure both standalone teams and org-based teams require ownership validation - Maintain orgId validation logic in buildTeamAuthorizationCondition methods Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Change from .optional() to .nullish() to allow both null and undefined - Fixes schema validation when tests pass orgId: null - Resolves authorization logic returning NOTHING_CONDITION for valid cases Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…rvices - Replace isOwnerOrAdmin method in InsightsBookingBaseService with checkPermission from PermissionCheckService - Replace isOwnerOrAdmin method in InsightsRoutingBaseService with checkPermission from PermissionCheckService - Use permission 'insights.read' with fallback roles MembershipRole.OWNER and MembershipRole.ADMIN - Maintain same method signature and behavior while leveraging PBAC system Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
eunjae-lee
changed the title
dosubot
bot
added
insights
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
packages/features/insights/server/trpc-router.ts (2)
685-696: Prisma: replace include with select.Per repo guideline, avoid include; use select to fetch only needed fields.
- include: { - team: { - select: { - id: true, - name: true, - logoUrl: true, - slug: true, - metadata: true, - }, - }, - }, + select: { + team: { + select: { + id: true, + name: true, + logoUrl: true, + slug: true, + metadata: true, + }, + }, + },
746-751: Prisma: replace include with select.Same guideline; fetch only the needed user fields.
-include: { - user: { - select: userSelect, - }, -}, +select: { + user: { + select: userSelect, + }, +},packages/lib/server/service/InsightsRoutingBaseService.ts (3)
818-823: Nullish check for targetId (avoid truthy bug).If a sentinel 0 ever sneaks in, this branch is skipped. Use != null.
- if (targetId && scope !== "user") { + if (targetId != null && scope !== "user") { const isOwnerOrAdmin = await this.isOwnerOrAdmin(this.options.userId, targetId); if (!isOwnerOrAdmin) { return NOTHING_CONDITION; } }
856-876: Team auth: use nullish check for orgId.Truthiness on orgId can misclassify valid numeric values (edge). Prefer != null.
- if (options.orgId) { + if (options.orgId != null) { // team under org const childTeamOfOrg = await teamRepo.findByIdAndParentId({ id: options.teamId, parentId: options.orgId, select: { id: true }, }); if (!childTeamOfOrg) { // teamId and its orgId does not match return NOTHING_CONDITION; } } else { // standalone team
291-297: bookingStatus: avoid UPPER to keep enum semantics.Type declares BookingStatus | null; casting to UPPER(text) returns e.g. 'ACCEPTED', which may diverge from enum values used elsewhere.
- UPPER(rfrd."bookingStatus"::text) as "bookingStatus", + rfrd."bookingStatus" as "bookingStatus",packages/lib/server/service/InsightsBookingBaseService.ts (2)
382-390: Nullish check for targetId (avoid truthy bug).Mirror fix from routing service.
- if (targetId && scope !== "user") { + if (targetId != null && scope !== "user") { const isOwnerOrAdmin = await this.isOwnerOrAdmin(this.options.userId, targetId); if (!isOwnerOrAdmin) { return NOTHING_CONDITION; } }
440-460: Team auth: use nullish check for orgId.Same edge-case as routing service.
- if (options.orgId) { + if (options.orgId != null) { // team under org const childTeamOfOrg = await teamRepo.findByIdAndParentId({ id: options.teamId, parentId: options.orgId, select: { id: true }, }); if (!childTeamOfOrg) { // teamId and its orgId does not match return NOTHING_CONDITION; } } else { // standalone team
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/features/insights/server/trpc-router.ts(1 hunks)packages/lib/server/service/InsightsBookingBaseService.ts(5 hunks)packages/lib/server/service/InsightsRoutingBaseService.ts(3 hunks)packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts(3 hunks)packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/server/service/InsightsBookingBaseService.tspackages/lib/server/service/InsightsRoutingBaseService.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/server/service/InsightsBookingBaseService.tspackages/features/insights/server/trpc-router.tspackages/lib/server/service/InsightsRoutingBaseService.tspackages/lib/server/service/__tests__/InsightsBookingService.integration-test.tspackages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/server/service/InsightsBookingBaseService.tspackages/features/insights/server/trpc-router.tspackages/lib/server/service/InsightsRoutingBaseService.tspackages/lib/server/service/__tests__/InsightsBookingService.integration-test.tspackages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/server/service/InsightsBookingBaseService.tspackages/features/insights/server/trpc-router.tspackages/lib/server/service/InsightsRoutingBaseService.tspackages/lib/server/service/__tests__/InsightsBookingService.integration-test.tspackages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
📚 Learning: 2025-07-24T08:39:06.185Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Applied to files:
packages/lib/server/service/InsightsBookingBaseService.tspackages/lib/server/service/__tests__/InsightsBookingService.integration-test.tspackages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
📚 Learning: 2025-08-26T08:08:23.395Z
Learnt from: SinghaAnirban005
PR: calcom/cal.com#23343
File: packages/features/insights/server/trpc-router.ts:1080-1101
Timestamp: 2025-08-26T08:08:23.395Z
Learning: In packages/features/insights/server/trpc-router.ts, when filtering personal event types (userId provided, no teamId, not isAll), the query correctly uses user.id (authenticated user) instead of the input userId parameter for security reasons. This prevents users from accessing other users' personal event types by passing arbitrary user IDs.
Applied to files:
packages/features/insights/server/trpc-router.tspackages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-08-14T10:48:52.586Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/ai/_router.ts:46-84
Timestamp: 2025-08-14T10:48:52.586Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/ai/_router.ts, the voiceId input parameter in the create endpoint is intentionally not forwarded to aiService.createAgent() as voice customization is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.
Applied to files:
packages/features/insights/server/trpc-router.ts
📚 Learning: 2025-07-15T13:02:17.403Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/lib/server/service/insightsRouting.ts:367-368
Timestamp: 2025-07-15T13:02:17.403Z
Learning: In the InsightsRoutingService (packages/lib/server/service/insightsRouting.ts), multi-select filter data is already validated by zod before reaching the buildFormFieldSqlCondition method, so null/undefined values are not present in filterValue.data arrays and don't need to be filtered out.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/lib/server/service/InsightsRoutingBaseService.ts
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts
🧬 Code graph analysis (4)
packages/lib/server/service/InsightsBookingBaseService.ts (2)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-290)packages/platform/libraries/index.ts (1)
MembershipRole(34-34)
packages/lib/server/service/InsightsRoutingBaseService.ts (1)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-290)
packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts (2)
packages/lib/server/service/InsightsBookingBaseService.ts (1)
InsightsBookingServicePublicOptions(125-130)packages/lib/server/service/InsightsBookingDIService.ts (1)
InsightsBookingService(13-29)
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts (2)
packages/lib/server/service/InsightsRoutingBaseService.ts (1)
InsightsRoutingServicePublicOptions(41-46)packages/lib/server/service/InsightsRoutingDIService.ts (1)
InsightsRoutingService(13-29)
🔇 Additional comments (9)
packages/lib/server/service/InsightsRoutingBaseService.ts (2)
22-39: Schema/type updates look correct.User/team scopes now allow nullish orgId; org scope still requires it. Public options align.
882-889: PBAC-based check is appropriate.Using PermissionCheckService with OWNER/ADMIN fallback matches rollout strategy.
Please confirm the permission string "insights.read" matches the registered permission in PBAC seeds.
packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts (2)
352-371: Null orgId user-scope test looks good.Covers the intended scenario.
444-464: Nothing condition test is correct.Team belongs to org but orgId null should yield NOTHING_CONDITION.
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts (2)
483-521: Null orgId + standalone team path is covered well.Asserts the intended team-only condition.
446-475: Unrelated team under org case is correct.Ensures NOT child-of-org yields NOTHING_CONDITION.
packages/lib/server/service/InsightsBookingBaseService.ts (2)
106-123: Schema: allow nullish orgId for user/team scopes.Matches router changes; good for standalone teams/users.
1274-1281: PBAC-based owner/admin check looks good.Leverages “insights.read” with OWNER/ADMIN fallback.
Confirm PBAC feature flag coverage for teams used in tests to avoid false negatives.
packages/features/insights/server/trpc-router.ts (1)
327-329: Approve — orgId should be left nullish; audit remaining '?? 0' fallbacksorgId change in packages/features/insights/server/trpc-router.ts is correct.
- Audit remaining "?? 0" occurrences from the rg output and either keep intentional numeric defaults or switch orgId/teamId uses to nullish. Notable hits to check: packages/features/instant-meeting/handleInstantMeeting.ts:37, packages/features/webhooks/lib/getWebhooks.ts:39, packages/features/insights/server/trpc-router.ts:846.
Comment on lines
+373
to
+392
| it("should build user scope conditions with undefined orgId", async () => { | ||
| const testData = await createTestData({ | ||
| teamRole: MembershipRole.OWNER, | ||
| orgRole: MembershipRole.OWNER, | ||
| }); | ||
|
|
||
| const service = new InsightsBookingService({ | ||
| prisma, | ||
| options: { | ||
| scope: "user", | ||
| userId: testData.user.id, | ||
| orgId: null, | ||
| }, | ||
| }); | ||
|
|
||
| const conditions = await service.getAuthorizationConditions(); | ||
| expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`); | ||
|
|
||
| await testData.cleanup(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix test: “undefined orgId” case currently passes null.
Set orgId to undefined to actually exercise that path.
- orgId: null,
+ orgId: undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("should build user scope conditions with undefined orgId", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const service = new InsightsBookingService({ | |
| prisma, | |
| options: { | |
| scope: "user", | |
| userId: testData.user.id, | |
| orgId: null, | |
| }, | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`); | |
| await testData.cleanup(); | |
| }); | |
| it("should build user scope conditions with undefined orgId", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const service = new InsightsBookingService({ | |
| prisma, | |
| options: { | |
| scope: "user", | |
| userId: testData.user.id, | |
| orgId: undefined, | |
| }, | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`); | |
| await testData.cleanup(); | |
| }); |
🤖 Prompt for AI Agents
In
packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts
around lines 373 to 392, the test intends to exercise the "undefined orgId" path
but sets orgId: null; change the test to pass orgId: undefined in the service
options so the code path for undefined is executed (update the object literal to
orgId: undefined and leave the rest unchanged).
Comment on lines
+466
to
+514
| it("should build team scope conditions with undefined orgId for standalone team", async () => { | ||
| const testData = await createTestData({ | ||
| teamRole: MembershipRole.OWNER, | ||
| orgRole: MembershipRole.OWNER, | ||
| }); | ||
|
|
||
| const standaloneTeam = await prisma.team.create({ | ||
| data: { | ||
| name: "Standalone Team 2", | ||
| slug: `standalone-team-2-${Date.now()}-${Math.random().toString(36).substring(7)}`, | ||
| isOrganization: false, | ||
| parentId: null, | ||
| }, | ||
| }); | ||
|
|
||
| await prisma.membership.create({ | ||
| data: { | ||
| userId: testData.user.id, | ||
| teamId: standaloneTeam.id, | ||
| role: MembershipRole.OWNER, | ||
| accepted: true, | ||
| }, | ||
| }); | ||
|
|
||
| const service = new InsightsBookingService({ | ||
| prisma, | ||
| options: { | ||
| scope: "team", | ||
| userId: testData.user.id, | ||
| orgId: null, | ||
| teamId: standaloneTeam.id, | ||
| }, | ||
| }); | ||
|
|
||
| const conditions = await service.getAuthorizationConditions(); | ||
| expect(conditions).toEqual( | ||
| Prisma.sql`(("teamId" = ${standaloneTeam.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[ | ||
| testData.user.id, | ||
| ]})) AND ("isTeamBooking" = false))` | ||
| ); | ||
|
|
||
| await prisma.membership.deleteMany({ | ||
| where: { teamId: standaloneTeam.id }, | ||
| }); | ||
| await prisma.team.delete({ | ||
| where: { id: standaloneTeam.id }, | ||
| }); | ||
| await testData.cleanup(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix test: “undefined orgId” case currently passes null (team scope).
Set orgId to undefined.
- orgId: null,
+ orgId: undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("should build team scope conditions with undefined orgId for standalone team", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const standaloneTeam = await prisma.team.create({ | |
| data: { | |
| name: "Standalone Team 2", | |
| slug: `standalone-team-2-${Date.now()}-${Math.random().toString(36).substring(7)}`, | |
| isOrganization: false, | |
| parentId: null, | |
| }, | |
| }); | |
| await prisma.membership.create({ | |
| data: { | |
| userId: testData.user.id, | |
| teamId: standaloneTeam.id, | |
| role: MembershipRole.OWNER, | |
| accepted: true, | |
| }, | |
| }); | |
| const service = new InsightsBookingService({ | |
| prisma, | |
| options: { | |
| scope: "team", | |
| userId: testData.user.id, | |
| orgId: null, | |
| teamId: standaloneTeam.id, | |
| }, | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual( | |
| Prisma.sql`(("teamId" = ${standaloneTeam.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[ | |
| testData.user.id, | |
| ]})) AND ("isTeamBooking" = false))` | |
| ); | |
| await prisma.membership.deleteMany({ | |
| where: { teamId: standaloneTeam.id }, | |
| }); | |
| await prisma.team.delete({ | |
| where: { id: standaloneTeam.id }, | |
| }); | |
| await testData.cleanup(); | |
| }); | |
| it("should build team scope conditions with undefined orgId for standalone team", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const standaloneTeam = await prisma.team.create({ | |
| data: { | |
| name: "Standalone Team 2", | |
| slug: `standalone-team-2-${Date.now()}-${Math.random().toString(36).substring(7)}`, | |
| isOrganization: false, | |
| parentId: null, | |
| }, | |
| }); | |
| await prisma.membership.create({ | |
| data: { | |
| userId: testData.user.id, | |
| teamId: standaloneTeam.id, | |
| role: MembershipRole.OWNER, | |
| accepted: true, | |
| }, | |
| }); | |
| const service = new InsightsBookingService({ | |
| prisma, | |
| options: { | |
| scope: "team", | |
| userId: testData.user.id, | |
| orgId: undefined, | |
| teamId: standaloneTeam.id, | |
| }, | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual( | |
| Prisma.sql`(("teamId" = ${standaloneTeam.id}) AND ("isTeamBooking" = true)) OR (("userId" = ANY(${[ | |
| testData.user.id, | |
| ]})) AND ("isTeamBooking" = false))` | |
| ); | |
| await prisma.membership.deleteMany({ | |
| where: { teamId: standaloneTeam.id }, | |
| }); | |
| await prisma.team.delete({ | |
| where: { id: standaloneTeam.id }, | |
| }); | |
| await testData.cleanup(); | |
| }); |
🤖 Prompt for AI Agents
In
packages/lib/server/service/__tests__/InsightsBookingService.integration-test.ts
around lines 466 to 514, the test intended to exercise the "undefined orgId"
case incorrectly passes orgId: null when constructing the InsightsBookingService
options; update the test to pass orgId: undefined instead (i.e., replace orgId:
null with orgId: undefined) so the service receives an undefined orgId as
intended and run the test to confirm it still types and behaves correctly.
Comment on lines
+361
to
+384
| it("should build user scope conditions with undefined orgId", async () => { | ||
| const testData = await createTestData({ | ||
| teamRole: MembershipRole.OWNER, | ||
| orgRole: MembershipRole.OWNER, | ||
| }); | ||
|
|
||
| const service = new InsightsRoutingService({ | ||
| prisma, | ||
| options: { | ||
| scope: "user", | ||
| userId: testData.user.id, | ||
| orgId: null, | ||
| teamId: undefined, | ||
| }, | ||
| filters: createDefaultFilters(), | ||
| }); | ||
|
|
||
| const conditions = await service.getAuthorizationConditions(); | ||
| expect(conditions).toEqual( | ||
| Prisma.sql`rfrd."formUserId" = ${testData.user.id} AND rfrd."formTeamId" IS NULL` | ||
| ); | ||
|
|
||
| await testData.cleanup(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix test: “undefined orgId” case currently passes null (user scope).
Set orgId to undefined.
- orgId: null,
+ orgId: undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("should build user scope conditions with undefined orgId", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const service = new InsightsRoutingService({ | |
| prisma, | |
| options: { | |
| scope: "user", | |
| userId: testData.user.id, | |
| orgId: null, | |
| teamId: undefined, | |
| }, | |
| filters: createDefaultFilters(), | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual( | |
| Prisma.sql`rfrd."formUserId" = ${testData.user.id} AND rfrd."formTeamId" IS NULL` | |
| ); | |
| await testData.cleanup(); | |
| }); | |
| it("should build user scope conditions with undefined orgId", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const service = new InsightsRoutingService({ | |
| prisma, | |
| options: { | |
| scope: "user", | |
| userId: testData.user.id, | |
| orgId: undefined, | |
| teamId: undefined, | |
| }, | |
| filters: createDefaultFilters(), | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual( | |
| Prisma.sql`rfrd."formUserId" = ${testData.user.id} AND rfrd."formTeamId" IS NULL` | |
| ); | |
| await testData.cleanup(); | |
| }); |
🤖 Prompt for AI Agents
In
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
around lines 361 to 384, the test titled "should build user scope conditions
with undefined orgId" currently passes orgId: null but should pass orgId:
undefined; update the test input to set orgId to undefined so it accurately
exercises the "undefined orgId" code path and re-run the assertion unchanged.
Comment on lines
+553
to
+598
| it("should build team scope conditions with undefined orgId for standalone team", async () => { | ||
| const testData = await createTestData({ | ||
| teamRole: MembershipRole.OWNER, | ||
| orgRole: MembershipRole.OWNER, | ||
| }); | ||
|
|
||
| const standaloneTeam = await prisma.team.create({ | ||
| data: { | ||
| name: "Standalone Team 2", | ||
| slug: `standalone-team-2-${randomUUID()}`, | ||
| isOrganization: false, | ||
| parentId: null, | ||
| }, | ||
| }); | ||
|
|
||
| await prisma.membership.create({ | ||
| data: { | ||
| userId: testData.user.id, | ||
| teamId: standaloneTeam.id, | ||
| role: MembershipRole.OWNER, | ||
| accepted: true, | ||
| }, | ||
| }); | ||
|
|
||
| const service = new InsightsRoutingService({ | ||
| prisma, | ||
| options: { | ||
| scope: "team", | ||
| userId: testData.user.id, | ||
| orgId: null, | ||
| teamId: standaloneTeam.id, | ||
| }, | ||
| filters: createDefaultFilters(), | ||
| }); | ||
|
|
||
| const conditions = await service.getAuthorizationConditions(); | ||
| expect(conditions).toEqual(Prisma.sql`rfrd."formTeamId" = ${standaloneTeam.id}`); | ||
|
|
||
| await prisma.membership.deleteMany({ | ||
| where: { teamId: standaloneTeam.id }, | ||
| }); | ||
| await prisma.team.delete({ | ||
| where: { id: standaloneTeam.id }, | ||
| }); | ||
| await testData.cleanup(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix test: “undefined orgId” case currently passes null (team scope).
Set orgId to undefined.
- orgId: null,
+ orgId: undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("should build team scope conditions with undefined orgId for standalone team", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const standaloneTeam = await prisma.team.create({ | |
| data: { | |
| name: "Standalone Team 2", | |
| slug: `standalone-team-2-${randomUUID()}`, | |
| isOrganization: false, | |
| parentId: null, | |
| }, | |
| }); | |
| await prisma.membership.create({ | |
| data: { | |
| userId: testData.user.id, | |
| teamId: standaloneTeam.id, | |
| role: MembershipRole.OWNER, | |
| accepted: true, | |
| }, | |
| }); | |
| const service = new InsightsRoutingService({ | |
| prisma, | |
| options: { | |
| scope: "team", | |
| userId: testData.user.id, | |
| orgId: null, | |
| teamId: standaloneTeam.id, | |
| }, | |
| filters: createDefaultFilters(), | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual(Prisma.sql`rfrd."formTeamId" = ${standaloneTeam.id}`); | |
| await prisma.membership.deleteMany({ | |
| where: { teamId: standaloneTeam.id }, | |
| }); | |
| await prisma.team.delete({ | |
| where: { id: standaloneTeam.id }, | |
| }); | |
| await testData.cleanup(); | |
| }); | |
| it("should build team scope conditions with undefined orgId for standalone team", async () => { | |
| const testData = await createTestData({ | |
| teamRole: MembershipRole.OWNER, | |
| orgRole: MembershipRole.OWNER, | |
| }); | |
| const standaloneTeam = await prisma.team.create({ | |
| data: { | |
| name: "Standalone Team 2", | |
| slug: `standalone-team-2-${randomUUID()}`, | |
| isOrganization: false, | |
| parentId: null, | |
| }, | |
| }); | |
| await prisma.membership.create({ | |
| data: { | |
| userId: testData.user.id, | |
| teamId: standaloneTeam.id, | |
| role: MembershipRole.OWNER, | |
| accepted: true, | |
| }, | |
| }); | |
| const service = new InsightsRoutingService({ | |
| prisma, | |
| options: { | |
| scope: "team", | |
| userId: testData.user.id, | |
| orgId: undefined, | |
| teamId: standaloneTeam.id, | |
| }, | |
| filters: createDefaultFilters(), | |
| }); | |
| const conditions = await service.getAuthorizationConditions(); | |
| expect(conditions).toEqual(Prisma.sql`rfrd."formTeamId" = ${standaloneTeam.id}`); | |
| await prisma.membership.deleteMany({ | |
| where: { teamId: standaloneTeam.id }, | |
| }); | |
| await prisma.team.delete({ | |
| where: { id: standaloneTeam.id }, | |
| }); | |
| await testData.cleanup(); | |
| }); |
🤖 Prompt for AI Agents
In
packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts
around lines 553 to 598, the test intends to cover the "undefined orgId" case
but currently passes orgId: null in the service options; change the options to
pass orgId: undefined (e.g., options: { scope: "team", userId: testData.user.id,
orgId: undefined, teamId: standaloneTeam.id }) so the test actually exercises
the undefined path, keeping the rest of setup/teardown the same.
sean-brydon
approved these changes
Sep 19, 2025
Member
sean-brydon
left a comment
sean-brydon
left a comment
There was a problem hiding this comment.
LGTM - i do find the null OR undefined a bit of a weird choice but it seems to be working well
Thanks for the added test cases
Contributor
Author
yeah initially I had only |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR makes the
orgIdparameter optional for bothuserandteamscopes in theInsightsRoutingBaseService, enabling support for users and teams that don't belong to organizations.Key Changes:
insightsRoutingServiceOptionsSchemato makeorgIdoptional for user and team scopesbuildTeamAuthorizationConditionmethod with validation logic for optionalorgIdorgIdscenariosBackground: The original schema required
orgIdfor user scope, but user authorization only checks personal forms (formUserIdandformTeamId IS NULL) without usingorgId. Similarly, teams can exist independently without belonging to organizations, making the requiredorgIdparameter unnecessarily restrictive.Visual Demo (For contributors especially)
N/A - This is a backend schema and validation change without UI impact.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Test the new optional orgId functionality:
User scope with no orgId:
Team scope for standalone teams:
parentId: nullNOTHING_CONDITIONfor teams that belong to orgs but noorgIdprovidedBackward compatibility:
orgIdprovided should continue working unchangedChecklist
Link to Devin run: https://app.devin.ai/sessions/dabcc6f380174f3b8fecf4017e7d6643
Requested by: @eunjae-lee
Critical Review Areas
🔍 Please pay special attention to:
NOTHING_CONDITIONvs allowing access is security-critical