Skip to content

Commit

Permalink
[REF] ErrorToolTip: make it non-generic
Browse files Browse the repository at this point in the history
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
  • Loading branch information
LucasLefevre committed Jan 7, 2025
1 parent 078bee3 commit dba11ab
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 80 deletions.
53 changes: 18 additions & 35 deletions src/components/error_tooltip/error_tooltip.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand All @@ -24,14 +23,8 @@ css/* scss */ `
}
`;

export interface ErrorToolTipMessage {
title: string;
message: string;
}

interface ErrorToolTipProps {
cellPosition: CellPosition;
errors: ErrorToolTipMessage[];
onClosed?: () => void;
}

Expand All @@ -40,10 +33,13 @@ export class ErrorToolTip extends Component<ErrorToolTipProps, SpreadsheetChildE
static template = "o-spreadsheet-ErrorToolTip";
static props = {
cellPosition: Object,
errors: Array,
onClosed: { type: Function, optional: true },
};

get dataValidationErrorMessage() {
return this.env.model.getters.getInvalidDataValidationMessage(this.props.cellPosition);
}

get evaluationError() {
const cell = this.env.model.getters.getEvaluatedCell(this.props.cellPosition);
if (cell.message) {
Expand Down Expand Up @@ -84,32 +80,19 @@ export class ErrorToolTip extends Component<ErrorToolTipProps, SpreadsheetChildE
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) {
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 };
},
};
8 changes: 3 additions & 5 deletions src/components/error_tooltip/error_tooltip.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@
</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"/>
</div>
<t t-if="dataValidationErrorMessage">
<div class="o-error-tooltip-title fw-bold text-danger">Invalid</div>
<div class="o-error-tooltip-message">
<t t-esc="error.message"/>
<t t-esc="dataValidationErrorMessage"/>
</div>
</t>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ exports[`Grid integration can display error tooltip 1`] = `
</div>
</div>
</div>
`;
62 changes: 23 additions & 39 deletions tests/popover/error_tooltip_component.test.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -31,89 +31,73 @@ mockChart();
describe("Error tooltip component", () => {
let fixture: HTMLElement;

async function mountErrorTooltip(model: Model, props: PropsOf<typeof ErrorToolTip>) {
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);
});
});
Expand Down

0 comments on commit dba11ab

Please sign in to comment.