From d143d01287779ba381a7f51c5c8ffd4544a2b1fd Mon Sep 17 00:00:00 2001
From: Lucas Perais <lpe@odoo.com>
Date: Tue, 23 Apr 2024 08:00:37 +0200
Subject: [PATCH] [FIX] web: handle field sorting should include id

Since commit 7d2baaa0c726a7b0dd4fe7226862f960c9c59b73 ("Unity read"), we are able to
pass a full specification to subfields in a view and retrive records directly according to that
specification. This could include "order".

In the case of a list view that has a `widget="handle"`, this order is automatically set to "[handle_field] ASC".

Before the unity read feature, it did not cause problems for one2manys because the ids of records were retrieved in python
using the  "natural order" of the model (the `model._order` slot), which usually had the right parameters.
(see `sale.order.line` for example). When fetching the ids of the one2many, those were already sorted in natural order.

In unity read, the natural order is overriden by the specification and became only "[handle_field] ASC". This was insufficient
as more often than not, sequences on model are set up with a default. So eventually, all records couls have the same sequence.
The sorting in SQL becomes undeterminate.

After this commit, we had the sorting key "id ASC" to avoid any unwanted results.

opw-3790378

see discord https://discord.com/channels/678381219515465750/687338039717920792/1231977078564585555 for a detailed discussion.

closes odoo/odoo#162933

Signed-off-by: Lucas Perais (lpe) <lpe@odoo.com>
---
 .../static/src/views/list/list_arch_parser.js |  3 ++-
 .../views/fields/one2many_field_tests.js      | 25 +++++++++++++++++++
 .../web/static/tests/views/list_view_tests.js |  6 ++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/addons/web/static/src/views/list/list_arch_parser.js b/addons/web/static/src/views/list/list_arch_parser.js
index 3a3662437d8..2d361f985d5 100644
--- a/addons/web/static/src/views/list/list_arch_parser.js
+++ b/addons/web/static/src/views/list/list_arch_parser.js
@@ -226,7 +226,8 @@ export class ListArchParser {
         });
 
         if (!treeAttr.defaultOrder.length && handleField) {
-            treeAttr.defaultOrder = stringToOrderBy(handleField);
+            const handleFieldSort = `${handleField}, id`;
+            treeAttr.defaultOrder = stringToOrderBy(handleFieldSort);
         }
 
         return {
diff --git a/addons/web/static/tests/views/fields/one2many_field_tests.js b/addons/web/static/tests/views/fields/one2many_field_tests.js
index 90eb8d5849e..ecb2986021c 100644
--- a/addons/web/static/tests/views/fields/one2many_field_tests.js
+++ b/addons/web/static/tests/views/fields/one2many_field_tests.js
@@ -2678,6 +2678,31 @@ QUnit.module("Fields", (hooks) => {
         ]);
     });
 
+    QUnit.test("one2many list order with handle widget", async (assert) => {
+        await makeView({
+            type: "form",
+            resModel: "partner",
+            serverData,
+            arch: `
+                <form>
+                    <field name="p">
+                        <tree editable="top">
+                            <field name="int_field" widget="handle"/>
+                            <field name="foo"/>
+                        </tree>
+                    </field>
+                </form>`,
+            resId: 1,
+            mockRPC(route, args) {
+                if (args.method === "web_read") {
+                    assert.step(`web_read`);
+                    assert.strictEqual(args.kwargs.specification.p.order, "int_field ASC, id ASC");
+                }
+            },
+        });
+        assert.verifySteps(["web_read"]);
+    });
+
     QUnit.test("one2many field when using the pager", async function (assert) {
         const ids = [];
         for (let i = 0; i < 45; i++) {
diff --git a/addons/web/static/tests/views/list_view_tests.js b/addons/web/static/tests/views/list_view_tests.js
index 314215cfdd8..7a488c02c63 100644
--- a/addons/web/static/tests/views/list_view_tests.js
+++ b/addons/web/static/tests/views/list_view_tests.js
@@ -10824,7 +10824,7 @@ QUnit.module("Views", (hooks) => {
     });
 
     QUnit.test("list with handle widget", async function (assert) {
-        assert.expect(11);
+        assert.expect(13);
 
         await makeView({
             type: "list",
@@ -10836,6 +10836,9 @@ QUnit.module("Views", (hooks) => {
                     <field name="amount" widget="float" digits="[5,0]"/>
                 </tree>`,
             mockRPC(route, args) {
+                if (args.method === "web_search_read") {
+                    assert.step(`web_search_read: order: ${args.kwargs.order}`);
+                }
                 if (route === "/web/dataset/resequence") {
                     assert.strictEqual(
                         args.offset,
@@ -10857,6 +10860,7 @@ QUnit.module("Views", (hooks) => {
             },
         });
 
+        assert.verifySteps(["web_search_read: order: int_field ASC, id ASC"]);
         let rows = target.querySelectorAll(".o_data_row");
         assert.strictEqual(
             rows[0].querySelector("[name='amount']").textContent,