[FIX] conditional_format: Add rule to unactive sheet

Adding a conditional formatting rule to a sheet that was never activated crashes.

Initializing the cf data structure for the sheet should be done at sheet
creation, not at sheet activation.

However, plugins (except `core`) don't know the newly created sheet id when
the `CREATE_SHEET` command is dispatched.

This commit changes the command parameters. It now requires the id.
This change would have been required anyway when mutli users is implemented
where commands will be shared across multiple spreadsheet instances.

Co-authored-by: fleodoo <[email protected]>

src/components/bottom_bar.ts

@@ -4,6 +4,7 @@ import { SpreadsheetEnv } from "../types";
 import { PLUS } from "./icons";
 import { MenuState, Menu } from "./menu";
 import { sheetMenuRegistry } from "../registries/index";
+import { uuidv4 } from "../helpers";
 const { Component } = owl;
 const { xml, css } = owl.tags;
 const { useState } = owl.hooks;
@@ -104,7 +105,7 @@ export class BottomBar extends Component<{}, SpreadsheetEnv> {
   menuState: MenuState = useState({ isOpen: false, position: null, menuItems: [] });
 
   addSheet() {
-    this.env.dispatch("CREATE_SHEET", { activate: true });
+    this.env.dispatch("CREATE_SHEET", { activate: true, id: uuidv4() });
   }
 
   activateSheet(name: string) {

src/plugins/conditional_format.ts

@@ -44,7 +44,10 @@ export class ConditionalFormatPlugin extends BasePlugin {
       case "ACTIVATE_SHEET":
         const activeSheet = cmd.to;
         this.computedStyles[activeSheet] = this.computedStyles[activeSheet] || {};
-        this.cfRules[activeSheet] = this.cfRules[activeSheet] || [];
+        this.isStale = true;
+        break;
+      case "CREATE_SHEET":
+        this.cfRules[cmd.id] = [];
         this.isStale = true;
         break;
       case "ADD_CONDITIONAL_FORMAT":

src/plugins/core.ts

@@ -12,7 +12,6 @@ import {
   sanitizeSheet,
   toCartesian,
   toXC,
-  uuidv4,
 } from "../helpers/index";
 import {
   Cell,
@@ -91,6 +90,7 @@ export class CorePlugin extends BasePlugin {
         break;
       case "CREATE_SHEET":
         const sheet = this.createSheet(
+          cmd.id,
           cmd.name || `Sheet${this.workbook.visibleSheets.length + 1}`,
           cmd.cols || 26,
           cmd.rows || 100
@@ -750,9 +750,9 @@ export class CorePlugin extends BasePlugin {
     this.history.updateSheet(_sheet, ["rows", row, "cells", col], cell);
   }
 
-  private createSheet(name: string, cols: number, rows: number): string {
+  private createSheet(id: string, name: string, cols: number, rows: number): string {
     const sheet: Sheet = {
-      id: uuidv4(),
+      id,
       name,
       cells: {},
       colNumber: cols,

src/registries/menus/menu_items_actions.ts

@@ -1,5 +1,5 @@
 import { Style, SpreadsheetEnv } from "../../types/index";
-import { numberToLetters } from "../../helpers/index";
+import { numberToLetters, uuidv4 } from "../../helpers/index";
 import { _lt } from "../../translation";
 
 //------------------------------------------------------------------------------
@@ -350,7 +350,7 @@ export const INSERT_COLUMNS_AFTER_ACTION = (env: SpreadsheetEnv) => {
 //------------------------------------------------------------------------------
 
 export const CREATE_SHEET_ACTION = (env: SpreadsheetEnv) => {
-  env.dispatch("CREATE_SHEET", { activate: true });
+  env.dispatch("CREATE_SHEET", { activate: true, id: uuidv4() });
 };
 
 //------------------------------------------------------------------------------

src/types/commands.ts

@@ -64,6 +64,7 @@ export interface ResizeRowsCommand extends BaseCommand {
  */
 export interface CreateSheetCommand extends BaseCommand {
   type: "CREATE_SHEET";
+  id: string;
   name?: string;
   cols?: number;
   rows?: number;

tests/components/bottom_bar_test.ts

@@ -1,9 +1,10 @@
 import { Component, hooks, tags } from "@odoo/owl";
 import { BottomBar } from "../../src/components/bottom_bar";
 import { Model } from "../../src/model";
-import { makeTestFixture, nextTick } from "../helpers";
+import { makeTestFixture, nextTick, mockUuidV4To } from "../helpers";
 import { triggerMouseEvent } from "../dom_helper";
 import { CommandResult } from "../../src/types";
+jest.mock("../../src/helpers/uuid", () => require("../__mocks__/uuid"));
 
 const { xml } = tags;
 const { useSubEnv } = hooks;
@@ -53,8 +54,9 @@ describe("BottomBar component", () => {
     const parent = new Parent(new Model());
     await parent.mount(fixture);
 
+    mockUuidV4To(42);
     triggerMouseEvent(".o-add-sheet", "click");
-    expect(parent.env.dispatch).toHaveBeenCalledWith("CREATE_SHEET", { activate: true });
+    expect(parent.env.dispatch).toHaveBeenCalledWith("CREATE_SHEET", { activate: true, id: "42" });
   });
 
   test("Can activate a sheet", async () => {

tests/menu_items_registry_test.ts

@@ -2,7 +2,8 @@ import { Model } from "../src";
 import { fontSizes } from "../src/fonts";
 import { FullMenuItem, topbarMenuRegistry } from "../src/registries/index";
 import { CommandResult, SpreadsheetEnv } from "../src/types";
-import { GridParent, makeTestFixture, nextTick } from "./helpers";
+import { GridParent, makeTestFixture, nextTick, mockUuidV4To } from "./helpers";
+jest.mock("../src/helpers/uuid", () => require("./__mocks__/uuid"));
 
 function getNode(_path: string[]): FullMenuItem {
   const path = [..._path];
@@ -386,8 +387,9 @@ describe("Menu Item actions", () => {
   });
 
   test("Insert -> new sheet", () => {
+    mockUuidV4To(42);
     doAction(["insert", "insert_sheet"], env);
-    expect(env.dispatch).toHaveBeenCalledWith("CREATE_SHEET", { activate: true });
+    expect(env.dispatch).toHaveBeenCalledWith("CREATE_SHEET", { activate: true, id: "42" });
   });
 
   describe("Format -> numbers", () => {

tests/plugins/clipboard_test.ts

@@ -86,7 +86,7 @@ describe("clipboard", () => {
     model.dispatch("SET_VALUE", { xc: "A1", text: "a1" });
     model.dispatch("CUT", { target: target("A1") });
     const to = model.getters.getActiveSheet();
-    model.dispatch("CREATE_SHEET", { activate: true });
+    model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
     const from = model.getters.getActiveSheet();
     model.dispatch("SET_VALUE", { xc: "A1", text: "a1Sheet2" });
     model.dispatch("PASTE", { target: target("B2") });

tests/plugins/conditional_formatting_test.ts

@@ -84,6 +84,35 @@ describe("conditional format", () => {
     expect(model.getters.getConditionalStyle("A4")).toEqual({ fillColor: "#0000FF" });
   });
 
+  test("Add conditional formating on inactive sheet", () => {
+    model = new Model();
+    model.dispatch("CREATE_SHEET", { id: "42" });
+    const [activeSheet, sheet] = model.getters.getSheets();
+    expect(sheet.id).not.toBe(model.getters.getActiveSheet());
+    model.dispatch("ADD_CONDITIONAL_FORMAT", {
+      cf: createEqualCF(["A1:A4"], "4", { fillColor: "#0000FF" }, "2"),
+      sheet: sheet.id,
+    });
+    model.dispatch("ACTIVATE_SHEET", {
+      from: activeSheet.id,
+      to: "42",
+    });
+    expect(model.getters.getConditionalFormats()).toEqual([
+      {
+        rule: {
+          values: ["4"],
+          operator: "Equal",
+          type: "CellIsRule",
+          style: {
+            fillColor: "#0000FF",
+          },
+        },
+        id: "2",
+        ranges: ["A1:A4"],
+      },
+    ]);
+  });
+
   test("remove a conditional format rule", () => {
     model.dispatch("SET_VALUE", { xc: "A1", text: "1" });
     model.dispatch("SET_VALUE", { xc: "A2", text: "2" });

tests/plugins/core_test.ts

@@ -171,7 +171,7 @@ describe("core", () => {
 
   test("can get row/col of inactive sheet", () => {
     const model = new Model();
-    model.dispatch("CREATE_SHEET");
+    model.dispatch("CREATE_SHEET", { id: "42" });
     const [, sheet2] = model.getters.getSheets();
     model.dispatch("RESIZE_ROWS", { sheet: sheet2.id, rows: [0], size: 24 });
     model.dispatch("RESIZE_COLUMNS", { sheet: sheet2.id, cols: [0], size: 42 });

tests/plugins/edition_test.ts

@@ -48,7 +48,7 @@ describe("edition", () => {
     const sheet1 = model["workbook"].visibleSheets[0];
     model.dispatch("START_EDITION", { text: "a" });
     expect(model.getters.getEditionMode()).toBe("editing");
-    model.dispatch("CREATE_SHEET", { activate: true });
+    model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
     expect(model.getters.getEditionMode()).toBe("inactive");
     expect(getCell(model, "A1")).toBe(null);
     model.dispatch("ACTIVATE_SHEET", { from: model.getters.getActiveSheet(), to: sheet1 });

tests/plugins/grid_manipulation_test.ts

@@ -559,7 +559,7 @@ describe("Rows", () => {
       let dimensions = model.getters.getGridSize();
       expect(dimensions).toEqual([192, 124]);
       const to = model.getters.getActiveSheet();
-      model.dispatch("CREATE_SHEET", { activate: true });
+      model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
       const from = model.getters.getActiveSheet();
       model.dispatch("ACTIVATE_SHEET", { from, to });
       dimensions = model.getters.getGridSize();

tests/plugins/plugins/evaluation_test.ts

@@ -16,7 +16,7 @@ describe("evaluate formula getter", () => {
   });
 
   test("in another sheet", () => {
-    model.dispatch("CREATE_SHEET");
+    model.dispatch("CREATE_SHEET", { id: "42" });
     const sheet2 = model["workbook"].visibleSheets[1];
     model.dispatch("SET_VALUE", { xc: "A1", text: "11", sheetId: sheet2 });
     expect(model.getters.evaluateFormula("=Sheet2!A1")).toBe(11);
@@ -53,7 +53,7 @@ describe("evaluate formula getter", () => {
   });
 
   test("using cells in other sheets", () => {
-    model.dispatch("CREATE_SHEET");
+    model.dispatch("CREATE_SHEET", { id: "42" });
     const s = model.getters.getSheets();
     model.dispatch("ACTIVATE_SHEET", { from: s[1].id, to: s[0].id });
     model.dispatch("SET_VALUE", { sheetId: s[1].id, xc: "A1", text: "12" });

tests/plugins/resizing_test.ts

@@ -53,7 +53,7 @@ describe("Model resizer", () => {
 
   test("Can resize row of inactive sheet", async () => {
     const model = new Model();
-    model.dispatch("CREATE_SHEET");
+    model.dispatch("CREATE_SHEET", { id: "42" });
     const [, sheet2] = model.getters.getSheets();
     model.dispatch("RESIZE_ROWS", { sheet: sheet2.id, size: 42, rows: [0] });
     expect(model.getters.getActiveSheet()).not.toBe(sheet2.id);
@@ -68,7 +68,7 @@ describe("Model resizer", () => {
 
   test("Can resize column of inactive sheet", async () => {
     const model = new Model();
-    model.dispatch("CREATE_SHEET");
+    model.dispatch("CREATE_SHEET", { id: "42" });
     const [, sheet2] = model.getters.getSheets();
     model.dispatch("RESIZE_COLUMNS", { sheet: sheet2.id, size: 42, cols: [0] });
     expect(model.getters.getActiveSheet()).not.toBe(sheet2.id);
@@ -77,7 +77,7 @@ describe("Model resizer", () => {
 
   test("changing sheets update the sizes", async () => {
     const model = new Model();
-    model.dispatch("CREATE_SHEET", { activate: true });
+    model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
     const sheet1 = model["workbook"].visibleSheets[0];
     const sheet2 = model["workbook"].visibleSheets[1];
 

tests/plugins/selection_test.ts

@@ -370,7 +370,7 @@ describe("multiple sheets", () => {
     model.dispatch("SELECT_CELL", { col: 2, row: 2 });
     expect(model.getters.getSelectedZones()).toEqual([toZone("C3")]);
 
-    model.dispatch("CREATE_SHEET", { activate: true });
+    model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
     expect(model.getters.getSelectedZones()).toEqual([toZone("A1")]);
     model.dispatch("SELECT_CELL", { col: 1, row: 1 });
     expect(model.getters.getSelectedZones()).toEqual([toZone("B2")]);

tests/plugins/sheets_test.ts

@@ -10,7 +10,7 @@ describe("sheets", () => {
     expect(model["workbook"].visibleSheets.length).toBe(1);
     expect(model["workbook"].activeSheet.name).toBe("Sheet1");
 
-    model.dispatch("CREATE_SHEET", { activate: true });
+    model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
     expect(model["workbook"].visibleSheets.length).toBe(2);
     expect(model["workbook"].activeSheet.name).toBe("Sheet2");
 
@@ -29,15 +29,21 @@ describe("sheets", () => {
 
     expect(model.getters.getActiveSheet()).toBe(sheet1);
     expect(model.getters.getSheets().map((s) => s.id)).toEqual([sheet1]);
-    model.dispatch("CREATE_SHEET");
+    model.dispatch("CREATE_SHEET", { id: "42" });
     const sheet2 = model["workbook"].visibleSheets[1];
     expect(model.getters.getActiveSheet()).toBe(sheet1);
     expect(model.getters.getSheets().map((s) => s.id)).toEqual([sheet1, sheet2]);
   });
 
   test("Can create a new sheet with given size and name", () => {
     const model = new Model();
-    model.dispatch("CREATE_SHEET", { rows: 2, cols: 4, name: "SheetTest", activate: true });
+    model.dispatch("CREATE_SHEET", {
+      rows: 2,
+      cols: 4,
+      name: "SheetTest",
+      activate: true,
+      id: "42",
+    });
     expect(model["workbook"].activeSheet.colNumber).toBe(4);
     expect(model["workbook"].activeSheet.cols.length).toBe(4);
     expect(model["workbook"].activeSheet.rowNumber).toBe(2);
@@ -48,7 +54,7 @@ describe("sheets", () => {
   test("Cannot create a sheet with a name already existent", () => {
     const model = new Model();
     const name = model["workbook"].activeSheet.name;
-    expect(model.dispatch("CREATE_SHEET", { name })).toEqual({
+    expect(model.dispatch("CREATE_SHEET", { name, id: "42" })).toEqual({
       status: "CANCELLED",
       reason: CancelledReason.WrongSheetName,
     });
@@ -69,7 +75,7 @@ describe("sheets", () => {
     expect(model["workbook"].activeSheet.name).toBe("Sheet1");
 
     model.dispatch("SET_VALUE", { xc: "A1", text: "3" });
-    model.dispatch("CREATE_SHEET", { activate: true });
+    model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
     expect(model["workbook"].activeSheet.name).toBe("Sheet2");
     model.dispatch("SET_VALUE", { xc: "A1", text: "=Sheet1!A1" });
     expect(getCell(model, "A1")!.value).toBe(3);
@@ -214,7 +220,7 @@ describe("sheets", () => {
 
   test("cells are updated when dependency in other sheet is updated", () => {
     const model = new Model();
-    model.dispatch("CREATE_SHEET", { activate: true });
+    model.dispatch("CREATE_SHEET", { activate: true, id: "42" });
     const sheet1 = model["workbook"].visibleSheets[0];
     const sheet2 = model["workbook"].visibleSheets[1];