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]Add "All use case" option to workspace form #7318

Merged
merged 8 commits into from
Jul 19, 2024
2 changes: 2 additions & 0 deletions changelogs/fragments/7318.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- [Workspace]Add "All use case" option to workspace form ([#7318](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7318))
Original file line number Diff line number Diff line change
Expand Up @@ -127,4 +127,17 @@ describe('useWorkspaceForm', () => {
})
);
});
it('should update selected use case', () => {
const { renderResult } = setup({
id: 'foo',
name: 'test-workspace-name',
features: ['use-case-observability'],
});

expect(renderResult.result.current.formData.useCase).toBe('observability');
act(() => {
renderResult.result.current.handleUseCaseChange('search');
});
expect(renderResult.result.current.formData.useCase).toBe('search');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import {

import { useApplications } from '../../hooks';
import {
getFirstUseCaseOfFeatureConfigs,
getUseCaseFeatureConfig,
getUseCaseFromFeatureConfig,
isUseCaseFeatureConfig,
} from '../../utils';
import { DataSource } from '../../../common/types';
Expand All @@ -30,8 +30,6 @@ import { WorkspacePermissionItemType } from './constants';

const workspaceHtmlIdGenerator = htmlIdGenerator();

const isNotNull = <T extends unknown>(value: T | null): value is T => !!value;

export const useWorkspaceForm = ({
application,
defaultValues,
Expand All @@ -51,10 +49,9 @@ export const useWorkspaceForm = ({
const [featureConfigs, setFeatureConfigs] = useState(
appendDefaultFeatureIds(defaultValues?.features ?? [])
);
const selectedUseCases = useMemo(
() => featureConfigs.map(getUseCaseFromFeatureConfig).filter(isNotNull),
[featureConfigs]
);
const selectedUseCase = useMemo(() => getFirstUseCaseOfFeatureConfigs(featureConfigs), [
featureConfigs,
]);
const [permissionSettings, setPermissionSettings] = useState<
Array<Pick<WorkspacePermissionSetting, 'id'> & Partial<WorkspacePermissionSetting>>
>(initialPermissionSettingsRef.current);
Expand All @@ -72,7 +69,7 @@ export const useWorkspaceForm = ({
name,
description,
features: featureConfigs,
useCases: selectedUseCases,
useCase: selectedUseCase,
color,
permissionSettings,
selectedDataSources,
Expand All @@ -92,14 +89,14 @@ export const useWorkspaceForm = ({
formIdRef.current = workspaceHtmlIdGenerator();
}

const handleUseCasesChange = useCallback(
(newUseCases: string[]) => {
const handleUseCaseChange = useCallback(
(newUseCase: string) => {
setFeatureConfigs((previousFeatureConfigs) => {
return [
...previousFeatureConfigs.filter(
(featureConfig) => !isUseCaseFeatureConfig(featureConfig)
),
...newUseCases.map((useCaseItem) => getUseCaseFeatureConfig(useCaseItem)),
getUseCaseFeatureConfig(newUseCase),
];
});
},
Expand Down Expand Up @@ -157,7 +154,7 @@ export const useWorkspaceForm = ({
numberOfChanges,
handleFormSubmit,
handleColorChange,
handleUseCasesChange,
handleUseCaseChange,
handleNameInputChange,
setPermissionSettings,
setSelectedDataSources,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export const WorkspaceDetailForm = (props: WorkspaceFormProps) => {
numberOfChanges,
handleFormSubmit,
handleColorChange,
handleUseCasesChange,
handleUseCaseChange,
setPermissionSettings,
handleNameInputChange,
setSelectedDataSources,
Expand Down Expand Up @@ -109,8 +109,8 @@ export const WorkspaceDetailForm = (props: WorkspaceFormProps) => {

<FormGroup title={workspaceUseCaseTitle}>
<WorkspaceUseCase
value={formData.useCases}
onChange={handleUseCasesChange}
value={formData.useCase}
onChange={handleUseCaseChange}
formErrors={formErrors}
availableUseCases={availableUseCases}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
numberOfChanges,
handleFormSubmit,
handleColorChange,
handleUseCasesChange,
handleUseCaseChange,
handleNameInputChange,
setPermissionSettings,
setSelectedDataSources,
Expand Down Expand Up @@ -85,8 +85,8 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
</EuiTitle>
<EuiSpacer size="s" />
<WorkspaceUseCase
value={formData.useCases}
onChange={handleUseCasesChange}
value={formData.useCase}
onChange={handleUseCaseChange}
formErrors={formErrors}
availableUseCases={availableUseCases}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,10 @@ const setup = (options?: Partial<WorkspaceUseCaseProps>) => {
id: 'system-use-case',
title: 'System use case',
description: 'System use case description',
features: [],
systematic: true,
},
]}
value={[]}
value=""
onChange={onChangeMock}
formErrors={formErrors}
{...options}
Expand All @@ -49,19 +48,19 @@ describe('WorkspaceUseCase', () => {
expect(renderResult.getByText('Search')).toBeInTheDocument();
});

it('should call onChange with new added use case', () => {
it('should call onChange with new checked use case', () => {
const { renderResult, onChangeMock } = setup();

expect(onChangeMock).not.toHaveBeenCalled();
fireEvent.click(renderResult.getByText('Observability'));
expect(onChangeMock).toHaveBeenLastCalledWith(['observability']);
expect(onChangeMock).toHaveBeenLastCalledWith('observability');
});

it('should call onChange without removed use case', () => {
const { renderResult, onChangeMock } = setup({ value: ['observability'] });
it('should not call onChange after checked use case clicked', () => {
const { renderResult, onChangeMock } = setup({ value: 'observability' });

expect(onChangeMock).not.toHaveBeenCalled();
fireEvent.click(renderResult.getByText('Observability'));
expect(onChangeMock).toHaveBeenLastCalledWith([]);
expect(onChangeMock).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@
import React, { useCallback } from 'react';
import { i18n } from '@osd/i18n';
import { EuiCheckableCard, EuiFlexGroup, EuiFlexItem, EuiFormRow, EuiText } from '@elastic/eui';

import { DEFAULT_NAV_GROUPS } from '../../../../../core/public';
import { WorkspaceUseCase as WorkspaceUseCaseObject } from '../../types';
import { WorkspaceFormErrors } from './types';
import './workspace_use_case.scss';

import './workspace_use_case.scss';
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate I suppose?


interface WorkspaceUseCaseCardProps {
id: string;
title: string;
Expand All @@ -31,7 +35,7 @@ const WorkspaceUseCaseCard = ({
return (
<EuiCheckableCard
id={id}
checkableType="checkbox"
checkableType="radio"
style={{ height: '100%' }}
label={title}
checked={checked}
Expand All @@ -47,10 +51,12 @@ const WorkspaceUseCaseCard = ({
};

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

export const WorkspaceUseCase = ({
Expand All @@ -61,11 +67,10 @@ export const WorkspaceUseCase = ({
}: WorkspaceUseCaseProps) => {
const handleCardChange = useCallback(
(id: string) => {
if (!value.includes(id)) {
onChange([...value, id]);
if (value === id) {
return;
}
onChange(value.filter((item) => item !== id));
onChange(id);
},
[value, onChange]
);
Expand All @@ -82,13 +87,14 @@ export const WorkspaceUseCase = ({
<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.includes(id)}
checked={!!value?.includes(id)}
onChange={handleCardChange}
/>
</EuiFlexItem>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jest.mock('../delete_workspace_modal', () => ({

function getWrapWorkspaceListInContext(
workspaceList = [
{ id: 'id1', name: 'name1', features: [] },
{ id: 'id1', name: 'name1', features: ['use-case-all'] },
{ id: 'id2', name: 'name2' },
{ id: 'id3', name: 'name3', features: ['use-case-observability'] },
]
Expand Down Expand Up @@ -69,6 +69,7 @@ describe('WorkspaceList', () => {
expect(getByText('name2')).toBeInTheDocument();

// should display use case
expect(getByText('All use case')).toBeInTheDocument();
expect(getByText('Observability')).toBeInTheDocument();
});
it('should be able to apply debounce search after input', async () => {
Expand Down
23 changes: 10 additions & 13 deletions src/plugins/workspace/public/components/workspace_list/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
import useObservable from 'react-use/lib/useObservable';
import { BehaviorSubject, of } from 'rxjs';
import { i18n } from '@osd/i18n';
import { debounce } from '../../../../../core/public';
import { debounce, DEFAULT_NAV_GROUPS } from '../../../../../core/public';
import { WorkspaceAttribute } from '../../../../../core/public';
import { useOpenSearchDashboards } from '../../../../../plugins/opensearch_dashboards_react/public';
import { navigateToWorkspaceDetail } from '../utils/workspace';
Expand All @@ -26,7 +26,7 @@ import { WORKSPACE_CREATE_APP_ID } from '../../../common/constants';

import { cleanWorkspaceId } from '../../../../../core/public';
import { DeleteWorkspaceModal } from '../delete_workspace_modal';
import { getUseCaseFromFeatureConfig } from '../../utils';
import { getFirstUseCaseOfFeatureConfigs, getUseCaseFromFeatureConfig } from '../../utils';
import { WorkspaceUseCase } from '../../types';

const WORKSPACE_LIST_PAGE_DESCRIPTION = i18n.translate('workspace.list.description', {
Expand Down Expand Up @@ -108,17 +108,14 @@ export const WorkspaceList = ({ registeredUseCases$ }: WorkspaceListProps) => {
if (!features || features.length === 0) {
return '';
}
const results: string[] = [];
features.forEach((featureConfig) => {
const useCaseId = getUseCaseFromFeatureConfig(featureConfig);
if (useCaseId) {
const useCase = registeredUseCases?.find(({ id }) => id === useCaseId);
if (useCase) {
results.push(useCase.title);
}
}
});
return results.join(', ');
const useCaseId = getFirstUseCaseOfFeatureConfigs(features);
const useCase =
useCaseId === DEFAULT_NAV_GROUPS.all.id
? DEFAULT_NAV_GROUPS.all
: registeredUseCases?.find(({ id }) => id === useCaseId);
if (useCase) {
return useCase.title;
}
},
},
{
Expand Down
3 changes: 3 additions & 0 deletions src/plugins/workspace/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
NavGroupStatus,
DEFAULT_NAV_GROUPS,
NavGroupType,
ALL_USE_CASE_ID,
} from '../../../core/public';
import {
WORKSPACE_FATAL_ERROR_APP_ID,
Expand All @@ -38,6 +39,7 @@ import { getWorkspaceColumn } from './components/workspace_column';
import { DataSourceManagementPluginSetup } from '../../../plugins/data_source_management/public';
import {
filterWorkspaceConfigurableApps,
getFirstUseCaseOfFeatureConfigs,
isAppAccessibleInWorkspace,
isNavGroupInFeatureConfigs,
} from './utils';
Expand Down Expand Up @@ -122,6 +124,7 @@ export class WorkspacePlugin implements Plugin<{}, {}, WorkspacePluginSetupDeps>
if (
navGroup.type !== NavGroupType.SYSTEM &&
currentWorkspace.features &&
getFirstUseCaseOfFeatureConfigs(currentWorkspace.features) !== ALL_USE_CASE_ID &&
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have a comment on this logic

!isNavGroupInFeatureConfigs(navGroup.id, currentWorkspace.features)
) {
return {
Expand Down
34 changes: 29 additions & 5 deletions src/plugins/workspace/public/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,18 @@ describe('workspace utils: featureMatchesConfig', () => {

it('should match features include by any use cases', () => {
const match = featureMatchesConfig(
['use-case-observability', 'use-case-analytics'],
['use-case-observability', 'use-case-search'],
STATIC_USE_CASES
);
expect(match({ id: 'dashboards' })).toBe(true);
expect(match({ id: 'observability-traces' })).toBe(true);
expect(match({ id: 'alerting' })).toBe(true);

/**
* The searchRelevance is a feature under search use case. Since each workspace only can be a specific use case,
* the feature matches will use first use case to check if features exists. The observability doesn't have
* searchRelevance feature, it will return false.
*/
expect(match({ id: 'searchRelevance' })).toBe(false);
expect(match({ id: 'not-in-any-use-case' })).toBe(false);
});
});
Expand Down Expand Up @@ -309,11 +315,11 @@ describe('workspace utils: filterWorkspaceConfigurableApps', () => {

describe('workspace utils: isFeatureIdInsideUseCase', () => {
it('should return false for invalid use case', () => {
expect(isFeatureIdInsideUseCase('discover', 'use-case-invalid', [])).toBe(false);
expect(isFeatureIdInsideUseCase('discover', 'invalid', [])).toBe(false);
});
it('should return false if feature not in use case', () => {
expect(
isFeatureIdInsideUseCase('discover', 'use-case-foo', [
isFeatureIdInsideUseCase('discover', 'foo', [
{
id: 'foo',
title: 'Foo',
Expand All @@ -325,7 +331,7 @@ describe('workspace utils: isFeatureIdInsideUseCase', () => {
});
it('should return true if feature id exists in use case', () => {
expect(
isFeatureIdInsideUseCase('discover', 'use-case-foo', [
isFeatureIdInsideUseCase('discover', 'foo', [
{
id: 'foo',
title: 'Foo',
Expand All @@ -335,6 +341,24 @@ describe('workspace utils: isFeatureIdInsideUseCase', () => {
])
).toBe(true);
});
it('should return true if feature id exists inside any use case', () => {
expect(
isFeatureIdInsideUseCase('searchRelevance', 'all', [
{
id: 'foo',
title: 'Foo',
description: 'Foo description',
features: ['discover'],
},
{
id: 'bar',
title: 'Bar',
description: 'Bar description',
features: ['searchRelevance'],
},
])
).toBe(true);
});
});

describe('workspace utils: isNavGroupInFeatureConfigs', () => {
Expand Down
Loading
Loading