-
Notifications
You must be signed in to change notification settings - Fork 920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Workspace]Add WorkspaceCollaboratorTypesService and AddCollaboratorsModal #8486
[Workspace]Add WorkspaceCollaboratorTypesService and AddCollaboratorsModal #8486
Conversation
Signed-off-by: Lin Wang <wonglam@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8486 +/- ##
=======================================
Coverage 60.94% 60.95%
=======================================
Files 3760 3766 +6
Lines 89301 89359 +58
Branches 13969 13977 +8
=======================================
+ Hits 54426 54468 +42
- Misses 31480 31493 +13
- Partials 3395 3398 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Lin Wang <wonglam@amazon.com>
export interface WorkspaceCollaboratorType { | ||
id: string; | ||
name: string; | ||
pluralName: string; | ||
// Will be assigned to this type in the final ACL permissions object | ||
permissionSettingType: WorkspaceCollaboratorPermissionSettingType; | ||
modal: { | ||
title: string; | ||
description?: string; | ||
// Will use name with ID suffix for fallback | ||
inputLabel?: string; | ||
inputDescription?: string; | ||
// Will use name with ID suffix for fallback | ||
inputPlaceholder?: string; | ||
}; | ||
instruction?: { | ||
title: string; | ||
detail: string; | ||
link?: string; | ||
}; | ||
// To match wether passed collaborator match this collaborator type | ||
collaboratorMatcher?: (collaborator: { | ||
type: WorkspaceCollaboratorPermissionSettingType; | ||
userOrGroupId: string; | ||
}) => boolean; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export interface WorkspaceCollaboratorType { | |
id: string; | |
name: string; | |
pluralName: string; | |
// Will be assigned to this type in the final ACL permissions object | |
permissionSettingType: WorkspaceCollaboratorPermissionSettingType; | |
modal: { | |
title: string; | |
description?: string; | |
// Will use name with ID suffix for fallback | |
inputLabel?: string; | |
inputDescription?: string; | |
// Will use name with ID suffix for fallback | |
inputPlaceholder?: string; | |
}; | |
instruction?: { | |
title: string; | |
detail: string; | |
link?: string; | |
}; | |
// To match wether passed collaborator match this collaborator type | |
collaboratorMatcher?: (collaborator: { | |
type: WorkspaceCollaboratorPermissionSettingType; | |
userOrGroupId: string; | |
}) => boolean; | |
} | |
export interface WorkspaceCollaboratorType { | |
id: string; | |
name: string; | |
onAdd: () => any | |
} | |
// `onAdd` function will be called when the dropdown menu been clicked | |
// It will show a modal, for example something like | |
{ | |
onAdd: () => coreStart.overlays.openModal(mount(<AddCollaboratorModal permissionType={} title="" {...} />)) | |
} |
This way, we maximize the flexibility of how the add collaborator modal should be look like, at the same time, it keeps the API simple. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess collaboratorMatcher
is needed either way because it will be used to populate the type column in collaborator table.
If we are gonna to use onAdd, I guess we need to add some callback functions like
onAdd: (props: { onChange: (value: valueTypeForCollaborator) => void }) => ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SuZhou-Joe what will onChange
do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think valueTypeForCollaborator
should be an array. Will refactor code like this and include the AddCollaboratorModal in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use a modal to maximize the flexibility, we need a callback to tell the parent component that user has added a user id. Or I do not know how we can communicate with parent component.
src/plugins/workspace/public/services/workspace_collaborator_types_service.ts
Show resolved
Hide resolved
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Hi @ruanyl @SuZhou-Joe , I've refactor code base our discussions. Could you help me take a look? Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick updates! It looks good to me, I left a few minor comments
...plugins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.test.tsx
Show resolved
Hide resolved
</> | ||
)} | ||
{collaborators.map((item, index) => ( | ||
<React.Fragment key={item.id}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this React.Fragment
is unnecessary
overlayRef?.close(); | ||
}} | ||
onAddCollaborators={async (collaborators) => { | ||
await onAddCollaborators(collaborators); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we try/catch here? perhaps could be a future improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do after catch an error? Do we need to prevent the modal close? Add a try catch would be better, but it's up to the collaborator type register. I think we can added it when we need it.
setCollaboratorTypes: (types: WorkspaceCollaboratorType[]) => { | ||
this.collaboratorTypes.setTypes(types); | ||
}, | ||
getAddCollaboratorsModal: () => AddCollaboratorsModal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we follow this pattern? It looks when plugin expose components, it exposes under ui
property
getAddCollaboratorsModal: () => AddCollaboratorsModal, | |
ui: { AddCollaboratorsModal }, |
setCollaboratorTypes: (types: WorkspaceCollaboratorType[]) => { | ||
this.collaboratorTypes.setTypes(types); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can expose the entire collaboratorTypes service.
setCollaboratorTypes: (types: WorkspaceCollaboratorType[]) => { | |
this.collaboratorTypes.setTypes(types); | |
}, | |
collaboratorTypes: this.collaboratorTypes; | |
}, |
…l/add_collaborators_modal.test.tsx Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Description
This PR is for adding
WorkspaceCollaboratorTypesService
to allow set customized collaborator types. These types will be used in the workspace settings refactor PR. It allow customized different UI when adding collaborators in workspace detail page. This PR added asetCollaboratorTypes
method in workspace setup return value to customize the collaborator using interface below.Here are some examples to register collaborator types:
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/0ea1c3462f20099b4f02c61c7092d2118262f8ea/src/plugins/workspace/public/register_default_collaborator_types.tsx
Screenshot
AddCollaboratorsModal for adding user
AddCollaboratorsModal for adding group
Testing the changes
Can refer new added testing cases in files below:
Changelog
Check List
yarn test:jest
yarn test:jest_integration