Import members feature for Workspace and MCP creation #265
Conversation
Amazing! 🚀 This still looks a bit off. |
|
|
||
| <ImportMembersDialog | ||
| open={isImportDialogOpen} | ||
| workspaceName={workspaceName} |
There was a problem hiding this comment.
Could you make the workspace/project name also visible in the dropdown when choosing if you want to import from a Project or Workspace?
Think this would make it cristal clear to the end-user which roles he is inheriting right now.
| () => | ||
| z | ||
| .object({ | ||
| parentType: z.union([z.literal('Workspace'), z.literal('Project'), z.literal('')]), |
There was a problem hiding this comment.
I do not think we need the last "empty" option here, no?
There was a problem hiding this comment.
With the change from the two checkboxes to the three steps (All / Users / ServiceAccounts), I removed zod from this component because there can't be any invalid state anymore.
| setValue('parentType', selected, { shouldValidate: true, shouldDirty: true, shouldTouch: true }); | ||
| }} | ||
| > | ||
| {!!workspaceName && <Option value="Workspace">{t('Entities.Workspace')}</Option>} |
There was a problem hiding this comment.
I am confused by the logic, we are checking if there is a workspace name but then use the Option set to "Project". I would expect the value to be "Workspace" in this case.
There was a problem hiding this comment.
I think this is correct. workspaceName is of type string | undefined. The logical operators work as follows:
!workspaceNameis of typebooleanand istrueifworkspaceNameis falsy (i.e. eitherundefinedor an empty string).!!workspaceNamejust reverses this and istrueifworkspaceNameis truthy (defined and non-empty).
So:
- The first
<Option value="Workspace" …>is rendered only when there is aworkspaceName - The second
<Option value="Project" …>is always rendered.
I changed the props of the component to make projectName a required string (which cannot be undefined) as our Import component requires this value to function properly.
| } | ||
|
|
||
| const membersData = parentResourceData?.spec?.members ?? []; | ||
| const mockedMembers: Member[] = membersData.map(({ name, namespace, kind, roles }) => ({ |
There was a problem hiding this comment.
What is this mockedMembers is it mixing a unit test into this? I feel like there should be no cleaning necessary before no? Maybe just confusing naming.
There was a problem hiding this comment.
This is probably a leftover from testing locally, I've removed it
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an import members feature that allows users to reuse member assignments from existing projects and workspaces when creating new workspaces or managed control planes (MCPs). The feature provides a dialog interface to select and import members, promoting consistency in member assignments across resources.
- Adds an import dialog component for browsing and selecting members from parent resources
- Updates the EditMembers component to include import functionality with filtering options
- Enhances the useApiResource hook to support conditional API calls
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/api/useApiResource.ts |
Adds optional disable parameter for conditional API calls |
src/lib/api/types/crate/resourceObject.ts |
Adds generic type parameter for improved type safety |
src/components/Members/ImportMembersDialog.tsx |
New dialog component for importing members from projects/workspaces |
src/components/Members/EditMembers.tsx |
Integrates import functionality with reuse button and member merging logic |
src/components/Members/MemberTable.tsx |
Updates column headers and consolidates edit/delete buttons |
| Various CSS and localization files | Styling and text updates for the import feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Added your feedback (@Lasserich ) with the following changes:
|
5 participants
What this PR does / why we need it:
Import members feature for Workspace and MCP creation that makes it easy to be consistent with assigning users to the resources.