From b3f1081082ce33afe2513c2712bd0204fc5d8a22 Mon Sep 17 00:00:00 2001
From: Aaron Bohy <aab@odoo.com>
Date: Fri, 11 Jan 2019 07:42:05 +0000
Subject: [PATCH] [FIX] web: form: call discardChanges while saving

Before this rev., there was a race condition when the user did
some changes on a form view (triggering an onchange), saved and
clicked on the breadcrumbs to leave the form view (before the
onchange returned). As a consequence, the dialog telling the user
that there are pending changes was displayed. This mostly occured
on the main_flow_tour since the removal of an unnecessary check,
and could be reproduced manually on a slow network.

Basically, the check if there were pending changes was done too
early (before the view was actually saved).
---
 .../src/js/views/basic/basic_controller.js    | 12 +++-
 addons/web/static/tests/views/form_tests.js   | 63 +++++++++++++++++++
 2 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/addons/web/static/src/js/views/basic/basic_controller.js b/addons/web/static/src/js/views/basic/basic_controller.js
index 76adef0a154..01bbd9cd2fe 100644
--- a/addons/web/static/src/js/views/basic/basic_controller.js
+++ b/addons/web/static/src/js/views/basic/basic_controller.js
@@ -38,6 +38,10 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
         FieldManagerMixin.init.call(this, this.model);
         this.handle = params.initialState.id;
         this.mode = params.mode || 'readonly';
+        // savingDef is used to ensure that we always wait for pending save
+        // operations to complete before checking if there are changes to
+        // discard when discardChanges is called
+        this.savingDef = $.when();
     },
     /**
      * @override
@@ -102,14 +106,15 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
         return true;
     },
     /**
-     * Waits for the mutex to be unlocked and then calls _.discardChanges.
+     * Waits for the mutex to be unlocked and for changes to be saved, then
+     * calls _.discardChanges.
      * This ensures that the confirm dialog isn't displayed directly if there is
      * a pending 'write' rpc.
      *
      * @see _.discardChanges
      */
     discardChanges: function (recordID, options) {
-        return this.mutex.exec(function () {})
+        return $.when(this.mutex.getUnlockedDef(), this.savingDef)
             .then(this._discardChanges.bind(this, recordID || this.handle, options));
     },
     /**
@@ -183,11 +188,12 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
         // mutex-protected as commitChanges function of x2m has to be aware of
         // all final changes made to a row.
         var self = this;
-        return this.mutex.getUnlockedDef().then(function () {
+        this.savingDef = this.mutex.getUnlockedDef().then(function () {
             return self.renderer.commitChanges(recordID || self.handle).then(function () {
                 return self.mutex.exec(self._saveRecord.bind(self, recordID, options));
             });
         });
+        return this.savingDef;
     },
     /**
      * @override
diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js
index 8099c58092a..e5b5f0cf36f 100644
--- a/addons/web/static/tests/views/form_tests.js
+++ b/addons/web/static/tests/views/form_tests.js
@@ -7254,6 +7254,69 @@ QUnit.module('Views', {
         form.destroy();
     });
 
+    QUnit.test('call canBeRemoved while saving', function (assert) {
+        assert.expect(10);
+
+        this.data.partner.onchanges = {
+            foo: function (obj) {
+                obj.display_name = obj.foo === 'trigger onchange' ? 'changed' : 'default';
+            },
+        };
+
+        var onchangeDef;
+        var createDef = $.Deferred();
+        var form = createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form><field name="display_name"/><field name="foo"/></form>',
+            mockRPC: function (route, args) {
+                var result = this._super.apply(this, arguments);
+                if (args.method === 'onchange') {
+                    return $.when(onchangeDef).then(_.constant(result));
+                }
+                if (args.method === 'create') {
+                    return $.when(createDef).then(_.constant(result));
+                }
+                return result;
+            },
+        });
+
+        // edit foo to trigger a delayed onchange
+        onchangeDef = $.Deferred();
+        testUtils.fields.editInput(form.$('.o_field_widget[name=foo]'), 'trigger onchange');
+
+        assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'default');
+
+        // save (will wait for the onchange to return), and will be delayed as well
+        testUtils.dom.click(form.$buttons.find('.o_form_button_save'));
+
+        assert.hasClass(form.$('.o_form_view'), 'o_form_editable');
+        assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'default');
+
+        // simulate a click on the breadcrumbs to leave the form view
+        form.canBeRemoved();
+
+        assert.hasClass(form.$('.o_form_view'), 'o_form_editable');
+        assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'default');
+
+        // unlock the onchange
+        onchangeDef.resolve();
+
+        assert.hasClass(form.$('.o_form_view'), 'o_form_editable');
+        assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'changed');
+
+        // unlock the create
+        createDef.resolve();
+
+        assert.hasClass(form.$('.o_form_view'), 'o_form_readonly');
+        assert.strictEqual(form.$('.o_field_widget[name=display_name]').text(), 'changed');
+        assert.containsNone(document.body, '.modal',
+            "should not display the 'Changes will be discarded' dialog");
+
+        form.destroy();
+    });
+
     QUnit.module('FormViewTABMainButtons');
 
     QUnit.test('using tab in an empty required string field should not move to the next field',function(assert) {