From 226536f264d8781abcb0f12a49d2419dc7b952d6 Mon Sep 17 00:00:00 2001 From: Ashwin Pc Date: Fri, 24 Mar 2023 11:42:19 +0000 Subject: [PATCH 1/6] Refactor persistnece for simpler logic Signed-off-by: Ashwin Pc --- .../application/components/right_nav.tsx | 23 +- .../use/use_persisted_agg_params.test.tsx | 439 +++++------------- .../utils/use/use_persisted_agg_params.ts | 131 +++--- 3 files changed, 186 insertions(+), 407 deletions(-) diff --git a/src/plugins/vis_builder/public/application/components/right_nav.tsx b/src/plugins/vis_builder/public/application/components/right_nav.tsx index c98638da28f1..b0d4102a76ed 100644 --- a/src/plugins/vis_builder/public/application/components/right_nav.tsx +++ b/src/plugins/vis_builder/public/application/components/right_nav.tsx @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import React, { useState } from 'react'; +import React, { useMemo, useState } from 'react'; import { EuiSuperSelect, EuiSuperSelectOption, @@ -22,25 +22,28 @@ import { useTypedDispatch, useTypedSelector, } from '../utils/state_management'; -import { usePersistedAggParams } from '../utils/use/use_persisted_agg_params'; +import { getPersistedAggParams } from '../utils/use/use_persisted_agg_params'; export const RightNav = () => { + const { ui, name: activeVisName } = useVisualizationType(); const [newVisType, setNewVisType] = useState(); const { services: { types }, } = useOpenSearchDashboards(); - const { ui, name: activeVisName } = useVisualizationType(); const dispatch = useTypedDispatch(); const StyleSection = ui.containerConfig.style.render; const { activeVisualization } = useTypedSelector((state) => state.visualization); - const aggConfigParams = activeVisualization?.aggConfigParams ?? []; - const persistedAggParams = usePersistedAggParams( - types, - aggConfigParams, - activeVisName, - newVisType - ); + const aggConfigParams = useMemo(() => activeVisualization?.aggConfigParams ?? [], [ + activeVisualization, + ]); + const persistedAggParams = useMemo(() => { + if (!newVisType) return aggConfigParams; + + const currentVisSchemas = types.get(activeVisName)?.ui.containerConfig.data.schemas.all ?? []; + const newVisSchemas = types.get(newVisType)?.ui.containerConfig.data.schemas.all ?? []; + return getPersistedAggParams(aggConfigParams, newVisSchemas, currentVisSchemas); + }, [aggConfigParams, activeVisName, newVisType, types]); const options: Array> = types.all().map(({ name, icon, title }) => ({ value: name, diff --git a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx b/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx index 6011ac33f71f..f677f3ef57ad 100644 --- a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx +++ b/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx @@ -3,350 +3,143 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { CreateAggConfigParams } from '../../../../../data/common'; -import { AggGroupNames } from '../../../../../data/common'; +import { AggGroupName, AggGroupNames, CreateAggConfigParams } from '../../../../../data/common'; import { Schema } from '../../../../../vis_default_editor/public'; -import { VisBuilderServices } from '../../../types'; -import { createVisBuilderServicesMock } from '../mocks'; -import { - AggMapping, - getSchemaMapping, - updateAggParams, - usePersistedAggParams, -} from './use_persisted_agg_params'; +import { getPersistedAggParams } from './use_persisted_agg_params'; -describe('new usePersistedAggParams', () => { - const types = { - get: jest.fn(), - }; - let schemaMetricTemplate: Schema; - let schemaBucketTemplate: Schema; - let oldVisualizationType: Schema[]; - let newVisualizationType: Schema[]; - let aggConfigParam: CreateAggConfigParams; - let aggConfigParams: CreateAggConfigParams[]; - - beforeEach(() => { - schemaMetricTemplate = { - aggFilter: [], - editor: '', - group: AggGroupNames.Metrics, - max: 1, - min: 1, - name: '', - params: [], - title: '', - defaults: '', - }; - schemaBucketTemplate = { - aggFilter: [], - editor: '', - group: AggGroupNames.Buckets, - max: 1, - min: 1, - name: '', - params: [], - title: '', - defaults: '', - }; - aggConfigParam = { - type: '', - schema: '', - }; +describe('getPersistedAggParams', () => { + const getSchema = ( + name: string, + group: AggGroupName, + aggFilter: string[] = ['*'], + max: number = Infinity + ): Schema => ({ + name, + group, + max, + min: 0, + aggFilter, + defaults: [], + editor: true, + params: [], + title: name, }); - test('return the correct metric-to-metric, bucket-to-bucket mapping and correct persisted aggregations', () => { - oldVisualizationType = [ - { ...schemaMetricTemplate, name: 'old metric 1' }, - { ...schemaBucketTemplate, name: 'old bucket 1' }, - { ...schemaMetricTemplate, name: 'old metric 2' }, - { ...schemaBucketTemplate, name: 'old bucket 2' }, - { ...schemaBucketTemplate, name: 'old bucket 3' }, + test('Should return the same aggConfigParams when the new vis type schemas have the same schemas as the existing schemas', () => { + const aggConfigParams: CreateAggConfigParams[] = [ + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'm2' }, + { type: 'avg', schema: 'b2' }, + { type: 'avg', schema: 'b2' }, + ]; + + const schemas = [ + getSchema('m1', AggGroupNames.Metrics), + getSchema('m2', AggGroupNames.Metrics), + getSchema('b2', AggGroupNames.Buckets), ]; - newVisualizationType = [ - { ...schemaMetricTemplate, name: 'new metric 1', max: 1 }, - { ...schemaMetricTemplate, name: 'new metric 2', max: 2 }, - { ...schemaBucketTemplate, name: 'new bucket 1', max: 4 }, - { ...schemaBucketTemplate, name: 'new bucket 2', max: 5 }, - { ...schemaBucketTemplate, name: 'new bucket 3', max: 6 }, + + const persistResult = getPersistedAggParams(aggConfigParams, schemas, schemas); + + expect(persistResult).toEqual(aggConfigParams); + }); + + test('Should select the next compatible schema when aggConfigParam schema exists but exceeds the max count of the schema', () => { + const aggConfigParams: CreateAggConfigParams[] = [ + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'b2' }, + { type: 'avg', schema: 'b2' }, ]; - types.get - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: oldVisualizationType, - }, - }, - }, - }, - }) - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: newVisualizationType, - }, - }, - }, - }, - }); - aggConfigParams = [ - { ...aggConfigParam, schema: 'old metric 1' }, - { ...aggConfigParam, schema: 'old bucket 1' }, - { ...aggConfigParam, schema: 'old metric 2' }, - { ...aggConfigParam, schema: 'old bucket 2' }, - { ...aggConfigParam, schema: 'old bucket 3' }, - { ...aggConfigParam, schema: 'old metric 2' }, + + const schemas = [ + getSchema('m1', AggGroupNames.Metrics, ['avg'], 1), + getSchema('m2', AggGroupNames.Metrics), + getSchema('b2', AggGroupNames.Buckets), ]; - const mappingResult = getSchemaMapping(oldVisualizationType, newVisualizationType); - expect(mappingResult).toMatchInlineSnapshot(` - Map { - "old metric 1" => Object { - "currentCount": 0, - "maxCount": 1, - "name": "new metric 1", - }, - "old metric 2" => Object { - "currentCount": 0, - "maxCount": 2, - "name": "new metric 2", - }, - "old bucket 1" => Object { - "currentCount": 0, - "maxCount": 4, - "name": "new bucket 1", - }, - "old bucket 2" => Object { - "currentCount": 0, - "maxCount": 5, - "name": "new bucket 2", - }, - "old bucket 3" => Object { - "currentCount": 0, - "maxCount": 6, - "name": "new bucket 3", - }, - } - `); - const persistResult = usePersistedAggParams( - types, - aggConfigParams, - 'old vis type', - 'new vis type' - ); - expect(persistResult).toMatchInlineSnapshot(` - Array [ - Object { - "schema": "new metric 1", - "type": "", - }, - Object { - "schema": "new bucket 1", - "type": "", - }, - Object { - "schema": "new metric 2", - "type": "", - }, - Object { - "schema": "new bucket 2", - "type": "", - }, - Object { - "schema": "new bucket 3", - "type": "", - }, - Object { - "schema": "new metric 2", - "type": "", - }, - ] - `); + const persistResult = getPersistedAggParams(aggConfigParams, schemas, schemas); + + const expected: CreateAggConfigParams[] = [...aggConfigParams]; + expected[1].schema = 'm2'; + expect(persistResult).toEqual(expected); }); - test('drop the schema fields when it can not be mapped or do not belong to either metric or bucket group', () => { - oldVisualizationType = [ - { ...schemaMetricTemplate, name: 'old metric 1' }, - { ...schemaMetricTemplate, name: 'old metric 2' }, - { ...schemaBucketTemplate, name: 'old bucket 1' }, - { ...schemaMetricTemplate, name: 'undefined group', group: AggGroupNames.None }, + test('Should select the next compatible schema when aggConfigParam schema exists but does not match the group in the new schema', () => { + const aggConfigParams: CreateAggConfigParams[] = [ + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'b1' }, + { type: 'avg', schema: 'b1' }, ]; - newVisualizationType = [ - { ...schemaMetricTemplate, name: 'new metric 1', max: 1 }, - { ...schemaBucketTemplate, name: 'new bucket 2', max: 1 }, + + const oldSchemas = [ + getSchema('m1', AggGroupNames.Metrics), + getSchema('b1', AggGroupNames.Buckets), ]; - types.get - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: oldVisualizationType, - }, - }, - }, - }, - }) - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: newVisualizationType, - }, - }, - }, - }, - }); - aggConfigParams = [ - { ...aggConfigParam, schema: 'old metric 1' }, - { ...aggConfigParam, schema: 'old bucket 1' }, + + const newSchemas = [ + getSchema('m1', AggGroupNames.Buckets), + getSchema('m2', AggGroupNames.Metrics), + getSchema('b1', AggGroupNames.Buckets), ]; - const mappingResult = getSchemaMapping(oldVisualizationType, newVisualizationType); - expect(mappingResult).toMatchInlineSnapshot(` - Map { - "old metric 1" => Object { - "currentCount": 0, - "maxCount": 1, - "name": "new metric 1", - }, - "old bucket 1" => Object { - "currentCount": 0, - "maxCount": 1, - "name": "new bucket 2", - }, - } - `); - const persistResult = usePersistedAggParams( - types, - aggConfigParams, - 'old vis type', - 'new vis type' - ); - expect(persistResult).toMatchInlineSnapshot(` - Array [ - Object { - "schema": "new metric 1", - "type": "", - }, - Object { - "schema": "new bucket 2", - "type": "", - }, - ] - `); + const persistResult = getPersistedAggParams(aggConfigParams, oldSchemas, newSchemas); + + const expected: CreateAggConfigParams[] = [...aggConfigParams]; + expected[0].schema = 'm2'; + expected[1].schema = 'm2'; + expect(persistResult).toEqual(expected); }); - test('aggregations with undefined schema remain undefined; schema will be set to undefined if aggregations that exceeds the max amount', () => { - oldVisualizationType = [{ ...schemaMetricTemplate, name: 'old metric 1' }]; - newVisualizationType = [{ ...schemaMetricTemplate, name: 'new metric 1', max: 1 }]; - types.get - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: oldVisualizationType, - }, - }, - }, - }, - }) - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: newVisualizationType, - }, - }, - }, - }, - }); - aggConfigParams = [ - { ...aggConfigParam, schema: undefined }, - { ...aggConfigParam, schema: 'old metric 1' }, - { ...aggConfigParam, schema: 'old metric 1' }, + test('Should select the next compatible schema with the correct aggfilters', () => { + const aggConfigParams: CreateAggConfigParams[] = [ + { type: 'count', schema: 'm1' }, + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'b1' }, + { type: 'avg', schema: 'b1' }, ]; - const mappingResult = getSchemaMapping(oldVisualizationType, newVisualizationType); - expect(mappingResult).toMatchInlineSnapshot(` - Map { - "old metric 1" => Object { - "currentCount": 0, - "maxCount": 1, - "name": "new metric 1", - }, - } - `); - const persistResult = usePersistedAggParams( - types, - aggConfigParams, - 'old vis type', - 'new vis type' - ); - expect(persistResult).toMatchInlineSnapshot(` - Array [ - Object { - "schema": undefined, - "type": "", - }, - Object { - "schema": "new metric 1", - "type": "", - }, - Object { - "schema": undefined, - "type": "", - }, - ] - `); - }); + const oldSchemas = [ + getSchema('m1', AggGroupNames.Metrics), + getSchema('b1', AggGroupNames.Buckets), + ]; + + const newSchemas = [ + getSchema('m2', AggGroupNames.Metrics, ['count']), + getSchema('m3', AggGroupNames.Metrics, ['!count']), + getSchema('b1', AggGroupNames.Buckets), + ]; - test('return an empty array when there are no aggregations for persistence', () => { - oldVisualizationType = [{ ...schemaMetricTemplate, name: 'old metric 1' }]; - newVisualizationType = [{ ...schemaMetricTemplate, name: 'new metric 1', max: 1 }]; - types.get - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: oldVisualizationType, - }, - }, - }, - }, - }) - .mockReturnValueOnce({ - ui: { - containerConfig: { - data: { - schemas: { - all: newVisualizationType, - }, - }, - }, - }, - }); + const persistResult = getPersistedAggParams(aggConfigParams, oldSchemas, newSchemas); - aggConfigParams = []; - const persistResult = usePersistedAggParams( - types, - aggConfigParams, - 'old vis type', - 'new vis type' - ); - expect(persistResult).toMatchInlineSnapshot(`Array []`); + const expected: CreateAggConfigParams[] = [...aggConfigParams]; + expected[0].schema = 'm2'; + expected[1].schema = 'm3'; + expect(persistResult).toEqual(expected); }); - test('return an empty array when there are no new vis type or old vis type', () => { - const persistResult = usePersistedAggParams(types, aggConfigParams); - expect(persistResult).toMatchInlineSnapshot(`Array []`); + test('Should drop aggConfigParam when no compatible schema is found', () => { + const aggConfigParams: CreateAggConfigParams[] = [ + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'm1' }, + { type: 'avg', schema: 'b1' }, + { type: 'avg', schema: 'b1' }, + ]; + + const oldSchemas = [ + getSchema('m1', AggGroupNames.Metrics), + getSchema('b1', AggGroupNames.Buckets), + ]; + + const newSchemas = [ + getSchema('m2', AggGroupNames.Metrics, ['count']), + getSchema('m3', AggGroupNames.Metrics, ['!count'], 1), + getSchema('b1', AggGroupNames.Buckets), + ]; + + const persistResult = getPersistedAggParams(aggConfigParams, oldSchemas, newSchemas); + + expect(persistResult.length).toBe(3); }); }); diff --git a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts b/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts index 980c5f3e9f38..41d1569211d2 100644 --- a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts +++ b/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts @@ -3,93 +3,76 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { AggGroupNames, CreateAggConfigParams } from '../../../../../data/common'; +import { CreateAggConfigParams, propFilter } from '../../../../../data/common'; import { Schema } from '../../../../../vis_default_editor/public'; -export const usePersistedAggParams = ( - types, +const filterByType = propFilter('type'); + +export const getPersistedAggParams = ( aggConfigParams: CreateAggConfigParams[], - oldVisType?: string, - newVisType?: string + oldVisSchemas: Schema[] = [], + newVisSchemas: Schema[] = [] ): CreateAggConfigParams[] => { - if (oldVisType && newVisType) { - const oldVisualizationType = types.get(oldVisType)?.ui.containerConfig.data.schemas.all; - const newVisualizationType = types.get(newVisType)?.ui.containerConfig.data.schemas.all; - const aggMapping = getSchemaMapping(oldVisualizationType, newVisualizationType); - const updatedAggConfigParams = aggConfigParams.map((aggConfigParam: CreateAggConfigParams) => - updateAggParams(aggConfigParam, aggMapping) - ); - return updatedAggConfigParams; - } - return []; -}; + const updatedAggConfigParams: CreateAggConfigParams[] = []; + const newVisSchemaCounts: Record = newVisSchemas.reduce((acc, schema: Schema) => { + acc[schema.name] = schema.max; + return acc; + }, {}); -// Map metric fields to metric fields, bucket fields to bucket fields -export const getSchemaMapping = ( - oldVisualizationType: Schema[], - newVisualizationType: Schema[] -): Map => { - const aggMap = new Map(); + // For each aggConfigParam, check if a compatible schema exists in the new visualization type + aggConfigParams.forEach((aggConfigParam) => { + const currentSchema = oldVisSchemas.find((schema: Schema) => { + return schema.name === aggConfigParam.schema; + }); - // currently Metrics, Buckets, and None are the three groups. We simply drop the aggregations that belongs to the None group - mapAggParamsSchema(oldVisualizationType, newVisualizationType, AggGroupNames.Metrics, aggMap); - mapAggParamsSchema(oldVisualizationType, newVisualizationType, AggGroupNames.Buckets, aggMap); + // see if a matching schma exists in the new visualization type + const matchingSchema = newVisSchemas.find((schema: Schema) => { + return schema.name === aggConfigParam.schema; + }); - return aggMap; -}; + // if the matching schema is same as the current schema, add the aggConfigParam to the updatedAggConfigParams + if ( + isSchemaEqual(matchingSchema, currentSchema) && + newVisSchemaCounts[matchingSchema!.name] > 0 + ) { + updatedAggConfigParams.push(aggConfigParam); + newVisSchemaCounts[matchingSchema!.name] -= 1; + return; + } -export interface AggMapping { - name: string; - maxCount: number; - currentCount: number; -} + // if a matching schema does not exist, check if a compatible schema exists + for (const schema of newVisSchemas) { + // Check if the schema group is the same + if (schema.group !== currentSchema!.group) continue; -export const mapAggParamsSchema = ( - oldVisualizationType: Schema[], - newVisualizationType: Schema[], - aggGroup: string, - map: Map -) => { - const oldSchemas = oldVisualizationType.filter((type) => type.group === aggGroup); - const newSchemas = newVisualizationType.filter((type) => type.group === aggGroup); + const compatibleSchema = filterByType([aggConfigParam], schema.aggFilter).length !== 0; - oldSchemas.forEach((oldSchema, index) => { - if (newSchemas[index]) { - const mappedNewSchema = { - name: newSchemas[index].name, - maxCount: newSchemas[index].max, - currentCount: 0, - }; - map.set(oldSchema.name, mappedNewSchema); + if (compatibleSchema && newVisSchemaCounts[schema.name] > 0) { + updatedAggConfigParams.push({ + ...aggConfigParam, + schema: schema.name, + }); + newVisSchemaCounts[schema.name] -= 1; + break; + } } }); + + return updatedAggConfigParams; }; -export const updateAggParams = ( - oldAggParam: CreateAggConfigParams, - aggMap: Map -) => { - const newAggParam = { ...oldAggParam }; - if (oldAggParam.schema) { - const newSchema = aggMap.get(oldAggParam.schema); - newAggParam.schema = newSchema - ? newSchema.currentCount < newSchema.maxCount - ? assignNewSchemaType(oldAggParam, aggMap, newSchema) - : undefined - : undefined; +function isSchemaEqual(schema1?: Schema, schema2?: Schema) { + // Check if schema1 and schema2 exist + if (!schema1 || !schema2) return false; + + if (schema1.name !== schema2.name) return false; + if (schema1.group !== schema2.group) return false; + + // Check if aggFilter is the same + if (schema1.aggFilter.length !== schema2.aggFilter.length) return false; + for (let i = 0; i < schema1.aggFilter.length; i++) { + if (schema1.aggFilter[i] !== schema2.aggFilter[i]) return false; } - return newAggParam; -}; -export const assignNewSchemaType = ( - oldAggParam: any, - aggMap: Map, - newSchema: AggMapping -) => { - aggMap.set(oldAggParam.schema, { - name: newSchema.name, - maxCount: newSchema.maxCount, - currentCount: newSchema.currentCount + 1, - }); - return aggMap.get(oldAggParam.schema)?.name; -}; + return true; +} From 0782d0ee1748f9744d7d81a6f0ef2f73ccb5fa66 Mon Sep 17 00:00:00 2001 From: Ashwin Pc Date: Fri, 24 Mar 2023 11:46:17 +0000 Subject: [PATCH 2/6] renamed files Signed-off-by: Ashwin Pc --- .../vis_builder/public/application/components/right_nav.tsx | 2 +- ...gg_params.test.tsx => get_persisted_agg_params.test.tsx} | 6 +++--- ..._persisted_agg_params.ts => get_persisted_agg_params.ts} | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) rename src/plugins/vis_builder/public/application/utils/{use/use_persisted_agg_params.test.tsx => get_persisted_agg_params.test.tsx} (96%) rename src/plugins/vis_builder/public/application/utils/{use/use_persisted_agg_params.ts => get_persisted_agg_params.ts} (94%) diff --git a/src/plugins/vis_builder/public/application/components/right_nav.tsx b/src/plugins/vis_builder/public/application/components/right_nav.tsx index b0d4102a76ed..37f101915f1e 100644 --- a/src/plugins/vis_builder/public/application/components/right_nav.tsx +++ b/src/plugins/vis_builder/public/application/components/right_nav.tsx @@ -22,7 +22,7 @@ import { useTypedDispatch, useTypedSelector, } from '../utils/state_management'; -import { getPersistedAggParams } from '../utils/use/use_persisted_agg_params'; +import { getPersistedAggParams } from '../utils/get_persisted_agg_params'; export const RightNav = () => { const { ui, name: activeVisName } = useVisualizationType(); diff --git a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx b/src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.test.tsx similarity index 96% rename from src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx rename to src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.test.tsx index f677f3ef57ad..0b9c0ec8e745 100644 --- a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.test.tsx +++ b/src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.test.tsx @@ -3,9 +3,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { AggGroupName, AggGroupNames, CreateAggConfigParams } from '../../../../../data/common'; -import { Schema } from '../../../../../vis_default_editor/public'; -import { getPersistedAggParams } from './use_persisted_agg_params'; +import { AggGroupName, AggGroupNames, CreateAggConfigParams } from '../../../../data/common'; +import { Schema } from '../../../../vis_default_editor/public'; +import { getPersistedAggParams } from './get_persisted_agg_params'; describe('getPersistedAggParams', () => { const getSchema = ( diff --git a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts b/src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.ts similarity index 94% rename from src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts rename to src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.ts index 41d1569211d2..8d0f5369a7d0 100644 --- a/src/plugins/vis_builder/public/application/utils/use/use_persisted_agg_params.ts +++ b/src/plugins/vis_builder/public/application/utils/get_persisted_agg_params.ts @@ -3,8 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { CreateAggConfigParams, propFilter } from '../../../../../data/common'; -import { Schema } from '../../../../../vis_default_editor/public'; +import { CreateAggConfigParams, propFilter } from '../../../../data/common'; +import { Schema } from '../../../../vis_default_editor/public'; const filterByType = propFilter('type'); From cf6bcb5713711578b8aae50fcc62ca9525b6d125 Mon Sep 17 00:00:00 2001 From: Ashwin P Chandran Date: Tue, 28 Mar 2023 01:57:36 +0000 Subject: [PATCH 3/6] only show warning when dropping fields Signed-off-by: Ashwin P Chandran --- .../application/components/right_nav.tsx | 53 +++++++++++-------- .../utils/state_management/shared_actions.ts | 2 +- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/plugins/vis_builder/public/application/components/right_nav.tsx b/src/plugins/vis_builder/public/application/components/right_nav.tsx index 37f101915f1e..d4fdca85b114 100644 --- a/src/plugins/vis_builder/public/application/components/right_nav.tsx +++ b/src/plugins/vis_builder/public/application/components/right_nav.tsx @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import React, { useMemo, useState } from 'react'; +import React, { useCallback, useMemo, useState } from 'react'; import { EuiSuperSelect, EuiSuperSelectOption, @@ -18,6 +18,7 @@ import './side_nav.scss'; import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public'; import { VisBuilderServices } from '../../types'; import { + ActiveVisPayload, setActiveVisualization, useTypedDispatch, useTypedSelector, @@ -26,7 +27,7 @@ import { getPersistedAggParams } from '../utils/get_persisted_agg_params'; export const RightNav = () => { const { ui, name: activeVisName } = useVisualizationType(); - const [newVisType, setNewVisType] = useState(); + const [confirmAggs, setConfirmAggs] = useState(); const { services: { types }, } = useOpenSearchDashboards(); @@ -37,13 +38,29 @@ export const RightNav = () => { const aggConfigParams = useMemo(() => activeVisualization?.aggConfigParams ?? [], [ activeVisualization, ]); - const persistedAggParams = useMemo(() => { - if (!newVisType) return aggConfigParams; - const currentVisSchemas = types.get(activeVisName)?.ui.containerConfig.data.schemas.all ?? []; - const newVisSchemas = types.get(newVisType)?.ui.containerConfig.data.schemas.all ?? []; - return getPersistedAggParams(aggConfigParams, newVisSchemas, currentVisSchemas); - }, [aggConfigParams, activeVisName, newVisType, types]); + const handleVisTypeChange = useCallback( + (newVisName) => { + const currentVisSchemas = types.get(activeVisName)?.ui.containerConfig.data.schemas.all ?? []; + const newVisSchemas = types.get(newVisName)?.ui.containerConfig.data.schemas.all ?? []; + const persistedAggParams = getPersistedAggParams( + aggConfigParams, + currentVisSchemas, + newVisSchemas + ); + + const newVis = { + name: newVisName, + aggConfigParams: persistedAggParams, + style: types.get(newVisName)?.ui.containerConfig.style.defaults, + }; + + if (persistedAggParams.length !== aggConfigParams.length) return setConfirmAggs(newVis); + + dispatch(setActiveVisualization(newVis)); + }, + [activeVisName, aggConfigParams, dispatch, types] + ); const options: Array> = types.all().map(({ name, icon, title }) => ({ value: name, @@ -58,9 +75,7 @@ export const RightNav = () => { { - setNewVisType(name); - }} + onChange={handleVisTypeChange} fullWidth data-test-subj="chartPicker" /> @@ -68,7 +83,7 @@ export const RightNav = () => {
- {newVisType && ( + {confirmAggs && ( { cancelButtonText={i18n.translate('visBuilder.rightNav.changeVisType.cancelText', { defaultMessage: 'Cancel', })} - onCancel={() => setNewVisType(undefined)} + onCancel={() => setConfirmAggs(undefined)} onConfirm={() => { - dispatch( - setActiveVisualization({ - name: newVisType, - style: types.get(newVisType)?.ui.containerConfig.style.defaults, - aggConfigParams: persistedAggParams, - }) - ); + dispatch(setActiveVisualization(confirmAggs)); - setNewVisType(undefined); + setConfirmAggs(undefined); }} maxWidth="300px" data-test-subj="confirmVisChangeModal" @@ -97,7 +106,7 @@ export const RightNav = () => {

diff --git a/src/plugins/vis_builder/public/application/utils/state_management/shared_actions.ts b/src/plugins/vis_builder/public/application/utils/state_management/shared_actions.ts index ebff4f91803c..be6cf7e4f41b 100644 --- a/src/plugins/vis_builder/public/application/utils/state_management/shared_actions.ts +++ b/src/plugins/vis_builder/public/application/utils/state_management/shared_actions.ts @@ -7,7 +7,7 @@ import { createAction } from '@reduxjs/toolkit'; import { CreateAggConfigParams } from '../../../../../data/common'; import { VisualizationType } from '../../../services/type_service/visualization_type'; -interface ActiveVisPayload { +export interface ActiveVisPayload { name: VisualizationType['name']; style: VisualizationType['ui']['containerConfig']['style']['defaults']; aggConfigParams: CreateAggConfigParams[]; From 2005395a985f4bcb945cff2b8a3535a2b3b08ff5 Mon Sep 17 00:00:00 2001 From: Ashwin P Chandran Date: Tue, 28 Mar 2023 03:46:38 +0000 Subject: [PATCH 4/6] delete unnecessary tests Signed-off-by: Ashwin P Chandran --- test/functional/apps/vis_builder/_base.ts | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/test/functional/apps/vis_builder/_base.ts b/test/functional/apps/vis_builder/_base.ts index 60dd1068347c..98d7efcf3d6c 100644 --- a/test/functional/apps/vis_builder/_base.ts +++ b/test/functional/apps/vis_builder/_base.ts @@ -6,11 +6,10 @@ import expect from '@osd/expect'; import { FtrProviderContext } from '../../ftr_provider_context'; +// TODO: Remove selenium functional tests since cypress tests exist export default function ({ getService, getPageObjects }: FtrProviderContext) { const PageObjects = getPageObjects(['visualize', 'visBuilder', 'visChart']); - const testSubjects = getService('testSubjects'); const log = getService('log'); - const retry = getService('retry'); describe('Basic tests for visBuilder app ', function () { before(async () => { @@ -42,19 +41,5 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { const isEmptyWorkspace = await PageObjects.visBuilder.isEmptyWorkspace(); expect(isEmptyWorkspace).to.be(true); }); - - it('should show warning before changing visualization type', async () => { - await PageObjects.visBuilder.selectVisType('metric', false); - const confirmModalExists = await testSubjects.exists('confirmVisChangeModal'); - expect(confirmModalExists).to.be(true); - - await testSubjects.click('confirmModalCancelButton'); - }); - - it('should change visualization type', async () => { - const pickerValue = await PageObjects.visBuilder.selectVisType('metric'); - - expect(pickerValue).to.eql('Metric'); - }); }); } From be6cbc467b9234b179fccabf1b3a4c765565f8e7 Mon Sep 17 00:00:00 2001 From: Ashwin P Chandran Date: Fri, 31 Mar 2023 02:02:34 +0000 Subject: [PATCH 5/6] addresses PR feedback Signed-off-by: Ashwin P Chandran --- .../vis_builder/public/application/components/right_nav.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/vis_builder/public/application/components/right_nav.tsx b/src/plugins/vis_builder/public/application/components/right_nav.tsx index d4fdca85b114..d4ec1180c528 100644 --- a/src/plugins/vis_builder/public/application/components/right_nav.tsx +++ b/src/plugins/vis_builder/public/application/components/right_nav.tsx @@ -55,7 +55,7 @@ export const RightNav = () => { style: types.get(newVisName)?.ui.containerConfig.style.defaults, }; - if (persistedAggParams.length !== aggConfigParams.length) return setConfirmAggs(newVis); + if (persistedAggParams.length <= aggConfigParams.length) return setConfirmAggs(newVis); dispatch(setActiveVisualization(newVis)); }, @@ -106,7 +106,7 @@ export const RightNav = () => {

From 6c7997ef00bf060f4fe5f1c6bd9595f870bd79dd Mon Sep 17 00:00:00 2001 From: Ashwin P Chandran Date: Fri, 31 Mar 2023 11:08:21 -0700 Subject: [PATCH 6/6] Fixes comparison --- .../vis_builder/public/application/components/right_nav.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/vis_builder/public/application/components/right_nav.tsx b/src/plugins/vis_builder/public/application/components/right_nav.tsx index d4ec1180c528..70b22d600ae2 100644 --- a/src/plugins/vis_builder/public/application/components/right_nav.tsx +++ b/src/plugins/vis_builder/public/application/components/right_nav.tsx @@ -55,7 +55,7 @@ export const RightNav = () => { style: types.get(newVisName)?.ui.containerConfig.style.defaults, }; - if (persistedAggParams.length <= aggConfigParams.length) return setConfirmAggs(newVis); + if (persistedAggParams.length < aggConfigParams.length) return setConfirmAggs(newVis); dispatch(setActiveVisualization(newVis)); },