From 6f0be0dfcacf35239cfd78e5b340a91a1d22ebfc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Sat, 29 Apr 2017 13:37:48 +0200
Subject: [PATCH] [FIX] web: prevent crash in some cases in list editable

Before this commit, when we moved from one line to the next, we did not
wait for the unselectRow to end.  This means that if the read (from the
unselect row) completes after the default_get, the list renderer was not
in a coherent state (currentRow was set to null), which could (and did)
cause a crash.

This was found by pressing the TAB key a few times, and editing some
values.

Also, we slightly improved the logging in the mock server.  Before this,
the responses from the mock server did not show from which route it came
from.  But in this test, we specifically make sure that the rpcs
complete in a different order, so it was not really optimal.
---
 .../js/views/list/list_editable_renderer.js   |  5 +-
 .../web/static/tests/helpers/mock_server.js   |  2 +-
 addons/web/static/tests/views/list_tests.js   | 51 +++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/addons/web/static/src/js/views/list/list_editable_renderer.js b/addons/web/static/src/js/views/list/list_editable_renderer.js
index 151d41e2e97..f9468418b9b 100644
--- a/addons/web/static/src/js/views/list/list_editable_renderer.js
+++ b/addons/web/static/src/js/views/list/list_editable_renderer.js
@@ -256,8 +256,9 @@ ListRenderer.include({
         if (this.currentRow < this.state.data.length - 1) {
             this._selectCell(this.currentRow + 1, 0);
         } else {
-            this._unselectRow();
-            this.trigger_up('add_record');
+            this._unselectRow().then(
+                this.trigger_up.bind(this, 'add_record')
+            );
         }
     },
     /**
diff --git a/addons/web/static/tests/helpers/mock_server.js b/addons/web/static/tests/helpers/mock_server.js
index 08b6719b446..72a75c3320d 100644
--- a/addons/web/static/tests/helpers/mock_server.js
+++ b/addons/web/static/tests/helpers/mock_server.js
@@ -106,7 +106,7 @@ var MockServer = Class.extend({
             if (logLevel === 1) {
                 console.log('Mock: ' + route, JSON.parse(resultString));
             } else if (logLevel === 2) {
-                console.log('%c[rpc] response:', 'color: blue; font-weight: bold;', JSON.parse(resultString));
+                console.log('%c[rpc] response' + route, 'color: blue; font-weight: bold;', JSON.parse(resultString));
             }
             return JSON.parse(resultString);
         });
diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js
index 92656d024c1..a31620a4cd3 100644
--- a/addons/web/static/tests/views/list_tests.js
+++ b/addons/web/static/tests/views/list_tests.js
@@ -1617,6 +1617,57 @@ QUnit.module('Views', {
         list.destroy();
     });
 
+    QUnit.test('navigation with tab and read completes after default_get', function (assert) {
+        assert.expect(8);
+
+        var defaultGetDef = $.Deferred();
+        var readDef = $.Deferred();
+
+        var list = createView({
+            View: ListView,
+            model: 'foo',
+            data: this.data,
+            arch: '<tree editable="bottom"><field name="foo"/><field name="int_field"/></tree>',
+            mockRPC: function (route, args) {
+                if (args.method) {
+                    assert.step(args.method);
+                }
+                var result = this._super.apply(this, arguments);
+                if (args.method === 'read') {
+                    return readDef.then(_.constant(result));
+                }
+                if (args.method === 'default_get') {
+                    return defaultGetDef.then(_.constant(result));
+                }
+                return result;
+            },
+        });
+
+        list.$('td:contains(-4)').last().click();
+
+        list.$('tr.o_selected_row input[name="int_field"]').val('1234').trigger('input');
+        list.$('tr.o_selected_row input[name="int_field"]').trigger({type: 'keydown', which: 9}); // tab
+
+        defaultGetDef.resolve();
+        assert.strictEqual(list.$('tbody tr.o_data_row').length, 4,
+            "should have 4 data rows");
+        readDef.resolve();
+        assert.strictEqual(list.$('tbody tr.o_data_row').length, 5,
+            "should have 5 data rows");
+        assert.strictEqual(list.$('td:contains(1234)').length, 1,
+            "should have a cell with new value");
+
+        // we trigger a tab to move to the second cell in the current row. this
+        // operation requires that this.currentRow is properly set in the
+        // list editable renderer.
+        list.$('tr.o_selected_row input[name="foo"]').trigger({type: 'keydown', which: 9}); // tab
+        assert.ok(list.$('tr.o_data_row:eq(4)').hasClass('o_selected_row'),
+            "5th row should be selected");
+
+        assert.verifySteps(['write', 'read', 'default_get']);
+        list.destroy();
+    });
+
 });
 
 });