From ebd17217c4e7e9ff06d97028de64fb2fc44d6266 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Thu, 8 Jun 2017 15:37:04 +0200 Subject: [FIX] web: BasicModel: don't write on readonly fields Ultimately, it seems that sending the value of readonly fields when saving is not such a good idea. Suppose that there is a computed field displayed as readonly in a form view. This field is computed from other records, e.g. from a one2many field which is editable in the form view. Finally, the inverse function of the computed field creates the one2many records. If the user adds some records to the one2many, then saves, the readonly computed field's value is sent to the server as well, and thus the inverse function is executed, which isn't what we want. This commit essentially reverts 0494d61274, except that we now take into account the readonly modifier to determine whether or not a value is sent to the server, and not only the readonly attribute of the field (which can be seen as a default). Before rev. 0494d61274, we made a difference between write and create RPCs, for an unclear reason. We don't do that anymore: readonly fields are never sent to the server (same behavior as before the new views). --- .../web/static/src/js/views/basic/basic_model.js | 61 ++++++++++++++++++++-- 1 file changed, 56 insertions(+), 5 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 61c285d6581..20066b858f1 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -704,6 +704,8 @@ var BasicModel = AbstractModel.extend({ * @param {boolean} [options.savePoint=false] if true, the record will only * be 'locally' saved: its changes written in a _savePoint key that can * be restored later by call discardChanges with option rollback to true + * @param {string} [options.viewType] current viewType. If not set, we will + * assume main viewType from the record * @returns {Deferred} * Resolved with the list of field names (whose value has been modified) */ @@ -718,6 +720,11 @@ var BasicModel = AbstractModel.extend({ if (newValue instanceof Array) { rec._savePoint = newValue.slice(0); } else { + // save the viewType of edition, so that the correct readonly modifiers + // can be evaluated when the record will be saved + for (var fieldName in (rec._changes || {})) { + rec._editionViewType[fieldName] = options.viewType; + } rec._savePoint = _.extend({}, newValue); } }); @@ -728,7 +735,7 @@ var BasicModel = AbstractModel.extend({ // id never changes, and should not be written delete record._changes.id; } - var changes = self._generateChanges(record); + var changes = self._generateChanges(record, options.viewType); if (method === 'create') { var fieldNames = record.getFieldNames(); @@ -1869,12 +1876,26 @@ var BasicModel = AbstractModel.extend({ * * @private * @param {Object} record + * @param {string} [viewType] current viewType. If not set, we will assume + * main viewType from the record. Note that if an editionViewType is + * specified for a field, it will take the priority over the viewType arg. * @returns {Object} a map from changed fields to their new value */ - _generateChanges: function (record) { + _generateChanges: function (record, viewType) { + viewType = viewType || record.viewType; var changes = _.extend({}, record._changes); var commands = this._generateX2ManyCommands(record, true); for (var fieldName in record.fields) { + // remove readonly fields from the list of changes + if (fieldName in changes || fieldName in commands) { + var editionViewType = record._editionViewType[fieldName] || viewType; + if (this._isFieldReadonly(record, fieldName, editionViewType)) { + delete changes[fieldName]; + continue; + } + } + + // process relational fields and handle the null case var type = record.fields[fieldName].type; if (type === 'one2many' || type === 'many2many') { if (commands[fieldName].length) { // replace localId by commands @@ -2199,6 +2220,31 @@ var BasicModel = AbstractModel.extend({ } return context; }, + /** + * Returns true if the field is readonly (checking first in the modifiers, + * and if there is no readonly modifier, checking the readonly attribute of + * the field). + * + * @private + * @param {Object} record an element from the localData + * @param {string} fieldName + * @param {string} [viewType] current viewType. If not set, we will assume + * main viewType from the record + * @returns {boolean} + */ + _isFieldReadonly: function (record, fieldName, viewType) { + var fieldInfo = record.fieldsInfo[viewType || record.viewType][fieldName]; + var modifiers; + if (fieldInfo) { + var rawModifiers = JSON.parse(fieldInfo.modifiers || "{}"); + modifiers = this._evalModifiers(record, rawModifiers); + } + if (modifiers && 'readonly' in modifiers) { + return modifiers.readonly; + } else { + return record.fields[fieldName].readonly; + } + }, /** * Returns true iff value is considered to be set for the given field's type. * @@ -2330,6 +2376,12 @@ var BasicModel = AbstractModel.extend({ viewType: params.viewType, }; + // _editionViewType is a dict whose keys are field names and which is populated when a field + // is edited with the viewType as value. This is useful for one2manys to determine whether + // or not a field is readonly (using the readonly modifiers of the view in which the field + // has been edited) + dataPoint._editionViewType = {}; + dataPoint.evalModifiers = this._evalModifiers.bind(this, dataPoint); dataPoint.getContext = this._getContext.bind(this, dataPoint); dataPoint.getDomain = this._getDomain.bind(this, dataPoint); @@ -2507,7 +2559,7 @@ var BasicModel = AbstractModel.extend({ .then(function () { // save initial changes, so they can be restored later, // if we need to discard. - self.save(record.id, {savePoint: true}) + self.save(record.id, {savePoint: true}); return record.id; }); @@ -2582,8 +2634,7 @@ var BasicModel = AbstractModel.extend({ * @param {string[]} fields changed fields * @param {string} [viewType] current viewType. If not set, we will assume * main viewType from the record - * @returns {Deferred} The returned deferred can fail, in which case the - * fail value will be the warning message received from the server + * @returns {Deferred} */ _performOnChange: function (record, fields, viewType) { var self = this; -- cgit v1.2.3 From 708c1c2915563a40a1e4c736b4fd11ffd89280f8 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Tue, 13 Jun 2017 08:46:15 +0200 Subject: [FIX] web: false id should remain false in evalContext 'id' is a particular field that should not be considered like other integer fields. For instance, in the evalContext, its value should not fallback to 0 if it is false. This would break domains in attrs like ['id', '=', False], which is supposed to be true on new (not saved yet) records. This was for example the case in the Product form view: go to Sales Orders form view, add a line, on the product field choose 'Create and Edit', try to save the product -> error: field Product Template is required but not set (actually it is invisible), as its required modifier says ['id', '!=', False], which was always the case. --- addons/web/static/src/js/views/basic/basic_model.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (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 20066b858f1..6c23550ba2e 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -2176,7 +2176,7 @@ var BasicModel = AbstractModel.extend({ if (context[fieldName] === null) { context[fieldName] = false; } - if (!field) { + if (!field || field.name === 'id') { continue; } if (field.type === 'float' || -- cgit v1.2.3 From a3bea9a62bdaf205c0a9cd75945f6f6e9b023d6d Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Tue, 13 Jun 2017 15:18:27 +0200 Subject: [FIX] web: Form: delete the correct record When the user duplicated a record 'A', and deleted the new record 'A (copy)' directly, the wrong record was deleted: 'A' was deleted instead of 'A (copy)', because de id of 'A' was bound to the delete handler, and it wasn't updated after the duplication. Also correctly updates the record's offset after the deletion, so that it doesn't produce a warning when deleting the last record of the dataset. --- addons/web/static/src/js/views/basic/basic_model.js | 1 + 1 file changed, 1 insertion(+) (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 6c23550ba2e..ae48a98dd13 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -208,6 +208,7 @@ var BasicModel = AbstractModel.extend({ delete self.localData[record.id]; } else { record.res_ids.splice(record.offset, 1); + record.offset = Math.min(record.offset, record.res_ids.length - 1); record.res_id = record.res_ids[record.offset]; record.count--; } -- cgit v1.2.3 From f6525c6b6ce04534e7fb2c282191243b8d3c4707 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Wed, 14 Jun 2017 13:42:38 +0200 Subject: [FIX] web: BasicModel: onchanges: send all values Since commit ebd17217c4, values of readonly fields inside x2many fields weren't sent for onchanges, whereas they should. This was for example an issue on the stock.picking form view (which is in state 'draft' by default). If the user added a stock.move, the state field on the move wasn't sent for the onchange, and thus the state field on the picking was set to false. --- .../web/static/src/js/views/basic/basic_model.js | 42 ++++++++++++++-------- 1 file changed, 28 insertions(+), 14 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 ae48a98dd13..d897a74c696 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -736,7 +736,7 @@ var BasicModel = AbstractModel.extend({ // id never changes, and should not be written delete record._changes.id; } - var changes = self._generateChanges(record, options.viewType); + var changes = self._generateChanges(record, {viewType: options.viewType}); if (method === 'create') { var fieldNames = record.getFieldNames(); @@ -1877,18 +1877,28 @@ var BasicModel = AbstractModel.extend({ * * @private * @param {Object} record - * @param {string} [viewType] current viewType. If not set, we will assume - * main viewType from the record. Note that if an editionViewType is + * @param {Object} [options] + * @param {boolean} [options.changesOnly=true] if true, only generates + * commands for fields that have changed (concerns x2many fields only) + * @param {boolean} [options.withReadonly=false] if false, doesn't generate + * changes for readonly fields + * @param {string} [options.viewType] current viewType. If not set, we will + * assume main viewType from the record. Note that if an editionViewType is * specified for a field, it will take the priority over the viewType arg. * @returns {Object} a map from changed fields to their new value */ - _generateChanges: function (record, viewType) { - viewType = viewType || record.viewType; + _generateChanges: function (record, options) { + options = options || {}; + var viewType = options.viewType || record.viewType; var changes = _.extend({}, record._changes); - var commands = this._generateX2ManyCommands(record, true); + var withReadonly = options.withReadonly || false; + var commands = this._generateX2ManyCommands(record, { + changesOnly: 'changesOnly' in options ? options.changesOnly : true, + withReadonly: withReadonly, + }); for (var fieldName in record.fields) { // remove readonly fields from the list of changes - if (fieldName in changes || fieldName in commands) { + if (!withReadonly && fieldName in changes || fieldName in commands) { var editionViewType = record._editionViewType[fieldName] || viewType; if (this._isFieldReadonly(record, fieldName, editionViewType)) { delete changes[fieldName]; @@ -1923,7 +1933,7 @@ var BasicModel = AbstractModel.extend({ * @returns {Object} the data */ _generateOnChangeData: function (record) { - var commands = this._generateX2ManyCommands(record, false); + var commands = this._generateX2ManyCommands(record, {withReadonly: true}); var data = _.extend(this.get(record.id, {raw: true}).data, commands); // one2many records have a parentID @@ -1946,12 +1956,16 @@ var BasicModel = AbstractModel.extend({ * or write them... * * @param {Object} record + * @param {Object} [options] * @param {boolean} [changesOnly=false] if true, only generates commands for * fields that have changed + * @param {boolean} [options.withReadonly=false] if false, doesn't generate + * changes for readonly fields in commands * @returns {Object} a map from some field names to commands */ - _generateX2ManyCommands: function (record, changesOnly) { + _generateX2ManyCommands: function (record, options) { var self = this; + options = options || {}; var commands = {}; var data = _.extend({}, record.data, record._changes); var type; @@ -1965,7 +1979,7 @@ var BasicModel = AbstractModel.extend({ continue; } var list = this.localData[data[fieldName]]; - if (changesOnly && !list._changes) { + if (options.changesOnly && !list._changes) { // if only changes are requested, skip if there is no change continue; } @@ -1983,7 +1997,7 @@ var BasicModel = AbstractModel.extend({ // generate update commands for records that have been // updated (it may happen with editable lists) _.each(relData, function (relRecord) { - var changes = self._generateChanges(relRecord); + var changes = self._generateChanges(relRecord, options); if (!_.isEmpty(changes)) { var command = x2ManyCommands.update(relRecord.res_id, changes); commands[fieldName].push(command); @@ -2002,7 +2016,7 @@ var BasicModel = AbstractModel.extend({ if (_.contains(keptIds, relIds[i])) { // this is an id that already existed relRecord = _.findWhere(relData, {res_id: relIds[i]}); - changes = this._generateChanges(relRecord); + changes = this._generateChanges(relRecord, options); if (!_.isEmpty(changes)) { command = x2ManyCommands.update(relRecord.res_id, changes); didChange = true; @@ -2013,11 +2027,11 @@ var BasicModel = AbstractModel.extend({ } else if (_.contains(addedIds, relIds[i])) { // this is a new id relRecord = _.findWhere(relData, {res_id: relIds[i]}); - changes = this._generateChanges(relRecord); + changes = this._generateChanges(relRecord, options); commands[fieldName].push(x2ManyCommands.create(changes)); } } - if (changesOnly && !didChange && addedIds.length === 0 && removedIds.length === 0) { + if (options.changesOnly && !didChange && addedIds.length === 0 && removedIds.length === 0) { // in this situation, we have no changed ids, no added // ids and no removed ids, so we can safely ignore the // last changes -- cgit v1.2.3