Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feat remove management permission mode in acl permission #195

Merged
merged 2 commits into from
Sep 25, 2023
Merged

Feat remove management permission mode in acl permission #195

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb5629d
feat: remove management permission mode and clearify library permissi…
wanglam Sep 22, 2023
d8aa1d1
address PR comments and add annotations
wanglam Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/core/utils/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export const WORKSPACE_PATH_PREFIX = '/w';
export enum WorkspacePermissionMode {
Read = 'read',
Write = 'write',
Management = 'management',
LibraryRead = 'library_read',
LibraryWrite = 'library_write',
}
Expand Down
142 changes: 90 additions & 52 deletions ...gins/workspace/public/components/workspace_creator/workspace_permission_setting_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React, { useCallback, useMemo, useRef } from 'react';
import React, { useCallback, useMemo } from 'react';
import {
EuiDescribedFormGroup,
EuiFlexGroup,
Expand All @@ -28,42 +28,55 @@ export type WorkspacePermissionSetting = (
modes: Array<
| WorkspacePermissionMode.LibraryRead
| WorkspacePermissionMode.LibraryWrite
| WorkspacePermissionMode.Management
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should WorkspacePermissionSetting just be

export type WorkspacePermissionSetting =
  | { type: 'user'; userId: string; modes: Array<WorkspacePermissionMode> }
  | { type: 'group'; group: string; modes: Array<WorkspacePermissionMode> };

| WorkspacePermissionMode.Read
| WorkspacePermissionMode.Write
>;
};

enum PermissionModeId {
Read = 'read',
ReadAndWrite = 'read+write',
Admin = 'admin',
}

const permissionModeOptions = [
{
id: WorkspacePermissionMode.LibraryRead,
label: 'View',
id: PermissionModeId.Read,
label: 'Read',
iconType: 'eye',
},
{
id: WorkspacePermissionMode.LibraryWrite,
label: 'Edit',
id: PermissionModeId.ReadAndWrite,
label: 'Read + Write',
iconType: 'pencil',
},
{
id: WorkspacePermissionMode.Management,
id: PermissionModeId.Admin,
label: 'Management',
iconType: 'visTimelion',
},
];

const optionIdToWorkspacePermissionModesMap: {
[key: string]: WorkspacePermissionMode[];
} = {
[PermissionModeId.Read]: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read],
[PermissionModeId.ReadAndWrite]: [
WorkspacePermissionMode.LibraryWrite,
WorkspacePermissionMode.Read,
],
[PermissionModeId.Admin]: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write],
};

const permissionTypeOptions = [
{ value: 'user' as const, inputDisplay: 'User' },
{ value: 'group' as const, inputDisplay: 'Group' },
];

const isWorkspacePermissionMode = (
test: string
): test is
| WorkspacePermissionMode.LibraryRead
| WorkspacePermissionMode.LibraryWrite
| WorkspacePermissionMode.Management =>
test === WorkspacePermissionMode.LibraryRead ||
test === WorkspacePermissionMode.LibraryWrite ||
test === WorkspacePermissionMode.Management;
const generateWorkspacePermissionItemKey = (
item: Partial<WorkspacePermissionSetting>,
index?: number
) => [item.type, item.userId, item.group, ...(item.modes ?? []), index].filter(Boolean).join('-');

interface WorkspacePermissionSettingInputProps {
index: number;
Expand All @@ -74,7 +87,8 @@ interface WorkspacePermissionSettingInputProps {
modes?: Array<
| WorkspacePermissionMode.LibraryRead
| WorkspacePermissionMode.LibraryWrite
| WorkspacePermissionMode.Management
| WorkspacePermissionMode.Read
| WorkspacePermissionMode.Write
>;
onTypeChange: (type: 'user' | 'group', index: number) => void;
onGroupOrUserIdChange: (
Expand All @@ -88,7 +102,8 @@ interface WorkspacePermissionSettingInputProps {
WorkspacePermissionMode: Array<
| WorkspacePermissionMode.LibraryRead
| WorkspacePermissionMode.LibraryWrite
| WorkspacePermissionMode.Management
| WorkspacePermissionMode.Read
| WorkspacePermissionMode.Write
>,
index: number
) => void;
Expand All @@ -111,16 +126,16 @@ const WorkspacePermissionSettingInput = ({
() => (group || userId ? [{ label: (group || userId) as string }] : []),
[group, userId]
);
const permissionModesIdToSelectMap = useMemo(
() => ({
[WorkspacePermissionMode.LibraryRead]: !!modes?.includes(WorkspacePermissionMode.LibraryRead),
[WorkspacePermissionMode.LibraryWrite]: !!modes?.includes(
WorkspacePermissionMode.LibraryWrite
),
[WorkspacePermissionMode.Management]: !!modes?.includes(WorkspacePermissionMode.Management),
}),
[modes]
);
const permissionModesSelectedId = useMemo(() => {
if (!modes) {
return undefined;
}
for (const key in optionIdToWorkspacePermissionModesMap) {
if (optionIdToWorkspacePermissionModesMap[key].every((mode) => modes?.includes(mode))) {
return key;
}
}
}, [modes]);

const handleTypeChange = useCallback(
(newType: 'user' | 'group') => {
Expand Down Expand Up @@ -154,16 +169,13 @@ const WorkspacePermissionSettingInput = ({
[index, type, onGroupOrUserIdChange]
);

const handlePermissionStateChange = useCallback(
const handlePermissionModeOptionChange = useCallback(
(id: string) => {
if (isWorkspacePermissionMode(id)) {
onPermissionModesChange(
modes?.includes(id) ? modes.filter((value) => value !== id) : [...(modes ?? []), id],
index
);
if (optionIdToWorkspacePermissionModesMap[id]) {
onPermissionModesChange([...optionIdToWorkspacePermissionModesMap[id]], index);
}
},
[index, modes, onPermissionModesChange]
[index, onPermissionModesChange]
);

const handleDelete = useCallback(() => {
Expand Down Expand Up @@ -195,11 +207,11 @@ const WorkspacePermissionSettingInput = ({
<EuiFlexItem grow={false}>
<EuiButtonGroup
legend="Permission Modes"
type="multi"
type="single"
options={permissionModeOptions}
onChange={handlePermissionStateChange}
isIconOnly
idToSelectedMap={permissionModesIdToSelectMap}
idSelected={permissionModesSelectedId}
onChange={handlePermissionModeOptionChange}
/>
</EuiFlexItem>
<EuiFlexItem grow={false}>
Expand Down Expand Up @@ -227,63 +239,89 @@ export const WorkspacePermissionSettingPanel = ({
onChange,
firstRowDeletable,
}: WorkspacePermissionSettingPanelProps) => {
const valueRef = useRef(value);
valueRef.current = value;
const transferredValue = useMemo(() => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean transformed?

if (!value) {
return [];
}
const result: Array<Partial<WorkspacePermissionSetting>> = [];
for (let i = 0; i < value.length; i++) {
const valueItem = value[i];
if (
!valueItem.modes ||
!valueItem.type ||
(valueItem.type === 'user' && !valueItem.userId) ||
(valueItem.type === 'group' && !valueItem.group)
) {
result.push(valueItem);
continue;
}
for (const key in optionIdToWorkspacePermissionModesMap) {
if (!Object.prototype.hasOwnProperty.call(optionIdToWorkspacePermissionModesMap, key)) {
continue;
}
const modesForCertainPermissionId = optionIdToWorkspacePermissionModesMap[key];
if (modesForCertainPermissionId.every((mode) => valueItem.modes?.includes(mode))) {
result.push({ ...valueItem, modes: modesForCertainPermissionId });
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help me to understand this piece of code? from L247-L267

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The passed permission records were based user & group level. One user or group will contains multi permission modes. Since each permission mode in the UI was single select, we can't display multi permission option in one row. So we need to separate these permission modes into multi rows. Will add some annotation to clarify the logic.

return result;
}, [value]);

const handleAddNewOne = useCallback(() => {
onChange?.([...(valueRef.current ?? []), {}]);
}, [onChange]);
onChange?.([...(transferredValue ?? []), {}]);
}, [onChange, transferredValue]);

const handleDelete = useCallback(
(index: number) => {
onChange?.((valueRef.current ?? []).filter((_item, itemIndex) => itemIndex !== index));
onChange?.((transferredValue ?? []).filter((_item, itemIndex) => itemIndex !== index));
},
[onChange]
[onChange, transferredValue]
);

const handlePermissionModesChange = useCallback<
WorkspacePermissionSettingInputProps['onPermissionModesChange']
>(
(modes, index) => {
onChange?.(
(valueRef.current ?? []).map((item, itemIndex) =>
(transferredValue ?? []).map((item, itemIndex) =>
index === itemIndex ? { ...item, modes } : item
)
);
},
[onChange]
[onChange, transferredValue]
);

const handleTypeChange = useCallback<WorkspacePermissionSettingInputProps['onTypeChange']>(
(type, index) => {
onChange?.(
(valueRef.current ?? []).map((item, itemIndex) =>
(transferredValue ?? []).map((item, itemIndex) =>
index === itemIndex ? { ...item, type } : item
)
);
},
[onChange]
[onChange, transferredValue]
);

const handleGroupOrUserIdChange = useCallback<
WorkspacePermissionSettingInputProps['onGroupOrUserIdChange']
>(
(userOrGroupIdWithType, index) => {
onChange?.(
(valueRef.current ?? []).map((item, itemIndex) =>
(transferredValue ?? []).map((item, itemIndex) =>
index === itemIndex
? { ...userOrGroupIdWithType, ...(item.modes ? { modes: item.modes } : {}) }
: item
)
);
},
[onChange]
[onChange, transferredValue]
);

return (
<EuiDescribedFormGroup title={<h3>Users, User Groups & Groups</h3>}>
{value?.map((item, index) => (
<React.Fragment key={index}>
{transferredValue?.map((item, index) => (
<React.Fragment key={generateWorkspacePermissionItemKey(item, index)}>
<EuiFormRow isInvalid={!!errors?.[index]} error={errors?.[index]}>
<WorkspacePermissionSettingInput
{...item}
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/workspace/public/workspace_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ type WorkspaceRoutePermissionItem = {
modes: Array<
| WorkspacePermissionMode.LibraryRead
| WorkspacePermissionMode.LibraryWrite
| WorkspacePermissionMode.Management
| WorkspacePermissionMode.Read
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be like this?

modes: WorkspacePermissionMode[]

| WorkspacePermissionMode.Write
>;
} & ({ type: 'user'; userId: string } | { type: 'group'; group: string });

Expand Down
10 changes: 8 additions & 2 deletions src/plugins/workspace/server/routes/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ export const WORKSPACES_API_BASE_URL = '/api/workspaces';
const workspacePermissionMode = schema.oneOf([
schema.literal(WorkspacePermissionMode.LibraryRead),
schema.literal(WorkspacePermissionMode.LibraryWrite),
schema.literal(WorkspacePermissionMode.Management),
schema.literal(WorkspacePermissionMode.Read),
schema.literal(WorkspacePermissionMode.Write),
]);

const workspacePermission = schema.oneOf([
Expand Down Expand Up @@ -184,7 +185,12 @@ export function registerRoutes({
permissions.push({
type: 'user',
userId: authInfo.user_name,
modes: [WorkspacePermissionMode.Management],
modes: [WorkspacePermissionMode.LibraryWrite],
});
permissions.push({
type: 'user',
userId: authInfo.user_name,
modes: [WorkspacePermissionMode.Write],
});
}

Expand Down
Loading
Loading