From 45f4c5c3523f1445645f69446211b1f752135e02 Mon Sep 17 00:00:00 2001 From: Chenyun Yang Date: Fri, 3 Mar 2023 15:15:29 +0000 Subject: [PATCH] [FIX] range: show ref error after removing col/row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previouly after we remove cols/rows that are used in formulas, the formula will not show error reminding invalid references, but keep the previous range and give the wrong results. This commit fixes this problem, so after deleting rows/cols the formulas refer to removed ones will show errors. The tests are co-authored with Adrien Minne (adrm). task 2719611 closes odoo/o-spreadsheet#2249 X-original-commit: a195f82888188d45427eb4bb4c1ac52835cd451c Signed-off-by: Rémi Rahir (rar) Signed-off-by: Yang Chenyun (chya) Co-authored-by: Adrien Minne --- src/plugins/core/range.ts | 18 +++- tests/collaborative/clipboard.test.ts | 10 +-- tests/formulas/formulas.test.ts | 44 +++++++++- tests/plugins/range.test.ts | 120 ++++++++++++++++++++++++-- tests/plugins/sheets.test.ts | 4 +- 5 files changed, 178 insertions(+), 18 deletions(-) diff --git a/src/plugins/core/range.ts b/src/plugins/core/range.ts index 5948fdd4f..829ce469e 100644 --- a/src/plugins/core/range.ts +++ b/src/plugins/core/range.ts @@ -81,6 +81,7 @@ export class RangeAdapter implements CommandHandler { newRange = this.createAdaptedRange(newRange, dimension, changeType, -toRemove); } else if (range.zone[start] >= min && range.zone[end] <= max) { changeType = "REMOVE"; + newRange = range.clone({ ...this.getInvalidRange() }); } else if (range.zone[start] <= max && range.zone[end] >= max) { const toRemove = max - range.zone[start] + 1; changeType = "RESIZE"; @@ -150,9 +151,10 @@ export class RangeAdapter implements CommandHandler { return { changeType: "NONE" }; } const invalidSheetName = this.getters.getSheetName(cmd.sheetId); - const sheetId = ""; - range = range.clone({ sheetId, invalidSheetName }); - + range = range.clone({ + ...this.getInvalidRange(), + invalidSheetName, + }); return { changeType: "REMOVE", range }; }, cmd.sheetId); @@ -446,4 +448,14 @@ export class RangeAdapter implements CommandHandler { return str; } + + private getInvalidRange() { + return { + parts: [], + prefixSheet: false, + zone: { left: -1, top: -1, right: -1, bottom: -1 }, + sheetId: "", + invalidXc: INCORRECT_RANGE_STRING, + }; + } } diff --git a/tests/collaborative/clipboard.test.ts b/tests/collaborative/clipboard.test.ts index dff6394e3..0f8dd5936 100644 --- a/tests/collaborative/clipboard.test.ts +++ b/tests/collaborative/clipboard.test.ts @@ -73,11 +73,11 @@ describe("Collaborative range manipulation", () => { ); expect([alice, bob, charlie]).toHaveSynchronizedValue( (user) => getCell(user, "A1", "Sheet2")?.content, - "=Sheet1!A2" + "=#REF" ); }); - test("cut and paste and delete target sheet concurrentlkjhgkjhg khg y", () => { + test("cut and paste and delete target sheet concurrently (delete first)", () => { setCellContent(alice, "A2", "=A1"); cut(alice, "A1"); createSheet(alice, { sheetId: "Sheet2", activate: true }); @@ -85,13 +85,13 @@ describe("Collaborative range manipulation", () => { deleteSheet(bob, "Sheet2"); paste(alice, "D4"); }); - expect([alice]).toHaveSynchronizedValue( + expect([alice, bob, charlie]).toHaveSynchronizedValue( (user) => getCell(user, "A2", "Sheet1")?.content, "=A1" ); }); - test("cut and paste and delete target sheet concurrently", () => { + test("cut and paste and delete target sheet concurrently (paste first)", () => { setCellContent(alice, "A2", "=A1"); cut(alice, "A1"); createSheet(alice, { sheetId: "Sheet2", activate: true }); @@ -101,7 +101,7 @@ describe("Collaborative range manipulation", () => { }); expect([alice, bob, charlie]).toHaveSynchronizedValue( (user) => getCell(user, "A2", "Sheet1")?.content, - "=Sheet2!D4" + "=#REF" ); }); diff --git a/tests/formulas/formulas.test.ts b/tests/formulas/formulas.test.ts index aea6b108c..c53c763f1 100644 --- a/tests/formulas/formulas.test.ts +++ b/tests/formulas/formulas.test.ts @@ -1,8 +1,13 @@ import { Model } from "../../src"; import { INCORRECT_RANGE_STRING } from "../../src/constants"; import { FormulaCell } from "../../src/types"; -import { createSheetWithName, setCellContent } from "../test_helpers/commands_helpers"; -import { getCell } from "../test_helpers/getters_helpers"; +import { + createSheetWithName, + deleteColumns, + deleteRows, + setCellContent, +} from "../test_helpers/commands_helpers"; +import { getCell, getCellContent, getCellText } from "../test_helpers/getters_helpers"; function moveFormula(model: Model, formula: string, offsetX: number, offsetY: number): string { const sheetId = model.getters.getActiveSheetId(); @@ -101,3 +106,38 @@ describe("createAdaptedRanges", () => { expect(moveFormula(model, "='Sheet 2'!B2", 1, 10)).toEqual("='Sheet 2'!C12"); }); }); + +describe("Remove columns/rows that are references of formula", () => { + let model: Model; + beforeEach(() => { + model = new Model(); + }); + + test("delete multiple columns, including the one in formula and the one before it", () => { + setCellContent(model, "A1", "=SUM(C1,D1)"); + deleteColumns(model, ["B", "C"]); + expect(getCellContent(model, "A1")).toEqual("#REF"); + expect(getCellText(model, "A1")).toEqual("=SUM(#REF,B1)"); + }); + + test("delete multiple columns, including the one in formula and the one after it", () => { + setCellContent(model, "A1", "=SUM(C1,D1)"); + deleteColumns(model, ["C", "D"]); + expect(getCellContent(model, "A1")).toEqual("#REF"); + expect(getCellText(model, "A1")).toEqual("=SUM(#REF,#REF)"); + }); + + test("delete multiple rows, including the one in formula and the one before it", () => { + setCellContent(model, "A1", "=SUM(C3,C4)"); + deleteRows(model, [1, 2]); + expect(getCellContent(model, "A1")).toEqual("#REF"); + expect(getCellText(model, "A1")).toEqual("=SUM(#REF,C2)"); + }); + + test("delete multiple rows, including the one in formula and the one after it", () => { + setCellContent(model, "A1", "=SUM(C3,C4)"); + deleteRows(model, [2, 3]); + expect(getCellContent(model, "A1")).toEqual("#REF"); + expect(getCellText(model, "A1")).toEqual("=SUM(#REF,#REF)"); + }); +}); diff --git a/tests/plugins/range.test.ts b/tests/plugins/range.test.ts index 29c5175a3..b3f0c027d 100644 --- a/tests/plugins/range.test.ts +++ b/tests/plugins/range.test.ts @@ -41,7 +41,7 @@ class PluginTestRange extends CorePlugin { const change = applyChange(range); switch (change.changeType) { case "REMOVE": - this.ranges.splice(i, 1); + this.ranges[i] = change.range; break; case "RESIZE": case "MOVE": @@ -87,9 +87,9 @@ describe("range plugin", () => { beforeEach(() => { m = new Model({ sheets: [ - { id: "s1", name: "s1", rows: 10, cols: 10 }, - { id: "s2", name: "s 2", rows: 10, cols: 10 }, - { id: "s1!!", name: "s1!!", rows: 10, cols: 10 }, + { id: "s1", name: "s1" }, + { id: "s2", name: "s 2" }, + { id: "s1!!", name: "s1!!" }, ], }); m.dispatch("USE_RANGE", { sheetId: m.getters.getActiveSheetId(), rangesXC: ["B2:D4"] }); @@ -132,6 +132,59 @@ describe("range plugin", () => { }); }); + describe("create a range and remove multiple columns", () => { + beforeEach(() => { + m = new Model({ + sheets: [ + { id: "s1", name: "s1" }, + { id: "s2", name: "s 2" }, + ], + }); + m.dispatch("USE_RANGE", { sheetId: m.getters.getActiveSheetId(), rangesXC: ["C2:F5"] }); + }); + + test("in the middle", () => { + deleteColumns(m, ["D", "E"]); + expect(m.getters.getUsedRanges()).toEqual(["C2:D5"]); + }); + + test("in the start", () => { + deleteColumns(m, ["B", "C"]); + expect(m.getters.getUsedRanges()).toEqual(["B2:D5"]); + }); + + test("in the end", () => { + deleteColumns(m, ["E", "F"]); + expect(m.getters.getUsedRanges()).toEqual(["C2:D5"]); + }); + + test("before the start", () => { + deleteColumns(m, ["A", "B"]); + expect(m.getters.getUsedRanges()).toEqual(["A2:D5"]); + }); + + test("after the end", () => { + deleteColumns(m, ["G", "H"]); + expect(m.getters.getUsedRanges()).toEqual(["C2:F5"]); + }); + + test("including one column before the start and the first column", () => { + deleteColumns(m, ["C", "B"]); + expect(m.getters.getUsedRanges()).toEqual(["B2:D5"]); + }); + + test("including one column after the end and the last column", () => { + deleteColumns(m, ["G", "F"]); + expect(m.getters.getUsedRanges()).toEqual(["C2:E5"]); + }); + + test("delete columns causing invalid reference will be marked as #REF", () => { + m.dispatch("USE_RANGE", { sheetId: m.getters.getActiveSheetId(), rangesXC: ["C1"] }); + deleteColumns(m, ["B", "C"]); + expect(m.getters.getUsedRanges()[1]).toEqual("#REF"); + }); + }); + describe("create a range and remove a row", () => { test("in the middle", () => { deleteRows(m, [2]); @@ -165,6 +218,61 @@ describe("range plugin", () => { }); }); + describe("create a range and remove multiple rows", () => { + beforeEach(() => { + m = new Model({ + sheets: [ + { id: "s1", name: "s1" }, + { id: "s2", name: "s 2" }, + ], + }); + m.dispatch("USE_RANGE", { sheetId: m.getters.getActiveSheetId(), rangesXC: ["C3:F7"] }); + }); + + test("in the middle", () => { + deleteRows(m, [3, 4]); + expect(m.getters.getUsedRanges()).toEqual(["C3:F5"]); + }); + + test("in the start", () => { + deleteRows(m, [2, 3]); + expect(m.getters.getUsedRanges()).toEqual(["C3:F5"]); + }); + + test("in the end", () => { + deleteRows(m, [5, 6]); + expect(m.getters.getUsedRanges()).toEqual(["C3:F5"]); + }); + + test("including one row before start and the first row", () => { + deleteRows(m, [1, 2]); + expect(m.getters.getUsedRanges()).toEqual(["C2:F5"]); + }); + + test("including one row after end and the last row", () => { + deleteRows(m, [6, 7]); + expect(m.getters.getUsedRanges()).toEqual(["C3:F6"]); + }); + + test("before the start", () => { + deleteRows(m, [0, 1]); + expect(m.getters.getUsedRanges()).toEqual(["C1:F5"]); + }); + + test("after the end", () => { + deleteRows(m, [7, 8]); + expect(m.getters.getUsedRanges()).toEqual(["C3:F7"]); + }); + + test("delete rows causing invalid reference will be marked as #REF", () => { + m.dispatch("USE_RANGE", { sheetId: m.getters.getActiveSheetId(), rangesXC: ["C3"] }); + deleteRows(m, [1, 2]); + expect(m.getters.getUsedRanges().length).toEqual(2); + expect(m.getters.getUsedRanges()[0]).toEqual("C2:F5"); + expect(m.getters.getUsedRanges()[1]).toEqual("#REF"); + }); + }); + describe("create a range and add a column", () => { test("after, in the middle", () => { addColumns(m, "after", "C", 1); @@ -291,7 +399,7 @@ describe("range plugin", () => { m.dispatch("USE_RANGE", { rangesXC: ["A1"], sheetId: "s2" }); expect(m.getters.getUsedRanges()).toEqual(["B2:D4", "'s 2'!A1"]); deleteSheet(m, "s2"); - expect(m.getters.getUsedRanges()).toEqual(["B2:D4"]); + expect(m.getters.getUsedRanges()).toEqual(["B2:D4", "#REF"]); }); }); @@ -305,7 +413,7 @@ describe("range plugin", () => { m.dispatch("USE_RANGE", { rangesXC: ["A1"], sheetId: "s2" }); expect(m.getters.getUsedRanges()).toEqual(["B2:D4", "'s 2'!A1"]); deleteSheet(m, "s2"); - expect(m.getters.getUsedRanges()).toEqual(["B2:D4"]); + expect(m.getters.getUsedRanges()).toEqual(["B2:D4", "#REF"]); }); }); }); diff --git a/tests/plugins/sheets.test.ts b/tests/plugins/sheets.test.ts index b042203e5..2381c5d45 100644 --- a/tests/plugins/sheets.test.ts +++ b/tests/plugins/sheets.test.ts @@ -862,8 +862,8 @@ describe("sheets", () => { const sheet2 = model.getters.getActiveSheetId(); setCellContent(model, "A1", "42"); model.dispatch("DELETE_SHEET", { sheetId: sheet2 }); - expect(getCellText(model, "A1")).toBe("=NEW_NAME!A1"); - expect(getEvaluatedCell(model, "A1").value).toBe("#ERROR"); + expect(getCellText(model, "A1")).toBe("=#REF"); + expect(getEvaluatedCell(model, "A1").value).toBe("#REF"); undo(model); activateSheet(model, sheet1); expect(getCellText(model, "A1")).toBe("=NEW_NAME!A1");