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

[VisBuilder] 2 way communication using UI actions and bug fixes #3732

Merged
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Use mirrors to download Node.js binaries to escape sporadic 404 errors ([#3619](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3619))
- [Multiple DataSource] Refactor dev tool console to use opensearch-js client to send requests ([#3544](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3544))
- [Data] Add geo shape filter field ([#3605](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3605))
- [VisBuilder] Adds UI actions handler ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
ashwin-pc marked this conversation as resolved.
Show resolved Hide resolved
- [Multiple DataSource] Allow create and distinguish index pattern with same name but from different datasources ([#3571](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3571))
- [Multiple DataSource] Integrate multiple datasource with dev tool console ([#3754](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3754))

Expand Down Expand Up @@ -114,6 +115,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Clean up and rebuild `@osd/pm` ([#3570](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3570))
- [Vega] Add Filter custom label for opensearchDashboardsAddFilter ([#3640](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3640))
- [Timeline] Fix y-axis label color in dark mode ([#3698](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3698))
- [VisBuilder] Fixes multiple warnings thrown on page load ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
- [VisBuilder] Fixes Firefox legend selection issue ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
- [VisBuilder] Fixes type errors ([#3732](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3732))
ashwin-pc marked this conversation as resolved.
Show resolved Hide resolved

### 🚞 Infrastructure

Expand Down
5 changes: 3 additions & 2 deletions src/plugins/vis_builder/opensearch_dashboards.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
"expressions",
"navigation",
"savedObjects",
"visualizations"
"visualizations",
"uiActions"
],
"requiredBundles": [
"charts",
Expand All @@ -20,4 +21,4 @@
"visDefaultEditor",
"visTypeVislib"
]
}
}
3 changes: 2 additions & 1 deletion src/plugins/vis_builder/public/application/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ import { DragDropProvider } from './utils/drag_drop/drag_drop_context';
import { LeftNav } from './components/left_nav';
import { TopNav } from './components/top_nav';
import { Workspace } from './components/workspace';
import './app.scss';
import { RightNav } from './components/right_nav';
import { useOpenSearchDashboards } from '../../../opensearch_dashboards_react/public';
import { VisBuilderServices } from '../types';
import { syncQueryStateWithUrl } from '../../../data/public';

import './app.scss';

export const VisBuilderApp = () => {
const {
services: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
} from '../utils/state_management';
import { getPersistedAggParams } from '../utils/get_persisted_agg_params';

export const RightNav = () => {
export const RightNavUI = () => {
const { ui, name: activeVisName } = useVisualizationType();
const [confirmAggs, setConfirmAggs] = useState<ActiveVisPayload | undefined>();
const {
Expand Down Expand Up @@ -121,3 +121,5 @@ const OptionItem = ({ icon, title }: { icon: IconType; title: string }) => (
<span>{title}</span>
</>
);

export const RightNav = React.memo(RightNavUI);
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { i18n } from '@osd/i18n';
import { EuiEmptyPrompt, EuiFlexGroup, EuiFlexItem, EuiIcon, EuiPanel } from '@elastic/eui';
import React, { FC, useState, useMemo, useEffect, useLayoutEffect } from 'react';
import React, { useState, useMemo, useEffect, useLayoutEffect } from 'react';
import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public';
import { IExpressionLoaderParams } from '../../../../expressions/public';
import { VisBuilderServices } from '../../types';
Expand All @@ -19,17 +19,19 @@ import fields_bg from '../../assets/fields_bg.svg';

import './workspace.scss';
import { ExperimentalInfo } from './experimental_info';
import { handleVisEvent } from '../utils/handle_vis_event';

export const Workspace: FC = ({ children }) => {
export const WorkspaceUI = () => {
const {
services: {
expressions: { ReactExpressionRenderer },
notifications: { toasts },
data,
uiActions,
},
} = useOpenSearchDashboards<VisBuilderServices>();
const { toExpression, ui } = useVisualizationType();
const { aggConfigs } = useAggs();
const { aggConfigs, indexPattern } = useAggs();
const [expression, setExpression] = useState<string>();
const [searchContext, setSearchContext] = useState<IExpressionLoaderParams['searchContext']>({
query: data.query.queryString.getQuery(),
Expand All @@ -44,15 +46,17 @@ export const Workspace: FC = ({ children }) => {
async function loadExpression() {
const schemas = ui.containerConfig.data.schemas;

const noAggs = aggConfigs?.aggs?.length === 0;
const noAggs = (aggConfigs?.aggs?.length ?? 0) === 0;
Copy link
Member

Choose a reason for hiding this comment

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

opinion - this reads worse to me than

const noAggs = !aggConfigs?.aggs?.length;
or even
const noAggs = aggConfigs?.aggs?.length == false;

Also, for conditionals like this I'd prefer hasNoAggs.

const schemaValidation = validateSchemaState(schemas, rootState.visualization);
const aggValidation = validateAggregations(aggConfigs?.aggs || []);

if (noAggs || !aggValidation.valid || !schemaValidation.valid) {
if (!aggValidation.valid || !schemaValidation.valid) {
setExpression(undefined);
if (noAggs) return; // don't show error when there are no active aggregations

const err = schemaValidation.errorMsg || aggValidation.errorMsg;

if (err) toasts.addWarning(err);
setExpression(undefined);

return;
}
Expand Down Expand Up @@ -91,6 +95,7 @@ export const Workspace: FC = ({ children }) => {
expression={expression}
searchContext={searchContext}
uiState={uiState}
onEvent={(event) => handleVisEvent(event, uiActions, indexPattern?.timeFieldName)}
/>
) : (
<EuiFlexItem className="vbWorkspace__empty" data-test-subj="emptyWorkspace">
Expand Down Expand Up @@ -127,3 +132,5 @@ export const Workspace: FC = ({ children }) => {
</section>
);
};

export const Workspace = React.memo(WorkspaceUI);

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ import {
showSaveModal,
} from '../../../../saved_objects/public';
import { VisBuilderServices } from '../..';
import { VisBuilderVisSavedObject } from '../../types';
import { VisBuilderSavedObject } from '../../types';
import { AppDispatch } from './state_management';
import { EDIT_PATH, VISBUILDER_SAVED_OBJECT } from '../../../common';
import { setEditorState } from './state_management/metadata_slice';
export interface TopNavConfigParams {
visualizationIdFromUrl: string;
savedVisBuilderVis: VisBuilderVisSavedObject;
savedVisBuilderVis: VisBuilderSavedObject;
saveDisabledReason?: string;
dispatch: AppDispatch;
originatingApp?: string;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { ExpressionRendererEvent } from '../../../../expressions/public';
import { VIS_EVENT_TO_TRIGGER } from '../../../../visualizations/public';
import { handleVisEvent } from './handle_vis_event';
import { uiActionsPluginMock } from '../../../../ui_actions/public/mocks';
import { Action, ActionType, createAction } from '../../../../ui_actions/public';

const executeFn = jest.fn();

function createTestAction<C extends object>(
type: string,
checkCompatibility: (context: C) => boolean,
autoExecutable = true
): Action<object> {
return createAction({
type: type as ActionType,
id: type,
isCompatible: (context: C) => Promise.resolve(checkCompatibility(context)),
execute: (context) => {
return executeFn(context);
},
shouldAutoExecute: () => Promise.resolve(autoExecutable),
});
}

let uiActions: ReturnType<typeof uiActionsPluginMock.createPlugin>;

describe('handleVisEvent', () => {
beforeEach(() => {
uiActions = uiActionsPluginMock.createPlugin();

executeFn.mockClear();
jest.useFakeTimers();
});

test('should trigger the correct event', async () => {
const event: ExpressionRendererEvent = {
name: 'filter',
data: {},
};
const action = createTestAction('test1', () => true);
const timeFieldName = 'test-timefeild-name';
uiActions.setup.addTriggerAction(VIS_EVENT_TO_TRIGGER.filter, action);

await handleVisEvent(event, uiActions.doStart(), timeFieldName);

jest.runAllTimers();

expect(executeFn).toBeCalledTimes(1);
expect(executeFn).toBeCalledWith(
expect.objectContaining({
data: { timeFieldName },
})
);
});

test('should trigger the default trigger when not found', async () => {
const event: ExpressionRendererEvent = {
name: 'test',
data: {},
};
const action = createTestAction('test2', () => true);
const timeFieldName = 'test-timefeild-name';
uiActions.setup.addTriggerAction(VIS_EVENT_TO_TRIGGER.filter, action);

await handleVisEvent(event, uiActions.doStart(), timeFieldName);

jest.runAllTimers();

expect(executeFn).toBeCalledTimes(1);
expect(executeFn).toBeCalledWith(
expect.objectContaining({
data: { timeFieldName },
})
);
});

test('should have the correct context for `applyfilter`', async () => {
const event: ExpressionRendererEvent = {
name: 'applyFilter',
data: {},
};
const action = createTestAction('test3', () => true);
const timeFieldName = 'test-timefeild-name';
uiActions.setup.addTriggerAction(VIS_EVENT_TO_TRIGGER.applyFilter, action);

await handleVisEvent(event, uiActions.doStart(), timeFieldName);

jest.runAllTimers();

expect(executeFn).toBeCalledTimes(1);
expect(executeFn).toBeCalledWith(
expect.objectContaining({
timeFieldName,
})
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { get } from 'lodash';
import { ExpressionRendererEvent } from '../../../../expressions/public';
import { VIS_EVENT_TO_TRIGGER } from '../../../../visualizations/public';
import { UiActionsStart } from '../../../../ui_actions/public';

export const handleVisEvent = async (
Copy link
Member

Choose a reason for hiding this comment

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

Just a question, do we need to add error handling here?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do see error handling being added here?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm i am mostly thinking if it's possible that await uiActions.getTrigger() fail?

event: ExpressionRendererEvent,
uiActions: UiActionsStart,
timeFieldName?: string
) => {
const triggerId = get(VIS_EVENT_TO_TRIGGER, event.name, VIS_EVENT_TO_TRIGGER.filter);
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved
let context;

if (triggerId === VIS_EVENT_TO_TRIGGER.applyFilter) {
context = {
timeFieldName,
...event.data,
};
} else {
context = {
data: {
timeFieldName,
...event.data,
},
};
}
ashwin-pc marked this conversation as resolved.
Show resolved Hide resolved

await uiActions.getTrigger(triggerId).exec(context);
};
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,10 @@ import {
import { EDIT_PATH, PLUGIN_ID } from '../../../../common';
import { VisBuilderServices } from '../../../types';
import { getCreateBreadcrumbs, getEditBreadcrumbs } from '../breadcrumbs';
import { getSavedVisBuilderVis } from '../get_saved_vis_builder_vis';
import {
useTypedDispatch,
setStyleState,
setVisualizationState,
VisualizationState,
} from '../state_management';
import { useTypedDispatch, setStyleState, setVisualizationState } from '../state_management';
import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public';
import { setEditorState } from '../state_management/metadata_slice';
import { validateVisBuilderState } from '../validations/vis_builder_state_validation';
import { getStateFromSavedObject } from '../../../saved_visualizations/transforms';

// This function can be used when instantiating a saved vis or creating a new one
// using url parameters, embedding and destroying it in DOM
Expand All @@ -39,6 +33,7 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined
history,
http: { basePath },
toastNotifications,
savedVisBuilderLoader,
} = services;
const toastNotification = (message: string) => {
toastNotifications.addDanger({
Expand All @@ -51,42 +46,22 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined

const loadSavedVisBuilderVis = async () => {
try {
const savedVisBuilderVis = await getSavedVisBuilderVis(services, visualizationIdFromUrl);
const savedVisBuilderVis = await getSavedVisBuilderVis(
savedVisBuilderLoader,
visualizationIdFromUrl
);

if (savedVisBuilderVis.id) {
chrome.setBreadcrumbs(getEditBreadcrumbs(savedVisBuilderVis.title, navigateToApp));
chrome.docTitle.change(savedVisBuilderVis.title);
const { title, state } = getStateFromSavedObject(savedVisBuilderVis);
chrome.setBreadcrumbs(getEditBreadcrumbs(title, navigateToApp));
chrome.docTitle.change(title);

dispatch(setStyleState(state.style));
dispatch(setVisualizationState(state.visualization));
} else {
chrome.setBreadcrumbs(getCreateBreadcrumbs(navigateToApp));
}

if (
savedVisBuilderVis.styleState !== '{}' &&
savedVisBuilderVis.visualizationState !== '{}'
) {
const styleState = JSON.parse(savedVisBuilderVis.styleState);
const vizStateWithoutIndex = JSON.parse(savedVisBuilderVis.visualizationState);
const visualizationState: VisualizationState = {
searchField: vizStateWithoutIndex.searchField,
activeVisualization: vizStateWithoutIndex.activeVisualization,
indexPattern: savedVisBuilderVis.searchSourceFields.index,
};

const validateResult = validateVisBuilderState({ styleState, visualizationState });
if (!validateResult.valid) {
throw new InvalidJSONProperty(
validateResult.errorMsg ||
i18n.translate('visBuilder.useSavedVisBuilderVis.genericJSONError', {
defaultMessage:
'Something went wrong while loading your saved object. The object may be corrupted or does not match the latest schema',
})
);
}

dispatch(setStyleState(styleState));
dispatch(setVisualizationState(visualizationState));
}

setSavedVisState(savedVisBuilderVis);
dispatch(setEditorState({ state: 'clean' }));
} catch (error) {
Expand Down Expand Up @@ -123,3 +98,12 @@ export const useSavedVisBuilderVis = (visualizationIdFromUrl: string | undefined

return savedVisState;
};

async function getSavedVisBuilderVis(
savedVisBuilderLoader: VisBuilderServices['savedVisBuilderLoader'],
visBuilderVisId?: string
) {
const savedVisBuilderVis = await savedVisBuilderLoader.get(visBuilderVisId);

return savedVisBuilderVis;
}
Loading