[FIX] web: prevent useless discard dialog from showing up

Here is a scenario that could cause an issue:
- open a form view with a many2one
- click on edit button
- click on small external button for many2one. this opens a modal form
  view
- click on save in the modal (without changing anything)
- click on save in the main record
- exit form view. this opens a discard dialog

There were two issues here:
- saving the modal dialog automatically marks the many2one field as changed.
  This was necessary to force reloading the data, because editing a sub value
  in the modal form view could have changed the display_name of the manyone
  field.  However, this is not necessary when no change was done.
- when saving a record, the _isDirty flag was reset to false only if an
  actual rpc was done.  However, it may happen that the flag is set to
  dirty (for example, when modifying a value inside a many2one), but the
  main record has no changed fields.

In this commit, we also remove the on_save handler in the
formviewdialog.  This is a small refactoring in a stable version, but no code
currently use it, and I believe that it will make the code much easier
to maintain (the previous code was really awkward), so I think that the
tradeoff is acceptable.

4 files changed, 107 insertions, 19 deletions

diff --git a/addons/web/static/src/js/fields/relational_fields.js b/addons/web/static/src/js/fields/relational_fields.js
index 19f2728a3ae..c9e6f218895 100644
--- a/addons/web/static/src/js/fields/relational_fields.js
+++ b/addons/web/static/src/js/fields/relational_fields.js
@@ -502,9 +502,11 @@ var FieldMany2One = AbstractField.extend({
                     title: _t("Open: ") + self.string,
                     view_id: view_id,
                     readonly: !self.can_write,
-                    on_saved: function () {
-                        self._setValue(self.value.data, {forceChange: true});
-                        self.trigger_up('reload', {db_id: self.value.id});
+                    on_saved: function (record, changed) {
+                        if (changed) {
+                            self._setValue(self.value.data, {forceChange: true});
+                            self.trigger_up('reload', {db_id: self.value.id});
+                        }
                     },
                 }).open();
             });
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 27a1e12ecfe..445967287bf 100644
--- a/addons/web/static/src/js/views/basic/basic_model.js
+++ b/addons/web/static/src/js/views/basic/basic_model.js
@@ -896,6 +896,9 @@ var BasicModel = AbstractModel.extend({
                 return def.resolve(changedFields);
             }
 
+            def.then(function () {
+                record._isDirty = false;
+            });
             // in the case of a write, only perform the RPC if there are changes to save
             if (method === 'create' || changedFields.length) {
                 var args = method === 'write' ? [[record.data.id], changes] : [changes];
@@ -917,7 +920,6 @@ var BasicModel = AbstractModel.extend({
 
                         // Erase changes as they have been applied
                         record._changes = {};
-                        record._isDirty = false;
 
                         // Update the data directly or reload them
                         if (shouldReload) {
diff --git a/addons/web/static/src/js/views/view_dialogs.js b/addons/web/static/src/js/views/view_dialogs.js
index da3fc94f2ce..f24bc9341b8 100644
--- a/addons/web/static/src/js/views/view_dialogs.js
+++ b/addons/web/static/src/js/views/view_dialogs.js
@@ -81,9 +81,9 @@ var FormViewDialog = ViewDialog.extend({
      * @param {Object} [options.fields_view] optional form fields_view
      * @param {boolean} [options.readonly=false] only applicable when not in
      *   creation mode
-     * @param {function} [options.on_save] callback to execute when clicking on
-     *   'Save' (form view's 'saveRecord' by default)
-     * @param {function} [options.on_saved] callback executed after on_save
+     * @param {function} [options.on_saved] callback executed after saving a
+     *   record.  It will be called with the record data, and a boolean which
+     *   indicates if something was changed
      * @param {BasicModel} [options.model] if given, it will be used instead of
      *  a new form view model
      * @param {string} [options.recordID] if given, the model has to be given as
@@ -211,24 +211,16 @@ var FormViewDialog = ViewDialog.extend({
 
     _save: function () {
         var self = this;
-        var def;
-        if (this.options.on_save) {
-            if (this.form_view.canBeSaved()) {
-                return $.Deferred().reject();
-            }
-            def = this.options.on_save(this.form_view.model.get(this.form_view.handle));
-        } else {
-            def = this.form_view.saveRecord(this.form_view.handle, {
+        return this.form_view.saveRecord(this.form_view.handle, {
                 stayInEdit: true,
                 reload: false,
                 savePoint: this.shouldSaveLocally,
                 viewType: 'form',
-            });
-        }
-        return $.when(def).then(function () {
+        }).then(function (changedFields) {
             // record might have been changed by the save (e.g. if this was a new record, it has an
             // id now), so don't re-use the copy obtained before the save
-            self.on_saved(self.form_view.model.get(self.form_view.handle));
+            var record = self.form_view.model.get(self.form_view.handle);
+            self.on_saved(record, !!changedFields.length);
         });
     },
 });
diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js
index 07b58764e30..8f789c88647 100644
--- a/addons/web/static/tests/fields/relational_fields_tests.js
+++ b/addons/web/static/tests/fields/relational_fields_tests.js
@@ -210,6 +210,98 @@ QUnit.module('relational_fields', {
         form.destroy();
     });
 
+    QUnit.test('editing a many2one, but not changing anything', function (assert) {
+        assert.expect(2);
+        var form = createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form string="Partners">' +
+                    '<sheet>' +
+                            '<field name="trululu"/>' +
+                    '</sheet>' +
+                '</form>',
+            archs: {
+                'partner,false,form': '<form string="Partners"><field name="display_name"/></form>',
+            },
+            res_id: 1,
+            mockRPC: function (route, args) {
+                if (args.method === 'get_formview_id') {
+                    assert.deepEqual(args.args[0], [4], "should call get_formview_id with correct id");
+                    return $.when(false);
+                }
+                return this._super(route, args);
+            },
+            viewOptions: {
+                ids: [1, 2],
+            },
+        });
+
+        form.$buttons.find('.o_form_button_edit').click();
+
+        // click on the external button (should do an RPC)
+        form.$('.o_external_button').click();
+        // save and close modal
+        $('.modal .modal-footer .btn-primary:first').click();
+        // save form
+        form.$buttons.find('.o_form_button_save').click();
+        // click next on pager
+        form.pager.$('.o_pager_next').click();
+
+        // this checks that the view did not ask for confirmation that the
+        // record is dirty
+        assert.strictEqual(form.pager.$el.text().trim(), '2 / 2',
+            'pager should be at second page');
+        form.destroy();
+    });
+
+    QUnit.test('editing a many2one (with form view opened with external button)', function (assert) {
+        assert.expect(1);
+        var form = createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form string="Partners">' +
+                    '<sheet>' +
+                            '<field name="trululu"/>' +
+                    '</sheet>' +
+                '</form>',
+            archs: {
+                'partner,false,form': '<form string="Partners"><field name="foo"/></form>',
+            },
+            res_id: 1,
+            mockRPC: function (route, args) {
+                if (args.method === 'get_formview_id') {
+                    return $.when(false);
+                }
+                return this._super(route, args);
+            },
+            viewOptions: {
+                ids: [1, 2],
+            },
+        });
+
+        form.$buttons.find('.o_form_button_edit').click();
+
+        // click on the external button (should do an RPC)
+        form.$('.o_external_button').click();
+
+        $('.modal input[name="foo"]').val('brandon').trigger('input');
+
+        // save and close modal
+        $('.modal .modal-footer .btn-primary:first').click();
+        // save form
+        form.$buttons.find('.o_form_button_save').click();
+        // click next on pager
+        form.pager.$('.o_pager_next').click();
+
+        // this checks that the view did not ask for confirmation that the
+        // record is dirty
+        assert.strictEqual(form.pager.$el.text().trim(), '2 / 2',
+            'pager should be at second page');
+        form.destroy();
+    });
+
     QUnit.test('many2ones in form views with show_adress', function (assert) {
         assert.expect(4);
         var form = createView({