From 825fa5b9f08b83c4b851798f6fa89f791cd3d522 Mon Sep 17 00:00:00 2001 From: Mathieu Duckerts-Antoine Date: Mon, 4 Jul 2022 12:30:42 +0000 Subject: [PATCH] [FIX] web: kanban: transaction error on record dragged twice Problem: Have - a slow network - a kanban view grouped then move a given record from a column to another twice quickly. An error occurs. This is due to the way the transaction system in kanban model works: the first move done in the first transaction being not yet finished, no other transaction can begin and an error is thrown. Solution: Subclass Error and catch the specific transaction error at the appropriate time to cancel the second order. Note that the problem does not occur in a not grouped kanban view because the hook useSortable prevent asking again a resequencing via a proper management of the class o_record_draggable. That class is also used in the grouped case but a rendering of the kanban view takes place possibly before the end of the transaction: the record is moved in any case to the targeted column and moved back if the overall operation fails for some reason. closes odoo/odoo#95235 Signed-off-by: Aaron Bohy (aab) --- .../static/src/views/kanban/kanban_model.js | 24 ++- .../static/tests/views/kanban_view_tests.js | 176 ++++++++++++++++++ 2 files changed, 193 insertions(+), 7 deletions(-) diff --git a/addons/web/static/src/views/kanban/kanban_model.js b/addons/web/static/src/views/kanban/kanban_model.js index e578d023a7f..21f146a711d 100644 --- a/addons/web/static/src/views/kanban/kanban_model.js +++ b/addons/web/static/src/views/kanban/kanban_model.js @@ -21,27 +21,33 @@ const { EventBus, markRaw } = owl; const FALSE = Symbol("false"); +class TransactionInProgress extends Error {} + +class NoTransactionInProgress extends Error {} + function makeTransactionManager() { const bus = new EventBus(); const transactions = {}; return { start: (id) => { if (transactions[id]) { - throw new Error(`Transaction in progress: commit or abort to start a new one.`); + throw new TransactionInProgress( + `Transaction in progress: commit or abort to start a new one.` + ); } transactions[id] = true; bus.trigger("START"); }, commit: (id) => { if (!transactions[id]) { - throw new Error(`No transaction in progress.`); + throw new NoTransactionInProgress(`No transaction in progress.`); } delete transactions[id]; bus.trigger("COMMIT"); }, abort: (id) => { if (!transactions[id]) { - throw new Error(`No transaction in progress.`); + throw new NoTransactionInProgress(`No transaction in progress.`); } delete transactions[id]; bus.trigger("ABORT"); @@ -406,7 +412,6 @@ export class KanbanDynamicGroupList extends DynamicGroupList { * @param {string} dataGroupId * @param {string} refId * @param {string} targetGroupId - * @returns {Promise} */ async moveRecord(dataRecordId, dataGroupId, refId, targetGroupId) { const sourceGroup = this.groups.find((g) => g.id === dataGroupId); @@ -418,7 +423,14 @@ export class KanbanDynamicGroupList extends DynamicGroupList { const record = sourceGroup.list.records.find((r) => r.id === dataRecordId); - this.model.transaction.start(dataRecordId); + try { + this.model.transaction.start(dataRecordId); + } catch (err) { + if (err instanceof TransactionInProgress) { + return; + } + throw err; + } // Move from one group to another try { @@ -445,8 +457,6 @@ export class KanbanDynamicGroupList extends DynamicGroupList { this.model.notify(); throw err; } - - return record; } // ------------------------------------------------------------------------ diff --git a/addons/web/static/tests/views/kanban_view_tests.js b/addons/web/static/tests/views/kanban_view_tests.js index ff3d707cdd2..da21cfcc166 100644 --- a/addons/web/static/tests/views/kanban_view_tests.js +++ b/addons/web/static/tests/views/kanban_view_tests.js @@ -10331,4 +10331,180 @@ QUnit.module("Views", (hooks) => { assert.hasClass(document.activeElement, "o_searchview_input"); } ); + + QUnit.test("no leak of TransactionInProgress (grouped case)", async (assert) => { + let def; + await makeView({ + type: "kanban", + resModel: "partner", + serverData, + arch: ` + + + + +
+ +
+ + + + `, + groupBy: ["state"], + async mockRPC(route) { + if (route === "/web/dataset/resequence") { + assert.step("resequence"); + await Promise.resolve(def); + } + }, + }); + + def = makeDeferred(); + + assert.containsOnce(target, ".o_kanban_group:nth-child(1) .o_kanban_record"); + assert.strictEqual( + target.querySelector(".o_kanban_group:nth-child(1) .o_kanban_record").innerText, + "yop" + ); + assert.containsOnce(target, ".o_kanban_group:nth-child(2) .o_kanban_record"); + assert.strictEqual( + target.querySelector(".o_kanban_group:nth-child(2) .o_kanban_record").innerText, + "blip" + ); + assert.containsN(target, ".o_kanban_group:nth-child(3) .o_kanban_record", 2); + + assert.verifySteps([]); + + // move "yop" from first to second column + await dragAndDrop( + ".o_kanban_group:nth-child(1) .o_kanban_record", + ".o_kanban_group:nth-child(2)" + ); + + assert.containsNone(target, ".o_kanban_group:nth-child(1) .o_kanban_record"); + assert.containsN(target, ".o_kanban_group:nth-child(2) .o_kanban_record", 2); + assert.deepEqual( + [...target.querySelectorAll(".o_kanban_group:nth-child(2) .o_kanban_record")].map( + (el) => el.innerText + ), + ["blip", "yop"] + ); + assert.containsN(target, ".o_kanban_group:nth-child(3) .o_kanban_record", 2); + + assert.verifySteps(["resequence"]); + + // try to move "yop" from second to third column + await dragAndDrop( + ".o_kanban_group:nth-child(2) .o_kanban_record:nth-child(3)", // move yop + ".o_kanban_group:nth-child(3)" + ); + + assert.containsNone(target, ".o_kanban_group:nth-child(1) .o_kanban_record"); + assert.containsN(target, ".o_kanban_group:nth-child(2) .o_kanban_record", 2); + assert.deepEqual( + [...target.querySelectorAll(".o_kanban_group:nth-child(2) .o_kanban_record")].map( + (el) => el.innerText + ), + ["blip", "yop"] + ); + assert.containsN(target, ".o_kanban_group:nth-child(3) .o_kanban_record", 2); + + assert.verifySteps([]); + + def.resolve(); + await nextTick(); + + // try again to move "yop" from second to third column + await dragAndDrop( + ".o_kanban_group:nth-child(2) .o_kanban_record:nth-child(3)", // move yop + ".o_kanban_group:nth-child(3)" + ); + + assert.containsNone(target, ".o_kanban_group:nth-child(1) .o_kanban_record"); + assert.containsOnce(target, ".o_kanban_group:nth-child(2) .o_kanban_record"); + assert.containsN(target, ".o_kanban_group:nth-child(3) .o_kanban_record", 3); + assert.deepEqual( + [...target.querySelectorAll(".o_kanban_group:nth-child(3) .o_kanban_record")].map( + (el) => el.innerText + ), + ["gnap", "blip", "yop"] + ); + + assert.verifySteps(["resequence"]); + }); + + QUnit.test("no leak of TransactionInProgress (not grouped case)", async (assert) => { + let def; + await makeView({ + type: "kanban", + resModel: "partner", + serverData, + arch: ` + + + + +
+ +
+ + + + `, + async mockRPC(route) { + if (route === "/web/dataset/resequence") { + assert.step("resequence"); + await Promise.resolve(def); + } + }, + }); + + def = makeDeferred(); + + assert.containsN(target, ".o_kanban_record:not(.o_kanban_ghost)", 4); + assert.deepEqual( + [...target.querySelectorAll(".o_kanban_record:not(.o_kanban_ghost)")].map( + (el) => el.innerText + ), + ["yop", "blip", "gnap", "blip"] + ); + + assert.verifySteps([]); + + // move "yop" to second place + await dragAndDrop(".o_kanban_record", ".o_kanban_record:nth-child(2)"); + + assert.deepEqual( + [...target.querySelectorAll(".o_kanban_record:not(.o_kanban_ghost)")].map( + (el) => el.innerText + ), + ["yop", "blip", "gnap", "blip"] + ); + assert.verifySteps(["resequence"]); + + // try again + await dragAndDrop(".o_kanban_record", ".o_kanban_record:nth-child(2)"); + + assert.verifySteps([]); + + def.resolve(); + await nextTick(); + + assert.deepEqual( + [...target.querySelectorAll(".o_kanban_record:not(.o_kanban_ghost)")].map( + (el) => el.innerText + ), + ["blip", "yop", "gnap", "blip"] + ); + + await dragAndDrop(".o_kanban_record:nth-child(2)", ".o_kanban_record:nth-child(3)"); + + assert.deepEqual( + [...target.querySelectorAll(".o_kanban_record:not(.o_kanban_ghost)")].map( + (el) => el.innerText + ), + ["blip", "gnap", "yop", "blip"] + ); + assert.verifySteps(["resequence"]); + }); });