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

Make sure customer always have a default datasource #6237

Merged
merged 4 commits into from
Mar 26, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multiple Datasource] Refactor data source menu and interface to allow cleaner selection of component and related configurations ([#6256](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6256))
- [Multiple Datasource] Allow top nav menu to mount data source menu for use case when both menus are mounted ([#6268](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6268))
- [Workspace] Add create workspace page ([#6179](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6179))
- [Multiple Datasource] Make sure customer always have a default datasource ([#6237](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6237))
- [Workspace] Add workspace list page ([#6182](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6182))


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('Datasource Management: Create Datasource Wizard', () => {

test('should create datasource successfully', async () => {
spyOn(utils, 'createSingleDataSource').and.returnValue({});

spyOn(utils, 'handleSetDefaultDatasource').and.returnValue({});
await act(async () => {
// @ts-ignore
await component.find(formIdentifier).first().prop('handleSubmit')(
Expand All @@ -62,6 +62,7 @@ describe('Datasource Management: Create Datasource Wizard', () => {
});
expect(utils.createSingleDataSource).toHaveBeenCalled();
expect(history.push).toBeCalledWith('');
expect(utils.handleSetDefaultDatasource).toHaveBeenCalled();
});

test('should fail to create datasource', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
getDataSources,
testConnection,
fetchDataSourceVersion,
handleSetDefaultDatasource,
} from '../utils';
import { LoadingMask } from '../loading_mask';

Expand All @@ -35,6 +36,7 @@ export const CreateDataSourceWizard: React.FunctionComponent<CreateDataSourceWiz
setBreadcrumbs,
http,
notifications: { toasts },
uiSettings,
} = useOpenSearchDashboards<DataSourceManagementContext>().services;

/* State Variables */
Expand Down Expand Up @@ -76,6 +78,8 @@ export const CreateDataSourceWizard: React.FunctionComponent<CreateDataSourceWiz
const version = await fetchDataSourceVersion(http, attributes);
attributes.dataSourceVersion = version.dataSourceVersion;
await createSingleDataSource(savedObjects.client, attributes);
// Set the first create data source as default data source.
await handleSetDefaultDatasource(savedObjects.client, uiSettings);
props.history.push('');
} catch (e) {
setIsLoading(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ describe('DataSourceTable', () => {

it('should delete confirm modal confirm button work normally', async () => {
spyOn(utils, 'deleteMultipleDataSources').and.returnValue(Promise.resolve({}));

spyOn(utils, 'setFirstDataSourceAsDefault').and.returnValue({});
act(() => {
// @ts-ignore
component.find(tableIdentifier).props().selection.onSelectionChange(getMappedDataSources);
Expand All @@ -143,10 +143,12 @@ describe('DataSourceTable', () => {
});
component.update();
expect(component.find(confirmModalIdentifier).exists()).toBe(false);
expect(utils.setFirstDataSourceAsDefault).toHaveBeenCalled();
});

it('should delete datasources & fail', async () => {
spyOn(utils, 'deleteMultipleDataSources').and.returnValue(Promise.reject({}));
spyOn(utils, 'setFirstDataSourceAsDefault').and.returnValue({});
act(() => {
// @ts-ignore
component.find(tableIdentifier).props().selection.onSelectionChange(getMappedDataSources);
Expand All @@ -162,6 +164,7 @@ describe('DataSourceTable', () => {
});
component.update();
expect(utils.deleteMultipleDataSources).toHaveBeenCalled();
expect(utils.setFirstDataSourceAsDefault).not.toHaveBeenCalled();
// @ts-ignore
expect(component.find(confirmModalIdentifier).exists()).toBe(false);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
} from '../../../../opensearch_dashboards_react/public';
import { DataSourceManagementContext, DataSourceTableItem, ToastMessageItem } from '../../types';
import { CreateButton } from '../create_button';
import { deleteMultipleDataSources, getDataSources } from '../utils';
import { deleteMultipleDataSources, getDataSources, setFirstDataSourceAsDefault } from '../utils';
import { LoadingMask } from '../loading_mask';

/* Table config */
Expand Down Expand Up @@ -232,6 +232,9 @@
// Fetch data sources
fetchDataSources();
setConfirmDeleteVisible(false);
// Check if default data source is deleted or not.
// if yes, then set the first existing datasource as default datasource.
setDefaultDataSource();
})
.catch(() => {
handleDisplayToastMessage({
Expand All @@ -245,6 +248,23 @@
});
};

const setDefaultDataSource = async () => {
try {
for (const dataSource of selectedDataSources) {
if (uiSettings.get('defaultDataSource') === dataSource.id) {
await setFirstDataSourceAsDefault(savedObjects.client, uiSettings, true);
}
}
} catch (e) {
handleDisplayToastMessage({

Check warning on line 259 in src/plugins/data_source_management/public/components/data_source_table/data_source_table.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_source_management/public/components/data_source_table/data_source_table.tsx#L259

Added line #L259 was not covered by tests
id: 'dataSourcesManagement.editDataSource.setDefaultDataSourceFailMsg',
defaultMessage: 'Unable to find a default datasource. Please set a new default datasource.',
});
} finally {
setIsDeleting(false);
}
};

/* Table selection handlers */
const onSelectionChange = (selected: DataSourceTableItem[]) => {
setSelectedDataSources(selected);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ describe('Datasource Management: Edit Datasource Wizard', () => {
});
test('should delete datasource successfully', async () => {
spyOn(utils, 'deleteDataSourceById').and.returnValue({});

spyOn(utils, 'setFirstDataSourceAsDefault').and.returnValue({});
spyOn(uiSettings, 'get').and.returnValue('test1');
await act(async () => {
// @ts-ignore
await component.find(formIdentifier).first().prop('onDeleteDataSource')(
Expand All @@ -147,9 +148,12 @@ describe('Datasource Management: Edit Datasource Wizard', () => {
});
expect(utils.deleteDataSourceById).toHaveBeenCalled();
expect(history.push).toBeCalledWith('');
expect(utils.setFirstDataSourceAsDefault).toHaveBeenCalled();
});
test('should fail to delete datasource', async () => {
spyOn(utils, 'deleteDataSourceById').and.throwError('error');
spyOn(utils, 'setFirstDataSourceAsDefault').and.returnValue({});
spyOn(uiSettings, 'get').and.returnValue('test1');
await act(async () => {
// @ts-ignore
await component.find(formIdentifier).first().prop('onDeleteDataSource')(
Expand All @@ -158,6 +162,7 @@ describe('Datasource Management: Edit Datasource Wizard', () => {
});
component.update();
expect(utils.deleteDataSourceById).toHaveBeenCalled();
expect(utils.setFirstDataSourceAsDefault).not.toHaveBeenCalled();
});
test('should test connection', () => {
spyOn(utils, 'testConnection');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
getDataSources,
testConnection,
updateDataSourceById,
setFirstDataSourceAsDefault,
} from '../utils';
import { getEditBreadcrumbs } from '../breadcrumbs';
import { EditDataSourceForm } from './components/edit_form/edit_data_source_form';
Expand Down Expand Up @@ -109,6 +110,10 @@
setIsLoading(true);
try {
await deleteDataSourceById(props.match.params.id, savedObjects.client);

// If deleted datasource is the default datasource, then set the first existing
// datasource as default datasource.
setDefaultDataSource();
props.history.push('');
} catch (e) {
setIsLoading(false);
Expand All @@ -119,6 +124,20 @@
}
};

const setDefaultDataSource = async () => {
try {
if (uiSettings.get('defaultDataSource') === dataSourceID) {
await setFirstDataSourceAsDefault(savedObjects.client, uiSettings, true);
}
} catch (e) {
setIsLoading(false);
handleDisplayToastMessage({

Check warning on line 134 in src/plugins/data_source_management/public/components/edit_data_source/edit_data_source.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_source_management/public/components/edit_data_source/edit_data_source.tsx#L133-L134

Added lines #L133 - L134 were not covered by tests
id: 'dataSourcesManagement.editDataSource.setDefaultDataSourceFailMsg',
defaultMessage: 'Unable to find a default datasource. Please set a new default datasource.',
});
}
};

/* Handle Test connection */
const handleTestConnection = async (attributes: DataSourceAttributes) => {
await testConnection(http, attributes, dataSourceID);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {
isValidUrl,
testConnection,
updateDataSourceById,
handleSetDefaultDatasource,
setFirstDataSourceAsDefault,
} from './utils';
import { coreMock } from '../../../../core/public/mocks';
import {
Expand All @@ -24,6 +26,8 @@ import {
mockDataSourceAttributesWithAuth,
mockErrorResponseForSavedObjectsCalls,
mockResponseForSavedObjectsCalls,
mockUiSettingsCalls,
getSingleDataSourceResponse,
} from '../mocks';
import {
AuthType,
Expand All @@ -36,6 +40,7 @@ import { AuthenticationMethod, AuthenticationMethodRegistry } from '../auth_regi
import { deepEqual } from 'assert';

const { savedObjects } = coreMock.createStart();
const { uiSettings } = coreMock.createStart();

describe('DataSourceManagement: Utils.ts', () => {
describe('Get data source', () => {
Expand Down Expand Up @@ -274,7 +279,50 @@ describe('DataSourceManagement: Utils.ts', () => {
expect(getDefaultAuthMethod(authenticationMethodRegistry)?.name).toBe(AuthType.NoAuth);
});
});

describe('handle set default datasource', () => {
beforeEach(() => {
jest.clearAllMocks(); // Reset all mock calls before each test
});
test('should set default datasource when it does not have default datasource ', async () => {
mockUiSettingsCalls(uiSettings, 'get', null);
mockResponseForSavedObjectsCalls(savedObjects.client, 'find', getDataSourcesResponse);
await handleSetDefaultDatasource(savedObjects.client, uiSettings);
expect(uiSettings.set).toHaveBeenCalled();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: according to the behavior which set the first data source in the list as default, maybe it's nice to assert that uiSettings.set is called with the first data source id.

expect(uiSettings.set).toHaveBeenCalledWith('defaultDataSource', 'test');

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would do

});
test('should not set default datasource when it has default datasouce', async () => {
mockUiSettingsCalls(uiSettings, 'get', 'test');
mockResponseForSavedObjectsCalls(savedObjects.client, 'find', getDataSourcesResponse);
await handleSetDefaultDatasource(savedObjects.client, uiSettings);
expect(uiSettings.set).not.toHaveBeenCalled();
});
});
describe('set first aataSource as default', () => {
beforeEach(() => {
jest.clearAllMocks(); // Reset all mock calls before each test
});
test('should set defaultDataSource if more than one data source exists', async () => {
mockResponseForSavedObjectsCalls(savedObjects.client, 'find', getDataSourcesResponse);
await setFirstDataSourceAsDefault(savedObjects.client, uiSettings, true);
expect(uiSettings.set).toHaveBeenCalled();
});
test('should set defaultDataSource if only one data source exists', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

For unit test of this function, I think it's worth to add a test to cover the case that when number of data source > 1 that uiSettings.set should not be called.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added!

mockResponseForSavedObjectsCalls(savedObjects.client, 'find', getSingleDataSourceResponse);
await setFirstDataSourceAsDefault(savedObjects.client, uiSettings, true);
expect(uiSettings.set).toHaveBeenCalled();
});
test('should not set defaultDataSource if no data source exists', async () => {
mockResponseForSavedObjectsCalls(savedObjects.client, 'find', { savedObjects: [] });
await setFirstDataSourceAsDefault(savedObjects.client, uiSettings, true);
expect(uiSettings.remove).toHaveBeenCalled();
expect(uiSettings.set).not.toHaveBeenCalled();
});
test('should not set defaultDataSource if no data source exists and no default datasouce', async () => {
mockResponseForSavedObjectsCalls(savedObjects.client, 'find', { savedObjects: [] });
await setFirstDataSourceAsDefault(savedObjects.client, uiSettings, false);
expect(uiSettings.remove).not.toHaveBeenCalled();
expect(uiSettings.set).not.toHaveBeenCalled();
});
});
describe('Check extractRegisteredAuthTypeCredentials method', () => {
test('Should extract credential field successfully', () => {
const authTypeToBeTested = 'Some Auth Type';
Expand Down
31 changes: 30 additions & 1 deletion src/plugins/data_source_management/public/components/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { HttpStart, SavedObjectsClientContract, SavedObject } from 'src/core/public';
import {
HttpStart,
SavedObjectsClientContract,
SavedObject,
IUiSettingsClient,
} from 'src/core/public';
import {
DataSourceAttributes,
DataSourceTableItem,
Expand Down Expand Up @@ -49,6 +54,30 @@ export async function getDataSourcesWithFields(
return response?.savedObjects;
}

export async function handleSetDefaultDatasource(
savedObjectsClient: SavedObjectsClientContract,
uiSettings: IUiSettingsClient
) {
if (uiSettings.get('defaultDataSource', null) === null) {
return await setFirstDataSourceAsDefault(savedObjectsClient, uiSettings, false);
}
}

export async function setFirstDataSourceAsDefault(
savedObjectsClient: SavedObjectsClientContract,
uiSettings: IUiSettingsClient,
exists: boolean
) {
if (exists) {
uiSettings.remove('defaultDataSource');
}
const listOfDataSources: DataSourceTableItem[] = await getDataSources(savedObjectsClient);
if (Array.isArray(listOfDataSources) && listOfDataSources.length >= 1) {
const datasourceId = listOfDataSources[0].id;
return await uiSettings.set('defaultDataSource', datasourceId);
}
}

export async function getDataSourceById(
id: string,
savedObjectsClient: SavedObjectsClientContract
Expand Down
24 changes: 24 additions & 0 deletions src/plugins/data_source_management/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import React from 'react';
import { throwError } from 'rxjs';
import { SavedObjectsClientContract } from 'opensearch-dashboards/public';
import { IUiSettingsClient } from 'src/core/public';
import { AuthType, DataSourceAttributes } from './types';
import { coreMock } from '../../../core/public/mocks';
import {
Expand Down Expand Up @@ -62,6 +63,21 @@ export const mockManagementPlugin = {
docLinks,
};

export const getSingleDataSourceResponse = {
savedObjects: [
{
id: 'test',
type: 'data-source',
description: 'test datasource',
title: 'test',
get(field: string) {
const me: any = this || {};
return me[field];
},
},
],
};

/* Mock data responses - JSON*/
export const getDataSourcesResponse = {
savedObjects: [
Expand Down Expand Up @@ -263,6 +279,14 @@ export const mockErrorResponseForSavedObjectsCalls = (
);
};

export const mockUiSettingsCalls = (
uiSettings: IUiSettingsClient,
uiSettingsMethodName: 'get' | 'set',
response: any
) => {
(uiSettings[uiSettingsMethodName] as jest.Mock).mockReturnValue(response);
};

export interface TestPluginReturn {
setup: DataSourceManagementPluginSetup;
doStart: () => DataSourceManagementPluginStart;
Expand Down
Loading