From bb10cd50c28f224146634365ac341de6e09644d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul=20=28ACSONE=29?= Date: Sun, 31 Dec 2017 15:43:49 +0100 Subject: [FIX] web: issue with id field in x2m form When the 'id' field is present in a x2m form, the x2m actions generated on create of the parent object are incorrect. --- addons/web/static/src/js/views/basic/basic_model.js | 6 +++--- 1 file changed, 3 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 1562bffdc42..2baafc52514 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -95,11 +95,13 @@ var x2ManyCommands = { // (0, _, {values}) CREATE: 0, create: function (values) { + delete values.id; return [x2ManyCommands.CREATE, false, values]; }, // (1, id, {values}) UPDATE: 1, update: function (id, values) { + delete values.id; return [x2ManyCommands.UPDATE, id, values]; }, // (2, id[, _]) @@ -2625,7 +2627,6 @@ 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); } @@ -2645,7 +2646,6 @@ var BasicModel = AbstractModel.extend({ relRecord = _.findWhere(relRecordUpdated, {res_id: list.res_ids[i]}); changes = relRecord ? this._generateChanges(relRecord, options) : {}; if (!_.isEmpty(changes)) { - delete changes.id; command = x2ManyCommands.update(relRecord.res_id, changes); didChange = true; } else { @@ -2656,7 +2656,7 @@ var BasicModel = AbstractModel.extend({ // this is a new id (maybe existing in DB, but new in JS) relRecord = _.findWhere(relRecordAdded, {res_id: list.res_ids[i]}); changes = this._generateChanges(relRecord, options); - if ('id' in changes) { + if (changes.id) { // the subrecord already exists in db delete changes.id; if (this.isNew(record.id)) { -- cgit v1.2.3 From b7ee1aa4b1ea906195c234020cb708a1da9de6a7 Mon Sep 17 00:00:00 2001 From: Christophe Matthieu Date: Wed, 6 Dec 2017 10:09:02 +0100 Subject: [FIX] web: sort and onchange in tabbed editable x2m Two attempts of dealing with sorting (and resequencing) in tabbed (multi-pages) x2many editable lists (see odoo/odoo@c2563db and odoo/odoo@983c7ae) have been previously done, using a static attribute (`keepChangesUnsorted`) on the datapoint. This solution sadly only covers both cases separately, not if they are combined. Morever, onchanges were not correctly managed in neither of these two cases. A new approach is used here, by "freezing" and "unfreezing" the datapoint when needed, which is more flexible. Some modifications have been introduced: - when ordering a x2many, all records are now considered (and not only the records on the current page) ; an additional read is thus done on the sorted field for all records in the x2many relation - `setSort` is now asynchronous as a `read` might be needed --- .../web/static/src/js/views/basic/basic_model.js | 395 ++++++++++++++------- 1 file changed, 259 insertions(+), 136 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 39f1be0d107..8a9fd496b39 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -37,6 +37,7 @@ odoo.define('web.BasicModel', function (require) { * offset: {integer}, * openGroupByDefault: {boolean}, * orderedBy: {Object[]}, + * orderedResIDs: {integer[]}, * parentID: {string}, * rawContext: {Object}, * relationField: {string}, @@ -410,29 +411,11 @@ var BasicModel = AbstractModel.extend({ return record; } - // here, type === 'list' - // apply potential changes (only for x2many lists): - // for list having the flag _keepChangesUnsorted set to true (typically, - // editable lists), we sort before apply changes by default (to keep the - // changes unsorted, i.e. the edited line at the same place). However, if - // the user forces a sort, all changes done so far are sorted (this is the - // purpose of the _keepChangesUnsortedCount key), and changes done later - // on won't be sorted. - var count; - if (element._keepChangesUnsorted) { - // only apply a subset (by default 0) of changes before sorting - count = element._keepChangesUnsortedCount || 0; - element = this._applyX2ManyOperations(element, {to: count, position: 'bottom'}); - } else { - // apply all changes before sorting - element = this._applyX2ManyOperations(element, {position: 'bottom'}); - } + // apply potential changes (only for x2many lists) + element = this._applyX2ManyOperations(element); this._sortList(element); - if (element._keepChangesUnsorted) { - // apply the remaining changes after the sort - element = this._applyX2ManyOperations(element, {from: count, position: 'bottom'}); - } - if (element._changes) { + + if (!element.orderedResIDs && element._changes) { _.each(element._changes, function (change) { if (change.operation === 'ADD' && change.isNew) { element.data = _.without(element.data, change.id); @@ -444,6 +427,7 @@ var BasicModel = AbstractModel.extend({ } }); } + var list = { aggregateValues: _.extend({}, element.aggregateValues), context: _.extend({}, element.context), @@ -921,6 +905,8 @@ var BasicModel = AbstractModel.extend({ // Erase changes as they have been applied record._changes = {}; + self.unfreezeOrder(record.id); + // Update the data directly or reload them if (shouldReload) { self._fetchRecord(record).then(function (record) { @@ -952,6 +938,20 @@ var BasicModel = AbstractModel.extend({ record.fields = _.extend({}, record.fields, viewInfo.fields); record.fieldsInfo = _.extend({}, record.fieldsInfo, viewInfo.fieldsInfo); }, + /** + * For list resources, this freezes the current records order. + * + * @param {string} listID a valid element ID of type list + */ + freezeOrder: function (listID) { + var list = this.localData[listID]; + if (list.type === 'record') { + return; + } + list = this._applyX2ManyOperations(list); + this._sortList(list); + this.localData[listID].orderedResIDs = list.res_ids; + }, /** * Manually sets a resource as dirty. This is used to notify that a field * has been modified, but with an invalid value. In that case, the value is @@ -968,7 +968,7 @@ var BasicModel = AbstractModel.extend({ * * @param {string} list_id id for the list resource * @param {string} fieldName valid field name - * @returns {this} so we can chain call this method + * @returns {Deferred} */ setSort: function (list_id, fieldName) { var list = this.localData[list_id]; @@ -979,21 +979,26 @@ var BasicModel = AbstractModel.extend({ delete change.isNew; }); } - // the user manually selected a sort order, so we sort the list with all - // the changes he made so far, even if it is editable, however, changes - // he'll made after won't be sorted if the list is editable - list._keepChangesUnsortedCount = list._changes ? list._changes.length : 0; if (list.orderedBy.length === 0) { list.orderedBy.push({name: fieldName, asc: true}); } else if (list.orderedBy[0].name === fieldName){ - list.orderedBy[0].asc = !list.orderedBy[0].asc; + if (!list.orderedResIDs) { + list.orderedBy[0].asc = !list.orderedBy[0].asc; + } } else { var orderedBy = _.reject(list.orderedBy, function (o) { return o.name === fieldName; }); list.orderedBy = [{name: fieldName, asc: true}].concat(orderedBy); } - return this; + + list.orderedResIDs = null; + if (list.static) { + // sorting might require to fetch the field for records where the + // sort field is still unknown (i.e. on other pages for example) + return this._fetchUngroupedList(list); + } + return $.when(); }, /** * Toggle the active value of given records (to archive/unarchive them) @@ -1048,6 +1053,34 @@ var BasicModel = AbstractModel.extend({ }); } }, + /** + * For a list datapoint, unfreezes the current records order and sorts it. + * For a record datapoint, unfreezes the x2many list datapoints. + * + * @param {string} elementID a valid element ID + */ + unfreezeOrder: function (elementID) { + var list = this.localData[elementID]; + if (list.type === 'record') { + var data = _.extend({}, list.data, list._changes); + for (var fieldName in data) { + var field = list.fields[fieldName]; + if (!field || !data[fieldName]) { + continue; + } + if (field.type === 'one2many' || field.type === 'many2many') { + var recordlist = this.localData[data[fieldName]]; + recordlist.orderedResIDs = null; + for (var index in recordlist.data) { + this.unfreezeOrder(recordlist.data[index]); + } + } + } + return; + } + list.orderedResIDs = null; + this._sortList(list); + }, //-------------------------------------------------------------------------- // Private @@ -1060,7 +1093,7 @@ var BasicModel = AbstractModel.extend({ * not by list or kanban controllers. * * @private - * @param {string} listID a valid handle for a list object + * @param {Object} list a valid list object * @param {Object} [options] * @param {string} [options.position=top] if the new record should be added * on top or on bottom of the list @@ -1080,6 +1113,12 @@ var BasicModel = AbstractModel.extend({ list._changes.push({operation: 'ADD', id: id, position: position, isNew: true}); var record = self.localData[id]; list._cache[record.res_id] = id; + if (list.orderedResIDs) { + var index = list.offset + (position !== 'top' ? list.limit : 0); + list.orderedResIDs.splice(index, 0, record.res_id); + // list could be a copy of the original one + self.localData[list.id].orderedResIDs = list.orderedResIDs; + } return id; }); }, @@ -1341,7 +1380,6 @@ var BasicModel = AbstractModel.extend({ // TODO: before registering the changes, verify that the x2many // value has changed record._changes[name] = list.id; - var shouldLoad = false; list._changes = list._changes || []; // save it in case of a [5] which will remove the _changes @@ -1361,11 +1399,7 @@ var BasicModel = AbstractModel.extend({ } if (command[0] === 1 && command[1]) { // updating an existing record - recID = _.find(list.data, function (childID) { - var child = self.localData[childID]; - return child.res_id === command[1]; - }); - rec = self.localData[recID]; + rec = self.localData[list._cache[command[1]]]; } if (!rec) { var params = { @@ -1391,18 +1425,25 @@ var BasicModel = AbstractModel.extend({ defs.push(self._applyOnChange(command[2], rec)); } else if (command[0] === 4) { // LINK TO - // the dataPoint id will be set when the record will be fetched (for now, - // this dataPoint doesn't exist yet) - list._changes.push({operation: 'ADD', id: null, resID: command[1]}); - shouldLoad = true; + rec = self.localData[list._cache[command[1]]]; + if (rec) { + // modifications done on a record are discarded if + // the onchange uses a LINK TO + self.discardChanges(rec.id); + } + // the dataPoint id will be set when the record will be + // fetched (for now, this dataPoint may not exist yet) + list._changes.push({ + operation: 'ADD', + id: rec ? rec.id : null, + resID: command[1], + }); } else if (command[0] === 5) { // DELETE ALL list._changes = [{operation: 'REMOVE_ALL'}]; } }); - if (shouldLoad) { - defs.push(self._readUngroupedList(list)); - } + defs.push(self._readUngroupedList(list)); } else { var newValue = self._parseServerValue(field, val); if (newValue !== oldValue) { @@ -1540,12 +1581,6 @@ var BasicModel = AbstractModel.extend({ if (idInCommands && change.operation === 'ADD') { idsToRemove = _.without(idsToRemove, change.id); } - // decrement _keepChangesUnsortedCount if we filter out an - // operation whose index is smaller than the count (as there - // will be one operation less to apply before sorting) - if (idInCommands && index < list._keepChangesUnsortedCount) { - list._keepChangesUnsortedCount--; - } return idInCommands; }); _.each(idsToRemove, function (id) { @@ -1653,18 +1688,7 @@ var BasicModel = AbstractModel.extend({ break; } }); - var idsInRange; - if (list.limit) { - idsInRange = list.res_ids.slice(list.offset, list.offset + list.limit); - } else { - idsInRange = list.res_ids; - } - list.data = []; - _.each(idsInRange, function (id) { - if (list._cache[id]) { - list.data.push(list._cache[id]); - } - }); + this._setDataInRange(list); return list; }, /** @@ -2344,7 +2368,6 @@ var BasicModel = AbstractModel.extend({ rawContext: rawContext, relationField: field.relation_field, viewType: view ? view.type : fieldInfo.viewType, - _keepChangesUnsorted: fieldInfo.keepChangesUnsorted, }); record.data[fieldName] = list.id; if (!fieldInfo.__no_fetch) { @@ -2624,6 +2647,7 @@ var BasicModel = AbstractModel.extend({ } }); list = this._applyX2ManyOperations(list); + this._sortList(list); if (type === 'many2many' || list._forceM2MLink) { var relRecordCreated = _.filter(relRecordAdded, function (rec) { return typeof rec.res_id === 'string'; @@ -3065,7 +3089,7 @@ var BasicModel = AbstractModel.extend({ offset: params.offset || (type === 'record' ? _.indexOf(res_ids, res_id) : 0), openGroupByDefault: params.openGroupByDefault, orderedBy: params.orderedBy || [], - _keepChangesUnsorted: params._keepChangesUnsorted, + orderedResIDs: params.orderedResIDs, parentID: params.parentID, rawContext: params.rawContext, ref: params.ref || res_id, @@ -3444,6 +3468,81 @@ var BasicModel = AbstractModel.extend({ return record; }); }, + /** + * Reads data from server for all missing fields. + * + * @private + * @param {Object} list a valid resource object + * @param {interger[]} resIDs + * @param {string[]} fieldNames to check and read if missing + * @returns {Deferred} + */ + _readMissingFields: function (list, resIDs, fieldNames) { + var self = this; + + var missingIDs = []; + for (var i = 0, len = resIDs.length; i < len; i++) { + var resId = resIDs[i]; + var dataPointID = list._cache[resId]; + if (!dataPointID) { + missingIDs.push(resId); + continue; + } + var record = self.localData[dataPointID]; + var data = _.extend({}, record.data, record._changes); + if (_.difference(fieldNames, _.keys(data)).length) { + missingIDs.push(resId); + } + } + + var def; + if (missingIDs.length && fieldNames.length) { + def = self._rpc({ + model: list.model, + method: 'read', + args: [missingIDs, fieldNames], + context: list.getContext(), + }); + } else { + def = $.when(_.map(missingIDs, function (id) { + return {id:id}; + })); + } + return def.then(function (records) { + _.each(resIDs, function (id) { + var dataPoint; + var data = _.findWhere(records, {id: id}); + if (id in list._cache) { + dataPoint = self.localData[list._cache[id]]; + if (data) { + self._parseServerData(fieldNames, dataPoint, data); + _.extend(dataPoint.data, data); + } + } else { + dataPoint = self._makeDataPoint({ + context: list.context, + data: data, + fieldsInfo: list.fieldsInfo, + fields: list.fields, + modelName: list.model, + parentID: list.id, + viewType: list.viewType, + }); + self._parseServerData(fieldNames, dataPoint, dataPoint.data); + + // add many2one records + list._cache[id] = dataPoint.id; + } + // set the dataPoint id in potential 'ADD' operation adding the current record + _.each(list._changes, function (change) { + if (change.operation === 'ADD' && !change.id && change.resID === id) { + change.id = dataPoint.id; + } + }); + }); + return list; + }); + }, /** * For a grouped list resource, this method fetches all group data by * performing a /read_group. It also tries to read open subgroups if they @@ -3503,6 +3602,7 @@ var BasicModel = AbstractModel.extend({ aggregateValues: aggregateValues, groupedBy: list.groupedBy.slice(1), orderedBy: list.orderedBy, + orderedResIDs: list.orderedResIDs, limit: list.limit, openGroupByDefault: list.openGroupByDefault, parentID: list.id, @@ -3555,70 +3655,44 @@ var BasicModel = AbstractModel.extend({ */ _readUngroupedList: function (list) { var self = this; - var def; - var ids = []; - var missingIds = []; + var def = $.when(); + // generate the current count and res_ids list by applying the changes - var listWithChanges = this._applyX2ManyOperations(list); - var currentCount = listWithChanges.count; - var currentResIDs = listWithChanges.res_ids; - var upper_bound = list.limit ? Math.min(list.offset + list.limit, currentCount) : currentCount; - var fieldNames = list.getFieldNames(); - for (var i = list.offset; i < upper_bound; i++) { - var id = currentResIDs[i]; - ids.push(id); - if (!list._cache[id]) { - missingIds.push(id); - } - } - if (missingIds.length) { - if (fieldNames.length) { - def = this._rpc({ - model: list.model, - method: 'read', - args: [missingIds, fieldNames], - context: list.getContext(), + list = this._applyX2ManyOperations(list); + + // for multi-pages list datapoints, we might need to read the + // order field first to apply the order on all pages + if (list.res_ids.length > list.limit) { + if (!list.orderedResIDs && list.orderedBy.length) { + var fieldNames = _.pluck(list.orderedBy, 'name'); + def = this._readMissingFields(list, _.filter(list.res_ids, _.isNumber), fieldNames); + def.then(function () { + self._sortList(list); }); - } else { - def = $.when(_.map(missingIds, function (id) { - return {id:id}; - })); } - } else { - def = $.when(); } - return def.then(function (records) { - list.data = []; - _.each(ids, function (id) { - var dataPoint; - if (id in list._cache) { - dataPoint = self.localData[list._cache[id]]; - } else { - dataPoint = self._makeDataPoint({ - context: list.context, - data: _.findWhere(records, {id: id}), - fieldsInfo: list.fieldsInfo, - fields: list.fields, - modelName: list.model, - parentID: list.id, - viewType: list.viewType, - }); - - // add many2one records - self._parseServerData(fieldNames, dataPoint, dataPoint.data); - list._cache[id] = dataPoint.id; + return def.then(function () { + var resIDs = []; + // generate the current count and res_ids list by applying the changes + var currentCount = list.count; + var currentResIDs = list.res_ids; + var upperBound = list.limit ? Math.min(list.offset + list.limit, currentCount) : currentCount; + var fieldNames = list.getFieldNames(); + for (var i = list.offset; i < upperBound; i++) { + var resId = currentResIDs[i]; + if (_.isNumber(resId)) { + resIDs.push(resId); } - // set the dataPoint id in potential 'ADD' operation adding the current record - _.each(list._changes, function (change) { - if (change.operation === 'ADD' && !change.id && change.resID === id) { - change.id = dataPoint.id; - } - }); - if (_.contains(list.res_ids, id)) { - list.data.push(dataPoint.id); + } + return self._readMissingFields(list, resIDs, fieldNames).then(function () { + if (list.res_ids.length <= list.limit) { + self._sortList(list); + } else { + // sortList has already been applied after first the read + self._setDataInRange(list); } + return list; }); - return list; }); }, /** @@ -3689,6 +3763,38 @@ var BasicModel = AbstractModel.extend({ return list; }); }, + /** + * Set data in range, i.e. according to the list offset and limit. + * + * @param {Object} list + */ + _setDataInRange: function (list) { + var idsInRange; + if (list.limit) { + idsInRange = list.res_ids.slice(list.offset, list.offset + list.limit); + } else { + idsInRange = list.res_ids; + } + list.data = []; + _.each(idsInRange, function (id) { + if (list._cache[id]) { + list.data.push(list._cache[id]); + } + }); + + // display newly created record in addition to the displayed records + if (list.limit) { + for (var i = list.offset + list.limit; i < list.res_ids.length; i++) { + var id = list.res_ids[i]; + var dataPointID = list._cache[id]; + if (_.findWhere(list._changes, {isNew: true, id: dataPointID})) { + list.data.push(dataPointID); + } else { + break; + } + } + } + }, /** * Change the offset of a record. Note that this does not reload the data. * The offset is used to load a different record in a list of record (for @@ -3720,14 +3826,28 @@ var BasicModel = AbstractModel.extend({ // only sort x2many lists return; } + var self = this; - if (list.orderedBy.length) { - var self = this; - + if (list.orderedResIDs) { + var orderedResIDs = {}; + for (var k = 0; k < list.orderedResIDs.length; k++) { + orderedResIDs[list.orderedResIDs[k]] = k; + } + list.res_ids.sort(function compareResIdIndexes (resId1, resId2) { + if (!(resId1 in orderedResIDs) && !(resId2 in orderedResIDs)) { + return 0; + } + if (!(resId1 in orderedResIDs)) { + return Infinity; + } + if (!(resId2 in orderedResIDs)) { + return -Infinity; + } + return orderedResIDs[resId1] - orderedResIDs[resId2]; + }); + } else if (list.orderedBy.length) { // sort records according to ordered_by[0] - var data = list.data; - var res_ids = list.res_ids; - var compareRecords = function (record1ID, record2ID, level) { + var compareRecords = function (resId1, resId2, level) { if(!level) { level = 0; } @@ -3735,6 +3855,17 @@ var BasicModel = AbstractModel.extend({ return 0; } var order = list.orderedBy[level]; + var record1ID = list._cache[resId1]; + var record2ID = list._cache[resId2]; + if (!record1ID && !record2ID) { + return 0; + } + if (!record1ID) { + return Infinity; + } + if (!record2ID) { + return -Infinity; + } var r1 = self.localData[record1ID]; var r2 = self.localData[record2ID]; var data1 = _.extend({}, r1.data, r1._changes); @@ -3747,17 +3878,9 @@ var BasicModel = AbstractModel.extend({ } return compareRecords(record1ID, record2ID, level + 1); }; - data.sort(compareRecords); - - // sort res_ids accordingly (only the current range of ids, the one - // mapping the data, needs to be sorted) - var preRangeIDs = res_ids.slice(0, list.offset); // resIDs before the range - var postRangeIDs = res_ids.slice(list.offset + list.limit); // resIDs after the range - var rangeIDs = _.map(data, function (dataPointID) { - return self.localData[dataPointID].res_id; - }); - list.res_ids = preRangeIDs.concat(rangeIDs).concat(postRangeIDs); + list.res_ids.sort(compareRecords); } + this._setDataInRange(list); }, /** * Updates the res_ids of the parent of a given element of type list. -- cgit v1.2.3 From 73e2ba991c0e74654f3c2e2ae02485196d25fe1e Mon Sep 17 00:00:00 2001 From: Christophe Matthieu Date: Wed, 6 Dec 2017 13:51:42 +0100 Subject: [FIX] web: add a stableSort method for sorting x2m Issue: create 60 lines in ordered x2many with the same value (0) for the order field and make a change ; the displayed records will be switched. The problem appears because the sorting function is not stable and the records with the same values can change place (hence change page), which is quite disturbing for the user. --- addons/web/static/src/js/views/basic/basic_model.js | 5 +++-- 1 file changed, 3 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 8a9fd496b39..78e9216be62 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -89,6 +89,7 @@ var Context = require('web.Context'); var core = require('web.core'); var Domain = require('web.Domain'); var session = require('web.session'); +var utils = require('web.utils'); var _t = core._t; @@ -3833,7 +3834,7 @@ var BasicModel = AbstractModel.extend({ for (var k = 0; k < list.orderedResIDs.length; k++) { orderedResIDs[list.orderedResIDs[k]] = k; } - list.res_ids.sort(function compareResIdIndexes (resId1, resId2) { + utils.stableSort(list.res_ids, function compareResIdIndexes (resId1, resId2) { if (!(resId1 in orderedResIDs) && !(resId2 in orderedResIDs)) { return 0; } @@ -3878,7 +3879,7 @@ var BasicModel = AbstractModel.extend({ } return compareRecords(record1ID, record2ID, level + 1); }; - list.res_ids.sort(compareRecords); + utils.stableSort(list.res_ids, compareRecords); } this._setDataInRange(list); }, -- cgit v1.2.3