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
9 changes: 7 additions & 2 deletions src/plugins/workspace/public/application.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ 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';

export const renderCreatorApp = ({ element }: AppMountParameters, services: Services) => {
Expand All @@ -26,10 +27,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 @@ -35,4 +35,5 @@ export interface WorkspaceFormProps {
onSubmit?: (formData: WorkspaceFormSubmitData) => void;
defaultValues?: WorkspaceFormData;
operationType?: WorkspaceOperationType;
restrictedApps?: Set<string>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,43 @@ import { AppNavLinkStatus, DEFAULT_APP_CATEGORIES } from '../../../../../core/pu
import { convertApplicationsToFeaturesOrGroups } from './utils';

describe('convertApplicationsToFeaturesOrGroups', () => {
it('should not filter out restrict Apps', () => {
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,
},
],
new Set(['foo1'])
)
).toEqual([
{
id: 'foo1',
name: 'Foo 1',
},
{
id: 'bar',
name: 'Bar',
},
]);
});

it('should filter out invisible features', () => {
expect(
convertApplicationsToFeaturesOrGroups([
Expand Down
18 changes: 14 additions & 4 deletions src/plugins/workspace/public/components/workspace_form/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,28 @@ export const getNumberOfErrors = (formErrors: WorkspaceFormErrors) => {
export const convertApplicationsToFeaturesOrGroups = (
applications: Array<
Pick<PublicAppInfo, 'id' | 'title' | 'category' | 'navLinkStatus' | 'chromeless'>
>
>,
restrictedApps?: Set<string>
) => {
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 }) =>
const visibleApplications = applications.filter(({ navLinkStatus, chromeless, category, id }) => {
/**
* Restrict apps are apps that can be configured into a workspace, but restrict to access
* because the current workspace didn't have the apps configured, such apps should NOT filter out
*/
if (restrictedApps && restrictedApps.has(id)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to keep the same source in workspace create, update and list page, can we use restrictedApps for all customized features for workspaces. We can remove the check below.

  return (
        navLinkStatus !== AppNavLinkStatus.hidden &&
        !chromeless &&
        !DEFAULT_SELECTED_FEATURES_IDS.includes(id) &&
        category?.id !== DEFAULT_APP_CATEGORIES.management.id
    );

return true;
}

return (
navLinkStatus !== AppNavLinkStatus.hidden &&
!chromeless &&
!DEFAULT_SELECTED_FEATURES_IDS.includes(id) &&
category?.id !== DEFAULT_APP_CATEGORIES.management.id
);
);
});

/**
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,19 @@ export interface WorkspaceFeatureSelectorProps {
>;
selectedFeatures: string[];
onChange: (newFeatures: string[]) => void;
restrictedApps?: Set<string>;
}

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

const handleFeatureChange = useCallback<EuiCheckboxGroupProps['onChange']>(
(featureId) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
applications={applications}
selectedFeatures={formData.features}
onChange={handleFeaturesChange}
restrictedApps={props.restrictedApps}
/>
</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';
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,31 @@ import { EuiPage, EuiPageBody, EuiPageHeader, EuiPageContent } from '@elastic/eu
import { i18n } from '@osd/i18n';
import { WorkspaceAttribute } from 'opensearch-dashboards/public';
import { useObservable } from 'react-use';
import { of } from 'rxjs';
import { BehaviorSubject, of } from 'rxjs';
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';
import { WorkspaceFormData } from '../workspace_form/types';

export interface WorkspaceUpdaterProps {
restrictedApps$?: BehaviorSubject<Set<string>>;
}

function getFormDataFromWorkspace(
currentWorkspace: WorkspaceAttribute | null | undefined
): WorkspaceFormData {
return (currentWorkspace || {}) as WorkspaceFormData;
}

export const WorkspaceUpdater = () => {
export const WorkspaceUpdater = (props: WorkspaceUpdaterProps) => {
const {
services: { application, workspaces, notifications, http, workspaceClient },
} = useOpenSearchDashboards<{ workspaceClient: WorkspaceClient }>();

const currentWorkspace = useObservable(workspaces ? workspaces.currentWorkspace$ : of(null));
const restrictedApps = useObservable(props.restrictedApps$ ?? of(undefined));
const [currentWorkspaceFormData, setCurrentWorkspaceFormData] = useState<WorkspaceFormData>(
getFormDataFromWorkspace(currentWorkspace)
);
Expand Down Expand Up @@ -108,6 +113,7 @@ export const WorkspaceUpdater = () => {
defaultValues={currentWorkspaceFormData}
onSubmit={handleWorkspaceFormSubmit}
operationType={WorkspaceOperationType.Update}
restrictedApps={restrictedApps}
/>
)}
</EuiPageContent>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import React, { useEffect } from 'react';
import { I18nProvider } from '@osd/i18n/react';
import { i18n } from '@osd/i18n';
import { useOpenSearchDashboards } from '../../../opensearch_dashboards_react/public';
import { WorkspaceUpdater } from './workspace_updater';
import { WorkspaceUpdater, WorkspaceUpdaterProps } from './workspace_updater';

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

return (
<I18nProvider>
<WorkspaceUpdater />
<WorkspaceUpdater {...props} />
</I18nProvider>
);
};
24 changes: 22 additions & 2 deletions src/plugins/workspace/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
AppNavLinkStatus,
AppUpdater,
AppStatus,
DEFAULT_APP_CATEGORIES,
} from '../../../core/public';
import {
WORKSPACE_FATAL_ERROR_APP_ID,
Expand All @@ -30,7 +31,11 @@ import { WorkspaceMenu } from './components/workspace_menu/workspace_menu';
import { getWorkspaceColumn } from './components/workspace_column';
import { isAppAccessibleInWorkspace } from './utils';

type WorkspaceAppType = (params: AppMountParameters, services: Services) => () => void;
type WorkspaceAppType = (
params: AppMountParameters,
services: Services,
props: Record<string, any>
) => () => void;

interface WorkspacePluginSetupDeps {
savedObjectsManagement?: SavedObjectsManagementPluginSetup;
Expand All @@ -41,6 +46,7 @@ export class WorkspacePlugin implements Plugin<{}, {}, WorkspacePluginSetupDeps>
private currentWorkspaceSubscription?: Subscription;
private currentWorkspaceIdSubscription?: Subscription;
private appUpdater$ = new BehaviorSubject<AppUpdater>(() => undefined);
private restrictedApps$ = new BehaviorSubject(new Set<string>());
private _changeSavedObjectCurrentWorkspace() {
if (this.coreStart) {
return this.coreStart.workspaces.currentWorkspaceId$.subscribe((currentWorkspaceId) => {
Expand All @@ -65,6 +71,20 @@ export class WorkspacePlugin implements Plugin<{}, {}, WorkspacePluginSetupDeps>
if (isAppAccessibleInWorkspace(app, currentWorkspace)) {
return;
}

if (app.status === AppStatus.inaccessible) {
return;
}

/**
* Restricted apps can be configured into a workspace, but they are not configured by the
* current workspace. Apps of management category can NOT configured into a workspace, so
* needs to be excluded.
*/
if (app.category?.id !== DEFAULT_APP_CATEGORIES.management.id) {
this.restrictedApps$.next(this.restrictedApps$.value.add(app.id));
}

/**
* Change the app to `inaccessible` if it is not configured in the workspace
* If trying to access such app, an "Application Not Found" page will be displayed
Expand Down Expand Up @@ -129,7 +149,7 @@ export class WorkspacePlugin implements Plugin<{}, {}, WorkspacePluginSetupDeps>
workspaceClient,
};

return renderApp(params, services);
return renderApp(params, services, { restrictedApps$: this.restrictedApps$ });
};

// create
Expand Down
Loading