Skip to content

Commit 4b6a03c

Browse files
committed
[FIX] collaborative: check all overlapping zones
Steps to reproduce: - merge A2:A3 and F1:F2 Then, concurrently: - Alice merges A1:A3 (increase the current merge) and C1:C2 - Bob removes merges in A2:A3 and F1:F2 Assuming Alice's revision arrives first to the server (`executed` in the code). When Bob's revision is transformed against Alice's, both zones in Bob's command are kept, which makes the command invalid since A2:A3 is no longer a merge (it is now A1:A3). Because it's invalid, the entire command is rejected, even though F1:F2 should still be un-merged to preserve Bob's intention. The merge transformation is wrong. The current code keeps a zone if at least one of the zones in the executed command doesn't overlap. But it should check all zones and should be kept if none are overlapping. Said differently: the zone should be dropped if it overlaps any of the `executed.target` zone. Task: 4736986 Part-of: #6138 Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com> Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
1 parent 2b1b12f commit 4b6a03c

File tree

3 files changed

+23
-5
lines changed

3 files changed

+23
-5
lines changed

src/collaborative/ot/ot_specific.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,8 @@ function mergeTransformation(
140140
}
141141
const target: Zone[] = [];
142142
for (const zone1 of toTransform.target) {
143-
for (const zone2 of executed.target) {
144-
if (!overlap(zone1, zone2)) {
145-
target.push({ ...zone1 });
146-
}
143+
if (executed.target.every((zone2) => !overlap(zone1, zone2))) {
144+
target.push(zone1);
147145
}
148146
}
149147
if (target.length) {

tests/collaborative/collaborative.test.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Model, UIPlugin } from "../../src";
22
import { DEFAULT_REVISION_ID, MESSAGE_VERSION } from "../../src/constants";
33
import { functionRegistry } from "../../src/functions";
4-
import { getDefaultCellHeight, range, toCartesian, toZone } from "../../src/helpers";
4+
import { getDefaultCellHeight, range, toCartesian, toZone, zoneToXc } from "../../src/helpers";
55
import { featurePluginRegistry } from "../../src/plugins";
66
import { Command, CommandResult, CoreCommand, DataValidationCriterion } from "../../src/types";
77
import { CollaborationMessage } from "../../src/types/collaborative/transport_service";
@@ -30,6 +30,7 @@ import {
3030
selectCell,
3131
setCellContent,
3232
setStyle,
33+
unMerge,
3334
undo,
3435
ungroupHeaders,
3536
} from "../test_helpers/commands_helpers";
@@ -380,6 +381,20 @@ describe("Multi users synchronisation", () => {
380381
);
381382
});
382383

384+
test("concurrent overlapping and non overlapping merge operations", () => {
385+
const sheetId = alice.getters.getActiveSheetId();
386+
merge(alice, "A2:A3");
387+
merge(alice, "F1:F2");
388+
network.concurrent(() => {
389+
merge(alice, "A1:A3, C1:C2");
390+
unMerge(bob, "A2:A3, F1:F2");
391+
});
392+
expect([alice, bob, charlie]).toHaveSynchronizedValue(
393+
(user) => user.getters.getMerges(sheetId).map(zoneToXc),
394+
["A1:A3", "C1:C2"]
395+
);
396+
});
397+
383398
test("Command not allowed is not dispatched to others users", () => {
384399
const spy = jest.spyOn(network, "sendMessage");
385400
setCellContent(alice, "A1", "hello", "invalidSheetId");

tests/collaborative/ot/ot_merged.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,11 @@ describe("OT with ADD_MERGE", () => {
6161
const result = transform(command, addMerge);
6262
expect(result).toBeUndefined();
6363
});
64+
test("some overlapping merges and some distinct merges", () => {
65+
const command = { ...cmd, target: target("A1:A3,E1:F2") };
66+
const result = transform(command, { ...addMerge, target: target("A2:A4,G1:G2") });
67+
expect(result).toEqual({ ...cmd, target: target("E1:F2") });
68+
});
6469
test("two overlapping merges in different sheets", () => {
6570
const command = { ...cmd, target: target("C3:D5"), sheetId: "another sheet" };
6671
const result = transform(command, addMerge);

0 commit comments

Comments
 (0)