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.

Sign up for GitHub

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

[VisBuilder] fixes filters for table visualisation #3210

Merged
merged 4 commits into from
Jan 9, 2023
+112 −52
Merged

[VisBuilder] fixes filters for table visualisation #3210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b89121e
fixes table vis for filter aggregation types
ashwin-pc Dec 16, 2022
cec1ae9
Fixes filter and add error boundry
ashwin-pc Jan 7, 2023
2e89d95
Merge branch 'main' into fix/visbuilder-table
ashwin-pc Jan 7, 2023
baa610c
Adds changelog
ashwin-pc Jan 7, 2023
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
5 changes: 3 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multi DataSource] Address UX comments on index pattern management stack ([#2611](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2611))
- [Multi DataSource] Apply get indices error handling in step index pattern ([#2652](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2652))
- [Vis Builder] Last Updated Timestamp for visbuilder savedobject is getting Generated ([#2628](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2628))
- [Vis Builder] fixes filters for table visualisation ([#3210](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3210))
- Removed Leftover X Pack references ([#2638](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2638))
- Removes Add Integration button ([#2723](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2723))
- Change geckodriver version to make consistency ([#2772](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2772))
Expand All @@ -81,7 +82,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Build] Fixed "Last Access Time" not being set by `scanCopy` on Windows ([#2964](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2964))
- [Vis Builder] Add global data persistence for vis builder #2896 ([#2896](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2896))
- Update `leaflet-vega` and fix its usage ([#3005](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3005))
- [Table Visualization][BUG] Fix Url content display issue in table ([#2918](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2918))
- [Table Visualization][bug] Fix Url content display issue in table ([#2918](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2918))
- Fixes misleading embaddable plugin error message ([#3043](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3043))
- [MD] Update dummy url in tests to follow lychee url allowlist ([#3099](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3099))
- Adds config override to fix obsolete theme:version config value of v8 (beta) rendering issue ([#3045](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3045))
Expand Down Expand Up @@ -148,7 +149,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [CVE-2022-37601] Bump loader-utils to 2.0.3 ([#2689](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2689))
- [CVE-2022-37599] Bump loader-utils to 2.0.4 ([#3031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3031))
- [CVE-2022-37603] Bump loader-utils to 2.0.4 ([#3031](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3031))
- [WS-2021-0638][Security] bump mocha to 10.1.0 ([#2711](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2711))
- [WS-2021-0638][security] bump mocha to 10.1.0 ([#2711](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2711))

### 📈 Features/Enhancements

Expand Down
150 changes: 102 additions & 48 deletions src/plugins/vis_builder/public/application/components/data_tab/secondary_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,24 @@
* SPDX-License-Identifier: Apache-2.0
*/

import React, { useCallback, useMemo, useState } from 'react';
import React, { Component, useCallback, useMemo, useState } from 'react';
import { cloneDeep, get } from 'lodash';
import { useDebounce } from 'react-use';
import { i18n } from '@osd/i18n';
import { EuiCallOut } from '@elastic/eui';
import { useTypedDispatch, useTypedSelector } from '../../utils/state_management';
import { DefaultEditorAggParams } from '../../../../../vis_default_editor/public';
import { Title } from './title';
import { useIndexPatterns, useVisualizationType } from '../../utils/use';
import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public';
import {
OpenSearchDashboardsContextProvider,
useOpenSearchDashboards,
} from '../../../../../opensearch_dashboards_react/public';
import { VisBuilderServices } from '../../../types';
import { AggParam, IAggType, IFieldParamType } from '../../../../../data/public';
import { saveDraftAgg, editDraftAgg } from '../../utils/state_management/visualization_slice';
import { setValidity } from '../../utils/state_management/metadata_slice';
import { Storage } from '../../../../../opensearch_dashboards_utils/public';

const EDITOR_KEY = 'CONFIG_PANEL';

Expand All @@ -27,13 +33,12 @@ export function SecondaryPanel() {
const dispatch = useTypedDispatch();
const vizType = useVisualizationType();
const indexPattern = useIndexPatterns().selected;
const { services } = useOpenSearchDashboards<VisBuilderServices>();
const {
services: {
data: {
search: { aggs: aggService },
},
data: {
search: { aggs: aggService },
},
} = useOpenSearchDashboards<VisBuilderServices>();
} = services;
const schemas = vizType.ui.containerConfig.data.schemas.all;

const aggConfigs = useMemo(() => {
Expand Down Expand Up @@ -98,49 +103,98 @@ export function SecondaryPanel() {
<div className="vbConfig__section vbConfig--secondary">
<Title title={selectedSchema?.title ?? 'Edit'} isSecondary closeMenu={closeMenu} />
{showAggParamEditor && (
<DefaultEditorAggParams
className="vbConfig__aggEditor"
agg={aggConfig!}
indexPattern={indexPattern!}
setValidity={handleSetValid}
setTouched={setTouched}
schemas={schemas}
formIsTouched={touched}
groupName={selectedSchema?.group ?? 'none'}
metricAggs={metricAggs}
state={{
data: {},
description: '',
title: '',
<OpenSearchDashboardsContextProvider
services={{
...services,
storage: new Storage(window.localStorage), // This is necessary for filters
}}
setAggParamValue={function <T extends string | number | symbol>(
aggId: string,
paramName: T,
value: any
): void {
aggConfig.params[paramName] = value;
dispatch(editDraftAgg(aggConfig.serialize()));
}}
onAggTypeChange={function (aggId: string, aggType: IAggType): void {
aggConfig.type = aggType;

// Persist field if the new agg type supports the existing field
const fieldParam = (aggType.params as AggParam[]).find(({ type }) => type === 'field');
if (fieldParam) {
const availableFields = (fieldParam as IFieldParamType).getAvailableFields(aggConfig);
const indexField = availableFields.find(
({ name }) => name === get(draftAgg, 'params.field')
);

if (indexField) {
aggConfig.params.field = indexField;
}
}

dispatch(editDraftAgg(aggConfig.serialize()));
}}
/>
>
<EditorErrorBoundary>
<DefaultEditorAggParams
className="vbConfig__aggEditor"
agg={aggConfig!}
indexPattern={indexPattern!}
setValidity={handleSetValid}
setTouched={setTouched}
schemas={schemas}
formIsTouched={touched}
groupName={selectedSchema?.group ?? 'none'}
metricAggs={metricAggs}
state={{
data: {},
description: '',
title: '',
}}
setAggParamValue={function <T extends string | number | symbol>(
aggId: string,
paramName: T,
value: any
): void {
aggConfig.params[paramName] = value;
dispatch(editDraftAgg(aggConfig.serialize()));
}}
onAggTypeChange={function (aggId: string, aggType: IAggType): void {
aggConfig.type = aggType;

// Persist field if the new agg type supports the existing field
const fieldParam = (aggType.params as AggParam[]).find(
({ type }) => type === 'field'
);
if (fieldParam) {
const availableFields = (fieldParam as IFieldParamType).getAvailableFields(
aggConfig
);
const indexField = availableFields.find(
({ name }) => name === get(draftAgg, 'params.field')
);

if (indexField) {
aggConfig.params.field = indexField;
}
}

dispatch(editDraftAgg(aggConfig.serialize()));
}}
/>
</EditorErrorBoundary>
</OpenSearchDashboardsContextProvider>
)}
</div>
);
}

class EditorErrorBoundary extends Component<{}, { error?: any }> {
state = {
error: undefined,
};

static getDerivedStateFromError(error: any) {
return { error };
}

componentDidCatch(error) {
// eslint-disable-next-line no-console
console.error(error);
}

render() {
if (this.state.error) {
return (
<EuiCallOut
title={i18n.translate('visBuilder.aggParamsEditor.errorTitle', {
defaultMessage: 'Error',
})}
color="danger"
iconType="alert"
>
<p>
{i18n.translate('visBuilder.aggParamsEditor.errorMsg', {
defaultMessage: 'Something went wrong while editing the aggregation',
})}
</p>
</EuiCallOut>
Comment on lines +183 to +195
Copy link
Member

Choose a reason for hiding this comment

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

Does this need UX signoff? Why not use a toast as we are for other errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

The toast error pattern is not ideal in the first place because even though the error persists, the toast will disappear. Also this is an error boundary that the user should no longer run into. Its just a safety mechanism to make sure that if it ever does, the whole app does not crash, since we inherit the DefaultAggParams component from visualize.

Also checking with @KrooshalUX about UX signoff.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KrooshalUX is okay with this UX but would also like a toast message to be shown as a part of the error. However that is not a blocker for this and I've created an issue to discuss the use of toasts in error boundaries.

);
}
return this.props.children;
}
}
4 changes: 2 additions & 2 deletions src/plugins/vis_builder/public/application/components/top_nav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import React, { useEffect, useState } from 'react';
import { useParams } from 'react-router-dom';
import { useUnmount } from 'react-use';
import { PLUGIN_ID } from '../../../common';
import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
import { getTopNavConfig } from '../utils/get_top_nav_config';
import { VisBuilderServices } from '../../types';
Expand All @@ -29,6 +28,7 @@ export const TopNav = () => {
navigation: {
ui: { TopNavMenu },
},
appName,
} = services;
const rootState = useTypedSelector((state) => state);
const dispatch = useTypedDispatch();
Expand Down Expand Up @@ -81,7 +81,7 @@ export const TopNav = () => {
return (
<div className="vbTopNav">
<TopNavMenu
appName={PLUGIN_ID}
appName={appName}
config={config}
setMenuMountPoint={setHeaderActionMenu}
indexPatterns={indexPattern ? [indexPattern] : []}
Expand Down
1 change: 1 addition & 0 deletions src/plugins/vis_builder/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ export class VisBuilderPlugin

const services: VisBuilderServices = {
...coreStart,
appName: PLUGIN_ID,
scopedHistory: this.currentHistory,
history: this.currentHistory,
osdUrlStateStorage: createOsdUrlStateStorage({
Expand Down
1 change: 1 addition & 0 deletions src/plugins/vis_builder/public/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export interface VisBuilderPluginStartDependencies {
}

export interface VisBuilderServices extends CoreStart {
appName: string;
Copy link
Member

Choose a reason for hiding this comment

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

The changes to set appName in VisBuilderServices seems fine, but it's unclear why it's necessary for this PR. I didn't see it used in secondary_panel.tsx?

Copy link
Member Author

@ashwin-pc ashwin-pc Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For the filters aggregation to work, it relies on the app name and storage option in services. Since app name was already being referenced in other places, I decided to add it to the root level service and simply reference that everywhere else too.

The filters agg uses storage (here it is local storage) with the app name as key to save and retrieve the history of all queries.

setHeaderActionMenu: AppMountParameters['setHeaderActionMenu'];
savedVisBuilderLoader: VisBuilderStart['savedVisBuilderLoader'];
toastNotifications: ToastsStart;
Expand Down
3 changes: 3 additions & 0 deletions src/plugins/vis_type_table/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,6 @@ import { TableVisPlugin as Plugin } from './plugin';
export function plugin(initializerContext: PluginInitializerContext) {
return new Plugin(initializerContext);
}

/* Public Types */
export { TableVisExpressionFunctionDefinition } from './table_vis_fn';
Comment on lines +13 to +14
Copy link
Member

Choose a reason for hiding this comment

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

Why? Appears unused in the rest of this PR.

Copy link
Member Author

@ashwin-pc ashwin-pc Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I slipped this in because there is a TS error related to this in the table agg fn which references it. This fixes that TS issue. Added it to the PR description.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!