Code review

elastic · Apr 24, 2024 · 1592b96 · 1592b96
1 parent c745590
commit 1592b96
Show file tree

Hide file tree

Showing 9 changed files with 68 additions and 70 deletions.
diff --git a/src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx b/src/plugins/chart_expressions/expression_xy/public/components/xy_chart.tsx
@@ -869,7 +869,7 @@ export function XYChart({
                   )
                 : undefined
             }
-            showLegendExtra={isHistogramViz && !!(legend.legendStats?.[0] === LegendStats.values)}
+            showLegendExtra={isHistogramViz && legend.legendStats?.[0] === LegendStats.values}
             ariaLabel={args.ariaLabel}
             ariaUseDefaultSummary={!args.ariaLabel}
             orderOrdinalBinsBy={

diff --git a/src/plugins/vis_types/pie/public/convert_to_lens/configurations/index.ts b/src/plugins/vis_types/pie/public/convert_to_lens/configurations/index.ts
@@ -26,9 +26,9 @@ const getLayers = (
     legendOpen !== undefined ? (legendOpen ? LegendDisplay.SHOW : LegendDisplay.HIDE) : undefined;
 
   const showValuesInLegend =
-    vis.params.labels.values ??
-    vis.params.legendStats?.[0] === LegendStats.values ??
-    vis.type.visConfig.defaults.showValuesInLegend;
+    vis.params.labels.values ?? vis.params.legendStats
+      ? vis.params.legendStats?.[0] === LegendStats.values
+      : vis.type.visConfig.defaults.showValuesInLegend;
 
   return [
     {

diff --git a/x-pack/plugins/lens/public/visualizations/partition/persistence.tsx b/x-pack/plugins/lens/public/visualizations/partition/persistence.tsx
@@ -18,16 +18,13 @@ export type PersistedPieVisualizationState = Omit<PieVisualizationState, 'layers
 };
 
 export function convertToRuntime(state: PersistedPieVisualizationState) {
-  if (state.layers.some((l) => 'showValuesInLegend' in l)) {
-    return convertToLegendStats(state);
-  }
-  return state;
+  let newState = cloneDeep(state) as unknown as PieVisualizationState;
+  newState = convertToLegendStats(newState);
+  return newState;
 }
 
 function convertToLegendStats(state: PieVisualizationState) {
-  const newState = cloneDeep(state) as unknown as PieVisualizationState;
-
-  newState.layers.forEach((l) => {
+  state.layers.forEach((l) => {
     if ('showValuesInLegend' in l) {
       l.legendStats = [
         ...new Set([
@@ -39,7 +36,7 @@ function convertToLegendStats(state: PieVisualizationState) {
     delete (l as PersistedPieLayerState).showValuesInLegend;
   });
 
-  return newState;
+  return state;
 }
 
 export function convertToPersistable(state: PieVisualizationState) {

diff --git a/x-pack/plugins/lens/public/visualizations/partition/render_helpers.ts b/x-pack/plugins/lens/public/visualizations/partition/render_helpers.ts
@@ -14,7 +14,7 @@ export const getLegendStats = (layer: PieLayerState, shape: PieChartType) => {
   if ('defaultLegendStats' in PartitionChartsMeta[shape]?.legend) {
     return (
       layer.legendStats ??
-      PartitionChartsMeta[shape]?.legend?.defaultLegendStats ?? [LegendStats.values]
+      PartitionChartsMeta[shape].legend.defaultLegendStats ?? [LegendStats.values]
     );
   }
 };

diff --git a/x-pack/plugins/lens/public/visualizations/partition/toolbar.tsx b/x-pack/plugins/lens/public/visualizations/partition/toolbar.tsx
@@ -249,7 +249,7 @@ export function PieToolbar(props: VisualizationToolbarProps<PieVisualizationStat
         mode={layer.legendDisplay}
         onDisplayChange={onLegendDisplayChange}
         legendStats={getLegendStats(layer, state.shape)}
-        allowLegendStats={'defaultLegendStats' in PartitionChartsMeta[state.shape]?.legend ?? false}
+        allowLegendStats={!!PartitionChartsMeta[state.shape]?.legend.defaultLegendStats}
         onLegendStatsChange={onLegendStatsChange}
         position={layer.legendPosition}
         onPositionChange={onLegendPositionChange}

diff --git a/x-pack/plugins/lens/public/visualizations/partition/visualization.tsx b/x-pack/plugins/lens/public/visualizations/partition/visualization.tsx
@@ -168,18 +168,19 @@ export const getPieVisualization = ({
   triggers: [VIS_EVENT_TO_TRIGGER.filter],
 
   initialize(addNewLayer, state, mainPalette) {
-    return state
-      ? convertToRuntime(state)
-      : {
-          shape: PieChartTypes.DONUT,
-          layers: [
-            newLayerState(
-              addNewLayer(),
-              mainPalette?.type === 'colorMapping' ? mainPalette.value : getColorMappingDefaults()
-            ),
-          ],
-          palette: mainPalette?.type === 'legacyPalette' ? mainPalette.value : undefined,
-        };
+    if (state) {
+      return convertToRuntime(state);
+    }
+    return {
+      shape: PieChartTypes.DONUT,
+      layers: [
+        newLayerState(
+          addNewLayer(),
+          mainPalette?.type === 'colorMapping' ? mainPalette.value : getColorMappingDefaults()
+        ),
+      ],
+      palette: mainPalette?.type === 'legacyPalette' ? mainPalette.value : undefined,
+    };
   },
 
   getPersistableState(state) {

diff --git a/x-pack/plugins/lens/public/visualizations/xy/persistence.ts b/x-pack/plugins/lens/public/visualizations/xy/persistence.ts
@@ -91,13 +91,9 @@ export function convertToRuntime(
   annotationGroups?: AnnotationGroups,
   references?: SavedObjectReference[]
 ) {
-  const outputState = needsInjectReferences(state)
-    ? injectReferences(state, annotationGroups, references)
-    : state;
-  if ('valuesInLegend' in outputState) {
-    return convertToLegendStats(outputState);
-  }
-  return outputState;
+  let newState = cloneDeep(injectReferences(state, annotationGroups, references));
+  newState = convertToLegendStats(newState);
+  return newState;
 }
 
 export function convertToPersistable(state: XYState) {
@@ -189,6 +185,9 @@ function injectReferences(
   if (!references || !references.length) {
     return state as XYState;
   }
+  if (!needsInjectReferences(state)) {
+    return state as XYState;
+  }
 
   if (!annotationGroups) {
     throw new Error(
@@ -272,12 +271,14 @@ function injectReferences(
 }
 
 function convertToLegendStats(state: XYState & { valuesInLegend?: unknown }) {
-  const newState = cloneDeep(state);
-  delete newState.valuesInLegend;
+  if (!('valuesInLegend' in state)) {
+    return state;
+  }
+  delete state.valuesInLegend;
   const result: XYState = {
-    ...newState,
+    ...state,
     legend: {
-      ...newState.legend,
+      ...state.legend,
       legendStats: [
         ...new Set([
           ...(state.valuesInLegend ? [LegendStats.values] : []),

diff --git a/x-pack/plugins/lens/public/visualizations/xy/visualization.tsx b/x-pack/plugins/lens/public/visualizations/xy/visualization.tsx
@@ -270,29 +270,28 @@ export const getXyVisualization = ({
     annotationGroups?: AnnotationGroups,
     references?: SavedObjectReference[]
   ) {
-    return state
-      ? convertToRuntime(state, annotationGroups!, references)
-      : {
-          title: 'Empty XY chart',
-          legend: { isVisible: true, position: Position.Right },
-          valueLabels: 'hide',
-          preferredSeriesType: defaultSeriesType,
-          layers: [
-            {
-              layerId: addNewLayer(),
-              accessors: [],
-              position: Position.Top,
-              seriesType: defaultSeriesType,
-              showGridlines: false,
-              layerType: LayerTypes.DATA,
-              palette: mainPalette?.type === 'legacyPalette' ? mainPalette.value : undefined,
-              colorMapping:
-                mainPalette?.type === 'colorMapping'
-                  ? mainPalette.value
-                  : getColorMappingDefaults(),
-            },
-          ],
-        };
+    if (state) {
+      return convertToRuntime(state, annotationGroups!, references);
+    }
+    return {
+      title: 'Empty XY chart',
+      legend: { isVisible: true, position: Position.Right },
+      valueLabels: 'hide',
+      preferredSeriesType: defaultSeriesType,
+      layers: [
+        {
+          layerId: addNewLayer(),
+          accessors: [],
+          position: Position.Top,
+          seriesType: defaultSeriesType,
+          showGridlines: false,
+          layerType: LayerTypes.DATA,
+          palette: mainPalette?.type === 'legacyPalette' ? mainPalette.value : undefined,
+          colorMapping:
+            mainPalette?.type === 'colorMapping' ? mainPalette.value : getColorMappingDefaults(),
+        },
+      ],
+    };
   },
 
   getLayerType(layerId, state) {

diff --git a/x-pack/test/functional/apps/lens/group6/legend_statistics.ts b/x-pack/test/functional/apps/lens/group6/legend_statistics.ts
@@ -27,51 +27,51 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
     expect(text).to.eql(expectedText);
   }
 
-  async function goToTimeRangeForXyChart() {
-    await PageObjects.lens.goToTimeRange(undefined, 'Sep 22, 2015 @ 23:30:00.000');
-  }
-
   describe('lens persisted and runtime state differences properties', () => {
     before(async () => {
       await kibanaServer.importExport.load(
         'x-pack/test/functional/fixtures/kbn_archiver/lens/legend_statistics'
       );
+      await kibanaServer.uiSettings.update({
+        'timepicker:timeDefaults':
+          '{  "from": "2015-09-18T19:37:13.000Z",  "to": "2015-09-22T23:30:30.000Z"}',
+      });
+    });
+
+    after(async () => {
+      await kibanaServer.uiSettings.unset('timepicker:timeDefaults');
     });
     describe('xy chart', () => {
       it('shows values in legend for legacy valuesInLegend===true property and saves it correctly', async () => {
         const title = 'xyValuesInLegendTrue';
         await loadSavedLens(title);
-        await goToTimeRangeForXyChart();
         await expectLegendOneItem('Count of records', '2');
         await PageObjects.lens.save(title);
         await loadSavedLens(title);
-        await goToTimeRangeForXyChart();
         await expectLegendOneItem('Count of records', '2');
       });
 
       it('does not show values in legend for legacy valuesInLegend===false prop', async () => {
         await loadSavedLens('xyValuesInLegendFalse');
-        await goToTimeRangeForXyChart();
         await expectLegendOneItem('Count of records');
       });
       it('shows values in legend for legendStats===["values"] prop', async () => {
         await loadSavedLens('xyLegendStats');
-        await goToTimeRangeForXyChart();
         await expectLegendOneItem('Count of records', '2');
       });
     });
     describe('waffle chart', () => {
       it('waffleshows values in legend for legacy valuesInLegend===true property', async () => {
         await loadSavedLens('waffleValuesInLegendTrue');
-        await expectLegendOneItem('Count of records', '14,005');
+        await expectLegendOneItem('Count of records', '14,003');
       });
       it('shows values in legend for legacy showValuesInLegend===false prop', async () => {
         await loadSavedLens('waffleValuesInLegendFalse');
         await expectLegendOneItem('Count of records', undefined);
       });
       it('shows values in legend for legendStats===["values"] prop', async () => {
         await loadSavedLens('waffleLegendStats');
-        await expectLegendOneItem('Count of records', '14,005');
+        await expectLegendOneItem('Count of records', '14,003');
       });
     });
   });