[IMP] errors: display error origin position

Purpose ------- Let's say a cell is evaluated to an error because one of its inputs is an error itself. The tooltip shows the error message, but you don't know where it comes from. If the formula has many reference, you have to check every one of those references to find the original error. As an example, if the formula =IF(OR(AND($D21<G$17,$D21>I$1),AND(I$1<$E21,I$1>$D21)),1,"") results with an error. Where does it come from ? D21 ? J1 ? I1 ? E21 ? Spec ---- Add below the error message the cell from which the error is coming from. The cell reference should be clickable to jump to cell. Task: 4452745
odoo · Jan 7, 2025 · 30f3c79 · 30f3c79
1 parent 3bfd3d1
commit 30f3c79
Show file tree

Hide file tree

Showing 6 changed files with 201 additions and 25 deletions.
diff --git a/src/components/error_tooltip/error_tooltip.ts b/src/components/error_tooltip/error_tooltip.ts
@@ -1,6 +1,7 @@
 import { Component } from "@odoo/owl";
+import { deepEquals, positionToZone } from "../../helpers";
 import { _t } from "../../translation";
-import { CellValueType } from "../../types";
+import { CellPosition, CellValueType, EvaluatedCell, SpreadsheetChildEnv } from "../../types";
 import { CellPopoverComponent, PopoverBuilders } from "../../types/cell_popovers";
 import { css } from "../helpers/css";
 
@@ -29,28 +30,64 @@ export interface ErrorToolTipMessage {
 }
 
 interface ErrorToolTipProps {
+  cellPosition: CellPosition;
   errors: ErrorToolTipMessage[];
   onClosed?: () => void;
 }
 
-export class ErrorToolTip extends Component<ErrorToolTipProps> {
+export class ErrorToolTip extends Component<ErrorToolTipProps, SpreadsheetChildEnv> {
   static maxSize = { maxHeight: ERROR_TOOLTIP_MAX_HEIGHT };
   static template = "o-spreadsheet-ErrorToolTip";
   static props = {
+    cellPosition: Object,
     errors: Array,
     onClosed: { type: Function, optional: true },
   };
+
+  get evaluationError() {
+    const cell = this.env.model.getters.getEvaluatedCell(this.props.cellPosition);
+    if (cell.message) {
+      return cell;
+    }
+    return undefined;
+  }
+
+  get errorOriginPositionString() {
+    const evaluationError = this.evaluationError;
+    const position = evaluationError?.errorOriginPosition;
+    if (!position || deepEquals(position, this.props.cellPosition)) {
+      return "";
+    }
+    const sheetId = position.sheetId;
+    return this.env.model.getters.getRangeString(
+      this.env.model.getters.getRangeFromZone(sheetId, positionToZone(position)),
+      this.env.model.getters.getActiveSheetId()
+    );
+  }
+
+  selectCell() {
+    const position = this.evaluationError?.errorOriginPosition;
+    if (!position) {
+      return;
+    }
+    const activeSheetId = this.env.model.getters.getActiveSheetId();
+    if (position.sheetId !== activeSheetId) {
+      this.env.model.dispatch("ACTIVATE_SHEET", {
+        sheetIdFrom: activeSheetId,
+        sheetIdTo: position.sheetId,
+      });
+    }
+    this.env.model.selection.selectCell(position.col, position.row);
+  }
 }
 
 export const ErrorToolTipPopoverBuilder: PopoverBuilders = {
   onHover: (position, getters): CellPopoverComponent<typeof ErrorToolTip> => {
     const cell = getters.getEvaluatedCell(position);
     const errors: ErrorToolTipMessage[] = [];
+    let evaluationError: EvaluatedCell | undefined;
     if (cell.type === CellValueType.error && !!cell.message) {
-      errors.push({
-        title: _t("Error"),
-        message: cell.message,
-      });
+      evaluationError = cell;
     }
 
     const validationErrorMessage = getters.getInvalidDataValidationMessage(position);
@@ -61,13 +98,16 @@ export const ErrorToolTipPopoverBuilder: PopoverBuilders = {
       });
     }
 
-    if (!errors.length) {
+    if (!errors.length && !evaluationError) {
       return { isOpen: false };
     }
 
     return {
       isOpen: true,
-      props: { errors: errors },
+      props: {
+        cellPosition: position,
+        errors,
+      },
       Component: ErrorToolTip,
       cellCorner: "TopRight",
     };

diff --git a/src/components/error_tooltip/error_tooltip.xml b/src/components/error_tooltip/error_tooltip.xml
@@ -1,6 +1,21 @@
 <templates>
   <t t-name="o-spreadsheet-ErrorToolTip">
     <div class="o-error-tooltip">
+      <t t-if="evaluationError">
+        <div class="o-error-tooltip-title fw-bold text-danger">Error</div>
+        <div class="o-error-tooltip-message">
+          <t t-esc="evaluationError.message"/>
+          <div class="fst-italic" t-if="errorOriginPositionString">
+            Caused by
+            <span
+              t-esc="errorOriginPositionString"
+              class="o-button-link"
+              t-on-click="selectCell"
+              title="Click to select the cell"
+            />
+          </div>
+        </div>
+      </t>
       <t t-foreach="props.errors" t-as="error" t-key="error_index">
         <div class="o-error-tooltip-title fw-bold text-danger">
           <t t-esc="error.title"/>

diff --git a/src/plugins/ui_core_views/cell_evaluation/evaluator.ts b/src/plugins/ui_core_views/cell_evaluation/evaluator.ts
@@ -366,11 +366,15 @@ export class Evaluator {
     );
 
     if (!isMatrix(formulaReturn)) {
-      return createEvaluatedCell(
+      const evaluatedCell = createEvaluatedCell(
         nullValueToZeroValue(formulaReturn),
         this.getters.getLocale(),
         cellData
       );
+      if (evaluatedCell.type === CellValueType.error) {
+        evaluatedCell.errorOriginPosition = formulaReturn.errorOriginPosition ?? formulaPosition;
+      }
+      return evaluatedCell;
     }
 
     this.assertSheetHasEnoughSpaceToSpreadFormulaResult(formulaPosition, formulaReturn);
@@ -494,6 +498,9 @@ export class Evaluator {
         this.getters.getLocale(),
         cell
       );
+      if (evaluatedCell.type === CellValueType.error) {
+        evaluatedCell.errorOriginPosition = matrixResult[i][j].errorOriginPosition ?? position;
+      }
       this.evaluatedCells.set(position, evaluatedCell);
     };
     return spreadValues;

diff --git a/src/types/misc.ts b/src/types/misc.ts
@@ -198,7 +198,14 @@ export interface RangeCompiledFormula extends Omit<CompiledFormula, "dependencie
 }
 
 export type Matrix<T = unknown> = T[][];
-export type FunctionResultObject = { value: CellValue; format?: Format; message?: string };
+
+export type FunctionResultObject = {
+  value: CellValue;
+  format?: Format;
+  errorOriginPosition?: CellPosition;
+  message?: string;
+};
+
 export type FunctionResultNumber = { value: number; format?: string };
 
 // FORMULA FUNCTION VALUE AND FORMAT INPUT

diff --git a/tests/evaluation/evaluation.test.ts b/tests/evaluation/evaluation.test.ts
@@ -22,7 +22,12 @@ import {
   getCellError,
   getEvaluatedCell,
 } from "../test_helpers/getters_helpers";
-import { evaluateCell, evaluateGrid, restoreDefaultFunctions } from "../test_helpers/helpers";
+import {
+  evaluateCell,
+  evaluateGrid,
+  restoreDefaultFunctions,
+  toCellPosition,
+} from "../test_helpers/helpers";
 import resetAllMocks = jest.resetAllMocks;
 
 describe("evaluateCells", () => {
@@ -1086,8 +1091,51 @@ describe("evaluateCells", () => {
     expect(getCellError(model, "C1")).toBe("Invalid expression");
   });
 
-  // TO DO: add tests for exp format (ex: 4E10)
-  // RO DO: add tests for DATE string format (ex match: "28 02 2020")
+  test("error original position from the cell itself", () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    setCellContent(model, "A1", "=0/0");
+    expect(getEvaluatedCell(model, "A1").errorOriginPosition).toEqual(
+      toCellPosition(sheetId, "A1")
+    );
+  });
+
+  test("error original position from simple references", () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    setCellContent(model, "A1", "=0/0");
+    setCellContent(model, "A2", "=A1");
+    setCellContent(model, "A3", "=A2");
+    const A1 = toCellPosition(sheetId, "A1");
+    expect(getEvaluatedCell(model, "A2").errorOriginPosition).toEqual(A1);
+    expect(getEvaluatedCell(model, "A3").errorOriginPosition).toEqual(A1);
+  });
+
+  test("error original position from a range reference", () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    setCellContent(model, "A1", "1");
+    setCellContent(model, "A2", "=0/0");
+    setCellContent(model, "A3", "=MAX(A1:A2)");
+    expect(getEvaluatedCell(model, "A3").errorOriginPosition).toEqual(
+      toCellPosition(sheetId, "A2")
+    );
+  });
+
+  test("error original position in a spilled result", () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    setCellContent(model, "A1", "1");
+    setCellContent(model, "A2", "=0/0");
+    setCellContent(model, "A3", "=TRANSPOSE(A1:A2)");
+    setCellContent(model, "A4", "=TRANSPOSE(A3:B3)");
+    expect(getEvaluatedCell(model, "B3").errorOriginPosition).toEqual(
+      toCellPosition(sheetId, "A2")
+    );
+    expect(getEvaluatedCell(model, "A5").errorOriginPosition).toEqual(
+      toCellPosition(sheetId, "A2")
+    );
+  });
 });
 
 describe("evaluate formula getter", () => {

diff --git a/tests/popover/error_tooltip_component.test.ts b/tests/popover/error_tooltip_component.test.ts
@@ -1,17 +1,16 @@
-import { Model } from "../../src";
-import {
-  ErrorToolTip,
-  ErrorToolTipMessage,
-} from "../../src/components/error_tooltip/error_tooltip";
+import { Model, PropsOf } from "../../src";
+import { ErrorToolTip } from "../../src/components/error_tooltip/error_tooltip";
 import { DEFAULT_CELL_HEIGHT, DEFAULT_CELL_WIDTH } from "../../src/constants";
 import {
   addDataValidation,
   createChart,
+  createSheet,
   merge,
   setCellContent,
 } from "../test_helpers/commands_helpers";
 import { TEST_CHART_DATA } from "../test_helpers/constants";
 import {
+  click,
   clickCell,
   gridMouseEvent,
   hoverCell,
@@ -24,28 +23,38 @@ import {
   mountComponent,
   mountSpreadsheet,
   nextTick,
+  toCellPosition,
 } from "../test_helpers/helpers";
 
 mockChart();
 
 describe("Error tooltip component", () => {
   let fixture: HTMLElement;
 
-  async function mountErrorTooltip(errors: ErrorToolTipMessage[]) {
-    ({ fixture } = await mountComponent(ErrorToolTip, { props: { errors } }));
+  async function mountErrorTooltip(model: Model, props: PropsOf<typeof ErrorToolTip>) {
+    ({ fixture } = await mountComponent(ErrorToolTip, {
+      props,
+      model,
+    }));
   }
 
   test("Can display an error message", async () => {
-    await mountErrorTooltip([{ message: "This is an error", title: "Error" }]);
+    await mountErrorTooltip(new Model(), {
+      errors: [{ message: "This is an error", title: "Error" }],
+      cellPosition: toCellPosition("sheet1", "A1"),
+    });
     expect(fixture.querySelector(".o-error-tooltip-title")?.textContent).toBe("Error");
     expect(fixture.querySelector(".o-error-tooltip-message")?.textContent).toBe("This is an error");
   });
 
   test("Can display multiple error messages", async () => {
-    await mountErrorTooltip([
-      { message: "This is an error", title: "Error" },
-      { message: "Invalid data", title: "Invalid" },
-    ]);
+    await mountErrorTooltip(new Model(), {
+      errors: [
+        { message: "This is an error", title: "Error" },
+        { message: "Invalid data", title: "Invalid" },
+      ],
+      cellPosition: toCellPosition("sheet1", "A1"),
+    });
     const titles = fixture.querySelectorAll(".o-error-tooltip-title");
     const messages = fixture.querySelectorAll(".o-error-tooltip-message");
     expect(titles).toHaveLength(2);
@@ -57,6 +66,56 @@ describe("Error tooltip component", () => {
     expect(titles[1].textContent).toBe("Invalid");
     expect(messages[1].textContent).toBe("Invalid data");
   });
+
+  test("can display error origin position", async () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    setCellContent(model, "A1", "=1/0");
+    setCellContent(model, "A2", "=A1");
+    await mountErrorTooltip(model, {
+      errors: [],
+      cellPosition: toCellPosition(sheetId, "A2"),
+    });
+    expect(".fst-italic").toHaveText(" Caused by A1");
+  });
+
+  test("can display error position from another sheet", async () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    createSheet(model, { sheetId: "sheet2" });
+    setCellContent(model, "A1", "=1/0", "sheet2");
+    setCellContent(model, "A2", "=Sheet2!A1");
+    await mountErrorTooltip(model, {
+      errors: [],
+      cellPosition: toCellPosition(sheetId, "A2"),
+    });
+    expect(".fst-italic").toHaveText(" Caused by Sheet2!A1");
+  });
+
+  test("clicking on error position selects the position", async () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    createSheet(model, { sheetId: "sheet2" });
+    setCellContent(model, "J10", "=1/0", "sheet2");
+    setCellContent(model, "A2", "=Sheet2!J10");
+    await mountErrorTooltip(model, {
+      errors: [],
+      cellPosition: toCellPosition(sheetId, "A2"),
+    });
+    click(fixture, ".o-button-link");
+    expect(model.getters.getActivePosition()).toEqual(toCellPosition("sheet2", "J10"));
+  });
+
+  test("does not display error origin position if it is the same cell", async () => {
+    const model = new Model();
+    const sheetId = model.getters.getActiveSheetId();
+    setCellContent(model, "A1", "=1/0");
+    await mountErrorTooltip(model, {
+      errors: [],
+      cellPosition: toCellPosition(sheetId, "A1"),
+    });
+    expect(".fst-italic").toHaveCount(0);
+  });
 });
 
 describe("Grid integration", () => {