From dba11abf7746c81e420670e20b0c6196fcd8d618 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= Date: Tue, 7 Jan 2025 13:24:41 +0100 Subject: [PATCH] [REF] ErrorToolTip: make it non-generic Since the previous commit, the ErrorToolTip component is no longer completely independant from the source of errors (it needs to know the evaluation error). So the remaining use of the "generic part" is now only for data validation. A generic system for a single use case is not a generic system. Let's make it more direct and move everything to the component. Task: 4452745 --- src/components/error_tooltip/error_tooltip.ts | 53 ++++++---------- .../error_tooltip/error_tooltip.xml | 8 +-- .../error_tooltip_component.test.ts.snap | 1 - tests/popover/error_tooltip_component.test.ts | 62 +++++++------------ 4 files changed, 44 insertions(+), 80 deletions(-) diff --git a/src/components/error_tooltip/error_tooltip.ts b/src/components/error_tooltip/error_tooltip.ts index d742f6a95..3ce55f4d8 100644 --- a/src/components/error_tooltip/error_tooltip.ts +++ b/src/components/error_tooltip/error_tooltip.ts @@ -1,7 +1,6 @@ import { Component } from "@odoo/owl"; import { deepEquals, positionToZone } from "../../helpers"; -import { _t } from "../../translation"; -import { CellPosition, CellValueType, EvaluatedCell, SpreadsheetChildEnv } from "../../types"; +import { CellPosition, CellValueType, SpreadsheetChildEnv } from "../../types"; import { CellPopoverComponent, PopoverBuilders } from "../../types/cell_popovers"; import { css } from "../helpers/css"; @@ -24,14 +23,8 @@ css/* scss */ ` } `; -export interface ErrorToolTipMessage { - title: string; - message: string; -} - interface ErrorToolTipProps { cellPosition: CellPosition; - errors: ErrorToolTipMessage[]; onClosed?: () => void; } @@ -40,10 +33,13 @@ export class ErrorToolTip extends Component => { const cell = getters.getEvaluatedCell(position); - const errors: ErrorToolTipMessage[] = []; - let evaluationError: EvaluatedCell | undefined; - if (cell.type === CellValueType.error && !!cell.message) { - evaluationError = cell; - } - - const validationErrorMessage = getters.getInvalidDataValidationMessage(position); - if (validationErrorMessage) { - errors.push({ - title: _t("Invalid"), - message: validationErrorMessage, - }); + if ( + (cell.type === CellValueType.error && !!cell.message) || + getters.getInvalidDataValidationMessage(position) + ) { + return { + isOpen: true, + props: { + cellPosition: position, + }, + Component: ErrorToolTip, + cellCorner: "TopRight", + }; } - - if (!errors.length && !evaluationError) { - return { isOpen: false }; - } - - return { - isOpen: true, - props: { - cellPosition: position, - errors, - }, - Component: ErrorToolTip, - cellCorner: "TopRight", - }; + return { isOpen: false }; }, }; diff --git a/src/components/error_tooltip/error_tooltip.xml b/src/components/error_tooltip/error_tooltip.xml index b2f276057..4f60336b2 100644 --- a/src/components/error_tooltip/error_tooltip.xml +++ b/src/components/error_tooltip/error_tooltip.xml @@ -16,12 +16,10 @@ - -
- -
+ +
Invalid
- +
diff --git a/tests/popover/__snapshots__/error_tooltip_component.test.ts.snap b/tests/popover/__snapshots__/error_tooltip_component.test.ts.snap index f85bc87c4..b883a1de5 100644 --- a/tests/popover/__snapshots__/error_tooltip_component.test.ts.snap +++ b/tests/popover/__snapshots__/error_tooltip_component.test.ts.snap @@ -21,7 +21,6 @@ exports[`Grid integration can display error tooltip 1`] = ` - `; diff --git a/tests/popover/error_tooltip_component.test.ts b/tests/popover/error_tooltip_component.test.ts index e547af3b5..87d035af5 100644 --- a/tests/popover/error_tooltip_component.test.ts +++ b/tests/popover/error_tooltip_component.test.ts @@ -1,4 +1,4 @@ -import { Model, PropsOf } from "../../src"; +import { Model } from "../../src"; import { ErrorToolTip } from "../../src/components/error_tooltip/error_tooltip"; import { DEFAULT_CELL_HEIGHT, DEFAULT_CELL_WIDTH } from "../../src/constants"; import { @@ -31,89 +31,73 @@ mockChart(); describe("Error tooltip component", () => { let fixture: HTMLElement; - async function mountErrorTooltip(model: Model, props: PropsOf) { + async function mountErrorTooltip(model: Model, xc: string) { ({ fixture } = await mountComponent(ErrorToolTip, { - props, + props: { + cellPosition: toCellPosition(model.getters.getActiveSheetId(), xc), + }, model, })); } - test("Can display an error message", async () => { - 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 a data validation error message", async () => { + const model = new Model(); + addDataValidation(model, "A1", "id", { type: "textContains", values: ["hi"] }); + setCellContent(model, "A1", "hello"); + await mountErrorTooltip(model, "A1"); + expect(".o-error-tooltip-title").toHaveText("Invalid"); + expect(".o-error-tooltip-message").toHaveText('The value must be a text that contains "hi"'); }); test("Can display multiple error messages", async () => { - await mountErrorTooltip(new Model(), { - errors: [ - { message: "This is an error", title: "Error" }, - { message: "Invalid data", title: "Invalid" }, - ], - cellPosition: toCellPosition("sheet1", "A1"), - }); + const model = new Model(); + addDataValidation(model, "A2", "id", { type: "textContains", values: ["hi"] }); + setCellContent(model, "A1", "=1/0"); + setCellContent(model, "A2", "=A1"); + await mountErrorTooltip(model, "A2"); const titles = fixture.querySelectorAll(".o-error-tooltip-title"); const messages = fixture.querySelectorAll(".o-error-tooltip-message"); expect(titles).toHaveLength(2); expect(messages).toHaveLength(2); expect(titles[0].textContent).toBe("Error"); - expect(messages[0].textContent).toBe("This is an error"); + expect(messages[0].textContent).toBe("The divisor must be different from zero. Caused by A1"); expect(titles[1].textContent).toBe("Invalid"); - expect(messages[1].textContent).toBe("Invalid data"); + expect(messages[1].textContent).toBe('The value must be a text that contains "hi"'); }); 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"), - }); + await mountErrorTooltip(model, "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"), - }); + await mountErrorTooltip(model, "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"), - }); + await mountErrorTooltip(model, "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"), - }); + await mountErrorTooltip(model, "A1"); expect(".fst-italic").toHaveCount(0); }); });