[IMP] Allows sorting filter menu in readonly mode

matho-odoo · matho-odoo · commit 0acb3d482d35 · 2026-04-13T20:23:35.000+02:00
Sorting a table should be possible even in readonly mode

Task: 6047484
diff --git a/doc/extending/command.md b/doc/extending/command.md
@@ -42,15 +42,7 @@ coreTypes.add("MY_COMMAND_NAME");
 
 ### Read-Only Mode
 
-In read-only mode, all core commands are cancelled with the `CommandResult` `Readonly` since the spreadsheet state cannot be modified.
-However, some locale commands still need to be executed, such as updating the active sheet.
-To allow a new local command in read-only mode, add its type to `readonlyAllowedCommands`:
-
-```ts
-import { readonlyAllowedCommands } from "@odoo/o-spreadsheet";
-
-readonlyAllowedCommands.add("MY_COMMAND_NAME");
-```
+All commands are allowed in readonly mode. Since the spreadsheet state cannot be modified , we use a specific transport service (ReadonlyTransportFilter) blocking revision to be send to the server.
 
 ## Reserved keywords in commands
 
diff --git a/src/components/figures/figure_carousel/figure_carousel.ts b/src/components/figures/figure_carousel/figure_carousel.ts
@@ -84,6 +84,9 @@ export class CarouselFigure extends Component<Props, SpreadsheetChildEnv> {
   }
 
   onCarouselDoubleClick() {
+    if (this.env.model.getters.isReadonly()) {
+      return;
+    }
     this.env.model.dispatch("SELECT_FIGURE", { figureId: this.props.figureUI.id });
     this.env.openSidePanel("CarouselPanel", { figureId: this.props.figureUI.id });
   }
diff --git a/src/components/figures/figure_chart/figure_chart.ts b/src/components/figures/figure_chart/figure_chart.ts
@@ -24,6 +24,9 @@ export class ChartFigure extends Component<Props, SpreadsheetChildEnv> {
   static components = { ChartDashboardMenu };
 
   onDoubleClick() {
+    if (this.env.model.getters.isReadonly()) {
+      return;
+    }
     this.env.model.dispatch("SELECT_FIGURE", { figureId: this.props.figureUI.id });
     this.env.openSidePanel("ChartPanel");
   }
diff --git a/src/components/filters/filter_menu/filter_menu.ts b/src/components/filters/filter_menu/filter_menu.ts
@@ -53,7 +53,7 @@ export class FilterMenu extends Component<Props, SpreadsheetChildEnv> {
       this.table.range.sheetId,
       this.table.range.zone
     );
-    return !this.env.model.getters.isReadonly() && coreTable?.type !== "dynamic";
+    return coreTable?.type !== "dynamic";
   }
 
   get table() {
diff --git a/src/components/grid/grid.ts b/src/components/grid/grid.ts
@@ -446,6 +446,28 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
     "Alt+Shift+ArrowDown": () => this.processHeaderGroupingKey("down"),
   };
 
+  private readonlyAllowedShortcuts = new Set([
+    "Tab",
+    "Shift+Tab",
+    "Escape",
+    "Ctrl+A",
+    "Ctrl+Z",
+    "Ctrl+Y",
+    "F4",
+    "Alt+Enter",
+    "Ctrl+Home",
+    "Ctrl+End",
+    "Shift+ ",
+    "Ctrl+ ",
+    "Ctrl+H",
+    "Ctrl+F",
+    "Ctrl+Shift+ ",
+    "Alt+Shift+ArrowRight",
+    "Alt+Shift+ArrowLeft",
+    "Alt+Shift+ArrowUp",
+    "Alt+Shift+ArrowDown",
+  ]);
+
   private focusComposerFromActiveCell() {
     const cell = this.env.model.getters.getActiveCell();
     cell.type === CellValueType.empty
@@ -647,6 +669,9 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
 
   onKeydown(ev: KeyboardEvent) {
     const keyDownString = keyboardEventToShortcutString(ev);
+    if (this.env.model.getters.isReadonly() && !this.readonlyAllowedShortcuts.has(keyDownString)) {
+      return;
+    }
     const handler = this.keyDownMapping[keyDownString];
     if (handler) {
       ev.preventDefault();
diff --git a/src/helpers/ui/sheet_interactive.ts b/src/helpers/ui/sheet_interactive.ts
@@ -9,6 +9,9 @@ export function interactiveRenameSheet(
   name: string,
   errorCallback: () => void
 ) {
+  if (env.model.getters.isReadonly()) {
+    return;
+  }
   const result = env.model.dispatch("RENAME_SHEET", {
     sheetId,
     newName: name,
diff --git a/src/index.ts b/src/index.ts
@@ -260,7 +260,6 @@ export type {
   TransportService,
 } from "./types/collaborative/transport_service";
 export {
-  canExecuteInReadonly,
   coreTypes,
   invalidateCFEvaluationCommands,
   invalidateChartEvaluationCommands,
@@ -269,7 +268,6 @@ export {
   isCoreCommand,
   isSheetDependent,
   lockedSheetAllowedCommands,
-  readonlyAllowedCommands,
 } from "./types/commands";
 export { CellErrorType, EvaluationError } from "./types/errors";
 
diff --git a/src/model.ts b/src/model.ts
@@ -29,7 +29,6 @@ import { StateObserver } from "./state_observer";
 import { _t, setDefaultTranslationMethod } from "./translation";
 import { StateUpdateMessage } from "./types/collaborative/transport_service";
 import {
-  canExecuteInReadonly,
   Command,
   CommandDispatcher,
   CommandHandler,
@@ -516,9 +515,6 @@ export class Model extends EventBus<any> implements CommandDispatcher {
   dispatch: CommandDispatcher["dispatch"] = (type: CommandTypes, payload?: any) => {
     const command: Command = createCommand(type, payload);
     const status: Status = this.status;
-    if (this.getters.isReadonly() && !canExecuteInReadonly(command)) {
-      return new DispatchResult(CommandResult.Readonly);
-    }
     if (!this.session.canApplyOptimisticUpdate()) {
       return new DispatchResult(CommandResult.WaitingSessionConfirmation);
     }
diff --git a/src/types/commands.ts b/src/types/commands.ts
@@ -197,29 +197,6 @@ export const invalidSubtotalFormulasCommands = new Set<CommandTypes>([
   "UPDATE_FILTER",
 ]);
 
-export const readonlyAllowedCommands = new Set<CommandTypes>([
-  "START",
-  "ACTIVATE_SHEET",
-
-  "COPY",
-
-  "RESIZE_SHEETVIEW",
-  "SET_VIEWPORT_OFFSET",
-  "SET_ZOOM",
-
-  "EVALUATE_CELLS",
-  "EVALUATE_CHARTS",
-
-  "SET_FORMULA_VISIBILITY",
-
-  "UPDATE_FILTER",
-  "UPDATE_CHART",
-  "UPDATE_CHART_REGION",
-  "UPDATE_CAROUSEL_ACTIVE_ITEM",
-
-  "UPDATE_PIVOT",
-]);
-
 export const lockedSheetAllowedCommands = new Set<Command["type"]>([
   // core commands
   "LOCK_SHEET",
@@ -357,10 +334,6 @@ export function isCoreCommand(cmd: Command): cmd is CoreCommand {
   return coreTypes.has(cmd.type as any);
 }
 
-export function canExecuteInReadonly(cmd: Command): boolean {
-  return readonlyAllowedCommands.has(cmd.type);
-}
-
 //#region Core Commands
 // ------------------------------------------------
 
diff --git a/tests/collaborative/collaborative.test.ts b/tests/collaborative/collaborative.test.ts
@@ -600,11 +600,6 @@ describe("Multi users synchronisation", () => {
       (user) => getCellContent(user, "A1"),
       "hello"
     );
-    setCellContent(david, "A1", "I'm David and I want access !");
-    expect([alice, bob, charlie, david]).toHaveSynchronizedValue(
-      (user) => getCellContent(user, "A1"),
-      "hello"
-    );
   });
 
   test("autofill overwrite style and format", () => {
diff --git a/tests/figures/chart/chart_animations.test.ts b/tests/figures/chart/chart_animations.test.ts
@@ -1,5 +1,5 @@
 import { Chart } from "chart.js";
-import { Model, readonlyAllowedCommands } from "../../../src";
+import { Model } from "../../../src";
 import { ChartAnimationStore } from "../../../src/components/figures/chart/chartJs/chartjs_animation_store";
 import { toChartDataSource } from "../../test_helpers/chart_helpers";
 import {
@@ -60,8 +60,6 @@ describe("Chart animations in dashboard", () => {
   });
 
   test("Animations are replayed only when chart data changes", async () => {
-    readonlyAllowedCommands.add("UPDATE_CELL");
-
     const model = new Model();
     createChart(model, {
       type: "bar",
@@ -81,13 +79,9 @@ describe("Chart animations in dashboard", () => {
     setCellContent(model, "A2", "6");
     await nextTick();
     expect(mockedChart.config.options.animation).toEqual({ animateRotate: true });
-
-    readonlyAllowedCommands.delete("UPDATE_CELL");
   });
 
   test("Treemap animation are not replayed when data does not change but runtime is re-created", async () => {
-    readonlyAllowedCommands.add("UPDATE_CELL");
-
     const model = new Model();
     createChart(model, {
       type: "treemap",
@@ -102,8 +96,6 @@ describe("Chart animations in dashboard", () => {
     setCellContent(model, "B1", "6");
     await nextTick();
     expect(mockedChart.config.options.animation).toBe(false);
-
-    readonlyAllowedCommands.delete("UPDATE_CELL");
   });
 
   test("Charts are animated when chart type changes", async () => {
diff --git a/tests/figures/chart/gauge/gauge_rendering.test.ts b/tests/figures/chart/gauge/gauge_rendering.test.ts
@@ -7,7 +7,7 @@ import {
   GAUGE_LABELS_FONT_SIZE,
   getGaugeRenderingConfig,
 } from "../../../../src/helpers/figures/charts/gauge_chart_rendering";
-import { readonlyAllowedCommands, Rect } from "../../../../src/types";
+import { Rect } from "../../../../src/types";
 import { GaugeAnimatedRuntime, GaugeChartRuntime } from "../../../../src/types/chart";
 import { MockCanvasRenderingContext2D } from "../../../setup/canvas.mock";
 import {
@@ -272,8 +272,6 @@ describe("Gauge chart component animation", () => {
   });
 
   test("Animations are replayed only when chart data changes", async () => {
-    readonlyAllowedCommands.add("UPDATE_CELL");
-
     const model = new Model();
     createGaugeChart(model, { dataRange: "A1" });
     model.updateMode("dashboard");
@@ -290,7 +288,5 @@ describe("Gauge chart component animation", () => {
     setCellContent(model, "A1", "6");
     await nextTick();
     expect(gaugeAnimationSpy).toHaveBeenCalledTimes(2);
-
-    readonlyAllowedCommands.delete("UPDATE_CELL");
   });
 });
diff --git a/tests/lock_sheet/lock_sheet_plugin.test.ts b/tests/lock_sheet/lock_sheet_plugin.test.ts
@@ -6,15 +6,13 @@ import {
   isCoreCommand,
   isSheetDependent,
   lockedSheetAllowedCommands,
-  readonlyAllowedCommands,
 } from "../../src";
 import { createChart, createSheet, lockSheet } from "../test_helpers/commands_helpers";
 import { TEST_COMMANDS } from "../test_helpers/constants";
 import { addPivot } from "../test_helpers/pivot_helpers";
 
 const allowedCommands: Command["type"][] = [];
 const rejectedCommands: Command["type"][] = [];
-const readonlyCommands: Command["type"][] = [];
 
 (Object.keys(TEST_COMMANDS) as CoreCommand["type"][]).forEach((cmdType, cmd) => {
   if (
@@ -25,9 +23,6 @@ const readonlyCommands: Command["type"][] = [];
   } else {
     rejectedCommands.push(cmdType);
   }
-  if (readonlyAllowedCommands.has(cmdType)) {
-    readonlyCommands.push(cmdType);
-  }
 });
 
 describe("Lock Sheet plugin", () => {
@@ -51,8 +46,9 @@ describe("Lock Sheet plugin", () => {
     }
   });
 
-  test("read only commands bypass lock in dashboard mode", () => {
-    for (const cmdType of readonlyCommands) {
+  test.each(["UPDATE_CHART", "UPDATE_PIVOT"])(
+    "%s command bypass lock in dashboard mode",
+    (cmdType: Command["type"]) => {
       const model = new Model();
       createSheet(model, { name: "Another sheet", position: 0 });
       createChart(model, { type: "bar" }, "chartId");
@@ -62,5 +58,5 @@ describe("Lock Sheet plugin", () => {
       const result = model.dispatch(cmdType, TEST_COMMANDS[cmdType]);
       expect(result).toBeSuccessfullyDispatched();
     }
-  });
+  );
 });
diff --git a/tests/model/model.test.ts b/tests/model/model.test.ts
@@ -174,11 +174,6 @@ describe("Model", () => {
     expect(model.getters.isReadonly()).toBe(true);
   });
 
-  test("Some commands are not dispatched in readonly mode", () => {
-    const model = new Model({}, { mode: "readonly" });
-    expect(setCellContent(model, "A1", "hello")).toBeCancelledBecause(CommandResult.Readonly);
-  });
-
   test("Moving the selection is allowed in readonly mode", () => {
     const model = new Model({}, { mode: "readonly" });
     expect(selectCell(model, "A15")).toBeSuccessfullyDispatched();
diff --git a/tests/table/filter_menu_component.test.ts b/tests/table/filter_menu_component.test.ts
@@ -5,6 +5,7 @@ import {
   filterTextCriterionOperators,
   UID,
 } from "../../src/types";
+import { getCellContent } from "../test_helpers";
 import {
   createDynamicTable,
   createTableWithFilter,
@@ -370,18 +371,23 @@ describe("Filter menu component", () => {
     });
   });
 
-  test("cannot sort filter table in readonly mode", async () => {
-    createTableWithFilter(model, "A10:B15");
+  test("can sort filter table in readonly mode", async () => {
+    createTableWithFilter(model, "A1:A3");
+    setCellContent(model, "A2", "1");
+    setCellContent(model, "A3", "-2");
     await nextTick();
-    await openFilterMenu("A10");
+    await openFilterMenu("A1");
     expect(
       [...fixture.querySelectorAll(".o-filter-menu-item")].map((el) => el.textContent?.trim())
-    ).toEqual(["Sort ascending (A ⟶ Z)", "Sort descending (Z ⟶ A)", "(Blanks)"]);
+    ).toEqual(["Sort ascending (A ⟶ Z)", "Sort descending (Z ⟶ A)", "-2", "1"]);
     model.updateMode("readonly");
     await nextTick();
     expect(
       [...fixture.querySelectorAll(".o-filter-menu-item")].map((el) => el.textContent?.trim())
-    ).toEqual(["(Blanks)"]);
+    ).toEqual(["Sort ascending (A ⟶ Z)", "Sort descending (Z ⟶ A)", "-2", "1"]);
+    await simulateClick(".o-filter-menu-item:nth-of-type(1)");
+    expect(getCellContent(model, "A2")).toBe("-2");
+    expect(getCellContent(model, "A3")).toBe("1");
   });
 
   test("cannot sort dynamic table", async () => {

Original file line number	Diff line number	Diff line change
`@@ -84,6 +84,9 @@ export class CarouselFigure extends Component<Props, SpreadsheetChildEnv> {`
`84`	`84`	`}`
`85`	`85`
`86`	`86`	`onCarouselDoubleClick() {`
	`87`	`+ if (this.env.model.getters.isReadonly()) {`
	`88`	`+ return;`
	`89`	`+ }`
`87`	`90`	`this.env.model.dispatch("SELECT_FIGURE", { figureId: this.props.figureUI.id });`
`88`	`91`	`this.env.openSidePanel("CarouselPanel", { figureId: this.props.figureUI.id });`
`89`	`92`	`}`
Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,9 @@ export class ChartFigure extends Component<Props, SpreadsheetChildEnv> {`
`24`	`24`	`static components = { ChartDashboardMenu };`
`25`	`25`
`26`	`26`	`onDoubleClick() {`
	`27`	`+ if (this.env.model.getters.isReadonly()) {`
	`28`	`+ return;`
	`29`	`+ }`
`27`	`30`	`this.env.model.dispatch("SELECT_FIGURE", { figureId: this.props.figureUI.id });`
`28`	`31`	`this.env.openSidePanel("ChartPanel");`
`29`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ export class FilterMenu extends Component<Props, SpreadsheetChildEnv> {`
`53`	`53`	`this.table.range.sheetId,`
`54`	`54`	`this.table.range.zone`
`55`	`55`	`);`
`56`		`- return !this.env.model.getters.isReadonly() && coreTable?.type !== "dynamic";`
	`56`	`+ return coreTable?.type !== "dynamic";`
`57`	`57`	`}`
`58`	`58`
`59`	`59`	`get table() {`