[FIX] range: show ref error after removing col/row

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 #2248

X-original-commit: a195f82
Signed-off-by: Rémi Rahir (rar) <[email protected]>
Signed-off-by: Yang Chenyun (chya) <[email protected]>
Co-authored-by: Adrien Minne <[email protected]>

src/plugins/core/range.ts

@@ -81,6 +81,7 @@ export class RangeAdapter implements CommandHandler<CoreCommand> {
               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<CoreCommand> {
             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<CoreCommand> {
 
     return str;
   }
+
+  private getInvalidRange() {
+    return {
+      parts: [],
+      prefixSheet: false,
+      zone: { left: -1, top: -1, right: -1, bottom: -1 },
+      sheetId: "",
+      invalidXc: INCORRECT_RANGE_STRING,
+    };
+  }
 }

tests/collaborative/clipboard.test.ts

@@ -73,25 +73,25 @@ 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 });
     network.concurrent(() => {
       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"
     );
   });
 

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)");
+  });
+});

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"]);
       });
     });
   });

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");