From 078bee35b18ea9d0a04ba4cd5520ac1574b67e64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= Date: Thu, 2 Jan 2025 17:26:15 +0100 Subject: [PATCH] [IMP] errors: display error origin position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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($D21I$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 --- src/components/error_tooltip/error_tooltip.ts | 56 +++++++++++-- .../error_tooltip/error_tooltip.xml | 15 ++++ .../cell_evaluation/evaluator.ts | 9 +- src/types/misc.ts | 9 +- tests/evaluation/evaluation.test.ts | 54 +++++++++++- .../error_tooltip_component.test.ts.snap | 3 + tests/popover/error_tooltip_component.test.ts | 83 ++++++++++++++++--- 7 files changed, 204 insertions(+), 25 deletions(-) diff --git a/src/components/error_tooltip/error_tooltip.ts b/src/components/error_tooltip/error_tooltip.ts index d870640817..d742f6a95a 100644 --- 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 { +export class ErrorToolTip extends Component { 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 => { 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 index 40dbd1a0d1..b2f2760579 100644 --- a/src/components/error_tooltip/error_tooltip.xml +++ b/src/components/error_tooltip/error_tooltip.xml @@ -1,6 +1,21 @@
+ +
Error
+
+ +
+ Caused by + +
+
+
diff --git a/src/plugins/ui_core_views/cell_evaluation/evaluator.ts b/src/plugins/ui_core_views/cell_evaluation/evaluator.ts index 0989a1fcad..ac682cf360 100644 --- 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 index df4214c252..a182809e42 100644 --- a/src/types/misc.ts +++ b/src/types/misc.ts @@ -198,7 +198,14 @@ export interface RangeCompiledFormula extends Omit = 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 index e47033ce76..79ff6293d9 100644 --- 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/__snapshots__/error_tooltip_component.test.ts.snap b/tests/popover/__snapshots__/error_tooltip_component.test.ts.snap index bb3cbf7d8c..f85bc87c43 100644 --- a/tests/popover/__snapshots__/error_tooltip_component.test.ts.snap +++ b/tests/popover/__snapshots__/error_tooltip_component.test.ts.snap @@ -17,8 +17,11 @@ exports[`Grid integration can display error tooltip 1`] = ` class="o-error-tooltip-message" > The divisor must be different from zero. +
+ +
`; diff --git a/tests/popover/error_tooltip_component.test.ts b/tests/popover/error_tooltip_component.test.ts index 513f356f23..e547af3b5d 100644 --- 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,6 +23,7 @@ import { mountComponent, mountSpreadsheet, nextTick, + toCellPosition, } from "../test_helpers/helpers"; mockChart(); @@ -31,21 +31,30 @@ 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) { + ({ 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", () => {