[FIX] web: revert to previous state when onchange fails

When an onchange fails (for example, because the onchange python code
raised a ValidationError, it is convenient to revert the form view to a
valid state, which is the previous state.

To do that, we keep in the _applyChange function the initial state, and
restore it in the case the onchange fails.

2 files changed, 41 insertions, 32 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 2cf96815b5e..27a1e12ecfe 100644
--- a/addons/web/static/src/js/views/basic/basic_model.js
+++ b/addons/web/static/src/js/views/basic/basic_model.js
@@ -1109,6 +1109,10 @@ var BasicModel = AbstractModel.extend({
         if (!options.doNotSetDirty) {
             record._isDirty = true;
         }
+        var initialData = {};
+        this._visitChildren(record, function (elem) {
+            initialData[elem.id] = $.extend(true, {}, _.pick(elem, 'data', '_changes'));
+        });
 
         // apply changes to local data
         for (var fieldName in changes) {
@@ -1137,12 +1141,18 @@ var BasicModel = AbstractModel.extend({
                     }
                 }
             }
-            var onchangeDef;
+            var onchangeDef = $.Deferred();
             if (onChangeFields.length) {
-                onchangeDef = self._performOnChange(record, onChangeFields, options.viewType).then(function (result) {
-                    delete record._warning;
-                    return _.keys(changes).concat(Object.keys(result && result.value || {}));
-                });
+                self._performOnChange(record, onChangeFields, options.viewType)
+                    .then(function (result) {
+                        delete record._warning;
+                        onchangeDef.resolve(_.keys(changes).concat(Object.keys(result && result.value || {})));
+                    }).fail(function () {
+                        self._visitChildren(record, function (elem) {
+                            _.extend(elem, initialData[elem.id]);
+                        });
+                        onchangeDef.resolve({});
+                    });
             } else {
                 onchangeDef = $.Deferred().resolve(_.keys(changes));
             }
@@ -3232,15 +3242,18 @@ var BasicModel = AbstractModel.extend({
                 });
                 return $.when.apply($, defs)
                     .then(function () {
-                        return self._performOnChange(record, fields_key).then(function () {
+                        var def = $.Deferred();
+                        self._performOnChange(record, fields_key).always(function () {
                             if (record._warning) {
                                 if (params.allowWarning) {
                                     delete record._warning;
                                 } else {
-                                    return $.Deferred().reject();
+                                    def.reject();
                                 }
                             }
+                            def.resolve();
                         });
+                        return def;
                     })
                     .then(function () {
                         return self._fetchRelationalData(record);
@@ -3307,10 +3320,7 @@ var BasicModel = AbstractModel.extend({
      * @param {string[]} fields changed fields
      * @param {string} [viewType] current viewType. If not set, we will assume
      *   main viewType from the record
-     * @returns {Deferred} Note: this deferred cannot fail. It is either in the
-     *   'success' or 'pending' state.  If the onchange rpc fails, it will be
-     *   resolved with an empty dictionary.  Note that the initial change was
-     *   already applied.
+     * @returns {Deferred}
      */
     _performOnChange: function (record, fields, viewType) {
         var self = this;
@@ -3330,9 +3340,7 @@ var BasicModel = AbstractModel.extend({
         var context = this._getContext(record, options);
         var currentData = this._generateOnChangeData(record, {changesOnly: false});
 
-        var def = $.Deferred();
-
-        self._rpc({
+        return self._rpc({
                 model: record.model,
                 method: 'onchange',
                 args: [idList, currentData, fields, onchangeSpec, context],
@@ -3357,12 +3365,9 @@ var BasicModel = AbstractModel.extend({
                     record._domains = _.extend(record._domains, result.domain);
                 }
                 return self._applyOnChange(result.value, record).then(function () {
-                    def.resolve(result);
+                    return result;
                 });
-            }).fail(function () {
-                def.resolve({});
             });
-        return def;
     },
     /**
      * Once a record is created and some data has been fetched, we need to do
diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js
index 2f7253abba0..9a326348d2c 100644
--- a/addons/web/static/tests/fields/relational_fields_tests.js
+++ b/addons/web/static/tests/fields/relational_fields_tests.js
@@ -6033,7 +6033,7 @@ QUnit.module('relational_fields', {
     QUnit.test('display correct value after validation error', function (assert) {
         assert.expect(4);
 
-        this.data.partner.onchanges.turtles = true;
+        this.data.partner.onchanges.turtles = function () { };
 
         var form = createView({
             View: FormView,
@@ -6050,17 +6050,17 @@ QUnit.module('relational_fields', {
                 '</form>',
             mockRPC: function (route, args) {
                 if (args.method === 'onchange') {
-                    // we simulate a validation error.  In the 'real' web client,
-                    // the server error will be used by the session to display
-                    // an error dialog.  From the point of view of the basic
-                    // model, the deferred is just rejected.
-                    return $.Deferred().reject();
+                    if (args.args[1].turtles[0][2].turtle_foo === 'pinky') {
+                        // we simulate a validation error.  In the 'real' web client,
+                        // the server error will be used by the session to display
+                        // an error dialog.  From the point of view of the basic
+                        // model, the deferred is just rejected.
+                        return $.Deferred().reject();
+                    }
                 }
                 if (args.method === 'write') {
                     assert.deepEqual(args.args[1].turtles[0], [1,2,{turtle_foo: 'foo'}],
-                        'should send the "bad" value');
-                    // we simulate a validation error
-                    return $.Deferred().reject();
+                        'should send the "good" value');
                 }
                 return this._super.apply(this, arguments);
             },
@@ -6070,21 +6070,25 @@ QUnit.module('relational_fields', {
 
         assert.strictEqual(form.$('.o_data_row .o_data_cell:nth(0)').text(), 'blip',
             "initial text should be correct");
+
+        // click and edit value to 'foo', which will trigger onchange
         form.$('.o_data_row .o_data_cell:nth(0)').click();
         form.$('.o_field_widget[name=turtle_foo]').val('foo').trigger('input');
+        form.$el.click();
+        assert.strictEqual(form.$('.o_data_row .o_data_cell:nth(0)').text(), 'foo',
+            "field should have been changed to foo");
 
-        // we try to validate the line. This triggers an onchange which will be
-        // rejected. The line will be returned to readonly mode, but with the
-        // new invalid value.
+        // click and edit value to 'pinky', which trigger a failed onchange
+        form.$('.o_data_row .o_data_cell:nth(0)').click();
+        form.$('.o_field_widget[name=turtle_foo]').val('pinky').trigger('input');
         form.$el.click();
 
         assert.strictEqual(form.$('.o_data_row .o_data_cell:nth(0)').text(), 'foo',
-            "turtle_foo text should now be foo (invalid value)");
+            "turtle_foo text should now be set back to foo");
 
         // we make sure here that when we save, the values are the current
         // values displayed in the field.
         form.$buttons.find('.o_form_button_save').click();
-        assert.strictEqual(form.mode, 'edit', "form view should still be in edit mode");
 
         form.destroy();
     });