[FIX] web: BasicModel: don't write on readonly fields

Ultimately, it seems that sending the value of readonly fields
when saving is not such a good idea. Suppose that there is a
computed field displayed as readonly in a form view. This field is
computed from other records, e.g. from a one2many field which is
editable in the form view. Finally, the inverse function of the
computed field creates the one2many records. If the user adds some
records to the one2many, then saves, the readonly computed field's
value is sent to the server as well, and thus the inverse function
is executed, which isn't what we want.

This commit essentially reverts 0494d61274, except that we now take
into account the readonly modifier to determine whether or not a
value is sent to the server, and not only the readonly attribute of
the field (which can be seen as a default). Before rev. 0494d61274,
we made a difference between write and create RPCs, for an unclear
reason. We don't do that anymore: readonly fields are never sent to
the server (same behavior as before the new views).

addons/web/static/src/js/views/basic/basic_controller.js

@@ -419,6 +419,7 @@ var BasicController = AbstractController.extend(FieldManagerMixin, {
             var saveDef = this.model.save(recordID, { // Save then leave edit mode
                 reload: options.reload,
                 savePoint: options.savePoint,
+                viewType: options.viewType,
             });
             if (!options.stayInEdit) {
                 saveDef = saveDef.then(function (fieldNames) {

addons/web/static/src/js/views/basic/basic_model.js

@@ -704,6 +704,8 @@ var BasicModel = AbstractModel.extend({
      * @param {boolean} [options.savePoint=false] if true, the record will only
      *   be 'locally' saved: its changes written in a _savePoint key that can
      *   be restored later by call discardChanges with option rollback to true
+     * @param {string} [options.viewType] current viewType. If not set, we will
+     *   assume main viewType from the record
      * @returns {Deferred}
      *   Resolved with the list of field names (whose value has been modified)
      */
@@ -718,6 +720,11 @@ var BasicModel = AbstractModel.extend({
                     if (newValue instanceof Array) {
                         rec._savePoint = newValue.slice(0);
                     } else {
+                        // save the viewType of edition, so that the correct readonly modifiers
+                        // can be evaluated when the record will be saved
+                        for (var fieldName in (rec._changes || {})) {
+                            rec._editionViewType[fieldName] = options.viewType;
+                        }
                         rec._savePoint = _.extend({}, newValue);
                     }
                 });
@@ -728,7 +735,7 @@ var BasicModel = AbstractModel.extend({
                 // id never changes, and should not be written
                 delete record._changes.id;
             }
-            var changes = self._generateChanges(record);
+            var changes = self._generateChanges(record, options.viewType);
 
             if (method === 'create') {
                 var fieldNames = record.getFieldNames();
@@ -1869,12 +1876,26 @@ var BasicModel = AbstractModel.extend({
      *
      * @private
      * @param {Object} record
+     * @param {string} [viewType] current viewType. If not set, we will assume
+     *   main viewType from the record. Note that if an editionViewType is
+     *   specified for a field, it will take the priority over the viewType arg.
      * @returns {Object} a map from changed fields to their new value
      */
-    _generateChanges: function (record) {
+    _generateChanges: function (record, viewType) {
+        viewType = viewType || record.viewType;
         var changes = _.extend({}, record._changes);
         var commands = this._generateX2ManyCommands(record, true);
         for (var fieldName in record.fields) {
+            // remove readonly fields from the list of changes
+            if (fieldName in changes || fieldName in commands) {
+                var editionViewType = record._editionViewType[fieldName] || viewType;
+                if (this._isFieldReadonly(record, fieldName, editionViewType)) {
+                    delete changes[fieldName];
+                    continue;
+                }
+            }
+
+            // process relational fields and handle the null case
             var type = record.fields[fieldName].type;
             if (type === 'one2many' || type === 'many2many') {
                 if (commands[fieldName].length) { // replace localId by commands
@@ -2199,6 +2220,31 @@ var BasicModel = AbstractModel.extend({
         }
         return context;
     },
+    /**
+     * Returns true if the field is readonly (checking first in the modifiers,
+     * and if there is no readonly modifier, checking the readonly attribute of
+     * the field).
+     *
+     * @private
+     * @param {Object} record an element from the localData
+     * @param {string} fieldName
+     * @param {string} [viewType] current viewType. If not set, we will assume
+     *   main viewType from the record
+     * @returns {boolean}
+     */
+    _isFieldReadonly: function (record, fieldName, viewType) {
+        var fieldInfo = record.fieldsInfo[viewType || record.viewType][fieldName];
+        var modifiers;
+        if (fieldInfo) {
+            var rawModifiers = JSON.parse(fieldInfo.modifiers || "{}");
+            modifiers = this._evalModifiers(record, rawModifiers);
+        }
+        if (modifiers && 'readonly' in modifiers) {
+            return modifiers.readonly;
+        } else {
+            return record.fields[fieldName].readonly;
+        }
+    },
     /**
      * Returns true iff value is considered to be set for the given field's type.
      *
@@ -2330,6 +2376,12 @@ var BasicModel = AbstractModel.extend({
             viewType: params.viewType,
         };
 
+        // _editionViewType is a dict whose keys are field names and which is populated when a field
+        // is edited with the viewType as value. This is useful for one2manys to determine whether
+        // or not a field is readonly (using the readonly modifiers of the view in which the field
+        // has been edited)
+        dataPoint._editionViewType = {};
+
         dataPoint.evalModifiers = this._evalModifiers.bind(this, dataPoint);
         dataPoint.getContext = this._getContext.bind(this, dataPoint);
         dataPoint.getDomain = this._getDomain.bind(this, dataPoint);
@@ -2507,7 +2559,7 @@ var BasicModel = AbstractModel.extend({
                     .then(function () {
                         // save initial changes, so they can be restored later,
                         // if we need to discard.
-                        self.save(record.id, {savePoint: true})
+                        self.save(record.id, {savePoint: true});
 
                         return record.id;
                     });
@@ -2582,8 +2634,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} The returned deferred can fail, in which case the
-     *   fail value will be the warning message received from the server
+     * @returns {Deferred}
      */
     _performOnChange: function (record, fields, viewType) {
         var self = this;

addons/web/static/src/js/views/view_dialogs.js

@@ -222,6 +222,7 @@ var FormViewDialog = ViewDialog.extend({
                 stayInEdit: true,
                 reload: false,
                 savePoint: this.shouldSaveLocally,
+                viewType: 'form',
             });
         }
         return $.when(def).then(function () {

addons/web/static/tests/views/basic_model_tests.js

@@ -572,6 +572,20 @@ QUnit.module('Views', {
         this.params.fieldNames = ['product_id', 'category', 'product_ids'];
         this.params.res_id = undefined;
         this.params.type = 'record';
+        this.params.fieldsInfo = {
+            form: {
+                category: {},
+                product_id: {},
+                product_ids: {
+                    fieldsInfo: {
+                        default: { name: {} },
+                    },
+                    relatedFields: this.data.product.fields,
+                    viewType: 'default',
+                },
+            },
+        };
+        this.params.viewType = 'form';
 
         var model = createModel({
             Model: BasicModel,
@@ -1015,6 +1029,19 @@ QUnit.module('Views', {
         this.params.fieldNames = ['total', 'product_ids'];
         this.params.res_id = undefined;
         this.params.type = 'record';
+        this.params.fieldsInfo = {
+            form: {
+                product_ids: {
+                    fieldsInfo: {
+                        default: { name: {} },
+                    },
+                    relatedFields: this.data.product.fields,
+                    viewType: 'default',
+                },
+                total: {},
+            },
+        };
+        this.params.viewType = 'form';
 
         var o2mRecordParams = {
             fields: this.data.product.fields,
@@ -1277,6 +1304,63 @@ QUnit.module('Views', {
         model.destroy();
     });
 
+    QUnit.test('dont write on readonly fields (write and create)', function (assert) {
+        assert.expect(6);
+
+        this.params.fieldNames = ['foo', 'bar'];
+        this.data.partner.onchanges.foo = function (obj) {
+            obj.bar = obj.foo.length;
+        };
+        this.data.partner.fields.bar.readonly = true;
+
+        var model = createModel({
+            Model: BasicModel,
+            data: this.data,
+            mockRPC: function (route, args) {
+                if (args.method === 'write') {
+                    assert.deepEqual(args.args[1], {foo: "verylongstring"},
+                        "should only save foo field");
+                }
+                if (args.method === 'create') {
+                    assert.deepEqual(args.args[0], {foo: "anotherverylongstring"},
+                        "should only save foo field");
+                }
+                return this._super(route, args);
+            },
+        });
+
+        model.load(this.params).then(function (resultID) {
+            var record = model.get(resultID);
+            assert.strictEqual(record.data.bar, 2,
+                "should be initialized with correct value");
+
+            model.notifyChanges(resultID, {foo: "verylongstring"});
+
+            record = model.get(resultID);
+            assert.strictEqual(record.data.bar, 14,
+                "should be changed with correct value");
+
+            model.save(resultID);
+        });
+
+        // start again, but with a new record
+        delete this.params.res_id;
+        model.load(this.params).then(function (resultID) {
+            var record = model.get(resultID);
+            assert.strictEqual(record.data.bar, false,
+                "should be initialized with correct value");
+
+            model.notifyChanges(resultID, {foo: "anotherverylongstring"});
+
+            record = model.get(resultID);
+            assert.strictEqual(record.data.bar, 21,
+                "should be changed with correct value");
+
+            model.save(resultID);
+        });
+        model.destroy();
+    });
+
     QUnit.test('default_get with one2many values', function (assert) {
         assert.expect(1);
 
@@ -1297,6 +1381,18 @@ QUnit.module('Views', {
             fields: this.data.partner.fields,
             modelName: 'partner',
             type: 'record',
+            fieldsInfo: {
+                form: {
+                    product_ids: {
+                        fieldsInfo: {
+                            default: { name: {} },
+                        },
+                        relatedFields: this.data.product.fields,
+                        viewType: 'default',
+                    },
+                },
+            },
+            viewType: 'form',
         };
         model.load(params).then(function (resultID) {
             assert.strictEqual(typeof resultID, 'string', "result should be a valid id");

addons/web/static/tests/views/form_tests.js

@@ -4979,5 +4979,65 @@ QUnit.module('Views', {
 
         form.destroy();
     });
+
+    QUnit.test('readonly fields are not sent when saving', function (assert) {
+        assert.expect(5);
+
+        // define an onchange on display_name to check that the value of readonly
+        // fields is correctly sent for onchanges
+        this.data.partner.onchanges = {
+            display_name: function () {},
+        };
+        var checkOnchange = false;
+
+        var form = createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: '<form string="Partners">' +
+                    '<field name="p">' +
+                        '<tree>' +
+                            '<field name="display_name"/>' +
+                        '</tree>' +
+                        '<form string="Partners">' +
+                            '<field name="display_name"/>' +
+                            '<field name="foo" attrs="{\'readonly\': [[\'display_name\', \'=\', \'readonly\']]}"/>' +
+                        '</form>' +
+                    '</field>' +
+                '</form>',
+            mockRPC: function (route, args) {
+                if (checkOnchange && args.method === 'onchange') {
+                    assert.strictEqual(args.args[1].foo, 'foo value',
+                        "readonly fields value should be sent for onchanges");
+                }
+                if (args.method === 'create') {
+                    assert.deepEqual(args.args[0], {
+                        p: [[0, false, {display_name: 'readonly'}]]
+                    }, "should not have sent the value of the readonly field");
+                }
+                return this._super.apply(this, arguments);
+            },
+        });
+
+        form.$('.o_field_x2many_list_row_add a').click();
+        assert.strictEqual($('.modal input.o_field_widget[name=foo]').length, 1,
+            'foo should be editable');
+        checkOnchange = true;
+        $('.modal .o_field_widget[name=foo]').val('foo value').trigger('input');
+        $('.modal .o_field_widget[name=display_name]').val('readonly').trigger('input');
+        checkOnchange = false;
+        assert.strictEqual($('.modal span.o_field_widget[name=foo]').length, 1,
+            'foo should be readonly');
+        $('.modal .modal-footer .btn-primary').click(); // close the modal
+
+        form.$('.o_data_row').click(); // re-open previous record
+        assert.strictEqual($('.modal .o_field_widget[name=foo]').text(), 'foo value',
+            "the edited value should have been kept");
+        $('.modal .modal-footer .btn-primary').click(); // close the modal
+
+        form.$buttons.find('.o_form_button_save').click(); // save the record
+
+        form.destroy();
+    });
 });
 });