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.

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] Fix workspace update issue #8570

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8570.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Fix workspace update issue ([#8570](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8570))
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export const WorkspaceDetail = (props: WorkspaceDetailPropsWithFormSubmitting) =
// When user has unsaved changes and navigates to other page, will show a confirm modal.
useEffect(() => {
onAppLeave((actions) => {
if (!props.isFormSubmitting && isEditing && numberOfChanges > 0) {
if (isEditing && numberOfChanges > 0) {
return actions.confirm(
i18n.translate('workspace.detail.navigate.message', {
defaultMessage: 'Any unsaved changes will be lost.',
Expand All @@ -151,7 +151,7 @@ export const WorkspaceDetail = (props: WorkspaceDetailPropsWithFormSubmitting) =
}
return actions.default();
});
}, [handleResetForm, isEditing, numberOfChanges, onAppLeave, props.isFormSubmitting]);
}, [handleResetForm, isEditing, numberOfChanges, onAppLeave]);

const handleSetDefaultWorkspace = useCallback(
async (workspace: WorkspaceAttribute) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export const WorkspaceDetailApp = (props: WorkspaceDetailPropsWithOnAppLeave) =>
}, [currentWorkspace, savedObjects, http, notifications]);

const handleWorkspaceFormSubmit = useCallback(
async (data: WorkspaceFormSubmitData, refresh?: boolean) => {
async (data: WorkspaceFormSubmitData) => {
let result;
if (isFormSubmitting) {
return;
Expand Down Expand Up @@ -121,28 +121,15 @@ export const WorkspaceDetailApp = (props: WorkspaceDetailPropsWithOnAppLeave) =>
dataSources: selectedDataSourceIds,
permissions: convertPermissionSettingsToPermissions(permissionSettings),
});
setIsFormSubmitting(false);
if (result?.success) {
notifications?.toasts.addSuccess({
title: i18n.translate('workspace.update.success', {
defaultMessage: 'Update workspace successfully',
}),
});
setIsFormSubmitting(false);
if (application && http && refresh) {
// Redirect page after one second, leave one second time to show update successful toast.
window.setTimeout(() => {
window.location.href = formatUrlWithWorkspaceId(
application.getUrlForApp(WORKSPACE_DETAIL_APP_ID, {
absolute: true,
}),
currentWorkspace.id,
http.basePath
);
}, 1000);
}
return result;
} else {
setIsFormSubmitting(false);
throw new Error(result?.error ? result?.error : 'update workspace failed');
}
} catch (error) {
Expand All @@ -156,7 +143,7 @@ export const WorkspaceDetailApp = (props: WorkspaceDetailPropsWithOnAppLeave) =>
return;
}
},
[isFormSubmitting, currentWorkspace, notifications?.toasts, workspaceClient, application, http]
[isFormSubmitting, currentWorkspace, notifications?.toasts, workspaceClient]
);

if (!workspaces || !application || !http || !savedObjects || !currentWorkspaceFormData) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,7 @@ describe('useWorkspaceForm', () => {
expect.objectContaining({
name: 'test-workspace-name',
features: ['use-case-observability'],
}),
true
})
);
});
it('should update selected use case', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,19 @@
[setFeatureConfigs]
);

const getSubmitFormData = (submitFormData: WorkspaceFormDataState) => {
return {
name: submitFormData.name!,
description: submitFormData.description,
color: submitFormData.color || '#FFFFFF',
features: submitFormData.features,
permissionSettings: submitFormData.permissionSettings as WorkspacePermissionSetting[],
selectedDataSourceConnections: submitFormData.selectedDataSourceConnections,
};
};
Comment on lines +92 to +101
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like the return value is same as WorkspaceFormDataState? can we use WorkspaceFormDataState diectly? or {...WorkspaceFormDataState}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there are some differences between the type of return values and WorkspaceFormDataState, such as the useCase attribute.


const handleFormSubmit = useCallback<FormEventHandler>(
(e) => {
async (e) => {
e.preventDefault();
const currentFormData = getFormDataRef.current();
currentFormData.permissionSettings = currentFormData.permissionSettings.filter(
Expand All @@ -107,34 +118,22 @@
return;
}

onSubmit?.(
{
name: currentFormData.name!,
description: currentFormData.description,
color: currentFormData.color || '#FFFFFF',
features: currentFormData.features,
permissionSettings: currentFormData.permissionSettings as WorkspacePermissionSetting[],
selectedDataSourceConnections: currentFormData.selectedDataSourceConnections,
},
true
);
const submitFormData = getSubmitFormData(currentFormData);
const result = await onSubmit?.(submitFormData);
if (result?.success) {
defaultValuesRef.current = submitFormData;
setIsEditing(false);

Check warning on line 125 in src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts#L124-L125

Added lines #L124 - L125 were not covered by tests
}
},
[onSubmit, permissionEnabled]
);

const handleSubmitPermissionSettings = async (settings: WorkspacePermissionSetting[]) => {
const currentFormData = getFormDataRef.current();
const result = await onSubmit?.(
{
name: currentFormData.name!,
description: currentFormData.description,
color: currentFormData.color || '#FFFFFF',
features: currentFormData.features,
permissionSettings: settings,
selectedDataSourceConnections: currentFormData.selectedDataSourceConnections,
},
false
);
const result = await onSubmit?.({
...getSubmitFormData(currentFormData),
permissionSettings: settings,
});
if (result) {
setPermissionSettings(settings);
}
Expand All @@ -150,7 +149,6 @@
setDescription(resetValues?.description ?? '');
setColor(resetValues?.color);
setFeatureConfigs(resetValues?.features ?? []);
setPermissionSettings(defaultValuesRef.current?.permissionSettings ?? []);
setFormErrors({});
setIsEditing(false);
}, []);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -553,79 +553,6 @@ describe('getNumberOfChanges', () => {
)
).toEqual(1);
});
it('should return consistent permission settings changes count', () => {
expect(
getNumberOfChanges(
{
name: 'foo',
permissionSettings: [
{
id: 0,
type: WorkspacePermissionItemType.User,
userId: 'user-1',
modes: [WorkspacePermissionMode.Write, WorkspacePermissionMode.LibraryWrite],
},
],
},
{
name: 'foo',
permissionSettings: [
{
id: 0,
type: WorkspacePermissionItemType.User,
userId: 'user-1',
modes: [WorkspacePermissionMode.Write, WorkspacePermissionMode.LibraryWrite],
},
],
}
)
).toEqual(0);
// for remove permission setting
expect(
getNumberOfChanges(
{
name: 'foo',
permissionSettings: [
{
id: 0,
type: WorkspacePermissionItemType.User,
userId: 'user-1',
modes: [WorkspacePermissionMode.Write, WorkspacePermissionMode.LibraryWrite],
},
{
id: 1,
type: WorkspacePermissionItemType.Group,
group: 'group-1',
modes: [WorkspacePermissionMode.Write, WorkspacePermissionMode.LibraryWrite],
},
],
},
{
name: 'foo',
/**
* These include three changes:
* 1.Remove permission setting#0
* 2.Modify permission setting#1
* 3.Add permission setting#2
*/
permissionSettings: [
{
id: 1,
type: WorkspacePermissionItemType.Group,
group: 'group-1',
modes: [WorkspacePermissionMode.Read, WorkspacePermissionMode.LibraryWrite],
},
{
id: 2,
type: WorkspacePermissionItemType.User,
userId: 'user-1',
modes: [WorkspacePermissionMode.Write, WorkspacePermissionMode.LibraryWrite],
},
],
}
)
).toEqual(3);
});
});

describe('isWorkspacePermissionSetting', () => {
Expand Down
20 changes: 0 additions & 20 deletions src/plugins/workspace/public/components/workspace_form/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -446,25 +446,5 @@ export const getNumberOfChanges = (
) {
count++;
}
// Count all new added permission settings
count +=
newFormData.permissionSettings?.reduce((prevNewAddedCount, setting) => {
if (!initialFormData.permissionSettings?.find((item) => item.id === setting.id)) {
prevNewAddedCount += 1;
}
return prevNewAddedCount;
}, 0) ?? 0;
count +=
initialFormData.permissionSettings?.reduce((prevDeletedAndModifiedCount, setting) => {
const newSetting = newFormData.permissionSettings?.find((item) => item.id === setting.id);
if (!newSetting) {
// Count all delete permission settings
prevDeletedAndModifiedCount += 1;
} else if (!isSamePermissionSetting(newSetting, setting)) {
// Count all modified permission settings
prevDeletedAndModifiedCount += 1;
}
return prevDeletedAndModifiedCount;
}, 0) ?? 0;
return count;
};
Loading