[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

1 files changed, 259 insertions, 136 deletions

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) {
@@ -953,6 +939,20 @@ var BasicModel = AbstractModel.extend({
         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
      * not sent to the basic model, but the record should still be flagged as
@@ -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,
@@ -3445,6 +3469,81 @@ var BasicModel = AbstractModel.extend({
         });
     },
     /**
+     * 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<Object>}
+     */
+    _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
      * were open before.
@@ -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;
         });
     },
     /**
@@ -3690,6 +3764,38 @@ var BasicModel = AbstractModel.extend({
         });
     },
     /**
+     * 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
      * example, a form view with a pager.  Clicking on next/previous actually
@@ -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.