[FIX] charts: Add missing "showValue" handler for Pie and waterfall c…

…harts The typing of chart.js is NOT our friend and it shows again. Expecially the type `ChartOptions` that applies a deepPartial on each of its properties, including the plugin recently introduced: `chartShowValuesPlugin`. Long story short, if provided with `showValues=true`, the plugin REQUIRES a callback function to format the data value. Unfortunately, the `DeepPartial` completely hides this requirement. This bug was also missed because we do not directly test chart.js lib in our tests and it will be the subject of a task in the near future. In the meantime, we will strengthen the test case in our main integration (Odoo). closes #4728 Task: 4081436 Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
odoo · Aug 7, 2024 · da06107 · da06107
1 parent 7705fc0
commit da06107
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 42 deletions.
diff --git a/src/helpers/figures/charts/bar_chart.ts b/src/helpers/figures/charts/bar_chart.ts
@@ -28,7 +28,6 @@ import { CellErrorType } from "../../../types/errors";
 import { Validator } from "../../../types/validator";
 import { toXlsxHexColor } from "../../../xlsx/helpers/colors";
 import { ColorGenerator } from "../../color";
-import { formatValue } from "../../format";
 import { createValidRange } from "../../range";
 import { AbstractChart } from "./abstract_chart";
 import {
@@ -38,6 +37,7 @@ import {
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
+  formatTickValue,
   getChartAxisTitleRuntime,
   getDefinedAxis,
   shouldRemoveFirstLabel,
@@ -247,23 +247,13 @@ function getBarConfiguration(
   };
   config.options.indexAxis = chart.horizontal ? "y" : "x";
 
-  const formatCallback = (value) => {
-    value = Number(value);
-    if (isNaN(value)) return value;
-    const { locale, format } = localeFormat;
-    return formatValue(value, {
-      locale,
-      format: !format && Math.abs(value) >= 1000 ? "#,##" : format,
-    });
-  };
-
   config.options.scales = {};
   const labelsAxis = { ticks: { padding: 5, color: fontColor } };
   const valuesAxis = {
     beginAtZero: true, // the origin of the y axis is always zero
     ticks: {
       color: fontColor,
-      callback: formatCallback,
+      callback: formatTickValue(localeFormat),
     },
   };
 
@@ -302,7 +292,7 @@ function getBarConfiguration(
     showValues: chart.showValues,
     background: chart.background,
     horizontal: chart.horizontal,
-    callback: formatCallback,
+    callback: formatTickValue(localeFormat),
   };
   return config;
 }

diff --git a/src/helpers/figures/charts/chart_common.ts b/src/helpers/figures/charts/chart_common.ts
@@ -8,6 +8,7 @@ import {
   DOMCoordinates,
   DOMDimension,
   Getters,
+  LocaleFormat,
   Range,
   RemoveColumnsRowsCommand,
   UID,
@@ -23,6 +24,7 @@ import {
 } from "../../../types/chart/chart";
 import { CellErrorType } from "../../../types/errors";
 import { relativeLuminance } from "../../color";
+import { formatValue } from "../../format";
 import { isDefined } from "../../misc";
 import { copyRangeWithNewSheetId } from "../../range";
 import { rangeReference } from "../../references";
@@ -428,3 +430,15 @@ export function getDefinedAxis(definition: ChartWithAxisDefinition): {
   useLeftAxis ||= !useRightAxis;
   return { useLeftAxis, useRightAxis };
 }
+
+export function formatTickValue(localeFormat: LocaleFormat) {
+  return (value: any) => {
+    value = Number(value);
+    if (isNaN(value)) return value;
+    const { locale, format } = localeFormat;
+    return formatValue(value, {
+      locale,
+      format: !format && Math.abs(value) >= 1000 ? "#,##" : format,
+    });
+  };
+}
diff --git a/src/helpers/figures/charts/chart_common_line_scatter.ts b/src/helpers/figures/charts/chart_common_line_scatter.ts
@@ -7,7 +7,12 @@ import { getChartTimeOptions, timeFormatLuxonCompatible } from "../../chart_date
 import { ColorGenerator, colorToRGBA, rgbaToHex } from "../../color";
 import { formatValue } from "../../format";
 import { deepCopy, findNextDefinedValue } from "../../misc";
-import { chartFontColor, getChartAxisTitleRuntime, getDefinedAxis } from "./chart_common";
+import {
+  chartFontColor,
+  formatTickValue,
+  getChartAxisTitleRuntime,
+  getDefinedAxis,
+} from "./chart_common";
 import {
   aggregateDataForLabels,
   filterEmptyDataPoints,
@@ -150,20 +155,12 @@ function getLineOrScatterConfiguration(
       title: getChartAxisTitleRuntime(chart.axesDesign?.x),
     },
   };
-  const formatCallback = (value) => {
-    value = Number(value);
-    if (isNaN(value)) return value;
-    const { locale, format } = options;
-    return formatValue(value, {
-      locale,
-      format: !format && Math.abs(value) >= 1000 ? "#,##" : format,
-    });
-  };
+
   const yAxis = {
     beginAtZero: true, // the origin of the y axis is always zero
     ticks: {
       color: fontColor,
-      callback: formatCallback,
+      callback: formatTickValue(options),
     },
   };
   const { useLeftAxis, useRightAxis } = getDefinedAxis(chart.getDefinition());
@@ -194,7 +191,7 @@ function getLineOrScatterConfiguration(
   config.options.plugins!.chartShowValuesPlugin = {
     showValues: chart.showValues,
     background: chart.background,
-    callback: formatCallback,
+    callback: formatTickValue(options),
   };
   return config;
 }

diff --git a/src/helpers/figures/charts/combo_chart.ts b/src/helpers/figures/charts/combo_chart.ts
@@ -10,7 +10,6 @@ import {
   CoreGetters,
   DataSet,
   ExcelChartDefinition,
-  Format,
   Getters,
   Range,
   RemoveColumnsRowsCommand,
@@ -28,7 +27,6 @@ import { CellErrorType } from "../../../types/errors";
 import { Validator } from "../../../types/validator";
 import { toXlsxHexColor } from "../../../xlsx/helpers/colors";
 import { ColorGenerator } from "../../color";
-import { formatValue } from "../../format";
 import { createValidRange } from "../../range";
 import { AbstractChart } from "./abstract_chart";
 import {
@@ -38,6 +36,7 @@ import {
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
+  formatTickValue,
   getChartAxisTitleRuntime,
   getDefinedAxis,
   shouldRemoveFirstLabel,
@@ -268,29 +267,19 @@ export function createComboChartRuntime(chart: ComboChart, getters: Getters): Co
       title: getChartAxisTitleRuntime(chart.axesDesign?.x),
     },
   };
-  const formatCallback = (format: Format | undefined) => {
-    return (value) => {
-      value = Number(value);
-      if (isNaN(value)) return value;
-      const { locale } = localeFormat;
-      return formatValue(value, {
-        locale,
-        format: !format && Math.abs(value) >= 1000 ? "#,##" : format,
-      });
-    };
-  };
+
   const leftVerticalAxis = {
     beginAtZero: true, // the origin of the y axis is always zero
     ticks: {
       color: fontColor,
-      callback: formatCallback(mainDataSetFormat),
+      callback: formatTickValue({ format: mainDataSetFormat, locale }),
     },
   };
   const rightVerticalAxis = {
     beginAtZero: true, // the origin of the y axis is always zero
     ticks: {
       color: fontColor,
-      callback: formatCallback(lineDataSetsFormat),
+      callback: formatTickValue({ format: lineDataSetsFormat, locale }),
     },
   };
   const definition = chart.getDefinition();
@@ -315,7 +304,7 @@ export function createComboChartRuntime(chart: ComboChart, getters: Getters): Co
   config.options.plugins!.chartShowValuesPlugin = {
     showValues: chart.showValues,
     background: chart.background,
-    callback: formatCallback(mainDataSetFormat),
+    callback: formatTickValue({ format: mainDataSetFormat, locale }),
   };
 
   const colors = new ColorGenerator();

diff --git a/src/helpers/figures/charts/pie_chart.ts b/src/helpers/figures/charts/pie_chart.ts
@@ -43,6 +43,7 @@ import {
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
+  formatTickValue,
   shouldRemoveFirstLabel,
   toExcelDataset,
   toExcelLabelRange,
@@ -241,7 +242,10 @@ function getPieConfiguration(
     return xLabel ? `${xLabel}: ${yLabelStr} (${percentage}%)` : `${yLabelStr} (${percentage}%)`;
   };
 
-  config.options.plugins!.chartShowValuesPlugin = { showValues: chart.showValues };
+  config.options.plugins!.chartShowValuesPlugin = {
+    showValues: chart.showValues,
+    callback: formatTickValue(localeFormat),
+  };
   return config;
 }
 

diff --git a/src/helpers/figures/charts/waterfall_chart.ts b/src/helpers/figures/charts/waterfall_chart.ts
@@ -42,6 +42,7 @@ import {
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
+  formatTickValue,
   getChartAxisTitleRuntime,
   transformChartDefinitionWithDataSetsWithZone,
   updateChartRangesWithDataSets,
@@ -312,10 +313,12 @@ function getWaterfallConfiguration(
       },
     },
   };
+
   config.options.plugins!.waterfallLinesPlugin = { showConnectorLines: chart.showConnectorLines };
   config.options.plugins!.chartShowValuesPlugin = {
     showValues: chart.showValues,
     background: chart.background,
+    callback: formatTickValue(localeFormat),
   };
 
   return config;