From 50bf8309f88fb6c8d8e27b14d40450e92e5a4e4e Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Thu, 16 Jan 2020 14:59:05 +0000
Subject: [PATCH] [FIX] web,*: fix race condition with m2o quick create
*sale,sale_product_configurator
Let's assume a form view containing a many2one with an onchange
that updates the value of a one2many. Do a quick create in the
many2one. While the name_create request is pending, add a row to
the one2many but do not leave it. When the name_create returns, and
the onchange has been performed, the one2many is reset, and the row
is no longer in edition (worse, it could be invalid, i.e. in a state
that the user could not have reached in a normal situation).
This commit fixes this issue by considering the whole [name_create +
onchange] operation as one. This operation is executed in the mutex
of the model, so the other request (adding a row to the one2many) is
delayed until the many2one value has been correctly set.
This fixes an issue with the sale and rental tours (on sale_order),
that had been deactivated for a while.
---
.../src/js/product_configurator_widget.js | 27 +++++-----
.../src/js/product_configurator_widget.js | 8 +--
.../static/src/js/fields/relational_fields.js | 17 ++-----
.../static/src/js/views/basic/basic_model.js | 50 ++++++++++++++++---
.../relational_fields/field_many2one_tests.js | 1 +
.../relational_fields/field_one2many_tests.js | 50 +++++++++++++++++++
.../tests/fields/relational_fields_tests.js | 23 +++++++++
7 files changed, 139 insertions(+), 37 deletions(-)
diff --git a/addons/sale/static/src/js/product_configurator_widget.js b/addons/sale/static/src/js/product_configurator_widget.js
index 33f146deef30..5aab1cc988fd 100644
--- a/addons/sale/static/src/js/product_configurator_widget.js
+++ b/addons/sale/static/src/js/product_configurator_widget.js
@@ -109,24 +109,23 @@ var ProductConfiguratorWidget = relationalFields.FieldMany2One.extend({
*
* @override
* @param {OdooEvent} ev
- * {boolean} ev.data.preventProductIdCheck prevent the product configurator widget
+ * @param {boolean} ev.data.preventProductIdCheck prevent the product configurator widget
* from looping forever when it needs to change the 'product_template_id'
*
* @private
*/
- _onFieldChanged: function (ev) {
- var self = this;
-
- this._super.apply(this, arguments);
-
- if (ev.data.changes && !ev.data.preventProductIdCheck && ev.data.changes.product_template_id) {
- self._onTemplateChange(ev.data.changes.product_template_id.id, ev.data.dataPointID);
- } else if (ev.data.changes && ev.data.changes.product_id) {
- self._onProductChange(ev.data.changes.product_id.id, ev.data.dataPointID).then(function (wizardOpened) {
- if (!wizardOpened) {
- self._onLineConfigured();
- }
- });
+ reset: async function (record, ev) {
+ await this._super(...arguments);
+ if (ev.target === this) {
+ if (ev.data.changes && !ev.data.preventProductIdCheck && ev.data.changes.product_template_id) {
+ this._onTemplateChange(record.data.product_template_id.data.id, ev.data.dataPointID);
+ } else if (ev.data.changes && ev.data.changes.product_id) {
+ this._onProductChange(record.data.product_id.data.id, ev.data.dataPointID).then(wizardOpened => {
+ if (!wizardOpened) {
+ this._onLineConfigured();
+ }
+ });
+ }
}
},
diff --git a/addons/sale_product_configurator/static/src/js/product_configurator_widget.js b/addons/sale_product_configurator/static/src/js/product_configurator_widget.js
index 318147bd583f..48f15bbe63e1 100644
--- a/addons/sale_product_configurator/static/src/js/product_configurator_widget.js
+++ b/addons/sale_product_configurator/static/src/js/product_configurator_widget.js
@@ -33,9 +33,11 @@ ProductConfiguratorWidget.include({
* @override
* @private
*/
- _onFieldChanged: function (ev) {
- this.restoreProductTemplateId = this.recordData.product_template_id;
- this.optionalProducts = (ev.data && ev.data.optionalProducts) || this.optionalProducts;
+ reset: function (record, ev) {
+ if (ev.target === this) {
+ this.restoreProductTemplateId = this.recordData.product_template_id;
+ this.optionalProducts = (ev.data && ev.data.optionalProducts) || this.optionalProducts;
+ }
this._super.apply(this, arguments);
},
diff --git a/addons/web/static/src/js/fields/relational_fields.js b/addons/web/static/src/js/fields/relational_fields.js
index c1f116784fa2..deff2fbebcfe 100644
--- a/addons/web/static/src/js/fields/relational_fields.js
+++ b/addons/web/static/src/js/fields/relational_fields.js
@@ -489,23 +489,12 @@ var FieldMany2One = AbstractField.extend({
dialog.on('closed', self, createDone);
};
if (self.nodeOptions.quick_create) {
- var nameCreateDef = self._rpc({
- model: self.field.relation,
- method: 'name_create',
- args: [name],
- context: self.record.getContext(self.recordParams),
- }).guardedCatch(function (reason) {
+ const prom = self.reinitialize({id: false, display_name: name});
+ prom.guardedCatch(reason => {
reason.event.preventDefault();
slowCreate();
});
- self.dp.add(nameCreateDef)
- .then(function (result) {
- if (self.mode === "edit") {
- self.reinitialize({id: result[0], display_name: result[1]});
- }
- createDone();
- })
- .guardedCatch(reject);
+ self.dp.add(prom).then(createDone).guardedCatch(reject);
} else {
slowCreate();
}
diff --git a/addons/web/static/src/js/views/basic/basic_model.js b/addons/web/static/src/js/views/basic/basic_model.js
index a4c6d0e87955..e43c7f2a0e6e 100644
--- a/addons/web/static/src/js/views/basic/basic_model.js
+++ b/addons/web/static/src/js/views/basic/basic_model.js
@@ -1472,7 +1472,7 @@ var BasicModel = AbstractModel.extend({
if (field && (field.type === 'one2many' || field.type === 'many2many')) {
defs.push(this._applyX2ManyChange(record, fieldName, changes[fieldName], options));
} else if (field && (field.type === 'many2one' || field.type === 'reference')) {
- defs.push(this._applyX2OneChange(record, fieldName, changes[fieldName]));
+ defs.push(this._applyX2OneChange(record, fieldName, changes[fieldName], options));
} else {
record._changes[fieldName] = changes[fieldName];
}
@@ -1523,18 +1523,39 @@ var BasicModel = AbstractModel.extend({
* onchange modifies a many2one field. For this reason, we need (sometimes)
* to do a /name_get to fetch a display_name.
*
+ * Moreover, for the many2one case, a new value can sometimes be set (i.e.
+ * a display_name is given, but no id). When this happens, we first do a
+ * name_create.
+ *
* @param {Object} record
* @param {string} fieldName
* @param {Object} [data]
+ * @param {Object} [options]
+ * @param {string} [options.viewType] current viewType. If not set, we will
+ * assume main viewType from the record
* @returns {Promise}
*/
- _applyX2OneChange: function (record, fieldName, data) {
+ _applyX2OneChange: async function (record, fieldName, data, options) {
+ options = options || {};
var self = this;
- if (!data || !data.id) {
+ if (!data || (!data.id && !data.display_name)) {
record._changes[fieldName] = false;
return Promise.resolve();
}
+ const field = record.fields[fieldName];
+ const coModel = field.type === 'reference' ? data.model : field.relation;
+ if (field.type === 'many2one' && !data.id && data.display_name) {
+ // only display_name given -> do a name_create
+ const result = await this._rpc({
+ model: coModel,
+ method: 'name_create',
+ args: [data.display_name],
+ context: this._getContext(record, {fieldName: fieldName, viewType: options.viewType}),
+ });
+ data = {id: result[0], display_name: result[1]};
+ }
+
// here, we check that the many2one really changed. If the res_id is the
// same, we do not need to do any extra work. It can happen when the
// user edited a manyone (with the small form view button) with an
@@ -1551,11 +1572,9 @@ var BasicModel = AbstractModel.extend({
return Promise.resolve();
}
var rel_data = _.pick(data, 'id', 'display_name');
- var field = record.fields[fieldName];
// the reference field doesn't store its co-model in its field metadata
// but directly in the data (as the co-model isn't fixed)
- var coModel = field.type === 'reference' ? data.model : field.relation;
var def;
if (rel_data.display_name === undefined) {
// TODO: refactor this to use _fetchNameGet
@@ -1802,7 +1821,7 @@ var BasicModel = AbstractModel.extend({
* (only supported by the 'CREATE' command.operation)
* @returns {Promise}
*/
- _applyX2ManyChange: function (record, fieldName, command, options) {
+ _applyX2ManyChange: async function (record, fieldName, command, options) {
if (command.operation === 'TRIGGER_ONCHANGE') {
// the purpose of this operation is to trigger an onchange RPC, so
// there is no need to apply any change on the record (the changes
@@ -1848,6 +1867,25 @@ var BasicModel = AbstractModel.extend({
// record added) or an array of dict of values
var data = _.isArray(command.ids) ? command.ids : [command.ids];
+ // name_create records for which there is no id (typically, could
+ // be the case of a quick_create in a many2many_tags, so data.length
+ // is 1)
+ for (const r of data) {
+ if (!r.id && r.display_name) {
+ const prom = this._rpc({
+ model: field.relation,
+ method: 'name_create',
+ args: [r.display_name],
+ context: this._getContext(record, {fieldName: fieldName, viewType: options.viewType}),
+ }).then(result => {
+ r.id = result[0];
+ r.display_name = result[1];
+ });
+ defs.push(prom);
+ }
+ }
+ await Promise.all(defs);
+
// Ensure the local data repository (list) boundaries can handle incoming records (data)
if (data.length + list.res_ids.length > list.limit) {
list.limit = data.length + list.res_ids.length;
diff --git a/addons/web/static/tests/fields/relational_fields/field_many2one_tests.js b/addons/web/static/tests/fields/relational_fields/field_many2one_tests.js
index 8c84cf4edf22..46c6ebbe7981 100644
--- a/addons/web/static/tests/fields/relational_fields/field_many2one_tests.js
+++ b/addons/web/static/tests/fields/relational_fields/field_many2one_tests.js
@@ -967,6 +967,7 @@ QUnit.module('fields', {}, function () {
},
});
var parent = new StandaloneWidget(model);
+ model.setParent(parent);
testUtils.mock.addMockEnvironment(parent, {
data: self.data,
mockRPC: function (route, args) {
diff --git a/addons/web/static/tests/fields/relational_fields/field_one2many_tests.js b/addons/web/static/tests/fields/relational_fields/field_one2many_tests.js
index 60f3398c88ca..8aacf099de4c 100644
--- a/addons/web/static/tests/fields/relational_fields/field_one2many_tests.js
+++ b/addons/web/static/tests/fields/relational_fields/field_one2many_tests.js
@@ -8741,6 +8741,56 @@ QUnit.module('fields', {}, function () {
form.destroy();
});
+ QUnit.test('one2many reset by onchange (of another field) while being edited', async function (assert) {
+ // In this test, we have a many2one and a one2many. The many2one has an onchange that
+ // updates the value of the one2many. We set a new value to the many2one (name_create)
+ // such that the onchange is delayed. During the name_create, we click to add a new row
+ // to the one2many. After a while, we unlock the name_create, which triggers the onchange
+ // and resets the one2many. At the end, we want the row to be in edition.
+ assert.expect(3);
+
+ const prom = testUtils.makeTestPromise();
+ this.data.partner.onchanges = {
+ trululu: obj => {
+ obj.p = [[5]].concat(obj.p);
+ },
+ };
+
+ const form = await createView({
+ View: FormView,
+ model: 'partner',
+ data: this.data,
+ arch: `
+ <form>
+ <field name="trululu"/>
+ <field name="p">
+ <tree editable="top"><field name="product_id" required="1"/></tree>
+ </field>
+ </form>`,
+ mockRPC: function (route, args) {
+ const result = this._super.apply(this, arguments);
+ if (args.method === 'name_create') {
+ return prom.then(() => result);
+ }
+ return result;
+ },
+ });
+
+ // set a new value for trululu (will delay the onchange)
+ await testUtils.fields.many2one.searchAndClickItem('trululu', {search: 'new value'});
+
+ // add a row in p
+ await testUtils.dom.click(form.$('.o_field_x2many_list_row_add a'));
+ assert.containsNone(form, '.o_data_row');
+
+ // resolve the name_create to trigger the onchange, and the reset of p
+ prom.resolve();
+ await testUtils.nextTick();
+ assert.containsOnce(form, '.o_data_row');
+ assert.hasClass(form.$('.o_data_row'), 'o_selected_row');
+
+ form.destroy();
+ });
});
});
});
diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js
index c3f9a95ea7d4..41f7a86afc49 100644
--- a/addons/web/static/tests/fields/relational_fields_tests.js
+++ b/addons/web/static/tests/fields/relational_fields_tests.js
@@ -1914,6 +1914,29 @@ QUnit.module('relational_fields', {
form.destroy();
});
+ QUnit.test('fieldmany2many tags: quick create a new record', async function (assert) {
+ assert.expect(3);
+
+ const form = await createView({
+ View: FormView,
+ model: 'partner',
+ data: this.data,
+ arch: `<form><field name="timmy" widget="many2many_tags"/></form>`,
+ });
+
+ assert.containsNone(form, '.o_field_many2manytags .badge');
+
+ await testUtils.fields.many2one.searchAndClickItem('timmy', {search: 'new value'});
+
+ assert.containsOnce(form, '.o_field_many2manytags .badge');
+
+ await testUtils.form.clickSave(form);
+
+ assert.strictEqual(form.el.querySelector('.o_field_many2manytags').innerText, 'new value');
+
+ form.destroy();
+ });
+
QUnit.module('FieldRadio');
QUnit.test('fieldradio widget on a many2one in a new record', async function (assert) {
--
GitLab