[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).

4 files changed, 277 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);
         }
     },
     /**
diff --git a/addons/web/static/tests/views/basic_model_tests.js b/addons/web/static/tests/views/basic_model_tests.js
index 318b715a504..b2a00f1ca09 100644
--- a/addons/web/static/tests/views/basic_model_tests.js
+++ b/addons/web/static/tests/views/basic_model_tests.js
@@ -2086,4 +2086,38 @@ QUnit.module('Views', {
         model.destroy();
     });
 
+    QUnit.test('only x2many lists (static) should be sorted client-side', function (assert) {
+        assert.expect(1);
+
+        this.params.modelName = 'partner_type';
+        this.params.res_id = undefined;
+        this.params.orderedBy = [{name: 'display_name', asc: true}];
+
+        var model = createModel({
+            Model: BasicModel,
+            data: this.data,
+            mockRPC: function (route) {
+                if (route === '/web/dataset/search_read') {
+                    // simulate randomn sort form the server
+                    return $.when({
+                        length: 3,
+                        records: [
+                            {id: 12, display_name: "gold", date: "2017-01-25"},
+                            {id: 15, display_name: "bronze"},
+                            {id: 14, display_name: "silver"},
+                        ],
+                    });
+                }
+                return this._super.apply(this, arguments);
+            },
+        });
+
+        model.load(this.params).then(function (resultID) {
+            var list = model.get(resultID);
+            assert.deepEqual(_.map(list.data, 'res_id'), [12, 15, 14],
+                "should have kept the order from the server");
+        });
+        model.destroy();
+    });
+
 });});
diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js
index 2ac373dbcd0..ce246ef17d6 100644
--- a/addons/web/static/tests/views/list_tests.js
+++ b/addons/web/static/tests/views/list_tests.js
@@ -773,6 +773,153 @@ QUnit.module('Views', {
         list.destroy();
     });
 
+    QUnit.test('use default_order on editable tree: sort on save', function (assert) {
+        assert.expect(8);
+
+        this.data.foo.records[0].o2m = [1, 3];
+
+        var form = createView({
+            View: FormView,
+            model: 'foo',
+            data: this.data,
+            arch: '<form>' +
+                    '<sheet>' +
+                        '<field name="o2m">' +
+                            '<tree editable="bottom" default_order="display_name">' +
+                                '<field name="display_name"/>' +
+                            '</tree>' +
+                        '</field>' +
+                    '</sheet>' +
+                '</form>',
+            res_id: 1,
+        });
+
+        form.$buttons.find('.o_form_button_edit').click();
+        assert.ok(form.$('tbody tr:first td:contains(Value 1)').length,
+            "Value 1 should be first");
+        assert.ok(form.$('tbody tr:eq(1) td:contains(Value 3)').length,
+            "Value 3 should be second");
+
+        var $o2m = form.$('.o_field_widget[name=o2m]');
+        form.$('.o_field_x2many_list_row_add a').click();
+        $o2m.find('.o_field_widget').val("Value 2").trigger('input');
+        assert.ok(form.$('tbody tr:first td:contains(Value 1)').length,
+            "Value 1 should be first");
+        assert.ok(form.$('tbody tr:eq(1) td:contains(Value 3)').length,
+            "Value 3 should be second");
+        assert.ok(form.$('tbody tr:eq(2) td input').val(),
+            "Value 2 should be third (shouldn't be sorted)");
+
+        form.$buttons.find('.o_form_button_save').click();
+        assert.ok(form.$('tbody tr:first td:contains(Value 1)').length,
+            "Value 1 should be first");
+        assert.ok(form.$('tbody tr:eq(1) td:contains(Value 2)').length,
+            "Value 2 should be second (should be sorted after saving)");
+        assert.ok(form.$('tbody tr:eq(2) td:contains(Value 3)').length,
+            "Value 3 should be third");
+
+        form.destroy();
+    });
+
+    QUnit.test('use default_order on editable tree: sort on demand', function (assert) {
+        assert.expect(8);
+
+        this.data.foo.records[0].o2m = [1, 3];
+        this.data.bar.fields = {name: {string: "Name", type: "char", sortable: true}};
+        this.data.bar.records[0].name = "Value 1";
+        this.data.bar.records[2].name = "Value 3";
+
+        var form = createView({
+            View: FormView,
+            model: 'foo',
+            data: this.data,
+            arch: '<form>' +
+                    '<sheet>' +
+                        '<field name="o2m">' +
+                            '<tree editable="bottom" default_order="name">' +
+                                '<field name="name"/>' +
+                            '</tree>' +
+                        '</field>' +
+                    '</sheet>' +
+                '</form>',
+            res_id: 1,
+        });
+
+        form.$buttons.find('.o_form_button_edit').click();
+        assert.ok(form.$('tbody tr:first td:contains(Value 1)').length,
+            "Value 1 should be first");
+        assert.ok(form.$('tbody tr:eq(1) td:contains(Value 3)').length,
+            "Value 3 should be second");
+
+        var $o2m = form.$('.o_field_widget[name=o2m]');
+        form.$('.o_field_x2many_list_row_add a').click();
+        $o2m.find('.o_field_widget').val("Value 2").trigger('input');
+        assert.ok(form.$('tbody tr:first td:contains(Value 1)').length,
+            "Value 1 should be first");
+        assert.ok(form.$('tbody tr:eq(1) td:contains(Value 3)').length,
+            "Value 3 should be second");
+        assert.ok(form.$('tbody tr:eq(2) td input').val(),
+            "Value 2 should be third (shouldn't be sorted)");
+
+        $o2m.find('.o_column_sortable').click();
+        assert.ok(form.$('tbody tr:first td:contains(Value 3)').length,
+            "Value 3 should be first");
+        assert.ok(form.$('tbody tr:eq(1) td:contains(Value 2)').length,
+            "Value 2 should be second (should be sorted after saving)");
+        assert.ok(form.$('tbody tr:eq(2) td:contains(Value 1)').length,
+            "Value 1 should be third");
+
+        form.destroy();
+    });
+
+    QUnit.test('use default_order on editable tree: sort on demand in page', function (assert) {
+        assert.expect(4);
+
+        this.data.bar.fields = {name: {string: "Name", type: "char", sortable: true}};
+
+        var ids = [];
+        for (var i=0; i<45; i++) {
+            var id = 4 + i;
+            ids.push(id);
+            this.data.bar.records.push({
+                id: id,
+                name: "Value " + id,
+            });
+        }
+        this.data.foo.records[0].o2m = ids;
+
+        var form = createView({
+            View: FormView,
+            model: 'foo',
+            data: this.data,
+            arch: '<form>' +
+                    '<sheet>' +
+                        '<field name="o2m">' +
+                            '<tree editable="bottom" default_order="name">' +
+                                '<field name="name"/>' +
+                            '</tree>' +
+                        '</field>' +
+                    '</sheet>' +
+                '</form>',
+            res_id: 1,
+        });
+
+        // Change page
+        form.$('.o_pager_next').click();
+        assert.ok(form.$('tbody tr:first td:contains(Value 44)').length,
+            "record 44 should be first");
+        assert.ok(form.$('tbody tr:eq(4) td:contains(Value 48)').length,
+            "record 48 should be last");
+
+        form.$('.o_column_sortable').click();
+        assert.ok(form.$('tbody tr:first td:contains(Value 48)').length,
+            "record 48 should be first");
+        assert.ok(form.$('tbody tr:eq(4) td:contains(Value 44)').length,
+            "record 44 should be first");
+
+        form.destroy();
+    });
+
     QUnit.test('can display button in edit mode', function (assert) {
         assert.expect(1);