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] feat: only allow essential use case when creating workspace if all data sources are serverless #7612

Merged
merged 8 commits into from
Aug 12, 2024
2 changes: 2 additions & 0 deletions changelogs/fragments/7612.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Only allow essential use case when creating workspace if all data sources are serverless ([#7612](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7612))
1 change: 1 addition & 0 deletions src/plugins/data_source/common/data_sources/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export enum AuthType {
SigV4 = 'sigv4',
}

// src/plugins/workspace/public/utils.ts Workspace plugin depends on this to do use case limitation.
export enum SigV4ServiceName {
OpenSearch = 'es',
OpenSearchServerless = 'aoss',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/workspace/opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
"opensearchDashboardsReact"
],
"optionalPlugins": ["savedObjectsManagement","management","dataSourceManagement","contentManagement"],
"requiredBundles": ["opensearchDashboardsReact", "home"]
"requiredBundles": ["opensearchDashboardsReact", "home","dataSource"]
}
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ export const WorkspaceDetailForm = (props: WorkspaceFormProps) => {
onChange={handleUseCaseChange}
formErrors={formErrors}
availableUseCases={availableUseCases}
savedObjects={savedObjects}
operationType={operationType}
/>
</FormGroup>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
onChange={handleUseCaseChange}
formErrors={formErrors}
availableUseCases={availableUseCases}
savedObjects={savedObjects}
operationType={operationType}
/>
</EuiPanel>
<EuiSpacer />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,23 @@
*/

import React from 'react';
import { fireEvent, render } from '@testing-library/react';
import { fireEvent, render, waitFor } from '@testing-library/react';
import { WORKSPACE_USE_CASES } from '../../../common/constants';
import { WorkspaceUseCase, WorkspaceUseCaseProps } from './workspace_use_case';
import { WorkspaceFormErrors } from './types';
import { coreMock } from '../../../../../core/public/mocks';
import { WorkspaceOperationType } from './constants';
import { getIsOnlyAllowEssentialUseCase } from '../../utils';

jest.mock('../../utils', () => ({
getIsOnlyAllowEssentialUseCase: jest.fn().mockResolvedValue(false),
}));
const mockCoreStart = coreMock.createStart();

const setup = (options?: Partial<WorkspaceUseCaseProps>) => {
const onChangeMock = jest.fn();
const formErrors: WorkspaceFormErrors = {};
const savedObjects = mockCoreStart.savedObjects;
const renderResult = render(
<WorkspaceUseCase
availableUseCases={[
Expand All @@ -29,6 +38,8 @@ const setup = (options?: Partial<WorkspaceUseCaseProps>) => {
value=""
onChange={onChangeMock}
formErrors={formErrors}
operationType={WorkspaceOperationType.Create}
savedObjects={savedObjects}
{...options}
/>
);
Expand Down Expand Up @@ -63,4 +74,13 @@ describe('WorkspaceUseCase', () => {
fireEvent.click(renderResult.getByText('Observability'));
expect(onChangeMock).not.toHaveBeenCalled();
});
it('should only display essential use case when creating workspace if getIsOnlyAllowEssentialUseCase returns true', async () => {
(getIsOnlyAllowEssentialUseCase as jest.Mock).mockResolvedValue(true);

const { renderResult } = setup();
await waitFor(() => {
expect(renderResult.queryByText('Analytics')).toBeInTheDocument();
expect(renderResult.queryByText('Observability')).not.toBeInTheDocument();
});
});
});
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 } from 'react';
import React, { useCallback, useState, useEffect } from 'react';
import { i18n } from '@osd/i18n';
import {
EuiCheckableCard,
Expand All @@ -17,6 +17,9 @@ import { DEFAULT_NAV_GROUPS } from '../../../../../core/public';
import { WorkspaceUseCase as WorkspaceUseCaseObject } from '../../types';
import { WorkspaceFormErrors } from './types';
import './workspace_use_case.scss';
import type { SavedObjectsStart } from '../../../../../core/public';
import { getIsOnlyAllowEssentialUseCase } from '../../utils';
import { WorkspaceOperationType } from './constants';

interface WorkspaceUseCaseCardProps {
id: string;
Expand Down Expand Up @@ -54,21 +57,49 @@ const WorkspaceUseCaseCard = ({
);
};

type AvailableUseCase = Pick<WorkspaceUseCaseObject, 'id' | 'title' | 'description' | 'systematic'>;

export interface WorkspaceUseCaseProps {
value: string | undefined;
onChange: (newValue: string) => void;
formErrors: WorkspaceFormErrors;
availableUseCases: Array<
Pick<WorkspaceUseCaseObject, 'id' | 'title' | 'description' | 'systematic'>
>;
availableUseCases: AvailableUseCase[];
savedObjects: SavedObjectsStart;
operationType: WorkspaceOperationType;
}

export const WorkspaceUseCase = ({
value,
onChange,
formErrors,
availableUseCases,
savedObjects,
operationType,
}: WorkspaceUseCaseProps) => {
const [displayedUseCases, setDisplayedUseCases] = useState(availableUseCases);
const [isOnlyAllowEssential, setIsOnlyAllowEssential] = useState(false);

useEffect(() => {
if (operationType === WorkspaceOperationType.Create) {
getIsOnlyAllowEssentialUseCase(savedObjects.client).then((result: boolean) => {
setIsOnlyAllowEssential(result);
});
}
}, [savedObjects, operationType]);
Comment on lines +81 to +87
Copy link
Member

Choose a reason for hiding this comment

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

It looks we can simply use useMemo instead of useEffect + useState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here useMemo would be hard to combine with promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I used useEffect + useState to make it clear.

Copy link
Member

Choose a reason for hiding this comment

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

right, that's a promise


useEffect(() => {
let allAvailableUseCases = availableUseCases
.filter((item) => !item.systematic)
.concat(DEFAULT_NAV_GROUPS.all);
// When creating and isOnlyAllowEssential is true, only display essential use case
if (isOnlyAllowEssential && operationType === WorkspaceOperationType.Create) {
allAvailableUseCases = allAvailableUseCases.filter(
(item) => item.id === DEFAULT_NAV_GROUPS.analytics.id
);
}
setDisplayedUseCases(allAvailableUseCases);
}, [availableUseCases, isOnlyAllowEssential, operationType]);
Copy link
Member

Choose a reason for hiding this comment

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

The same here, we can simply use useMemo, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.


return (
<EuiCompressedFormRow
label={i18n.translate('workspace.form.workspaceUseCase.name.label', {
Expand All @@ -79,20 +110,17 @@ export const WorkspaceUseCase = ({
fullWidth
>
<EuiFlexGroup>
{availableUseCases
.filter((item) => !item.systematic)
.concat(DEFAULT_NAV_GROUPS.all)
.map(({ id, title, description }) => (
<EuiFlexItem key={id}>
<WorkspaceUseCaseCard
id={id}
title={title}
description={description}
checked={value === id}
onChange={onChange}
/>
</EuiFlexItem>
))}
{displayedUseCases.map(({ id, title, description }) => (
<EuiFlexItem key={id}>
<WorkspaceUseCaseCard
id={id}
title={title}
description={description}
checked={value === id}
onChange={onChange}
/>
</EuiFlexItem>
))}
</EuiFlexGroup>
</EuiCompressedFormRow>
);
Expand Down
57 changes: 55 additions & 2 deletions src/plugins/workspace/public/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ import {
isEqualWorkspaceUseCase,
USE_CASE_PREFIX,
prependWorkspaceToBreadcrumbs,
getIsOnlyAllowEssentialUseCase,
} from './utils';
import { WorkspaceAvailability } from '../../../core/public';
import { coreMock } from '../../../core/public/mocks';
import { WORKSPACE_DETAIL_APP_ID, WORKSPACE_USE_CASES } from '../common/constants';
import { SigV4ServiceName } from '../../../plugins/data_source/common/data_sources';

const startMock = coreMock.createStart();
const STATIC_USE_CASES = [
Expand Down Expand Up @@ -379,15 +381,16 @@ describe('workspace utils: getDataSourcesList', () => {
{
id: 'id1',
get: () => {
return 'title1';
return 'mock_value';
},
},
],
});
expect(await getDataSourcesList(mockedSavedObjectClient, [])).toStrictEqual([
{
id: 'id1',
title: 'title1',
title: 'mock_value',
auth: 'mock_value',
},
]);
});
Expand All @@ -398,6 +401,56 @@ describe('workspace utils: getDataSourcesList', () => {
});
});

describe('workspace utils: getIsOnlyAllowEssentialUseCase', () => {
const mockedSavedObjectClient = startMock.savedObjects.client;

it('should return true when all data sources are serverless', async () => {
mockedSavedObjectClient.find = jest.fn().mockResolvedValue({
savedObjects: [
{
id: 'id1',
get: () => {
return {
credentials: {
service: SigV4ServiceName.OpenSearchServerless,
},
};
},
},
],
});
expect(await getIsOnlyAllowEssentialUseCase(mockedSavedObjectClient)).toBe(true);
});

it('should return false when not all data sources are serverless', async () => {
mockedSavedObjectClient.find = jest.fn().mockResolvedValue({
savedObjects: [
{
id: 'id1',
get: () => {
return {
credentials: {
service: SigV4ServiceName.OpenSearchServerless,
},
};
},
},
{
id: 'id2',
get: () => {
return {
credentials: {
service: SigV4ServiceName.OpenSearch,
},
};
},
},
],
});
expect(await getIsOnlyAllowEssentialUseCase(mockedSavedObjectClient)).toBe(false);
});
});

describe('workspace utils: convertNavGroupToWorkspaceUseCase', () => {
it('should convert nav group to consistent workspace use case', () => {
expect(
Expand Down
16 changes: 15 additions & 1 deletion src/plugins/workspace/public/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
} from '../../../core/public';
import { DEFAULT_SELECTED_FEATURES_IDS, WORKSPACE_DETAIL_APP_ID } from '../common/constants';
import { WorkspaceUseCase } from './types';
import { SigV4ServiceName } from '../../../plugins/data_source/common/data_sources';

export const USE_CASE_PREFIX = 'use-case-';

Expand Down Expand Up @@ -202,7 +203,7 @@
return client
.find({
type: 'data-source',
fields: ['id', 'title'],
fields: ['id', 'title', 'auth'],
perPage: 10000,
workspaces,
})
Expand All @@ -212,9 +213,11 @@
return objects.map((source) => {
const id = source.id;
const title = source.get('title');
const auth = source.get('auth');
return {
id,
title,
auth,
};
});
} else {
Expand All @@ -223,6 +226,17 @@
});
};

// If all connected data sources are serverless, will only allow to select essential use case.
export const getIsOnlyAllowEssentialUseCase = async (client: SavedObjectsStart['client']) => {
const allDataSources = await getDataSourcesList(client, ['*']);
if (allDataSources.length > 0) {
return allDataSources.every(
(ds) => ds?.auth?.credentials?.service === SigV4ServiceName.OpenSearchServerless
);
}
return false;

Check warning on line 237 in src/plugins/workspace/public/utils.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/workspace/public/utils.ts#L237

Added line #L237 was not covered by tests
raintygao marked this conversation as resolved.
Show resolved Hide resolved
};

export const convertNavGroupToWorkspaceUseCase = ({
id,
title,
Expand Down
Loading