[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 #4788 Task: 4081436 X-original-commit: da06107 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
odoo · Aug 7, 2024 · 38f2906 · 38f2906
1 parent f477abd
commit 38f2906
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 41 deletions.
diff --git a/src/helpers/figures/charts/bar_chart.ts b/src/helpers/figures/charts/bar_chart.ts
@@ -27,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 {
@@ -39,6 +38,7 @@ import {
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
+  formatTickValue,
   getChartAxisTitleRuntime,
   getDefinedAxis,
   getTrendDatasetForBarChart,
@@ -267,23 +267,13 @@ export function createBarChartRuntime(chart: BarChart, getters: Getters): BarCha
   };
   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),
     },
   };
 
@@ -323,7 +313,7 @@ export function createBarChartRuntime(chart: BarChart, getters: Getters): BarCha
     showValues: chart.showValues,
     background: chart.background,
     horizontal: chart.horizontal,
-    callback: formatCallback,
+    callback: formatTickValue(localeFormat),
   };
 
   const colors = new ColorGenerator();

diff --git a/src/helpers/figures/charts/chart_common.ts b/src/helpers/figures/charts/chart_common.ts
@@ -17,6 +17,7 @@ import {
   DOMCoordinates,
   DOMDimension,
   Getters,
+  LocaleFormat,
   Range,
   RemoveColumnsRowsCommand,
   UID,
@@ -33,6 +34,7 @@ import {
 } from "../../../types/chart/chart";
 import { CellErrorType } from "../../../types/errors";
 import { lightenColor, relativeLuminance } from "../../color";
+import { formatValue } from "../../format";
 import { isDefined, range } from "../../misc";
 import { copyRangeWithNewSheetId } from "../../range";
 import { rangeReference } from "../../references";
@@ -545,3 +547,15 @@ export function interpolateData(
       return [];
   }
 }
+
+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
@@ -18,6 +18,7 @@ import {
   TREND_LINE_XAXIS_ID,
   chartFontColor,
   computeChartPadding,
+  formatTickValue,
   getChartAxisTitleRuntime,
   getDefinedAxis,
   getFullTrendingLineDataSet,
@@ -260,20 +261,12 @@ export function createLineOrScatterChartRuntime(
   config.options.scales = {
     x: xAxis,
   };
-  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());
@@ -304,7 +297,7 @@ export function createLineOrScatterChartRuntime(
   config.options.plugins!.chartShowValuesPlugin = {
     showValues: chart.showValues,
     background: chart.background,
-    callback: formatCallback,
+    callback: formatTickValue(options),
   };
 
   if (

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 {
@@ -40,6 +38,7 @@ import {
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
+  formatTickValue,
   getChartAxisTitleRuntime,
   getDefinedAxis,
   getTrendDatasetForBarChart,
@@ -274,29 +273,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();
@@ -321,7 +310,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;