[FIX] chart: label header prevents creating linear/time charts

The helpers to check if the labels of a chart can be used to create a linear/time axis weren't taking into account that the first label should sometimes be ignored because of `chart.dataSetsHaveTitle`. It was impossible to create a linear/time chart when the first label was a (non-number) title. Also the helper `getChartLabelFormat` only supported columns as label ranges, where the label range can be rows as well. closes #5706 Task: 4543496 Signed-off-by: Rémi Rahir (rar) <[email protected]> Signed-off-by: Adrien Minne (adrm) <[email protected]>
odoo · Feb 13, 2025 · d732b75 · d732b75
1 parent 07b2310
commit d732b75
Show file tree

Hide file tree

Showing 6 changed files with 136 additions and 62 deletions.
diff --git a/src/components/side_panel/chart/line_chart/line_chart_config_panel.ts b/src/components/side_panel/chart/line_chart/line_chart_config_panel.ts
@@ -9,7 +9,12 @@ export class LineConfigPanel extends GenericChartConfigPanel {
   get canTreatLabelsAsText() {
     const chart = this.env.model.getters.getChart(this.props.figureId);
     if (chart && chart instanceof LineChart) {
-      return canChartParseLabels(chart.labelRange, this.env.model.getters);
+      return canChartParseLabels(
+        chart.getDefinition(),
+        chart.dataSets,
+        chart.labelRange,
+        this.env.model.getters
+      );
     }
     return false;
   }

diff --git a/src/components/side_panel/chart/scatter_chart/scatter_chart_config_panel.ts b/src/components/side_panel/chart/scatter_chart/scatter_chart_config_panel.ts
@@ -9,7 +9,12 @@ export class ScatterConfigPanel extends GenericChartConfigPanel {
   get canTreatLabelsAsText() {
     const chart = this.env.model.getters.getChart(this.props.figureId);
     if (chart && chart instanceof ScatterChart) {
-      return canChartParseLabels(chart.labelRange, this.env.model.getters);
+      return canChartParseLabels(
+        chart.getDefinition(),
+        chart.dataSets,
+        chart.labelRange,
+        this.env.model.getters
+      );
     }
     return false;
   }

diff --git a/src/helpers/figures/charts/chart_factory.ts b/src/helpers/figures/charts/chart_factory.ts
@@ -13,12 +13,14 @@ import {
   UID,
   Zone,
 } from "../../../types";
+import { LineChartDefinition } from "../../../types/chart";
 import { ChartDefinition, ChartRuntime } from "../../../types/chart/chart";
 import { CoreGetters, Getters } from "../../../types/getters";
 import { Validator } from "../../../types/validator";
 import { getZoneArea, zoneToDimension, zoneToXc } from "../../zones";
 import { AbstractChart } from "./abstract_chart";
 import { createDataSets } from "./chart_common";
+import { LineChart } from "./line_chart";
 import { canChartParseLabels, getData } from "./runtime";
 
 /**
@@ -131,20 +133,21 @@ export function getSmartChartDefinition(zone: Zone, getters: Getters): ChartDefi
   // Only display legend for several datasets.
   const newLegendPos = dataSetZone.right === dataSetZone.left ? "none" : "top";
 
-  const labelRange = labelRangeXc ? getters.getRangeFromSheetXC(sheetId, labelRangeXc) : undefined;
-  if (canChartParseLabels(labelRange, getters)) {
-    return {
-      title: {},
-      dataSets,
-      labelsAsText: false,
-      stacked: false,
-      aggregated: false,
-      cumulative: false,
-      labelRange: labelRangeXc,
-      type: "line",
-      dataSetsHaveTitle,
-      legendPosition: newLegendPos,
-    };
+  const lineChartDefinition: LineChartDefinition = {
+    title: {},
+    dataSets,
+    labelsAsText: false,
+    stacked: false,
+    aggregated: false,
+    cumulative: false,
+    labelRange: labelRangeXc,
+    type: "line",
+    dataSetsHaveTitle,
+    legendPosition: newLegendPos,
+  };
+  const chart = new LineChart(lineChartDefinition, sheetId, getters);
+  if (canChartParseLabels(lineChartDefinition, chart.dataSets, chart.labelRange, getters)) {
+    return lineChartDefinition;
   }
   const _dataSets = createDataSets(getters, dataSets, sheetId, dataSetsHaveTitle);
   if (

diff --git a/src/helpers/figures/charts/runtime/chart_data_extractor.ts b/src/helpers/figures/charts/runtime/chart_data_extractor.ts
@@ -40,6 +40,8 @@ import { isDateTimeFormat } from "../../../format/format";
 import { deepCopy, findNextDefinedValue, range } from "../../../misc";
 import { isNumber } from "../../../numbers";
 import { recomputeZones } from "../../../recompute_zones";
+import { positions } from "../../../zones";
+import { shouldRemoveFirstLabel } from "../chart_common";
 import { truncateLabel } from "../chart_ui_common";
 
 export function getBarChartData(
@@ -51,11 +53,7 @@ export function getBarChartData(
   const labelValues = getChartLabelValues(getters, dataSets, labelRange);
   let labels = labelValues.formattedValues;
   let dataSetsValues = getChartDatasetValues(getters, dataSets);
-  if (
-    definition.dataSetsHaveTitle &&
-    dataSetsValues[0] &&
-    labels.length > dataSetsValues[0].data.length
-  ) {
+  if (shouldRemoveFirstLabel(labelRange, dataSets[0], definition.dataSetsHaveTitle || false)) {
     labels.shift();
   }
 
@@ -124,15 +122,16 @@ export function getLineChartData(
   labelRange: Range | undefined,
   getters: Getters
 ): ChartRuntimeGenerationArgs {
-  const axisType = getChartAxisType(definition, labelRange, getters);
+  const axisType = getChartAxisType(definition, dataSets, labelRange, getters);
   const labelValues = getChartLabelValues(getters, dataSets, labelRange);
   let labels = axisType === "linear" ? labelValues.values : labelValues.formattedValues;
   let dataSetsValues = getChartDatasetValues(getters, dataSets);
-  if (
-    definition.dataSetsHaveTitle &&
-    dataSetsValues[0] &&
-    labels.length > dataSetsValues[0].data.length
-  ) {
+  const removeFirstLabel = shouldRemoveFirstLabel(
+    labelRange,
+    dataSets[0],
+    definition.dataSetsHaveTitle || false
+  );
+  if (removeFirstLabel) {
     labels.shift();
   }
 
@@ -146,7 +145,7 @@ export function getLineChartData(
 
   const leftAxisFormat = getChartDatasetFormat(getters, dataSets, "left");
   const rightAxisFormat = getChartDatasetFormat(getters, dataSets, "right");
-  const labelsFormat = getChartLabelFormat(getters, labelRange);
+  const labelsFormat = getChartLabelFormat(getters, labelRange, removeFirstLabel);
   const axisFormats = { y: leftAxisFormat, y1: rightAxisFormat, x: labelsFormat };
 
   const trendDataSetsValues: (Point[] | undefined)[] = [];
@@ -194,11 +193,7 @@ export function getPieChartData(
   const labelValues = getChartLabelValues(getters, dataSets, labelRange);
   let labels = labelValues.formattedValues;
   let dataSetsValues = getChartDatasetValues(getters, dataSets);
-  if (
-    definition.dataSetsHaveTitle &&
-    dataSetsValues[0] &&
-    labels.length > dataSetsValues[0].data.length
-  ) {
+  if (shouldRemoveFirstLabel(labelRange, dataSets[0], definition.dataSetsHaveTitle || false)) {
     labels.shift();
   }
 
@@ -229,11 +224,7 @@ export function getRadarChartData(
   const labelValues = getChartLabelValues(getters, dataSets, labelRange);
   let labels = labelValues.formattedValues;
   let dataSetsValues = getChartDatasetValues(getters, dataSets);
-  if (
-    definition.dataSetsHaveTitle &&
-    dataSetsValues[0] &&
-    labels.length > dataSetsValues[0].data.length
-  ) {
+  if (shouldRemoveFirstLabel(labelRange, dataSets[0], definition.dataSetsHaveTitle || false)) {
     labels.shift();
   }
 
@@ -263,7 +254,7 @@ export function getGeoChartData(
 ): GeoChartRuntimeGenerationArgs {
   const labelValues = getChartLabelValues(getters, dataSets, labelRange);
   let labels = labelValues.formattedValues;
-  if (definition.dataSetsHaveTitle) {
+  if (shouldRemoveFirstLabel(labelRange, dataSets[0], definition.dataSetsHaveTitle || false)) {
     labels.shift();
   }
   let dataSetsValues = getChartDatasetValues(getters, dataSets);
@@ -462,53 +453,83 @@ function normalizeLabels(
 }
 
 function getChartAxisType(
-  chart: GenericDefinition<LineChartDefinition>,
+  definition: GenericDefinition<LineChartDefinition>,
+  dataSets: DataSet[],
   labelRange: Range | undefined,
   getters: Getters
 ): AxisType {
-  if (isDateChart(chart, labelRange, getters) && isLuxonTimeAdapterInstalled()) {
+  if (isDateChart(definition, dataSets, labelRange, getters) && isLuxonTimeAdapterInstalled()) {
     return "time";
   }
-  if (isLinearChart(chart, labelRange, getters)) {
+  if (isLinearChart(definition, dataSets, labelRange, getters)) {
     return "linear";
   }
   return "category";
 }
 
 function isDateChart(
   definition: GenericDefinition<LineChartDefinition>,
+  dataSets: DataSet[],
   labelRange: Range | undefined,
   getters: Getters
 ): boolean {
-  return !definition.labelsAsText && canBeDateChart(labelRange, getters);
+  return !definition.labelsAsText && canBeDateChart(definition, dataSets, labelRange, getters);
 }
 
 function isLinearChart(
   definition: GenericDefinition<LineChartDefinition>,
+  dataSets: DataSet[],
   labelRange: Range | undefined,
   getters: Getters
 ): boolean {
-  return !definition.labelsAsText && canBeLinearChart(labelRange, getters);
+  return !definition.labelsAsText && canBeLinearChart(definition, dataSets, labelRange, getters);
 }
 
-export function canChartParseLabels(labelRange: Range | undefined, getters: Getters): boolean {
-  return canBeDateChart(labelRange, getters) || canBeLinearChart(labelRange, getters);
+export function canChartParseLabels(
+  definition: GenericDefinition<LineChartDefinition>,
+  dataSets: DataSet[],
+  labelRange: Range | undefined,
+  getters: Getters
+): boolean {
+  return (
+    canBeDateChart(definition, dataSets, labelRange, getters) ||
+    canBeLinearChart(definition, dataSets, labelRange, getters)
+  );
 }
 
-function canBeDateChart(labelRange: Range | undefined, getters: Getters): boolean {
-  if (!labelRange || !canBeLinearChart(labelRange, getters)) {
+function canBeDateChart(
+  definition: GenericDefinition<LineChartDefinition>,
+  dataSets: DataSet[],
+  labelRange: Range | undefined,
+  getters: Getters
+): boolean {
+  if (!labelRange || !canBeLinearChart(definition, dataSets, labelRange, getters)) {
     return false;
   }
-  const labelFormat = getChartLabelFormat(getters, labelRange);
+  const removeFirstLabel = shouldRemoveFirstLabel(
+    labelRange,
+    dataSets[0],
+    definition.dataSetsHaveTitle || false
+  );
+  const labelFormat = getChartLabelFormat(getters, labelRange, removeFirstLabel);
   return Boolean(labelFormat && timeFormatLuxonCompatible.test(labelFormat));
 }
 
-function canBeLinearChart(labelRange: Range | undefined, getters: Getters): boolean {
+function canBeLinearChart(
+  definition: GenericDefinition<LineChartDefinition>,
+  dataSets: DataSet[],
+  labelRange: Range | undefined,
+  getters: Getters
+): boolean {
   if (!labelRange) {
     return false;
   }
 
   const labels = getters.getRangeValues(labelRange);
+  if (shouldRemoveFirstLabel(labelRange, dataSets[0], definition.dataSetsHaveTitle || false)) {
+    labels.shift();
+  }
+
   if (labels.some((label) => isNaN(Number(label)) && label)) {
     return false;
   }
@@ -656,22 +677,21 @@ function aggregateDataForLabels(
 
 export function getChartLabelFormat(
   getters: Getters,
-  range: Range | undefined
+  range: Range | undefined,
+  shouldRemoveFirstLabel: boolean
 ): Format | undefined {
   if (!range) return undefined;
 
-  const {
-    sheetId,
-    zone: { left, top, bottom },
-  } = range;
-  for (let row = top; row <= bottom; row++) {
-    const format = getters.getEvaluatedCell({ sheetId, col: left, row }).format;
-    if (format) {
-      return format;
-    }
+  const { sheetId, zone } = range;
+
+  const formats = positions(zone).map(
+    (position) => getters.getEvaluatedCell({ sheetId, ...position }).format
+  );
+  if (shouldRemoveFirstLabel) {
+    formats.shift();
   }
 
-  return undefined;
+  return formats.find((format) => format !== undefined);
 }
 
 function getChartLabelValues(

diff --git a/tests/figures/chart/__snapshots__/chart_plugin.test.ts.snap b/tests/figures/chart/__snapshots__/chart_plugin.test.ts.snap
@@ -1043,7 +1043,6 @@ exports[`datasource tests create chart with only the dataset title (no data) 1`]
     "data": {
       "datasets": [],
       "labels": [
-        "P4",
         "P5",
         "P6",
       ],

diff --git a/tests/figures/chart/chart_plugin.test.ts b/tests/figures/chart/chart_plugin.test.ts
@@ -2734,6 +2734,29 @@ describe("Linear/Time charts", () => {
     expect(chart.data!.datasets![0].data![0]).toEqual({ y: 10, x: formattedValue });
   });
 
+  test("date chart: rows datasets/labels are supported", () => {
+    setGrid(model, { A1: "2", B1: "3", A2: "1", B2: "10" });
+    setFormat(model, "B1", "mm/dd/yyyy");
+    createChart(
+      model,
+      {
+        type: "line",
+        dataSets: [{ dataRange: "A2:B2" }],
+        labelRange: "A1:B1",
+        labelsAsText: false,
+        dataSetsHaveTitle: false,
+      },
+      chartId
+    );
+
+    const chart = (model.getters.getChartRuntime(chartId) as LineChartRuntime).chartJsConfig;
+    expect(chart.data!.datasets![0].data).toEqual([
+      { y: 1, x: "2" },
+      { y: 10, x: "01/02/1900" },
+    ]);
+    expect(chart.options?.scales?.x?.type).toEqual("time");
+  });
+
   test.each(["bar", "line", "pie"] as const)("long labels are truncated in %s chart", (type) => {
     setCellContent(model, "A2", "This is a very long label that should be truncated");
     setCellContent(model, "B1", "First dataset");
@@ -2800,6 +2823,25 @@ describe("Linear/Time charts", () => {
     expect(data.datasets![0].data![1]).toEqual({ y: 11, x: undefined });
   });
 
+  test("can create linear chart with non-number header in the label range", () => {
+    setGrid(model, { A1: "x", A2: "1", B1: "y", B2: "10" });
+    createChart(
+      model,
+      {
+        type: "line",
+        dataSets: [{ dataRange: "B1:B2" }],
+        labelRange: "A1:A2",
+        labelsAsText: false,
+        dataSetsHaveTitle: true,
+      },
+      chartId
+    );
+    const chart = (model.getters.getChartRuntime(chartId) as LineChartRuntime).chartJsConfig;
+    expect(chart.options?.scales?.x?.type).toEqual("linear");
+    expect(chart.data!.labels).toEqual(["1"]);
+    expect(chart.data!.datasets![0].data).toEqual([{ y: 10, x: "1" }]);
+  });
+
   test("snapshot test of chartJS configuration for linear chart", () => {
     createChart(
       model,
-Original file line number
+Diff line change
@@ Expand Up @@
         "data": {
           "datasets": [],
           "labels": [
-            "P4",
             "P5",
             "P6",
           ],
@@ Expand Down @@