fix: Allow room owners to edit retention policy #37319
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThe pull request modifies retention policy editing logic across three related files in the room information editor. Changes include updating guard conditions to check for retention policy object existence, adding room subscription-based ownership checks, and allowing room owners to edit retention settings alongside explicit permission checks. Changes
Sequence DiagramsequenceDiagram
participant Component as EditRoomInfo
participant Permissions as useEditRoomPermissions
participant InitialValues as useEditRoomInitialValues
participant Subscription as useRoomSubscription
rect rgb(240, 248, 255)
Note over Component,Subscription: Permission & Ownership Check
Permissions->>Subscription: Get room subscription
Subscription-->>Permissions: subscription.roles
Permissions->>Permissions: Derive isRoomOwner from roles
Permissions->>Permissions: Check hasRetentionPermission
Permissions->>Permissions: canEditRetentionPolicy = <br/>hasRetentionPermission || isRoomOwner
Permissions-->>Component: Permissions ready
end
rect rgb(255, 250, 240)
Note over Component,InitialValues: Initial Values Setup
InitialValues->>Subscription: Get room subscription
InitialValues->>InitialValues: Check retentionPolicy exists<br/>(not just enabled)
InitialValues-->>Component: Initial values with retention data
end
rect rgb(245, 255, 245)
Note over Component: Display Retention UI
Component->>Component: Show Retention section if<br/>retentionPolicy !== undefined
Component->>Component: Enable editing if<br/>canEditRetentionPolicy
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Hemang360
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/useEditRoomInitialValues.ts (2)
37-40: Consider extracting the room owner check logic to a shared hook.The logic for determining
isRoomOwnerandcanEditRoomRetentionPolicyis duplicated inuseEditRoomPermissions.ts(lines 15, 33-35). Consider extracting this to a shared hook likeuseCanEditRoomRetentionPolicy(room)to follow DRY principles and ensure consistency.Example refactor:
// hooks/useCanEditRoomRetentionPolicy.ts export const useCanEditRoomRetentionPolicy = (room: IRoom) => { const subscription = useRoomSubscription(); const hasRetentionPermission = usePermission('edit-room-retention-policy', room._id); const isRoomOwner = Boolean(subscription?.roles?.includes('owner')); return hasRetentionPermission || isRoomOwner; };Then use it in both files:
-const subscription = useRoomSubscription(); -const hasRetentionPermission = usePermission('edit-room-retention-policy', room._id); -const isRoomOwner = Boolean(subscription?.roles?.includes('owner')); -const canEditRoomRetentionPolicy = hasRetentionPermission || isRoomOwner; +const canEditRoomRetentionPolicy = useCanEditRoomRetentionPolicy(room);
84-86: Minor: Redundant dependency in memoization array.Line 85 includes
subscriptionin the dependency array, but line 84 already includescanEditRoomRetentionPolicy, which is derived fromsubscription. While not incorrect, includingsubscriptionis technically redundant since React will already re-compute whencanEditRoomRetentionPolicychanges.That said, keeping it explicit may improve code clarity, so this is just a minor observation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx(1 hunks)apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/useEditRoomInitialValues.ts(4 hunks)apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/useEditRoomPermissions.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/useEditRoomPermissions.ts (2)
packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)IRoomWithRetentionPolicy(404-413)packages/ui-contexts/src/index.ts (1)
usePermission(55-55)
🔇 Additional comments (3)
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/useEditRoomInitialValues.ts (1)
60-68: LGTM! Guard change correctly enables retention policy editing.The change from
retentionPolicy?.enabledtoretentionPolicy &&is correct and aligns with the PR objective. This allows room owners to see and enable retention policy settings even when the policy exists but is currently disabled.apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/useEditRoomPermissions.ts (1)
15-15: LGTM! Room owner check logic is correct.The implementation correctly allows room owners to edit retention policy alongside users with explicit permission. This addresses the PR objective where room-level "Owner" role wasn't being recognized by room-scoped permission checks.
Note: Code duplication with
useEditRoomInitialValues.ts(lines 37-40) has been flagged separately with a refactoring suggestion.Also applies to: 33-35
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx (1)
232-232: LGTM! Retention policy UI now visible for configuration.The change from
retentionPolicy?.enabledtoretentionPolicy !== undefinedcorrectly allows the retention policy UI to be shown whenever a retention policy object exists, even if currently disabled. This enables room owners to view and enable retention settings, which aligns with the PR objective.
dougfabris
changed the title
2 participants
Proposed changes (including videos or screenshots)
Room owners were unable to modify retention policy settings even when the 'edit-room-retention-policy' permission was assigned to the Owner role in admin settings. This occurred because the permission check used a room-scoped
query (room._id), but "Owner" is a room-level role rather than a global one, causing the scoped permission check to fail.
retentionpolicy.mp4
Issue(s)
Closes #37308
Steps to test or reproduce
Further comments
Summary by CodeRabbit