Skip to content

Commit

Permalink
[FIX] Datavalidation: coreView plugin should not dispatch
Browse files Browse the repository at this point in the history
See #5216 - CoreView plugins replay remote revisions and should not take
the risk to dispatch during the replay. The code that was assigning
arbitrary default values to the cells of a boolean datavalidation has
been moved to a UI feature plugin, hence avoiding the problem raised in
PR #5216.

Task: 4241141
  • Loading branch information
rrahir committed Nov 18, 2024
1 parent f562b8e commit f46e94d
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 16 deletions.
4 changes: 3 additions & 1 deletion src/plugins/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
SortPlugin,
UIOptionsPlugin,
} from "./ui_feature";
import { DataValidationInsertionPlugin } from "./ui_feature/datavalidation_insertion";
import { HistoryPlugin } from "./ui_feature/local_history";
import { SplitToColumnsPlugin } from "./ui_feature/split_to_columns";
import { TableAutofillPlugin } from "./ui_feature/table_autofill";
Expand Down Expand Up @@ -78,7 +79,8 @@ export const featurePluginRegistry = new Registry<UIPluginConstructor>()
.add("collaborative", CollaborativePlugin)
.add("history", HistoryPlugin)
.add("data_cleanup", DataCleanupPlugin)
.add("table_autofill", TableAutofillPlugin);
.add("table_autofill", TableAutofillPlugin)
.add("datavalidation_insert", DataValidationInsertionPlugin);

// Plugins which have a state, but which should not be shared in collaborative
export const statefulUIPluginRegistry = new Registry<UIPluginConstructor>()
Expand Down
15 changes: 0 additions & 15 deletions src/plugins/ui_core_views/evaluation_data_validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,27 +56,12 @@ export class EvaluationDataValidationPlugin extends UIPlugin {
}
switch (cmd.type) {
case "ADD_DATA_VALIDATION_RULE":
const ranges = cmd.ranges.map((range) => this.getters.getRangeFromRangeData(range));
if (cmd.rule.criterion.type === "isBoolean") {
this.setContentToBooleanCells({ ...cmd.rule, ranges });
}
delete this.validationResults[cmd.sheetId];
break;
case "REMOVE_DATA_VALIDATION_RULE":
delete this.validationResults[cmd.sheetId];
break;
}
}

private setContentToBooleanCells(rule: DataValidationRule) {
for (const position of getCellPositionsInRanges(rule.ranges)) {
const evaluatedCell = this.getters.getEvaluatedCell(position);
if (evaluatedCell.type !== CellValueType.boolean) {
this.dispatch("UPDATE_CELL", { ...position, content: "FALSE" });
}
}
}

isDataValidationInvalid(cellPosition: CellPosition): boolean {
return !this.getValidationResultForCell(cellPosition).isValid;
}
Expand Down
37 changes: 37 additions & 0 deletions src/plugins/ui_feature/datavalidation_insertion.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { getCellPositionsInRanges, isBoolean } from "../../helpers";
import { CellValueType, Command, isMatrix } from "../../types/index";
import { UIPlugin } from "../ui_plugin";

export class DataValidationInsertionPlugin extends UIPlugin {
handle(cmd: Command) {
switch (cmd.type) {
case "ADD_DATA_VALIDATION_RULE":
if (cmd.rule.criterion.type === "isBoolean") {
const ranges = cmd.ranges.map((range) => this.getters.getRangeFromRangeData(range));
for (const position of getCellPositionsInRanges(ranges)) {
const cell = this.getters.getCell(position);
const evaluatedCell = this.getters.getEvaluatedCell(position);

if (!cell?.content) {
this.dispatch("UPDATE_CELL", { ...position, content: "FALSE" });
// In this case, a cell has been updated in the core plugin but
// not yet evaluated. This can occur after a paste operation.
} else if (cell?.content && evaluatedCell.type === CellValueType.empty) {
let value: string | undefined;
if (cell.content.startsWith("=")) {
const result = this.getters.evaluateFormula(position.sheetId, cell.content);
value = (isMatrix(result) ? result[0][0] : result)?.toString();
} else {
value = cell.content;
}
if (!value || !isBoolean(value)) {
this.dispatch("UPDATE_CELL", { ...position, content: "FALSE" });
}
} else if (evaluatedCell.type !== CellValueType.boolean) {
this.dispatch("UPDATE_CELL", { ...position, content: "FALSE" });
}
}
}
}
}
}
42 changes: 42 additions & 0 deletions tests/collaborative/collaborative_clipboard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { LineChartDefinition } from "../../src/types/chart";
import { MockTransportService } from "../__mocks__/transport_service";
import {
addColumns,
addDataValidation,
copy,
createChart,
createSheet,
Expand Down Expand Up @@ -123,4 +124,45 @@ describe("Collaborative range manipulation", () => {
"D4"
);
});

test("Can copy boolean datavalidation while preserving the cell values", () => {
setCellContent(alice, "A1", "TRUE");
setCellContent(alice, "A3", "not a boolean");
setCellContent(alice, "A4", "=TRANSPOSE(A1)");
setCellContent(alice, "A5", "=TEXT(5)");
setCellContent(alice, "A6", "=NOT(A1)");
setCellContent(alice, "A7", "7");
addDataValidation(alice, "A1:A7", "id", { type: "isBoolean", values: [] });

copy(alice, "A1:A7");
paste(alice, "B1");
expect([alice, bob, charlie]).toHaveSynchronizedValue(
(user) => getCell(user, "B1")?.content,
"TRUE"
);
expect([alice, bob, charlie]).toHaveSynchronizedValue(
(user) => getCell(user, "B2")?.content,
"FALSE" // A2 was empty, which is falsy
);
expect([alice, bob, charlie]).toHaveSynchronizedValue(
(user) => getCell(user, "B3")?.content,
"FALSE" // text is not a boolean -> falsy
);
expect([alice, bob, charlie]).toHaveSynchronizedValue(
(user) => getCell(user, "B4")?.content,
"=TRANSPOSE(B1)" // is truthy
);
expect([alice, bob, charlie]).toHaveSynchronizedValue(
(user) => getCell(user, "B5")?.content,
"FALSE" // text is not a boolean -> falsy
);
expect([alice, bob, charlie]).toHaveSynchronizedValue(
(user) => getCell(user, "B6")?.content,
"=NOT(B1)"
);
expect([alice, bob, charlie]).toHaveSynchronizedValue(
(user) => getCell(user, "B7")?.content,
"FALSE" // a number which does not represent a boolean is falsy
);
});
});

0 comments on commit f46e94d

Please sign in to comment.