[Workspace] Add privacy levels to the workspace #8907
Conversation
Signed-off-by: Kapian1234 <[email protected]>
…ch-Dashboards into workspace_privacy_settings
Signed-off-by: Kapian1234 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8907 +/- ##
==========================================
+ Coverage 60.88% 60.89% +0.01%
==========================================
Files 3802 3805 +3
Lines 91070 91143 +73
Branches 14374 14392 +18
==========================================
+ Hits 55444 55501 +57
- Misses 32085 32097 +12
- Partials 3541 3545 +4
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: Kapian1234 <[email protected]>
…enSearch-Dashboards into workspace_privacy_settings
…ch-Dashboards into workspace_privacy_settings
Signed-off-by: Kapian1234 <[email protected]>
| /> | ||
| </EuiPanel> | ||
| <EuiFlexGroup direction="column"> | ||
| <EuiFlexItem> |
There was a problem hiding this comment.
Any reason to use flex item here? I think can be replaced with EuiSpacer
There was a problem hiding this comment.
<WorkspaceCollaboratorPrivacySettingPanel
permissionSettings={permissionSettings}
handleSubmitPermissionSettings={handleSubmitPermissionSettings}
/>
<EuiSpacer />
<WorkspaceCollaboratorTable
permissionSettings={permissionSettings}
displayedCollaboratorTypes={displayedCollaboratorTypes}
handleSubmitPermissionSettings={handleSubmitPermissionSettings}
/>
it would be like this if not using flex
There was a problem hiding this comment.
Try to wrap with a regular div.
<div>
<WorkspaceCollaboratorPrivacySettingPanel
permissionSettings={permissionSettings}
handleSubmitPermissionSettings={handleSubmitPermissionSettings}
/>
<EuiSpacer />
<EuiPanel>
<WorkspaceCollaboratorTable
permissionSettings={permissionSettings}
displayedCollaboratorTypes={displayedCollaboratorTypes}
handleSubmitPermissionSettings={handleSubmitPermissionSettings}
/>
</EuiPanel>
</div>
It won't using a horizontal layout.
There was a problem hiding this comment.
BTW, the Edit button seems not aligned with "Workspace privacy" title. Shall we fix it?
There was a problem hiding this comment.
alignItems="baseline" would fix it. Thanks for reminder!
| const modes = permissionSettings.find( | ||
| (item) => item.type === WorkspacePermissionItemType.User && item.userId === '*' | ||
| )?.modes; | ||
| if (modes) { |
There was a problem hiding this comment.
I'm prefer to separate an util function to convert permission settings to privacy type and , can be refactor like code below:
if(modes?.includes(WorkspacePermissionMode.LibraryWrite)){
return WorkspacePrivacyItemType.AnyoneCanEdit;
}
if(modes?.includes(WorkspacePermissionMode.LibraryRead)){
return WorkspacePrivacyItemType.AnyoneCanView;
}
It would be more accurate to me. The privacyType would be "PrivateToCollaborators" if modes doesn't contain any LibraryWrite or LibraryRead.
| defaultMessage: 'Workspace Privacy', | ||
| })} | ||
| > | ||
| <EuiSuperSelect |
There was a problem hiding this comment.
How about separate a WorkspacePrivacySettingSelect to share with collaborators and details page?
| }), | ||
| }; | ||
|
|
||
| const handleChange = async () => { |
There was a problem hiding this comment.
Prefer separate an util function (generatePermissionSettingsWithPrivacyType) to do this convert. The whole function can be refactor with code below:
const generatePermissionSettingsWithPrivacyType = (permissionSettings, privacyType)=>{
const newSettings = permissionSettings.filter(
(item) =>
!(item.type === WorkspacePermissionItemType.User && item.userId === "*")
);
if (
privacyType === WorkspacePrivacyItemType.AnyoneCanView ||
privacyType === WorkspacePrivacyItemType.AnyoneCanEdit
) {
newSettings.push({
id: generateNextPermissionSettingsId(permissionSettings),
type: WorkspacePermissionItemType.User,
userId: "*",
modes:
optionIdToWorkspacePermissionModesMap[
privacyType === WorkspacePrivacyItemType.AnyoneCanView
? PermissionModeId.Read
: PermissionModeId.ReadAndWrite
],
});
}
return newSettings;
}
let result;
try {
result = await handleSubmitPermissionSettings(
generatePermissionSettingsWithPrivacyType(permissionSettings, selectedPrivacyType) as WorkspacePermissionSetting[]
);
}catch(error){
notifications?.toasts?.addError();
return;
}
if (result?.success) {
notifications?.toasts?.addSuccess(successfulMessage);
}| (item) => !(item.type === WorkspacePermissionItemType.User && item.userId === '*') | ||
| ); | ||
| newSettings.push({ | ||
| id: newSettings.length, |
There was a problem hiding this comment.
Prefer using generateNextPermissionSettingsId to generate next id.
| const [privacyType, setPrivacyType] = useState(WorkspacePrivacyItemType.PrivateToCollaborators); | ||
| const workspaceAdmin = permissionSettings[0]; | ||
|
|
||
| const options = [ |
There was a problem hiding this comment.
Can be refactor with
[WorkspacePrivacyItemType.PrivateToCollaborators, WorkspacePrivacyItemType.AnyoneCanView, WorkspacePrivacyItemType.AnyoneCanEdit].map((id)=>(
{
id,
title: privacyType2CopyMap[value].title,
description: privacyType2CopyMap[value].description,
}))| goToCollaborators, | ||
| onGoToCollaboratorsChange, | ||
| }: WorkspacePrivacySettingProps) => { | ||
| const [privacyType, setPrivacyType] = useState(WorkspacePrivacyItemType.PrivateToCollaborators); |
There was a problem hiding this comment.
Can we move privacyType to useWorkspaceForm, then we can share with create and detail page. Then we don't need to pass permissionSettings and onPermissionChange in this component. And the privacyType should calculate from permissionSettings in the initial rendering.
| defaultMessage: 'Workspace privacy', | ||
| })} | ||
| // isInvalid={!!formErrors.features} | ||
| // error={formErrors.features?.message} |
| label={i18n.translate('workspace.form.privacy.name.label', { | ||
| defaultMessage: 'Workspace privacy', | ||
| })} | ||
| // isInvalid={!!formErrors.features} |
| }, | ||
| ]; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Can be removed after move privacyType to useWorkspaceForm
| @@ -145,7 +146,7 @@ export const WorkspaceCreator = (props: WorkspaceCreatorProps) => { | |||
| ?.features[0].id; | |||
| // Redirect page after one second, leave one second time to show create successful toast. | |||
| window.setTimeout(() => { | |||
| if (isPermissionEnabled) { | |||
| if (isPermissionEnabled && goToCollaborators) { | |||
There was a problem hiding this comment.
Should fix the should redirect to workspace setting collaborators page if permission enabled in src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx unit test failed due to this change.
| @@ -110,6 +114,15 @@ export const WorkspaceCreatorForm = (props: WorkspaceCreatorFormProps) => { | |||
| </EuiPanel> | |||
| </> | |||
| )} | |||
| <EuiSpacer size="m" /> | |||
| {isDashboardAdmin && ( | |||
| <WorkspacePrivacySettingPanel | |||
There was a problem hiding this comment.
I think we should add permission enabled check here.
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
| initialFormData.permissionSettings && | ||
| convertPermissionsToPrivacyType(newFormData.permissionSettings) !== | ||
| convertPermissionsToPrivacyType(initialFormData.permissionSettings) | ||
| ) { |
There was a problem hiding this comment.
This number of changes will be failed when permissionSettings not exists. How about refactor with code below:
convertPermissionsToPrivacyType(newFormData.permissionSettings ?? []) !==
convertPermissionsToPrivacyType(initialFormData.permissionSettings ?? [])…collaborators page Signed-off-by: Kapian1234 <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Description
Issues Resolved
Screenshot
Workspace create
Collaborators
Workspace details
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration