[FIX] web: in x2manys, do not do onchange if invalid line

In general, with the new views, we execute immediately and recursively
all onchanges, even from a x2many in a form view.  It allows for nice
interactivity, for example, recomputing immediately a total or a tax in
a sale order when an order line changed.

However, we are used to treat x2manys in a slightly weird way: for
example, if you add a line in a one2many, then click elsewhere, most of
the time, it is validated.  But if the line is 'incomplete' (I mean, if
a required field is not set), the line will not be validated, but be
automatically discarded.  If it was dirty, a confirm dialog will open.

So, to keep in line with that semantic, we have decided that onchange
will just not be done until the line is 'valid'.  This will avoid
trouble when someone add a line, it triggers some onchange which changes
some values in the form, then click somewhere else, the line is
discarded, but the changed values persist.  Most of the time, it
probably won't be a problem, because another onchange will be triggered,
but it is some useless work anyway.

2 files changed, 86 insertions, 7 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 87c9f3c57bc..62898a338bd 100644
--- a/addons/web/static/src/js/views/basic/basic_model.js
+++ b/addons/web/static/src/js/views/basic/basic_model.js
@@ -825,7 +825,6 @@ var BasicModel = AbstractModel.extend({
     _applyChange: function (recordID, changes) {
         var self = this;
         var record = this.localData[recordID];
-        var onChangeFields = []; // the fields that have changed and that have an on_change
         var field;
         var defs = [];
         record._changes = record._changes || {};
@@ -840,12 +839,19 @@ var BasicModel = AbstractModel.extend({
             } else {
                 record._changes[fieldName] = changes[fieldName];
             }
-            if (field.onChange) {
-                onChangeFields.push(fieldName);
-            }
         }
 
         return $.when.apply($, defs).then(function () {
+            var onChangeFields = []; // the fields that have changed and that have an on_change
+            for (var fieldName in changes) {
+                field = record.fields[fieldName];
+                if (field.onChange) {
+                    var isX2Many = field.type === 'one2many' || field.type === 'many2many';
+                    if (!isX2Many || (self._isX2ManyValid(record._changes[fieldName] || record.data[fieldName]))) {
+                        onChangeFields.push(fieldName);
+                    }
+                }
+            }
             var onchangeDef;
             if (onChangeFields.length) {
                 onchangeDef = self._performOnChange(record, onChangeFields).then(function (result) {
@@ -1949,6 +1955,29 @@ var BasicModel = AbstractModel.extend({
         return Object.keys(fieldsInfo && fieldsInfo[element.viewType] || {});
     },
     /**
+     * return true if a list element is 'valid'. Such an element is valid if it
+     * has no sub record with an unset required field.
+     *
+     * This method is meant to be used to check if a x2many change will trigger
+     * an onchange.
+     *
+     * @param {string} id id for a local resource of type 'list'. This is
+     *   assumed to be a list element for a one2many
+     * @returns {boolean}
+     */
+    _isX2ManyValid: function (id) {
+        var isValid = true;
+        var element = this.get(id, {raw: true});
+        _.each(element.data, function (rec) {
+            for (var fieldName in rec.fields) {
+                if (rec.fields[fieldName].required && !rec.data[fieldName]) {
+                    isValid = false;
+                }
+            }
+        });
+        return isValid;
+    },
+    /**
      * Helper method for the load entry point.
      *
      * @see load
diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js
index 23274f395a8..6e0fc199b6c 100644
--- a/addons/web/static/tests/fields/relational_fields_tests.js
+++ b/addons/web/static/tests/fields/relational_fields_tests.js
@@ -91,7 +91,7 @@ QUnit.module('relational_fields', {
             turtle: {
                 fields: {
                     display_name: { string: "Displayed name", type: "char" },
-                    turtle_foo: {string: "Foo", type: "char", default: "My little Foo Value"},
+                    turtle_foo: {string: "Foo", type: "char"},
                     turtle_bar: {string: "Bar", type: "boolean", default: true},
                     turtle_int: {string: "int", type: "integer", sortable: true},
                     turtle_qux: {string: "Qux", type: "float", digits: [16,1], required: true, default: 1.5},
@@ -2677,6 +2677,7 @@ QUnit.module('relational_fields', {
     QUnit.test('one2many list editable = top', function (assert) {
         assert.expect(6);
 
+        this.data.turtle.fields.turtle_foo.default = "default foo turtle";
         var form = createView({
             View: FormView,
             model: 'partner',
@@ -2711,7 +2712,7 @@ QUnit.module('relational_fields', {
 
         assert.strictEqual(form.$('.o_data_row').length, 2,
             "should have 2 data rows");
-        assert.strictEqual(form.$('tr.o_data_row:first input').val(), 'My little Foo Value',
+        assert.strictEqual(form.$('tr.o_data_row:first input').val(), 'default foo turtle',
             "first row should be the new value");
         assert.ok(form.$('tr.o_data_row:first').hasClass('o_selected_row'),
             "first row should be selected");
@@ -2722,6 +2723,7 @@ QUnit.module('relational_fields', {
 
     QUnit.test('one2many list editable = bottom', function (assert) {
         assert.expect(6);
+        this.data.turtle.fields.turtle_foo.default = "default foo turtle";
 
         var form = createView({
             View: FormView,
@@ -2757,7 +2759,7 @@ QUnit.module('relational_fields', {
 
         assert.strictEqual(form.$('.o_data_row').length, 2,
             "should have 2 data rows");
-        assert.strictEqual(form.$('tr.o_data_row:eq(1) input').val(), 'My little Foo Value',
+        assert.strictEqual(form.$('tr.o_data_row:eq(1) input').val(), 'default foo turtle',
             "second row should be the new value");
         assert.ok(form.$('tr.o_data_row:eq(1)').hasClass('o_selected_row'),
             "second row should be selected");
@@ -2800,6 +2802,54 @@ QUnit.module('relational_fields', {
         form.destroy();
     });
 
+    QUnit.test('one2many list editable, no onchange when required field is not set', function (assert) {
+        assert.expect(7);
+
+        this.data.turtle.fields.turtle_foo.required = true;
+        this.data.partner.onchanges = {
+            turtles: function (obj) {
+                obj.int_field = obj.turtles.length;
+            },
+        };
+        this.data.partner.records[0].int_field = 0;
+        this.data.partner.records[0].turtles = [];
+
+        var form = createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form string="Partners">' +
+                    '<field name="int_field"/>' +
+                    '<field name="turtles">' +
+                        '<tree editable="top">' +
+                            '<field name="turtle_int"/>' +
+                            '<field name="turtle_foo"/>' +
+                        '</tree>' +
+                    '</field>' +
+                '</form>',
+            mockRPC: function (route, args) {
+                assert.step(args.method);
+                return this._super.apply(this, arguments);
+            },
+            res_id: 1,
+        });
+        form.$buttons.find('.o_form_button_edit').click();
+
+        assert.strictEqual(form.$('.o_form_field[name="int_field"]').val(), "0",
+            "int_field should start with value 0");
+        form.$('.o_form_field_x2many_list_row_add a').click();
+        assert.strictEqual(form.$('.o_form_field[name="int_field"]').val(), "0",
+            "int_field should still be 0 (no onchange should have been done yet");
+
+        assert.verifySteps(['read', 'default_get'], "no onchange should have been applied");
+
+        form.$('.o_field_widget[name="turtle_foo"]').val("some text").trigger('input');
+        assert.strictEqual(form.$('.o_form_field[name="int_field"]').val(), "1",
+            "int_field should now be 1 (the onchange should have been done");
+
+        form.destroy();
+    });
+
     QUnit.module('FieldMany2Many');
 
     QUnit.test('many2many kanban: edition', function (assert) {