From a4fa2a33f985b00914b40fca5781472319962c87 Mon Sep 17 00:00:00 2001
From: XavierDo <xdo@odoo.com>
Date: Tue, 25 Sep 2018 16:36:13 +0200
Subject: [PATCH] [FIX] web: order read_group by aggregable fields only in list
 view

The read_group method will ignore and display a warning when giving an
order on a non-aggregable field. Therefore, when accessing a list of
record from a list view group by some field and ordered on another one,
a warning will appear in logs (making the nightly clikall test yellow)

The solution here is to only order by current group by field and on
aggregable fields. We want to keep the user order to know the ASC/DESC
information.
---
 .../static/src/js/views/basic/basic_model.js  |  5 ++-
 addons/web/static/tests/views/list_tests.js   | 44 ++++++++++++++++++-
 2 files changed, 46 insertions(+), 3 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 695e8147c7f..94d0ea78c0e 100644
--- a/addons/web/static/src/js/views/basic/basic_model.js
+++ b/addons/web/static/src/js/views/basic/basic_model.js
@@ -4006,6 +4006,9 @@ var BasicModel = AbstractModel.extend({
         var groupByField = list.groupedBy[0];
         var rawGroupBy = groupByField.split(':')[0];
         var fields = _.uniq(list.getFieldNames().concat(rawGroupBy));
+        var orderedBy = _.filter(list.orderedBy, function(order){
+            return order.name === rawGroupBy || list.fields[order.name].group_operator !== undefined;
+        });
         return this._rpc({
                 model: list.model,
                 method: 'read_group',
@@ -4013,7 +4016,7 @@ var BasicModel = AbstractModel.extend({
                 domain: list.domain,
                 context: list.context,
                 groupBy: list.groupedBy,
-                orderBy: list.orderedBy,
+                orderBy: orderedBy,
                 lazy: true,
             })
             .then(function (groups) {
diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js
index cb1007c26c8..63e7013c824 100644
--- a/addons/web/static/tests/views/list_tests.js
+++ b/addons/web/static/tests/views/list_tests.js
@@ -23,7 +23,7 @@ QUnit.module('Views', {
                     foo: {string: "Foo", type: "char"},
                     bar: {string: "Bar", type: "boolean"},
                     date: {string: "Some Date", type: "date"},
-                    int_field: {string: "int_field", type: "integer", sortable: true},
+                    int_field: {string: "int_field", type: "integer", sortable: true, group_operator: "sum"},
                     qux: {string: "my float", type: "float"},
                     m2o: {string: "M2O field", type: "many2one", relation: "bar"},
                     o2m: {string: "O2M field", type: "one2many", relation: "bar"},
@@ -868,7 +868,6 @@ QUnit.module('Views', {
 
     QUnit.test('groups can be sorted on aggregates', function (assert) {
         assert.expect(10);
-
         var list = createView({
             View: ListView,
             model: 'foo',
@@ -902,6 +901,47 @@ QUnit.module('Views', {
         list.destroy();
     });
 
+    QUnit.test('groups cannot be sorted on non-aggregable fields', function (assert) {
+        assert.expect(6);
+        this.data.foo.fields.sort_field = {string: "sortable_field", type: "sting", sortable: true, default: "value"};
+        _.each(this.data.records, function(elem) {
+            elem.sort_field = "value" + elem.id;
+        });
+        this.data.foo.fields.foo.sortable= true;
+        var list = createView({
+            View: ListView,
+            model: 'foo',
+            data: this.data,
+            groupBy: ['foo'],
+            arch: '<tree editable="bottom"><field name="foo" /><field name="int_field"/><field name="sort_field"/></tree>',
+            mockRPC: function (route, args) {
+                if (args.method === 'read_group') {
+                    assert.step(args.kwargs.orderby || 'default order');
+                }
+                return this._super.apply(this, arguments);
+            },
+        });
+        //we cannot sort by sort_field since it doesn't have a group_operator
+        list.$('.o_column_sortable:eq(2)').click(); // sort (sort_field ASC(invalid) -> default order)
+        //we can sort by int_field since it has a group_operator
+        list.$('.o_column_sortable:eq(1)').click(); // sort (int_field ASC)
+        //we keep previous order
+        list.$('.o_column_sortable:eq(2)').click(); // sort (sort_field ASC(invalid) -> int_field ASC)
+        //we can sort on foo since we are groupped by foo + previous order
+        list.$('.o_column_sortable:eq(0)').click(); // sort (foo ASC, int_field ASK")
+
+        assert.verifySteps([
+            'default order',
+            'default order',
+            'int_field ASC',
+            'int_field ASC',
+            'foo ASC, int_field ASC'
+        ]);
+
+        list.destroy();
+    });
+
+
     QUnit.test('properly apply onchange in simple case', function (assert) {
         assert.expect(2);