Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP] errors: display error origin position #5413

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 48 additions & 8 deletions src/components/error_tooltip/error_tooltip.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just inject that value directly to the errorTooltip component as you did with the EvaluationError. The builder is no longer relevant if it only manages a single case. Or you could keep the builder and have it yield a component... but the former seems easier

Expand All @@ -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",
};
Expand Down
15 changes: 15 additions & 0 deletions src/components/error_tooltip/error_tooltip.xml
Original file line number Diff line number Diff line change
@@ -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"/>
Expand Down
9 changes: 8 additions & 1 deletion src/plugins/ui_core_views/cell_evaluation/evaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 8 additions & 1 deletion src/types/misc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
54 changes: 51 additions & 3 deletions tests/evaluation/evaluation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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")
Comment on lines -1089 to -1090
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 years old TODOs... 😛

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", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ exports[`Grid integration can display error tooltip 1`] = `
class="o-error-tooltip-message"
>
The divisor must be different from zero.

</div>



</div>
</div>
`;
83 changes: 71 additions & 12 deletions tests/popover/error_tooltip_component.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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);
Expand All @@ -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", () => {
Expand Down