From be3163e608ea7b1e2ab9f633dc8c1e3dc65c26dd Mon Sep 17 00:00:00 2001 From: FrancoisGe Date: Thu, 28 Jul 2022 09:52:20 +0000 Subject: [PATCH] [FIX] web: wait for onchanges to determine record validity Before this commit, the checkValidity of a record did not wait for the application of the current updates. This was causing crashes or records being abandoned despite an update being in progress. It is necessary to wait until all updates have been applied before checking the validity of a record. For an x2many in list mode: - create a new line - edit the line in a valid way - click outside the x2many while the onchange is in progress. Result before: - display a crash Result after: - X2many correctly updated For a list view: - Create a new record - edit a field of the record in a valid way - click outside the list view Result before: - abandon the record Result after: - the record was saved closes odoo/odoo#96725 Signed-off-by: Aaron Bohy (aab) --- .../src/views/basic_relational_model.js | 30 +++++----- .../src/views/fields/relational_utils.js | 2 +- .../web/static/src/views/relational_model.js | 20 +++++-- .../views/fields/one2many_field_tests.js | 57 +++++++++++++++++++ .../web/static/tests/views/list_view_tests.js | 53 +++++++++++++++++ 5 files changed, 143 insertions(+), 19 deletions(-) diff --git a/addons/web/static/src/views/basic_relational_model.js b/addons/web/static/src/views/basic_relational_model.js index 4ba0d9397c5..c6c7d15dc73 100644 --- a/addons/web/static/src/views/basic_relational_model.js +++ b/addons/web/static/src/views/basic_relational_model.js @@ -335,7 +335,12 @@ export class Record extends DataPoint { // Getters // ------------------------------------------------------------------------- - checkValidity() { + async checkValidity(urgent) { + if (!urgent) { + const proms = []; + this.model.env.bus.trigger("RELATIONAL_MODEL:NEED_LOCAL_CHANGES", { proms }); + await Promise.all([...proms, this._updatePromise]); + } for (const fieldName in this.activeFields) { const fieldType = this.fields[fieldName].type; if (fieldName in this._requiredFields) { @@ -352,7 +357,7 @@ export class Record extends DataPoint { continue; case "one2many": case "many2many": - if (!this.checkX2ManyValidity(fieldName)) { + if (!(await this.checkX2ManyValidity(fieldName))) { this.setInvalidField(fieldName); } break; @@ -414,10 +419,10 @@ export class Record extends DataPoint { return evalDomain(required, this.evalContext); } - checkX2ManyValidity(fieldName) { + async checkX2ManyValidity(fieldName) { const list = this.data[fieldName]; const record = list.editedRecord; - if (record && record.isNew && !record.checkValidity()) { + if (record && record.isNew && !(await record.checkValidity())) { if (record.canBeAbandoned) { list.abandonRecord(record.id); } else { @@ -603,7 +608,9 @@ export class Record extends DataPoint { throw new Error("couldn't find x2many field name"); } const operation = { operation: "UPDATE", id: this.__bm_handle__, data }; - await this.__syncParent(operation); + const prom = this.__syncParent(operation); + prom.catch(resolveUpdatePromise); + await prom; } else { const prom = this.model.__bm__.notifyChanges(this.__bm_handle__, data, { viewType: this.__viewType, @@ -631,11 +638,8 @@ export class Record extends DataPoint { this._savePromise = new Promise((r) => { resolveSavePromise = r; }); - const proms = []; - this.model.env.bus.trigger("RELATIONAL_MODEL:NEED_LOCAL_CHANGES", { proms }); - await Promise.all([...proms, this._updatePromise]); this._closeInvalidFieldsNotification(); - if (!this.checkValidity()) { + if (!(await this.checkValidity())) { const invalidFields = [...this._invalidFields].map((fieldName) => { return `
  • ${escape(this.fields[fieldName].string || fieldName)}
    • `; }, this); @@ -702,7 +706,7 @@ export class Record extends DataPoint { this.model.env.bus.trigger("RELATIONAL_MODEL:WILL_SAVE_URGENTLY"); await Promise.resolve(); this.__syncData(); - if (this.isDirty && this.checkValidity()) { + if (this.isDirty && (await this.checkValidity(true))) { this.model.__bm__.save(this.__bm_handle__, { reload: false }); } this.model.__bm__.bypassMutex = false; @@ -782,14 +786,14 @@ export class StaticList extends DataPoint { const editedRecord = this.editedRecord; if (editedRecord && editedRecord.id === record.id && mode === "readonly") { - const valid = record.checkValidity(); + const valid = await record.checkValidity(); if (valid) { this.editedRecord = null; } return valid; } if (editedRecord) { - const isValid = editedRecord.checkValidity(); + const isValid = await editedRecord.checkValidity(); if (!isValid) { if (editedRecord.canBeAbandoned) { this.abandonRecord(editedRecord.id); @@ -1091,7 +1095,7 @@ export class StaticList extends DataPoint { this.__syncData(); this.editedRecord = null; await editedRecord.switchMode("readonly"); - } else if (editedRecord.checkValidity()) { + } else if (await editedRecord.checkValidity()) { await editedRecord.switchMode("readonly"); } else { if (!editedRecord.isDirty) { diff --git a/addons/web/static/src/views/fields/relational_utils.js b/addons/web/static/src/views/fields/relational_utils.js index f7fc9e143e3..d114231aef8 100644 --- a/addons/web/static/src/views/fields/relational_utils.js +++ b/addons/web/static/src/views/fields/relational_utils.js @@ -477,7 +477,7 @@ class X2ManyFieldDialog extends owl.Component { } async save({ saveAndNew }) { - if (this.record.checkValidity()) { + if (await this.record.checkValidity()) { this.record = (await this.props.save(this.record, { saveAndNew })) || this.record; } else { return false; diff --git a/addons/web/static/src/views/relational_model.js b/addons/web/static/src/views/relational_model.js index 84d558ce7ae..c3ebbdfd2bd 100644 --- a/addons/web/static/src/views/relational_model.js +++ b/addons/web/static/src/views/relational_model.js @@ -101,8 +101,9 @@ async function toggleArchive(model, resModel, resIds, doArchive, context) { async function unselectRecord(editedRecord, abandonRecord) { if (editedRecord) { + const isValid = await editedRecord.checkValidity(); const canBeAbandoned = editedRecord.canBeAbandoned; - if (!canBeAbandoned && editedRecord.checkValidity()) { + if (isValid && !canBeAbandoned) { return editedRecord.switchMode("readonly"); } else if (canBeAbandoned) { return abandonRecord(editedRecord.id); @@ -552,7 +553,16 @@ export class Record extends DataPoint { this.invalidateCache(); } - checkValidity() { + async checkValidity() { + if (!this._urgentSave) { + const proms = []; + this.model.env.bus.trigger("RELATIONAL_MODEL:NEED_LOCAL_CHANGES", { proms }); + await Promise.all([...proms, this.model.mutex.getUnlockedDef()]); + } + return this._checkValidity(); + } + + _checkValidity() { for (const fieldName in this._requiredFields) { const fieldType = this.fields[fieldName].type; if (!evalDomain(this._requiredFields[fieldName], this.evalContext)) { @@ -737,7 +747,7 @@ export class Record extends DataPoint { isX2ManyValid(fieldName) { const value = this.data[fieldName]; - return value.records.every((r) => r.checkValidity()); + return value.records.every(async (r) => await r.checkValidity()); } async load(params = {}) { @@ -1214,7 +1224,7 @@ export class Record extends DataPoint { */ async _save(options = { stayInEdition: false, noReload: false }) { this.model.env.bus.trigger("RELATIONAL_MODEL:NEED_LOCAL_CHANGES"); - if (!this.checkValidity()) { + if (!(await this._checkValidity())) { const invalidFields = [...this._invalidFields].map((fieldName) => { return `
  • ${escape(this.fields[fieldName].string || fieldName)}
    • `; }, this); @@ -2580,7 +2590,7 @@ export class StaticList extends DataPoint { this.onRecordWillSwitchMode = async (record, mode) => { const editedRecord = this.editedRecord; if (editedRecord && editedRecord.id === record.id && mode === "readonly") { - const valid = record.checkValidity(); + const valid = await record.checkValidity(); if (valid) { this.editedRecord = null; } diff --git a/addons/web/static/tests/views/fields/one2many_field_tests.js b/addons/web/static/tests/views/fields/one2many_field_tests.js index 4c6415cca1c..41e24be93ea 100644 --- a/addons/web/static/tests/views/fields/one2many_field_tests.js +++ b/addons/web/static/tests/views/fields/one2many_field_tests.js @@ -9559,6 +9559,63 @@ QUnit.module("Fields", (hooks) => { assert.verifySteps(["onchange"]); }); + QUnit.test("edit a field with a slow onchange in one2many", async function (assert) { + serverData.models.turtle.onchanges = { + turtle_foo: function () {}, + }; + + let def; + await makeView({ + type: "form", + resModel: "partner", + serverData, + arch: ` +
    + + + + + +
    `, + async mockRPC(route, args) { + assert.step(args.method); + if (args.method === "onchange") { + await Promise.resolve(def); + } + }, + }); + + assert.verifySteps(["get_views", "onchange"]); + + const value = "hello"; + + // add a new line + await addRow(target); + + assert.verifySteps(["onchange"]); + + // we want to add a delay to simulate an onchange + def = makeDeferred(); + + // write something in the field + await editInput(target, "[name=turtle_foo] input", value); + assert.strictEqual(target.querySelector("[name=turtle_foo] input").value, value); + + await click(target, ".o_form_view"); + + // check that nothing changed before the onchange finished + assert.strictEqual(target.querySelector("[name=turtle_foo] input").value, value); + + assert.verifySteps(["onchange"]); + + // unlock onchange + def.resolve(); + await nextTick(); + + // check the current line is added with the correct content + assert.strictEqual(target.querySelector(".o_data_row [name=turtle_foo]").innerText, value); + }); + QUnit.test( "no deadlock when leaving a one2many line with uncommitted changes", async function (assert) { diff --git a/addons/web/static/tests/views/list_view_tests.js b/addons/web/static/tests/views/list_view_tests.js index 478204d7488..2c35930d2a7 100644 --- a/addons/web/static/tests/views/list_view_tests.js +++ b/addons/web/static/tests/views/list_view_tests.js @@ -14250,4 +14250,57 @@ QUnit.module("Views", (hooks) => { assert.verifySteps(["write"]); }); + + QUnit.test("edit a field with a slow onchange in a new row", async function (assert) { + serverData.models.foo.onchanges = { + int_field: function () {}, + }; + serverData.models.foo.records = []; + + let def; + await makeView({ + type: "list", + resModel: "foo", + serverData, + arch: ` + + + `, + async mockRPC(route, args) { + assert.step(args.method); + if (args.method === "onchange") { + await Promise.resolve(def); + } + }, + }); + + assert.verifySteps(["get_views", "web_search_read"]); + + const value = "14"; + // add a new line + await click(target.querySelector(".o_list_button_add")); + + assert.verifySteps(["onchange"]); + + // we want to add a delay to simulate an onchange + def = makeDeferred(); + + // write something in the field + await editInput(target, "[name=int_field] input", value); + assert.strictEqual(target.querySelector("[name=int_field] input").value, value); + + await click(target, ".o_list_view"); + + // check that nothing changed before the onchange finished + assert.strictEqual(target.querySelector("[name=int_field] input").value, value); + assert.verifySteps(["onchange"]); + + // unlock onchange + def.resolve(); + await nextTick(); + + // check the current line is added with the correct content + assert.strictEqual(target.querySelector(".o_data_row [name=int_field]").innerText, value); + assert.verifySteps(["create", "read"]); + }); });