feat: event type pbac #22618
feat: event type pbac #22618
Conversation
WalkthroughRefactors event-types viewing and creation to a PBAC-driven design and extracts membership/event-group logic into new use-case classes. Adds EventGroupBuilder, TeamAccessUseCase, ProfilePermissionProcessor, EventTypeGroupFilter, and multiple permission/transform utilities; replaces owner-only procedures with createEventPbacProcedure for update/delete/duplicate mutations; updates create and getUserEventGroups handlers to use PBAC checks and the new builders; introduces tests for builder/use-case/filter behavior; and adds optional scoped permission registry props to some settings UI components. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.ts📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Files:
🧬 Code Graph Analysis (1)packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts (2)
🔇 Additional comments (8)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
✅ No security or compliance issues detected. Reviewed everything up to 1bfed57. Security Overview
Detected Code Changes
Reply to this PR with |
a1d62f0 to
a4a0da4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (1)
229-229: Minor formatting inconsistency.There are extra closing braces and spaces that appear to be formatting artifacts.
- /> - ))}{" "} + /> + ))}- /> - ))} - </div>{" "} + /> + ))} + </div>Also applies to: 254-254
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx(3 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx(3 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx(3 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx
**/*.{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:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
📚 Learning: applies to **/*.tsx : always use `t()` for text localization in frontend code; direct text embedding...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.tsx : Always use `t()` for text localization in frontend code; direct text embedding should trigger a warning
Applied to files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (25)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/SimplePermissionItem.tsx (9)
3-3: LGTM! Well-structured prop addition for scoped registry.The addition of the optional
scopedRegistryprop follows good TypeScript practices and maintains backward compatibility with the fallback pattern. This enhancement allows for dynamic permission registry scoping while preserving existing functionality.Also applies to: 16-16, 24-24, 29-29
45-45: Proper usage of dynamic registry for resource labels.The implementation correctly uses the scoped registry (when provided) to look up resource labels, maintaining the same API pattern as before while adding flexibility for different permission contexts.
3-3: LGTM! Clean implementation of scoped registry support.The addition of the optional
scopedRegistryprop with proper TypeScript typing and fallback to the globalPERMISSION_REGISTRYmaintains backward compatibility while enabling flexible permission scoping.Also applies to: 16-16, 24-24, 29-29
45-45: Good use of dynamic registry for resource labels.The implementation correctly uses the scoped registry for resource label translation, maintaining consistency with the fallback pattern established above.
3-3: LGTM! Clean import addition.The PermissionRegistry type import is properly added alongside the existing Resource import.
16-16: LGTM! Well-designed optional prop.The scopedRegistry prop is properly typed and optional, ensuring backward compatibility while enabling dynamic permission registry scoping.
24-24: LGTM! Consistent prop destructuring.The scopedRegistry prop is properly destructured, maintaining consistency with the interface definition.
29-29: LGTM! Clean fallback pattern.The registry selection uses a simple and effective fallback pattern that maintains backward compatibility while enabling scoped registry functionality.
45-45: LGTM! Proper localization implementation.The resource label translation correctly uses the registry for i18n key lookup and properly uses
t()for localization, following the coding guidelines. The fallback to the resource string is good defensive programming.apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (9)
110-121: Excellent optimization of memoization pattern.The refactor to compute both
filteredResourcesandscopedRegistryin a singleuseMemohook is efficient and follows React best practices. This ensures both values are derived from the same dependencies and prevents unnecessary re-computations.
227-227: Consistent prop passing for scoped registry.The addition of
scopedRegistryprop to bothAdvancedPermissionGroupandSimplePermissionItemcomponents ensures consistent permission data scoping across the role management UI.Also applies to: 251-251
244-244: Correct usage of scoped registry for resource iteration.Using
scopedRegistryinstead of the globalPERMISSION_REGISTRYfor iterating over resources maintains consistency with the scoped permission context.
110-121: Excellent optimization of memoized values.The refactoring to return both
filteredResourcesandscopedRegistryfrom a singleuseMemohook prevents duplicate computation of the scoped registry and ensures consistent usage throughout the component. The memoization dependencies are correctly set.
227-227: Consistent prop passing of scoped registry.The implementation correctly passes the memoized
scopedRegistryto child components (AdvancedPermissionGroupandSimplePermissionItem) and uses it consistently for resource iteration. This maintains the scoped permission context throughout the component hierarchy.Also applies to: 244-244, 251-251
110-121: LGTM! Efficient memoization optimization.The refactoring consolidates related computations into a single
useMemohook, computingscopedRegistryonce and reusing it for both filtering and component props. The dependencies are correctly specified.
227-227: LGTM! Consistent scoped registry propagation.The scopedRegistry prop is properly passed to AdvancedPermissionGroup, enabling context-specific permission configurations.
244-244: LGTM! Consistent scoped registry usage.Using
scopedRegistryinstead of the globalPERMISSION_REGISTRYmaintains consistency with the scoped permission approach throughout the component.
251-251: LGTM! Complete scoped registry pattern.The scopedRegistry prop is properly passed to SimplePermissionItem, completing the consistent pattern of providing scoped permission configurations to all child components.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/AdvancedPermissionGroup.tsx (7)
5-5: LGTM! Consistent prop addition pattern.The addition of the optional
scopedRegistryprop follows the same pattern as the other permission components, maintaining consistency across the codebase while enabling dynamic registry scoping.Also applies to: 20-20, 30-30
34-34: Proper conditional logic for registry selection.The implementation correctly uses the scoped registry when provided, otherwise falls back to the global
PERMISSION_REGISTRY. This defensive pattern ensures backward compatibility while enabling the new scoped functionality.
5-5: Consistent implementation of scoped registry pattern.The implementation follows the same pattern as
SimplePermissionItem, with proper TypeScript typing, destructuring, and conditional fallback logic. This maintains consistency across permission components and enables flexible registry scoping.Also applies to: 20-20, 30-30, 34-34
5-5: LGTM! Consistent import pattern.The PermissionRegistry type import follows the same pattern as SimplePermissionItem, maintaining architectural consistency.
20-20: LGTM! Consistent API design.The optional scopedRegistry prop follows the same pattern as SimplePermissionItem, demonstrating good consistency in the component API design.
30-30: LGTM! Consistent prop handling.The scopedRegistry prop destructuring maintains the same pattern used across permission components, enhancing maintainability.
34-34: LGTM! Consistent fallback pattern.The resourceConfig assignment uses the same clean conditional fallback pattern as SimplePermissionItem, maintaining consistency and backward compatibility.
|
|
||
| if (ctx.user.organizationId) { | ||
| try { | ||
| const orgPermissions = await getResourcePermissions({ |
There was a problem hiding this comment.
I don't think this function throws.
| console.warn(`User ${userId} is not a member of team ${teamId}`); | ||
| throw new TRPCError({ code: "UNAUTHORIZED" }); | ||
| } | ||
|
|
||
| const isSystemAdmin = ctx.user.role === "ADMIN"; |
There was a problem hiding this comment.
not sure if we should handle this case separately.
System admins basically skip all the pbac rules, right? I wonder if it's baked into getResourcePermissions and all sort of the new helper methods.
There was a problem hiding this comment.
Hmmm - i dont know we if want system admins to have * on every user. Some have impersonation disabled etc. Might need to think about this more to enable it globally
| // Re-export the compareMembership function for backward compatibility | ||
| export { compareMembership } from "./utils/permissionUtils"; |
There was a problem hiding this comment.
| // Re-export the compareMembership function for backward compatibility | |
| export { compareMembership } from "./utils/permissionUtils"; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/pbac/_router.tsx (1)
18-47: LGTM! Enhanced permission string validation handles nested resources correctly.The new validation logic properly addresses complex resource names using a longest-match strategy. The special case handling for
._resourceand the sorted resource matching ensure accurate parsing of permission strings like "organization.attributes.read".Consider caching the sorted resources array to avoid sorting on every validation:
+// Cache sorted resources outside the validation function +const sortedResourceValues = Object.values(Resource).sort((a, b) => b.length - a.length); + const permissionStringSchema = z.custom<PermissionString>((val) => { if (typeof val !== "string") return false; // Handle special case for _resource if (val.endsWith("._resource")) { return true; } - // Find the longest matching resource from the end - const resourceValues = Object.values(Resource); let matchedResource: string | null = null; let remainingAction: string | null = null; - // Sort resources by length (longest first) to match the most specific resource - const sortedResources = resourceValues.sort((a, b) => b.length - a.length); - - for (const resource of sortedResources) { + for (const resource of sortedResourceValues) {packages/features/pbac/infrastructure/repositories/RoleRepository.ts (1)
11-38: Excellent consistency with router validation logic.The
parsePermissionStringhelper function implements the same longest-match strategy as the router validation, ensuring consistent handling of complex permission strings throughout the system. The documentation is clear and the special case handling for._resourceis appropriate.Consider caching the sorted resources array for performance, similar to the suggestion for the router:
+// Cache sorted resources outside the function +const sortedResourceValues = Object.values(Resource).sort((a, b) => b.length - a.length); + function parsePermissionString(permissionString: string): { resource: string; action: string } { // Handle special case for _resource if (permissionString.endsWith("._resource")) { const resource = permissionString.substring(0, permissionString.length - 10); return { resource, action: "_resource" }; } - // Find the longest matching resource from the end - const resourceValues = Object.values(Resource); - - // Sort resources by length (longest first) to match the most specific resource - const sortedResources = resourceValues.sort((a, b) => b.length - a.length); - - for (const resource of sortedResources) { + for (const resource of sortedResourceValues) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/features/pbac/infrastructure/repositories/RoleRepository.ts(2 hunks)packages/trpc/server/routers/viewer/pbac/_router.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*Repository.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/features/pbac/infrastructure/repositories/RoleRepository.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/features/pbac/infrastructure/repositories/RoleRepository.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/features/pbac/infrastructure/repositories/RoleRepository.tspackages/trpc/server/routers/viewer/pbac/_router.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/trpc/server/routers/viewer/pbac/_router.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
📚 Learning: applies to **/*repository.ts : repository files must include `repository` suffix, prefix with techno...
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*Repository.ts : Repository files must include `Repository` suffix, prefix with technology if applicable (e.g., `PrismaAppRepository.ts`), and use PascalCase matching the exported class
Applied to files:
packages/features/pbac/infrastructure/repositories/RoleRepository.ts
📚 Learning: in signup-view.tsx, when checking if redirecturl contains certain strings, using explicit && checks ...
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/trpc/server/routers/viewer/pbac/_router.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/features/pbac/infrastructure/repositories/RoleRepository.ts (1)
87-88: LGTM! Proper integration of enhanced permission parsing.The usage of
parsePermissionStringin the create method correctly replaces the previous simple split approach, ensuring complex permission strings are properly parsed before database insertion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts (1)
32-44: Consider extracting permission mapping to improve maintainability.The switch statement maps permission strings to permission fields, but this creates a tight coupling between the permission strings and the
TeamPermissionsinterface structure.Consider extracting this mapping to a separate utility function or configuration:
+const PERMISSION_MAPPING = { + "eventType.read": () => true, + "eventType.create": (permissions: TeamPermissions) => permissions.canCreate, + "eventType.update": (permissions: TeamPermissions) => permissions.canEdit, + "eventType.delete": (permissions: TeamPermissions) => permissions.canDelete, +} as const; - switch (permission) { - case "eventType.read": - // All groups with permissions can read - return true; - case "eventType.create": - return permissions.canCreate; - case "eventType.update": - return permissions.canEdit; - case "eventType.delete": - return permissions.canDelete; - default: - return false; - } + const permissionCheck = PERMISSION_MAPPING[permission]; + return permissionCheck ? permissionCheck(permissions) : false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.ts(2 hunks)packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.tspackages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.tspackages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.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/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.tspackages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.tspackages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
📚 Learning: in cal.com's getusereventgroups handler refactor (pr #22618), the membershipcount field for team eve...
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.tspackages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.tspackages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts
📚 Learning: in cal.com's event type system, the membershipcount field for team event groups is intentionally set...
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally set to 0, as confirmed by sean-brydon (PR author). This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior that maintains consistency with the previous implementation.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.tspackages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.tspackages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts
📚 Learning: in cal.com's event type system, the membershipcount field for team event groups is intentionally har...
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally hard-coded to 0, as confirmed by sean-brydon. This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.tspackages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.tspackages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts
🧬 Code Graph Analysis (1)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts (1)
packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts (1)
EventTypeGroup(10-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (10)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts (1)
60-102: LGTM! Comprehensive permission filtering test coverage.The test suite thoroughly covers all permission scenarios and edge cases for the
EventTypeGroupFilter.has()method, including proper handling of user groups (which always have permissions) and team groups (which depend on the permissions map).packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts (3)
6-13: LGTM! Clean class structure with proper encapsulation.The constructor properly initializes the private fields and the class maintains immutability by creating new instances in filtering methods.
20-48: LGTM! Solid permission filtering logic with clear user vs team group handling.The
has()method correctly:
- Grants all permissions to user groups (teamId is null/undefined)
- Uses the permissions map for team groups
- Provides sensible defaults for unknown permissions
- Handles the case where team permissions are not found in the map
119-124: LGTM! Clean factory function for fluent API.The
filterEventsfactory function provides a clean entry point for the fluent filtering API.packages/trpc/server/routers/viewer/eventTypes/getUserEventGroups.handler.ts (6)
10-13: LGTM! Clean import organization for new abstractions.The imports are well-organized and bring in the necessary new classes for the refactored implementation.
38-43: LGTM! Good dependency injection pattern.The dependencies object properly encapsulates the repository and use case dependencies, making the code more testable and modular.
45-52: LGTM! Clean delegation to EventGroupBuilder.The handler properly delegates the complex event group building logic to the specialized
EventGroupBuilderclass, passing all necessary parameters.
54-56: LGTM! Proper application of permission filtering.The use of
EventTypeGroupFilterto filter by"eventType.read"permission is appropriate and aligns with the access control requirements.
58-60: LGTM! Clean profile processing delegation.The handler properly uses the
ProfilePermissionProcessorto handle profile permission logic, maintaining separation of concerns.
68-69: LGTM! Good backward compatibility consideration.Re-exporting the
compareMembershipfunction maintains backward compatibility for existing code that may depend on importing it from this module.
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
Outdated
Show resolved
Hide resolved
…GroupFilter.test.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…c' into feat/event-type-pbac
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts (1)
163-166: Remove leftover diff markers and duplicate declaration; use the correct permission key.You have committed diff artifacts here (lines starting with "-" and "+"). This causes the Biome parse errors and the duplicate
resultdeclaration flagged in static analysis. Also, the old permission"eventType.edit"must be replaced with the standardized"eventType.update".Apply this fix:
- const result = filterEvents(allGroups, mockPermissionsMap).has("eventType.edit").readOnly(false).get(); - const result = filterEvents(allGroups, mockPermissionsMap).has("eventType.update").readOnly(false).get(); + const result = filterEvents(allGroups, mockPermissionsMap) + .has("eventType.update") + .readOnly(false) + .get();Run this quick check to ensure no other stray diff markers made it into source files:
#!/bin/bash # Search for lines that start with a lone '+' or '-' followed by a space (common leftover from diffs) rg -n '^[[:space:]]*[+-][[:space:]]+[^/*+-]' --glob '!**/node_modules/**'Expected: no matches. If matches appear in this test file, remove the offending lines.
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts (1)
35-36: Align team mocks’ membershipCount with intended semantics (0).Per the retrieved learnings for this PR, team event groups intentionally report
membershipCount: 0to preserve legacy behavior. While these tests don’t assert onmembershipCount, aligning the mocks avoids confusion and reduces accidental reliance later.metadata: { - membershipCount: 5, + membershipCount: 0, readOnly: false, }, ... metadata: { - membershipCount: 3, + membershipCount: 0, readOnly: true, },Also applies to: 50-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.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/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally set to 0, as confirmed by sean-brydon (PR author). This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior that maintains consistency with the previous implementation.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally hard-coded to 0, as confirmed by sean-brydon. This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
🧬 Code Graph Analysis (1)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts (3)
packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts (1)
EventTypeGroup(10-25)packages/platform/libraries/index.ts (1)
MembershipRole(98-98)packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.ts (3)
filterEvents(119-124)count(100-102)exists(108-110)
🪛 Biome (2.1.2)
packages/trpc/server/routers/viewer/eventTypes/utils/EventTypeGroupFilter.test.ts
[error] 164-164: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 165-165: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 165-165: Shouldn't redeclare 'result'. Consider to delete it or rename it.
'result' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
E2E results are ready! |
|
We should probably remove manage already in this PR. It's still showing at the moment: |
Seed your database by "yarn seed-pbac"
Look at the roles created in orgs/teams roles and permissions in settings.
Create new roles/edit old ones to remove all permission from event type - assign it to a user.
Impersonate the user and try to CRUD an event type.
Tick on permissions in the role and keep testing the event type.
Impersonate a team without pbac enabled and ensure everything works as expected