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

[Multiple DataSource]Add datasource query for DataSourceView component to add label when only pass id #6315

Merged
merged 16 commits into from
Apr 3, 2024
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [BUG][MD]Fix schema for test connection to separate validation based on auth type ([#5997](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5997))
- [Discover] Enable 'Back to Top' Feature in Discover for scrolling to top ([#6008](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6008))
- [Discover] Fix lazy loading of the legacy table from getting stuck ([#6041](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6041))
- [MDS] Add data source query to fetch missing lable for DataSourceView ([#6315](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6315))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it a bug or a new feature/enhancement?

- [BUG][Discover] Allow saved sort from search embeddable to load in Dashboard ([#5934](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5934))
- [osd/std] Add additional recovery from false-positives in handling of long numerals ([#5956](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5956), [#6245](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6245))
- [osd/std] Add fallback mechanism when recovery from false-positives in handling of long numerals fails ([#6253](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6253))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@ export function DataSourceMenu<T>(props: DataSourceMenuProps<T>): ReactElement |
const { componentType, componentConfig } = props;

function renderDataSourceView(config: DataSourceViewConfig): ReactElement | null {
const { activeOption, fullWidth } = config;
const { activeOption, fullWidth, savedObjects, notifications } = config;
return (
<DataSourceView
selectedOption={activeOption && activeOption.length > 0 ? activeOption : undefined}
savedObjectsClient={savedObjects!}
Copy link
Collaborator

Choose a reason for hiding this comment

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

saved object client is not required

notifications={notifications!.toasts}
Copy link
Collaborator

Choose a reason for hiding this comment

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

notification service is not required

fullWidth={fullWidth}
selectedOption={activeOption && activeOption.length > 0 ? activeOption : undefined}
/>
);
}
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,59 @@
import { ShallowWrapper, shallow } from 'enzyme';
import React from 'react';
import { DataSourceView } from './data_source_view';
import { SavedObjectsClientContract } from 'opensearch-dashboards/public';
import { notificationServiceMock } from '../../../../../core/public/mocks';
import { getSingleDataSourceResponse, mockResponseForSavedObjectsCalls } from '../../mocks';
import { render } from '@testing-library/react';

describe('DataSourceView', () => {
let component: ShallowWrapper<any, Readonly<{}>, React.Component<{}, {}, any>>;
let client: SavedObjectsClientContract;
const { toasts } = notificationServiceMock.createStartContract();

beforeEach(() => {
client = {
get: jest.fn().mockResolvedValue([]),
} as any;
mockResponseForSavedObjectsCalls(client, 'get', getSingleDataSourceResponse);
});

it('should render normally with local cluster not hidden', () => {
component = shallow(
<DataSourceView fullWidth={false} selectedOption={[{ id: 'test1', label: 'test1' }]} />
<DataSourceView
savedObjectsClient={client}
notifications={toasts}
fullWidth={false}
selectedOption={[{ id: 'test1', label: 'test1' }]}
/>
);
expect(component).toMatchSnapshot();
expect(toasts.addWarning).toBeCalledTimes(0);
});
it('should show popover when click on button', async () => {
const container = render(
<DataSourceView
savedObjectsClient={client}
notifications={toasts}
fullWidth={false}
selectedOption={[{ id: 'test1', label: 'test1' }]}
/>
);
const button = await container.findByTestId('dataSourceViewContextMenuHeaderLink');
button.click();
expect(container).toMatchSnapshot();
});
it('should call getDataSourceById when only pass id no label', async () => {
component = shallow(
<DataSourceView
savedObjectsClient={client}
notifications={toasts}
fullWidth={false}
selectedOption={[{ id: 'test1' }]}
/>
);
expect(component).toMatchSnapshot();
expect(client.get).toBeCalledWith('data-source', 'test1');
expect(toasts.addWarning).toBeCalledTimes(0);
yujin-emma marked this conversation as resolved.
Show resolved Hide resolved
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@
import React from 'react';
import { i18n } from '@osd/i18n';
import { EuiPopover, EuiButtonEmpty, EuiButtonIcon, EuiContextMenu } from '@elastic/eui';
import { SavedObjectsClientContract, ToastsStart } from 'opensearch-dashboards/public';
import { DataSourceOption } from '../data_source_menu/types';
import { getDataSourceById } from '../utils';

interface DataSourceViewProps {
savedObjectsClient: SavedObjectsClientContract;
Copy link
Collaborator

Choose a reason for hiding this comment

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

savedobjects and notifications should be optional config

notifications: ToastsStart;
fullWidth: boolean;
selectedOption?: DataSourceOption[];
}
Expand All @@ -19,6 +23,8 @@
}

export class DataSourceView extends React.Component<DataSourceViewProps, DataSourceViewState> {
private _isMounted: boolean = false;

constructor(props: DataSourceViewProps) {
super(props);

Expand All @@ -28,6 +34,32 @@
};
}

componentWillUnmount() {
this._isMounted = false;
}
async componentDidMount() {
Comment on lines +38 to +41
Copy link
Member

Choose a reason for hiding this comment

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

just curious: why do we need componentDidMount and componentWillUnmount for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to check the props.selectedOption.label exist, if not, need to call getDataSourceById to get the title back and set this value, since we only have one option, we only need to call at most once before rendering, this is the reason why put a componentDidMount here. As for this._isMounted = false; in componentWillUnmount, this refer to code here: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx#L65

this._isMounted = true;
const selectedOption = this.props.selectedOption;
if (selectedOption && selectedOption.length === 1) {
if (selectedOption[0].id && !selectedOption[0].label) {
yujin-emma marked this conversation as resolved.
Show resolved Hide resolved
const title = (await getDataSourceById(selectedOption[0].id, this.props.savedObjectsClient))
.title;
if (!title) {
this.props.notifications.addWarning(
i18n.translate('dataSource.fetchDataSourceError', {
defaultMessage: `Invalid selectedOption: ${selectedOption[0].id}`,
yujin-emma marked this conversation as resolved.
Show resolved Hide resolved
})
);
} else {
if (!this._isMounted) return;
this.setState({

Check warning on line 55 in src/plugins/data_source_management/public/components/data_source_view/data_source_view.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/data_source_management/public/components/data_source_view/data_source_view.tsx#L55

Added line #L55 was not covered by tests
selectedOption: [{ id: selectedOption[0].id, label: title }],
yujin-emma marked this conversation as resolved.
Show resolved Hide resolved
});
}
}
}
}

onClick() {
this.setState({ ...this.state, isPopoverOpen: !this.state.isPopoverOpen });
}
Expand All @@ -46,10 +78,10 @@
/>
);

let items = [];
let items: Array<{ name: string | undefined; disabled: boolean }> = [];
yujin-emma marked this conversation as resolved.
Show resolved Hide resolved

if (this.props.selectedOption) {
items = this.props.selectedOption.map((option) => {
if (this.state.selectedOption) {
items = this.state.selectedOption.map((option) => {
Comment on lines +83 to +84
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain me this - why are we using this.state for selectedOption now instead of this.props?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we pass the selectedOption from props, there can be some empty field, e.g. label is optional, we will use state to reconstruct this and render with the state one

Copy link
Member

Choose a reason for hiding this comment

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

+1, props should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not only copy the value from props, but also add missing label in the state, therefore, only state.selectedOption contains sufficient information to render, props.selectedOption can miss the label for some plugins

return {
name: option.label,
disabled: true,
Expand Down Expand Up @@ -78,8 +110,8 @@
size="s"
disabled={true}
>
{this.props.selectedOption && this.props.selectedOption.length > 0
? this.props.selectedOption[0].label
{this.state.selectedOption && this.state.selectedOption.length > 0
? this.state.selectedOption[0].label
: ''}
</EuiButtonEmpty>
<EuiPopover
Expand Down
Loading