From d365a289d4ed3f557fa46155419f5f517860fe9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Tue, 17 Oct 2017 16:29:35 +0200 Subject: [FIX] web: generate proper x2many commands in some cases When creating a new record with a non empty default value for a one2many field, we did send the command 0 (for each one2many line) instead of the command 1 (update). This was a pretty big problem, because then the server would try to create a line with possibly incomplete data. For example, it was not possible to create expense sheets: go to expense menu, select one expense in the list view, click on the action menu 'submit to manager', this opens a form view with a one2many with the selected expense, then save. Before this commit, this would lead to a server error. --- addons/web/static/src/js/views/basic/basic_model.js | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') 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 073d8599f06..8798e8cdb2e 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -875,7 +875,10 @@ var BasicModel = AbstractModel.extend({ // id never changes, and should not be written delete record._changes.id; } - var changes = self._generateChanges(record, {viewType: options.viewType}); + var changes = self._generateChanges(record, {viewType: options.viewType, changesOnly: method !== 'create'}); + + // id field should never be written/changed + delete changes.id; if (method === 'create') { var fieldNames = record.getFieldNames(); @@ -2438,7 +2441,7 @@ var BasicModel = AbstractModel.extend({ changes = _.extend({}, record.data, record._changes); } else { changes = _.extend({}, record._changes); - }; + } var withReadonly = options.withReadonly || false; var commands = this._generateX2ManyCommands(record, { changesOnly: 'changesOnly' in options ? options.changesOnly : true, @@ -2475,6 +2478,7 @@ var BasicModel = AbstractModel.extend({ changes[fieldName] = false; } } + return changes; }, /** @@ -2621,7 +2625,13 @@ var BasicModel = AbstractModel.extend({ // this is a new id relRecord = _.findWhere(relRecordAdded, {res_id: list.res_ids[i]}); changes = this._generateChanges(relRecord, options); - commands[fieldName].push(x2ManyCommands.create(changes)); + if ('id' in changes) { + var id = changes.id; + delete changes.id; + commands[fieldName].push(x2ManyCommands.update(id, changes)); + } else { + commands[fieldName].push(x2ManyCommands.create(changes)); + } } } if (options.changesOnly && !didChange && addedIds.length === 0 && removedIds.length === 0) { -- cgit v1.2.3 From d8f03e64fd46f45446c69f6b851813c559d0d5c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Thu, 19 Oct 2017 10:08:38 +0200 Subject: [FIX] web: do not send id in update commands for m2m When we generate commands for many2many, it could happen that the id was in the list of changed fields (when we are in a new records, with a non empty initial onchange). When this happens, the server is not happy. To reproduce this, open the uninstall wizard of a module in the 'App' application. This creates a new record with a non-empty many2many field. Then click on 'Show technical modules', this will trigger an onchange which should not send the id field. --- addons/web/static/src/js/views/basic/basic_model.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') 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 8798e8cdb2e..3591ac3cf94 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -2595,6 +2595,7 @@ var BasicModel = AbstractModel.extend({ _.each(relRecordUpdated, function (relRecord) { var changes = self._generateChanges(relRecord, options); if (!_.isEmpty(changes)) { + delete changes.id; var command = x2ManyCommands.update(relRecord.res_id, changes); commands[fieldName].push(command); } @@ -2626,9 +2627,8 @@ var BasicModel = AbstractModel.extend({ relRecord = _.findWhere(relRecordAdded, {res_id: list.res_ids[i]}); changes = this._generateChanges(relRecord, options); if ('id' in changes) { - var id = changes.id; delete changes.id; - commands[fieldName].push(x2ManyCommands.update(id, changes)); + commands[fieldName].push(x2ManyCommands.update(relRecord.res_id, changes)); } else { commands[fieldName].push(x2ManyCommands.create(changes)); } -- cgit v1.2.3 From 823cfd6ab1b30c0f86acd18e1de98c9bc988594d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 20 Oct 2017 10:53:46 +0200 Subject: [FIX] web: make sure m2m_checkbox works twice in a row We had an interesting bug with the field many2many_checkbox: in some cases, it could cause a crash, because it could not find an element in the localdata. To reproduce this, start with a non empty field many2many checkbox, then uncheck them twice. The reason is that the datapoint of type 'list' that represents the current value is actually modified, and the replace_with operation assumed that it could find the localID by looking into its data. I have the feeling that a more complete fix is to make sure that the datapoint is not modified (in its 'data' key), but is modified in its '_changes' key. However, this would probably be a bigger change. --- addons/web/static/src/js/views/basic/basic_model.js | 3 +++ 1 file changed, 3 insertions(+) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') 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 3591ac3cf94..b4a0b999453 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -1541,6 +1541,9 @@ var BasicModel = AbstractModel.extend({ removedDef = this._applyX2ManyChange(record, fieldName, { operation: 'DELETE', ids: _.map(removedIds, function (resID) { + if (resID in list._cache) { + return list._cache[resID]; + } return _.findWhere(listData, {res_id: resID}).id; }), }); -- cgit v1.2.3 From 635572b49d5b70a32b717cf2d52da49b10aac28e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 20 Oct 2017 15:29:10 +0200 Subject: [FIX] web: properly handle failures in onchanges With the new views, we mainly focused on making them work. But we also need to be able to handle unusual cases, such as some RPC failing. An example of such a problem was the following: - a form view with a one2many with an onchange - edit mode, change a field in the one2many, click elsewhere - the onchange triggers, calls the server - it may happen that the server fails with a ValidationError - the onchange deferred fails, the line in the one2many is restored to the value known by the list renderer, which is the previous value (but the new value was applied to the model) - we now have a 'corrupted' state: clicking on save will send the invalid value, even though the UI displays the previous one. We solve this problem in this commit by making sure that the onchange deferred returned by the _performOnchange always succeed (or stays pending). If the rpc fails, the deferred will succeed with an empty dictionary. This means that the list renderer will be updated with the value from the model. Note that it may not be intuitive that a failure in the onchange is not seen as an error from the perspective of the _performOnChange, but I think that it still makes sense: - the validation/network errors are not really a concern of the UI. The UI just display the values entered by the user. - if an onchange fails (for example, because of a coding error), we do not really want to prevent the user from working. Also, when the onchange is performed, the initial change was already applied to the local data. Rollbacking them would be a more difficult task. Note that we also improve the mock server in this commit, to allow mocking failures. --- addons/web/static/src/js/views/basic/basic_model.js | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) (limited to 'addons/web/static/src/js/views/basic/basic_model.js') 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 b4a0b999453..717dcc9f6af 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -3283,11 +3283,17 @@ var BasicModel = AbstractModel.extend({ * This method is quite important: it is supposed to perform the /onchange * rpc and apply the result. * + * The changes that triggered the onchange are assumed to have already been + * applied to the record. + * * @param {Object} record * @param {string[]} fields changed fields * @param {string} [viewType] current viewType. If not set, we will assume * main viewType from the record - * @returns {Deferred} + * @returns {Deferred} Note: this deferred cannot fail. It is either in the + * 'success' or 'pending' state. If the onchange rpc fails, it will be + * resolved with an empty dictionary. Note that the initial change was + * already applied. */ _performOnChange: function (record, fields, viewType) { var self = this; @@ -3307,7 +3313,9 @@ var BasicModel = AbstractModel.extend({ var context = this._getContext(record, options); var currentData = this._generateOnChangeData(record, {changesOnly: false}); - return self._rpc({ + var def = $.Deferred(); + + self._rpc({ model: record.model, method: 'onchange', args: [idList, currentData, fields, onchangeSpec, context], @@ -3332,9 +3340,12 @@ var BasicModel = AbstractModel.extend({ record._domains = _.extend(record._domains, result.domain); } return self._applyOnChange(result.value, record).then(function () { - return result; + def.resolve(result); }); + }).fail(function () { + def.resolve({}); }); + return def; }, /** * Once a record is created and some data has been fetched, we need to do -- cgit v1.2.3