[FIX] web: BasicModel: correctly sort editable lists

This rev. fixes several issues with the sorting of dataPoints of
type list in the BasicModel.

First, only 'static' lists (i.e. x2many lists) must be sorted
client-side, and results of search_read RPCs must remain unsorted
as they are already retrieved sorted from the server.

Second, editable and non-editable x2many lists don't behave the
same with respect to sorting (we reproduced in this commit the
behavior of saas-15). With editable lists, the edited rows are
unsorted, except if the user explicitely ask for a sort (by
clicking on a column header), until the form view is saved. With
non-editable lists (as for x2many kanban), the sort is performed
directly when a record is created or updated (as the edition is
done from a dialog).

2 files changed, 96 insertions, 34 deletions

diff --git a/addons/web/static/src/js/services/data_manager.js b/addons/web/static/src/js/services/data_manager.js
index 77857ef2d31..105d7f30d69 100644
--- a/addons/web/static/src/js/services/data_manager.js
+++ b/addons/web/static/src/js/services/data_manager.js
@@ -305,18 +305,6 @@ return core.Class.extend({
                 viewType = viewType === 'tree' ? 'list' : viewType;
                 innerFieldsView.type = viewType;
                 attrs.views[viewType] = self._processFieldsView(_.extend({}, innerFieldsView));
-
-                // default_order is like:
-                //   'name,id desc'
-                // but we need it like:
-                //   [{name: 'id', asc: false}, {name: 'name', asc: true}]
-                var defaultOrder = innerFieldsView.arch.attrs.default_order;
-                if (defaultOrder) {
-                    attrs.orderedBy = _.map(defaultOrder.split(','), function (order) {
-                        order = order.trim().split(' ');
-                        return {name: order[0], asc: order[1] !== 'desc'};
-                    });
-                }
             });
             delete field.views;
         }
@@ -346,6 +334,23 @@ return core.Class.extend({
                     }
                 }
                 attrs.mode = mode;
+                if (mode in attrs.views) {
+                    var view = attrs.views[mode];
+                    // process the default_order, which is like 'name,id desc'
+                    // but we need it like [{name: 'name', asc: true}, {name: 'id', asc: false}]
+                    var defaultOrder = view.arch.attrs.default_order;
+                    if (defaultOrder) {
+                        attrs.orderedBy = _.map(defaultOrder.split(','), function (order) {
+                            order = order.trim().split(' ');
+                            return {name: order[0], asc: order[1] !== 'desc'};
+                        });
+                    }
+                    // detect editables list has they behave differently with respect
+                    // to the sorting (changes are not sorted directly)
+                    if (mode === 'list' && view.arch.attrs.editable) {
+                         attrs.keepChangesUnsorted = true;
+                     }
+                }
             }
             if (attrs.Widget.prototype.fieldsToFetch) {
                 attrs.viewType = 'default';
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 4e3e5823223..9f6a3e80d13 100644
--- a/addons/web/static/src/js/views/basic/basic_model.js
+++ b/addons/web/static/src/js/views/basic/basic_model.js
@@ -337,7 +337,6 @@ var BasicModel = AbstractModel.extend({
         }
 
         if (element.type === 'record') {
-
             var data = _.extend({}, element.data, element._changes);
             var relDataPoint;
             for (var fieldName in data) {
@@ -411,16 +410,34 @@ var BasicModel = AbstractModel.extend({
         }
 
         // here, type === 'list'
-        // apply potential changes (only for x2many lists)
-        element = this._applyX2ManyOperations(element);
-        var listData = _.map(element.data, function (elemID) {
-            return self.get(elemID, options);
-        });
+        // 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});
+        } else {
+            // apply all changes before sorting
+            element = this._applyX2ManyOperations(element);
+        }
+        this._sortList(element);
+        if (element._keepChangesUnsorted) {
+            // apply the remaining changes after the sort
+            element = this._applyX2ManyOperations(element, {from: count});
+        }
         var list = {
             aggregateValues: _.extend({}, element.aggregateValues),
             context: _.extend({}, element.context),
             count: element.count,
-            data: listData,
+            data: _.map(element.data, function (elemID) {
+                return self.get(elemID, options);
+            }),
             domain: element.domain.slice(0),
             fields: element.fields,
             getContext: element.getContext,
@@ -434,7 +451,7 @@ var BasicModel = AbstractModel.extend({
             offset: element.offset,
             orderedBy: element.orderedBy,
             res_id: element.res_id,
-            res_ids: element.res_ids,
+            res_ids: element.res_ids.slice(0),
             type: 'list',
             value: element.value,
             viewType: element.viewType,
@@ -442,7 +459,6 @@ var BasicModel = AbstractModel.extend({
         if (element.fieldsInfo) {
             list.fieldsInfo = element.fieldsInfo;
         }
-        this._sortList(list);
         return list;
     },
     /**
@@ -877,7 +893,10 @@ var BasicModel = AbstractModel.extend({
         if (list.type === 'record') {
             return;
         }
-        list.offset = 0;
+        // 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){
@@ -1366,11 +1385,17 @@ var BasicModel = AbstractModel.extend({
                 // no 'ADD' operation for that dataPoint, as it would mean
                 // that the record wasn't in the relation yet
                 var idsToRemove = command.ids;
-                list._changes = _.reject(list._changes, function (change) {
+                list._changes = _.reject(list._changes, function (change, index) {
                     var idInCommands = _.contains(command.ids, change.id);
                     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) {
@@ -1423,19 +1448,26 @@ var BasicModel = AbstractModel.extend({
      *
      * @private
      * @param {Object} dataPoint of type list
+     * @param {Object} [range] the range of operations to apply
+     * @param {Object} [range.from=0] the index of the first operation to apply
+     * @param {Object} [range.to=length] the index of the last operation to apply
      * @returns {Object} element of type list in which the commands have been
      *   applied
      */
-    _applyX2ManyOperations: function (list) {
+    _applyX2ManyOperations: function (list, range) {
         if (!list.static) {
             // this function only applies on x2many lists
             return list;
         }
         var self = this;
         list = _.extend({}, list);
-        list.data = list.data.slice(0);
         list.res_ids = list.res_ids.slice(0);
-        _.each(list._changes, function (change) {
+        var changes = list._changes || [];
+        if (range) {
+            var to = range.to === 0 ? 0 : (range.to || changes.length);
+            changes = changes.slice(range.from || 0, to);
+        }
+        _.each(changes, function (change) {
             var relRecord;
             if (change.id) {
                 relRecord = self.localData[change.id];
@@ -2148,6 +2180,7 @@ 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) {
@@ -2834,6 +2867,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,
             parentID: params.parentID,
             rawContext: params.rawContext,
             relationField: params.relationField,
@@ -3367,7 +3401,9 @@ var BasicModel = AbstractModel.extend({
                         change.id = dataPoint.id;
                     }
                 });
-                list.data.push(dataPoint.id);
+                if (_.contains(list.res_ids, id)) {
+                    list.data.push(dataPoint.id);
+                }
             });
             return list;
         });
@@ -3458,20 +3494,32 @@ var BasicModel = AbstractModel.extend({
     },
     /**
      * Do a in-memory sort of a list resource data points. This method assumes
-     * that the list data has already been fetched, and that the changes have
-     * been applied. Its intended use is for static datasets, such as a one2many
-     * in a form view.
+     * that the list data has already been fetched, and that the changes that
+     * need to be sorted have already been applied. Its intended use is for
+     * static datasets, such as a one2many in a form view.
      *
-     * @param {Object} list list dataPoint on which changes have been applied
+     * @param {Object} list list dataPoint on which (some) changes might have
+     *   been applied; it is a copy of an internal dataPoint, not the result of
+     *   get
      */
     _sortList: function (list) {
+        if (!list.static) {
+            // only sort x2many lists
+            return;
+        }
+
         if (list.orderedBy.length) {
+            var self = this;
+
             // sort records according to ordered_by[0]
             var order = list.orderedBy[0];
             var data = list.data;
-            data.sort(function (r1, r2) {
-                var data1 = r1.data;
-                var data2 = r2.data;
+            var res_ids = list.res_ids;
+            data.sort(function (record1ID, record2ID) {
+                var r1 = self.localData[record1ID];
+                var r2 = self.localData[record2ID];
+                var data1 = _.extend({}, r1.data, r1._changes);
+                var data2 = _.extend({}, r2.data, r2._changes);
                 if (data1[order.name] < data2[order.name]) {
                     return order.asc ? -1 : 1;
                 }
@@ -3480,6 +3528,15 @@ var BasicModel = AbstractModel.extend({
                 }
                 return 0;
             });
+
+            // 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);
         }
     },
     /**