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 apps are missing when updating a workspace #6459

Merged
18 changes: 14 additions & 4 deletions src/plugins/workspace/public/application.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,18 @@ import { WorkspaceFatalError } from './components/workspace_fatal_error';
import { WorkspaceCreatorApp } from './components/workspace_creator_app';
import { WorkspaceUpdaterApp } from './components/workspace_updater_app';
import { WorkspaceListApp } from './components/workspace_list_app';
import { WorkspaceUpdaterProps } from './components/workspace_updater';
import { Services } from './types';
import { WorkspaceCreatorProps } from './components/workspace_creator/workspace_creator';

export const renderCreatorApp = ({ element }: AppMountParameters, services: Services) => {
export const renderCreatorApp = (
{ element }: AppMountParameters,
services: Services,
props: WorkspaceCreatorProps
) => {
ReactDOM.render(
<OpenSearchDashboardsContextProvider services={services}>
<WorkspaceCreatorApp />
<WorkspaceCreatorApp {...props} />
</OpenSearchDashboardsContextProvider>,
element
);
Expand All @@ -26,10 +32,14 @@ export const renderCreatorApp = ({ element }: AppMountParameters, services: Serv
};
};

export const renderUpdaterApp = ({ element }: AppMountParameters, services: Services) => {
export const renderUpdaterApp = (
{ element }: AppMountParameters,
services: Services,
props: WorkspaceUpdaterProps
) => {
ReactDOM.render(
<OpenSearchDashboardsContextProvider services={services}>
<WorkspaceUpdaterApp />
<WorkspaceUpdaterApp {...props} />
</OpenSearchDashboardsContextProvider>,
element
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,21 @@ describe('WorkspaceCreator', () => {
});

it('should not create workspace when name is empty', async () => {
const { getByTestId } = render(<WorkspaceCreator />);
const { getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
fireEvent.click(getByTestId('workspaceForm-bottomBar-createButton'));
expect(workspaceClientCreate).not.toHaveBeenCalled();
});

it('should not create workspace with invalid name', async () => {
const { getByTestId } = render(<WorkspaceCreator />);
const { getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText');
fireEvent.input(nameInput, {
target: { value: '~' },
Expand All @@ -103,7 +111,11 @@ describe('WorkspaceCreator', () => {
});

it('should not create workspace with invalid description', async () => {
const { getByTestId } = render(<WorkspaceCreator />);
const { getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText');
fireEvent.input(nameInput, {
target: { value: 'test workspace name' },
Expand All @@ -116,15 +128,23 @@ describe('WorkspaceCreator', () => {
});

it('cancel create workspace', async () => {
const { findByText, getByTestId } = render(<WorkspaceCreator />);
const { findByText, getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
fireEvent.click(getByTestId('workspaceForm-bottomBar-cancelButton'));
await findByText('Discard changes?');
fireEvent.click(getByTestId('confirmModalConfirmButton'));
expect(navigateToApp).toHaveBeenCalled();
});

it('create workspace with detailed information', async () => {
const { getByTestId } = render(<WorkspaceCreator />);
const { getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText');
fireEvent.input(nameInput, {
target: { value: 'test workspace name' },
Expand Down Expand Up @@ -155,7 +175,11 @@ describe('WorkspaceCreator', () => {

it('create workspace with customized features', async () => {
setHrefSpy.mockReset();
const { getByTestId } = render(<WorkspaceCreator />);
const { getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText');
fireEvent.input(nameInput, {
target: { value: 'test workspace name' },
Expand All @@ -181,7 +205,11 @@ describe('WorkspaceCreator', () => {

it('should show danger toasts after create workspace failed', async () => {
workspaceClientCreate.mockReturnValue({ result: { id: 'failResult' }, success: false });
const { getByTestId } = render(<WorkspaceCreator />);
const { getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText');
fireEvent.input(nameInput, {
target: { value: 'test workspace name' },
Expand All @@ -198,7 +226,11 @@ describe('WorkspaceCreator', () => {
workspaceClientCreate.mockImplementation(async () => {
throw new Error();
});
const { getByTestId } = render(<WorkspaceCreator />);
const { getByTestId } = render(
<WorkspaceCreator
workspaceConfigurableApps$={new BehaviorSubject([...PublicAPPInfoMap.values()])}
/>
);
const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText');
fireEvent.input(nameInput, {
target: { value: 'test workspace name' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,27 @@
import React, { useCallback } from 'react';
import { EuiPage, EuiPageBody, EuiPageHeader, EuiPageContent, EuiSpacer } from '@elastic/eui';
import { i18n } from '@osd/i18n';
import { useObservable } from 'react-use';
import { BehaviorSubject, of } from 'rxjs';

import { PublicAppInfo } from 'opensearch-dashboards/public';
import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
import { WorkspaceForm, WorkspaceFormSubmitData, WorkspaceOperationType } from '../workspace_form';
import { WORKSPACE_OVERVIEW_APP_ID } from '../../../common/constants';
import { formatUrlWithWorkspaceId } from '../../../../../core/public/utils';
import { WorkspaceClient } from '../../workspace_client';

export const WorkspaceCreator = () => {
export interface WorkspaceCreatorProps {
workspaceConfigurableApps$?: BehaviorSubject<PublicAppInfo[]>;
}

export const WorkspaceCreator = (props: WorkspaceCreatorProps) => {
const {
services: { application, notifications, http, workspaceClient },
} = useOpenSearchDashboards<{ workspaceClient: WorkspaceClient }>();
const workspaceConfigurableApps = useObservable(
props.workspaceConfigurableApps$ ?? of(undefined)
);

const handleWorkspaceFormSubmit = useCallback(
async (data: WorkspaceFormSubmitData) => {
Expand Down Expand Up @@ -80,6 +91,7 @@ export const WorkspaceCreator = () => {
application={application}
onSubmit={handleWorkspaceFormSubmit}
operationType={WorkspaceOperationType.Create}
workspaceConfigurableApps={workspaceConfigurableApps}
/>
)}
</EuiPageContent>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import { I18nProvider } from '@osd/i18n/react';
import { i18n } from '@osd/i18n';
import { useOpenSearchDashboards } from '../../../opensearch_dashboards_react/public';
import { WorkspaceCreator } from './workspace_creator';
import { WorkspaceCreatorProps } from './workspace_creator/workspace_creator';

export const WorkspaceCreatorApp = () => {
export const WorkspaceCreatorApp = (props: WorkspaceCreatorProps) => {
const {
services: { chrome },
} = useOpenSearchDashboards();
Expand All @@ -29,7 +30,7 @@ export const WorkspaceCreatorApp = () => {

return (
<I18nProvider>
<WorkspaceCreator />
<WorkspaceCreator {...props} />
</I18nProvider>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import type { WorkspaceOperationType } from './constants';
import type { ApplicationStart } from '../../../../../core/public';
import type { ApplicationStart, PublicAppInfo } from '../../../../../core/public';

export interface WorkspaceFormSubmitData {
name: string;
Expand Down Expand Up @@ -35,4 +35,5 @@ export interface WorkspaceFormProps {
onSubmit?: (formData: WorkspaceFormSubmitData) => void;
defaultValues?: WorkspaceFormData;
operationType?: WorkspaceOperationType;
workspaceConfigurableApps?: PublicAppInfo[];
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,35 +7,6 @@ import { AppNavLinkStatus, DEFAULT_APP_CATEGORIES } from '../../../../../core/pu
import { convertApplicationsToFeaturesOrGroups } from './utils';

describe('convertApplicationsToFeaturesOrGroups', () => {
it('should filter out invisible features', () => {
expect(
convertApplicationsToFeaturesOrGroups([
{ id: 'foo1', title: 'Foo 1', navLinkStatus: AppNavLinkStatus.hidden },
{ id: 'foo2', title: 'Foo 2', navLinkStatus: AppNavLinkStatus.visible, chromeless: true },
{
id: 'foo3',
title: 'Foo 3',
navLinkStatus: AppNavLinkStatus.visible,
category: DEFAULT_APP_CATEGORIES.management,
},
{
id: 'workspace_overview',
title: 'Workspace Overview',
navLinkStatus: AppNavLinkStatus.visible,
},
{
id: 'bar',
title: 'Bar',
navLinkStatus: AppNavLinkStatus.visible,
},
])
).toEqual([
{
id: 'bar',
name: 'Bar',
},
]);
});
it('should group same category applications in same feature group', () => {
expect(
convertApplicationsToFeaturesOrGroups([
Expand Down
17 changes: 2 additions & 15 deletions src/plugins/workspace/public/components/workspace_form/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import {
AppNavLinkStatus,
DEFAULT_APP_CATEGORIES,
PublicAppInfo,
} from '../../../../../core/public';
import { PublicAppInfo } from '../../../../../core/public';
import { DEFAULT_SELECTED_FEATURES_IDS } from '../../../common/constants';

import { WorkspaceFeature, WorkspaceFeatureGroup, WorkspaceFormErrors } from './types';
Expand Down Expand Up @@ -48,23 +44,14 @@ export const convertApplicationsToFeaturesOrGroups = (
) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the applications only need id, title and category. Can we remove navLinkStatus and chromeless?

const UNDEFINED = 'undefined';

// Filter out all hidden applications and management applications and default selected features
const visibleApplications = applications.filter(
({ navLinkStatus, chromeless, category, id }) =>
navLinkStatus !== AppNavLinkStatus.hidden &&
!chromeless &&
!DEFAULT_SELECTED_FEATURES_IDS.includes(id) &&
category?.id !== DEFAULT_APP_CATEGORIES.management.id
);

/**
*
* Convert applications to features map, the map use category label as
* map key and group all same category applications in one array after
* transfer application to feature.
*
**/
const categoryLabel2Features = visibleApplications.reduce<{
const categoryLabel2Features = applications.reduce<{
[key: string]: WorkspaceFeature[];
}>((previousValue, application) => {
const label = application.category?.label || UNDEFINED;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
WorkspaceFeatureSelector,
WorkspaceFeatureSelectorProps,
} from './workspace_feature_selector';
import { AppNavLinkStatus } from '../../../../../core/public';
import { AppNavLinkStatus, AppStatus } from '../../../../../core/public';

const setup = (options?: Partial<WorkspaceFeatureSelectorProps>) => {
const onChangeMock = jest.fn();
Expand All @@ -19,28 +19,36 @@ const setup = (options?: Partial<WorkspaceFeatureSelectorProps>) => {
title: 'App 1',
category: { id: 'category-1', label: 'Category 1' },
navLinkStatus: AppNavLinkStatus.visible,
status: AppStatus.accessible,
appRoute: '/app-1',
},
{
id: 'app-2',
title: 'App 2',
category: { id: 'category-1', label: 'Category 1' },
navLinkStatus: AppNavLinkStatus.visible,
status: AppStatus.accessible,
appRoute: '/app-2',
},
{
id: 'app-3',
title: 'App 3',
category: { id: 'category-2', label: 'Category 2' },
navLinkStatus: AppNavLinkStatus.visible,
status: AppStatus.accessible,
appRoute: '/app-3',
},
{
id: 'app-4',
title: 'App 4',
navLinkStatus: AppNavLinkStatus.visible,
status: AppStatus.accessible,
appRoute: '/app-4',
},
];
const renderResult = render(
<WorkspaceFeatureSelector
applications={applications}
workspaceConfigurableApps={applications}
selectedFeatures={[]}
onChange={onChangeMock}
{...options}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,20 @@ import { PublicAppInfo } from '../../../../../core/public';
import { isWorkspaceFeatureGroup, convertApplicationsToFeaturesOrGroups } from './utils';

export interface WorkspaceFeatureSelectorProps {
applications: Array<
Pick<PublicAppInfo, 'id' | 'title' | 'category' | 'chromeless' | 'navLinkStatus'>
>;
selectedFeatures: string[];
onChange: (newFeatures: string[]) => void;
workspaceConfigurableApps?: PublicAppInfo[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we only need id, title and category for workspaceConfigurableApps here. Could we change to Pick<PublicAppInfo, 'id' | 'title' | 'category'>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I don't see a strong need to turn it into Pick<PublicAppInfo, 'id' | 'title' | 'category'> in this case. I understood that passing minimum set of data to child components is a best practice, but the application which type is PublicAppInfo is provided by the core module, the components expect PublicAppInfo type of data sounds reasonable to me as well.

I'm not holding a strong opinion, what else concerns do you have?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big issue. We can keep current implementation.

}

export const WorkspaceFeatureSelector = ({
applications,
selectedFeatures,
onChange,
workspaceConfigurableApps,
}: WorkspaceFeatureSelectorProps) => {
const featuresOrGroups = useMemo(() => convertApplicationsToFeaturesOrGroups(applications), [
applications,
]);
const featuresOrGroups = useMemo(
() => convertApplicationsToFeaturesOrGroups(workspaceConfigurableApps ?? []),
[workspaceConfigurableApps]
);

const handleFeatureChange = useCallback<EuiCheckboxGroupProps['onChange']>(
(featureId) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
formData,
formErrors,
selectedTab,
applications,
numberOfErrors,
handleFormSubmit,
handleColorChange,
Expand Down Expand Up @@ -133,9 +132,9 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
<EuiHorizontalRule margin="xs" />
<EuiSpacer size="s" />
<WorkspaceFeatureSelector
applications={applications}
selectedFeatures={formData.features}
onChange={handleFeaturesChange}
workspaceConfigurableApps={props.workspaceConfigurableApps}
/>
</EuiPanel>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
* SPDX-License-Identifier: Apache-2.0
*/

export { WorkspaceUpdater } from './workspace_updater';
export { WorkspaceUpdater, WorkspaceUpdaterProps } from './workspace_updater';
Loading
Loading