feat: allow organization members to be assigned multiple groups#1919
Conversation
Router image scan passed✅ No security vulnerabilities found in image: |
…ltiple-groups # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go
…e-part-of-multiple-groups' into wilson/eng-7240-allow-users-to-be-part-of-multiple-groups
…ltiple-groups # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go # controlplane/src/core/bufservices/organization/leaveOrganization.ts # controlplane/src/core/repositories/OrganizationRepository.ts
…ltiple-groups # Conflicts: # controlplane/test/organization-groups.test.ts
…ltiple-groups # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go
WalkthroughThis change set migrates the organization invitation and member group management system from supporting a single group per invitation/member to supporting multiple groups. It updates protobuf definitions, database schema, repositories, service logic, and UI components to handle arrays of group IDs instead of single group IDs. Test suites and utility functions are refactored accordingly, and audit log types/actions are expanded for group membership changes. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (2)controlplane/src/core/repositories/OrganizationGroupRepository.ts (1)controlplane/test/organization-groups.test.ts (1)🧬 Code Graph Analysis (1)controlplane/src/core/repositories/OrganizationGroupRepository.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
🔇 Additional comments (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
controlplane/src/core/repositories/OrganizationRepository.ts (1)
1011-1031: Address the empty groups behavior and consider transaction safety.The early return when
groups.length === 0prevents users from being removed from all groups. Based on past review comments, this seems to be a point of discussion - should users be allowed to have zero groups?Additionally, consider wrapping the delete and insert operations in a transaction to ensure atomicity.
public async updateMemberGroups(input: { orgMemberID: string; groups: string[] }) { - if (input.groups.length === 0) { - // Prevent updating the groups if no new groups were provided - return; - } + return this.db.transaction(async (tx) => { + await tx + .delete(schema.organizationGroupMembers) + .where(eq(schema.organizationGroupMembers.organizationMemberId, input.orgMemberID)); - await this.db - .delete(schema.organizationGroupMembers) - .where(eq(schema.organizationGroupMembers.organizationMemberId, input.orgMemberID)); - - await this.db - .insert(schema.organizationGroupMembers) - .values( - input.groups.map((groupId) => ({ - organizationMemberId: input.orgMemberID, - groupId, - })), - ) - .onConflictDoNothing() - .execute(); + if (input.groups.length > 0) { + await tx + .insert(schema.organizationGroupMembers) + .values( + input.groups.map((groupId) => ({ + organizationMemberId: input.orgMemberID, + groupId, + })), + ) + .onConflictDoNothing() + .execute(); + } + }); }controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
233-237: Verify groups exist before adding user to organizationThe code adds the organization member before checking if invitation groups exist. This could leave the member without any groups if the invitation has no groups.
Consider checking groups before adding the member:
+const invitationGroups = await this.getPendingInvitationGroups(invitation[0].id); +if (invitationGroups.length === 0) { + throw new Error('Invitation must have at least one group'); +} + const insertedMember = await orgRepo.addOrganizationMember({ userID: input.userId, organizationID: input.organizationId, }); -const invitationGroups = await this.getPendingInvitationGroups(invitation[0].id); -if (invitationGroups.length === 0) { - return; -}
🧹 Nitpick comments (4)
proto/wg/cosmo/platform/v1/platform.proto (1)
1366-1366: Fix field naming convention.The field name
orgMemberUserIDshould follow snake_case convention as per protobuf style guidelines.Apply this diff to fix the naming convention:
- string orgMemberUserID = 1; + string org_member_user_id = 1;controlplane/migrations/0125_acoustic_jackpot.sql (1)
28-30: Improve readability by separating index creation statements.The index creation logic is correct, but the formatting could be improved for better readability.
-CREATE INDEX IF NOT EXISTS "org_inv_invitation_idx" ON "organization_invitation_groups" USING btree ("invitation_id");--> statement-breakpoint -CREATE INDEX IF NOT EXISTS "org_inv_group_id" ON "organization_invitation_groups" USING btree ("group_id");--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "org_inv_invitation_idx" ON "organization_invitation_groups" USING btree ("invitation_id"); +--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "org_inv_group_id" ON "organization_invitation_groups" USING btree ("group_id"); +--> statement-breakpointstudio/src/components/multi-group-select.tsx (1)
88-103: Potential performance issue with array operations in onSelectThe current implementation creates a new array from the Set and then maps over it to find groups, which could be inefficient for large group lists. Consider using a Map for O(1) lookups.
Consider optimizing the group lookup:
+const groupsMap = new Map(availableGroups.map(g => [g.groupId, g])); onSelect={() => { const currentValue = new Set(value); if (currentValue.has(group.groupId)) { currentValue.delete(group.groupId); } else { currentValue.add(group.groupId); } onValueChange( currentValue.size === 0 ? [] - : Array.from(currentValue) - .map((id) => availableGroups.find((group) => group.groupId === id)!) - .filter((group) => !!group) + : Array.from(currentValue) + .map((id) => groupsMap.get(id)) + .filter((group): group is OrganizationGroup => !!group) ); }}controlplane/src/core/bufservices/user/updateOrgMemberGroup.ts (1)
133-134: Consider using more descriptive variable namesThe variable names
groupsToAddToandgroupsToRemoveFromcould be more concise while maintaining clarity.-const groupsToAddTo = newGroups.difference(existingGroups); -const groupsToRemoveFrom = existingGroups.difference(newGroups); +const groupsToAdd = newGroups.difference(existingGroups); +const groupsToRemove = existingGroups.difference(newGroups);Also update the usage in lines 136, 146, and 177.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (29)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(4 hunks)controlplane/migrations/0125_acoustic_jackpot.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/bin/migrate-groups.ts(1 hunks)controlplane/src/core/bufservices/user/acceptOrDeclineInvitation.ts(1 hunks)controlplane/src/core/bufservices/user/inviteUser.ts(3 hunks)controlplane/src/core/bufservices/user/updateOrgMemberGroup.ts(2 hunks)controlplane/src/core/repositories/OrganizationGroupRepository.ts(1 hunks)controlplane/src/core/repositories/OrganizationInvitationRepository.ts(6 hunks)controlplane/src/core/repositories/OrganizationRepository.ts(1 hunks)controlplane/src/db/models.ts(2 hunks)controlplane/src/db/schema.ts(1 hunks)controlplane/src/types/index.ts(1 hunks)controlplane/test/delete-user.test.ts(1 hunks)controlplane/test/organization-groups.test.ts(13 hunks)controlplane/test/organization/leave-organization.test.ts(2 hunks)controlplane/test/test-util.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(2 hunks)studio/src/components/audit-log-table.tsx(1 hunks)studio/src/components/group-select.tsx(2 hunks)studio/src/components/layout/dashboard-layout.tsx(1 hunks)studio/src/components/member-groups/delete-group-dialog.tsx(1 hunks)studio/src/components/member-groups/group-resource-selector.tsx(1 hunks)studio/src/components/member-groups/group-rule-builder.tsx(1 hunks)studio/src/components/members/update-member-group-dialog.tsx(2 hunks)studio/src/components/multi-group-select.tsx(1 hunks)studio/src/hooks/use-check-user-access.ts(1 hunks)studio/src/pages/[organizationSlug]/apikeys.tsx(2 hunks)studio/src/pages/[organizationSlug]/members.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
studio/src/hooks/use-check-user-access.ts (1)
studio/src/lib/constants.ts (1)
roles(215-276)
controlplane/test/organization-groups.test.ts (4)
controlplane/src/core/test-util.ts (1)
genID(53-55)controlplane/test/test-util.ts (2)
createOrganizationGroup(425-457)SetupTest(66-391)controlplane/src/db/schema.ts (1)
users(1062-1068)controlplane/src/core/repositories/OrganizationGroupRepository.ts (1)
OrganizationGroupRepository(9-343)
studio/src/pages/[organizationSlug]/members.tsx (1)
studio/src/components/multi-group-select.tsx (1)
MultiGroupSelect(12-119)
🪛 Buf (1.54.0)
proto/wg/cosmo/platform/v1/platform.proto
1366-1366: Field name "orgMemberUserID" should be lower_snake_case, such as "org_member_user_id".
(FIELD_LOWER_SNAKE_CASE)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (34)
studio/src/components/member-groups/group-resource-selector.tsx (1)
8-8: LGTM! Import path updated correctly.The import path adjustment for
PopoverContentWithScrollableContentreflects the component reorganization and maintains consistency with similar changes across the codebase.controlplane/migrations/meta/_journal.json (1)
879-886: Migration entry properly tracked.The new migration entry for multi-group invitation support follows the established format and incremental indexing pattern correctly.
studio/src/components/member-groups/group-rule-builder.tsx (1)
15-15: LGTM! Consistent import path update.The import path change aligns with the component reorganization seen in other member-groups components, maintaining consistency across the codebase.
studio/src/components/member-groups/delete-group-dialog.tsx (1)
151-151: LGTM! Prop naming standardized.The change from
onGroupChangetoonValueChangestandardizes the event handler naming convention across components while maintaining the same callback functionality.studio/src/components/layout/dashboard-layout.tsx (1)
154-154: LGTM! Role-based navigation separator enhancement.The conditional separator addition improves navigation visual grouping by showing a separator after Feature Flags for non-admin/developer users, following the established pattern of role-based UI customization.
controlplane/test/delete-user.test.ts (1)
238-241: LGTM - Test correctly updated for multi-group API.The test properly adapts to the new
groupsarray parameter while maintaining the same test logic. The single group ID is now wrapped in an array, which aligns with the multi-group support changes.studio/src/components/group-select.tsx (1)
8-8: LGTM - Consistent prop naming improvement.The renaming from
onGroupChangetoonValueChangeimproves consistency with React component conventions. All usages are updated correctly and the functionality remains unchanged.Also applies to: 13-13, 51-51
controlplane/test/organization/leave-organization.test.ts (2)
1-1: LGTM - Good import cleanup.Removing unused imports improves code maintainability and reduces clutter.
Also applies to: 3-4
43-43: LGTM - Test correctly updated for multi-group API.The test properly adapts to the new
groupsarray parameter while preserving the original test logic of assigning a single group to the member.studio/src/components/audit-log-table.tsx (1)
88-92: LGTM - Proper audit log action support added.The new conditional branches correctly handle the
member_group.addedandmember_group.removedaudit actions with appropriate descriptive text. The implementation follows the existing pattern and enhances audit log readability for group membership changes.controlplane/src/types/index.ts (1)
282-282: ✅ OrganizationInvitationDTO updated for multi-group support
- The interface now requires
groups: { groupId: string; kcGroupId: string | null }[], replacing the old optional singlegroupId?.- Backend handlers (inviteUser), repository methods, and tests have been updated to build and persist multiple invitation groups.
- Breaking change: all consumers of this endpoint (e.g. frontend calls to inviteUser) must be updated to send an array of group IDs (
req.groups: string[]) instead of a singlegroupId.studio/src/hooks/use-check-user-access.ts (1)
28-30: Good performance improvement using Set for role checking.The change from Array to Set improves the lookup performance from O(n) to O(1), which is particularly beneficial when checking against multiple roles. The logic correctly maintains the same functionality while being more efficient.
studio/src/pages/[organizationSlug]/apikeys.tsx (1)
253-257: Consistent prop naming across GroupSelect usages.The callback prop rename from
onGroupChangetoonValueChangemaintains consistency with the GroupSelect component's interface changes and standardizes the API across all usages.Also applies to: 628-628
controlplane/src/db/models.ts (1)
91-92: Appropriate audit log types for multi-group membership tracking.The addition of generic
'added'and'removed'actions, along with specific'member_group.added'and'member_group.removed'full actions, provides comprehensive audit logging for the new multi-group membership feature.Also applies to: 151-152
controlplane/src/core/bufservices/user/inviteUser.ts (1)
85-102: Well-implemented multi-group invitation logic.The implementation correctly:
- Validates each group exists within the organization
- Collects validated group IDs for the invitation
- Ensures at least one group is provided for new invitations
- Maintains backward compatibility for re-invitations
The error handling and validation flow are appropriate for the multi-group feature.
Also applies to: 168-175, 204-204
proto/wg/cosmo/platform/v1/platform.proto (1)
999-999: LGTM: Multi-group support correctly implemented.The change from single
groupIdtorepeated string groupsproperly enables inviting users to multiple groups simultaneously while maintaining backward compatibility through proper field numbering.controlplane/src/bin/migrate-groups.ts (1)
426-450: LGTM: Migration correctly implements multi-group data model.The migration properly transitions from the single
groupIdcolumn to the many-to-manyorganizationInvitationGroupsjoin table. The two-step approach (query pending invitations, then insert join records) maintains data integrity while preserving the original business logic of assigning pending invitations to the developer group.controlplane/test/test-util.ts (1)
425-457: LGTM! Well-structured test utility function.The function properly handles group creation with optional rules, includes appropriate validation with expect statements, and follows good testing practices.
studio/src/components/members/update-member-group-dialog.tsx (1)
15-15: LGTM! Comprehensive update for multi-group support.The component has been properly updated to support multiple group selection:
- State management correctly handles arrays
- Effect properly initializes from member props
- Validation logic updated for array length
- Mutation input correctly sends array of group IDs
- All user-facing text updated to plural forms
Also applies to: 18-18, 26-31, 36-36, 43-43, 49-49, 57-57, 64-64, 84-86, 91-95, 100-100
connect/src/wg/cosmo/platform/v1/platform_pb.ts (4)
7445-7447: LGTM: Correct implementation of repeated groups field.The migration from single group to repeated groups is implemented correctly with proper TypeScript array typing and protobuf field definition.
7458-7458: LGTM: Proper protobuf field definition for repeated groups.The field definition correctly specifies the repeated string type for the groups field while maintaining the same field number (2), which preserves backward compatibility.
10477-10478: No discrepancies in UpdateOrgMemberGroupRequest schema
The generated TS fields (orgMemberUserID = 1,groups = 2) exactly match the .proto definition for UpdateOrgMemberGroupRequest. No renumbering or evolution issues detected.
10460-10462: No backward-incompatible changes in UpdateOrgMemberGroupRequest field numbersOur search shows that in
proto/wg/cosmo/platform/v1/platform.protothe message is defined as:• orgMemberUserID = 1
• groups = 2and there is no record of these numbers having been reassigned or a prior field removed. Existing clients should continue to work without change.
File:
- proto/wg/cosmo/platform/v1/platform.proto (lines 1366–1367)
controlplane/migrations/0125_acoustic_jackpot.sql (3)
1-5: LGTM - Clean join table structure.The new join table properly implements the many-to-many relationship between invitations and groups with appropriate UUID types and constraints.
7-11: LGTM - Proper data migration with NULL handling.The migration correctly transfers existing single group associations to the new join table while properly filtering out NULL values to prevent constraint violations.
13-26: LGTM - Proper constraint management with appropriate cascade behavior.The foreign key constraints are correctly configured with cascade deletes, and the duplicate constraint error handling ensures the migration is idempotent.
controlplane/src/db/schema.ts (1)
1541-1558: LGTM - Well-structured join table with proper relationships.The new
organizationInvitationGroupstable correctly implements the many-to-many relationship with appropriate cascade delete behavior and indexes on foreign keys. The schema aligns perfectly with the migration file.studio/src/pages/[organizationSlug]/members.tsx (5)
70-70: LGTM - Correct import for multi-group selection.The import of
MultiGroupSelectcomponent is properly added to support the new multi-group functionality.
74-74: LGTM - Proper schema validation for multiple groups.The schema correctly validates an array of UUID strings with a minimum of one group, ensuring users must select at least one group when inviting members.
92-92: LGTM - Correct form state watching for groups array.The form properly watches the
groupsarray field to track selected groups.
104-104: LGTM - Mutation correctly sends groups array.The mutation call properly sends the
groupsarray to the backend API, aligning with the multi-group invitation functionality.
136-144: LGTM - Proper MultiGroupSelect integration.The component is correctly integrated with proper value binding and form state updates. The mapping from group objects to group IDs maintains proper form state management.
controlplane/test/organization-groups.test.ts (1)
411-478: Comprehensive test for multiple group membershipExcellent test coverage for the multiple group membership feature, including verification of both database state and Keycloak synchronization.
controlplane/src/core/repositories/OrganizationInvitationRepository.ts (1)
196-204: Consider batch insert optimizationGood use of batch insert for invitation groups. This is more efficient than individual inserts.
wilsonrivera
deleted the
wilson/eng-7240-allow-users-to-be-part-of-multiple-groups
branch
2 participants
Motivation and Context
This PR introduces a way for organizations to assign members to multiple groups. This way organizations aren't forced to create different groups just to remove or add a subset of permissions.
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores