From d93726f9ac6d64a39cd95f35cf61a898e4cd2009 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Mon, 28 Aug 2017 16:14:31 +0200 Subject: [PATCH] [FIX] web,mail: only send fields in view for onchanges The field 'display_name' is automatically added to the list of fields to fetch when fetching a record. Before this rev., its value was also sent alongside the fields in the view for onchange RPCs. It shouldn't: only the fields in the view should be sent. It produced a weird bug in the res.partner form view: open a contact (company), add a sub-contact, save -> the display_name of the contact was then '' (and 'Contacts' was displayed in the breadcrumbs instead of the display_name of the opened contact). This is because 'display_name' was sent to the onchange, which returned '' as value (because of another bug python side). So as it's value changed, when saving, the new display_name was sent to the server (even if this field was actually readonly). Before this rev., we also always fetched 'display_name' when fetching list of records (read or search_read). This wasn't a bug per se, but it might be a computed field, and it isn't necessary, so we stop fetching it in that case if it isn't in the view (as it was done in saas-15, before the new views). --- addons/mail/static/tests/chatter_tests.js | 14 +-- .../static/src/js/services/data_manager.js | 5 -- .../static/src/js/views/basic/basic_model.js | 14 ++- .../tests/fields/relational_fields_tests.js | 6 +- addons/web/static/tests/views/form_tests.js | 90 ++++++++++++++++++- 5 files changed, 110 insertions(+), 19 deletions(-) diff --git a/addons/mail/static/tests/chatter_tests.js b/addons/mail/static/tests/chatter_tests.js index 760a6bcf6cd..b5d7feab56e 100644 --- a/addons/mail/static/tests/chatter_tests.js +++ b/addons/mail/static/tests/chatter_tests.js @@ -530,7 +530,7 @@ QUnit.test('form activity widget: schedule next activity', function (assert) { return $.when(); } if (args.method === 'read' && checkReadArgs) { - assert.deepEqual(args.args[1], ['activity_ids', 'message_ids'], + assert.deepEqual(args.args[1], ['activity_ids', 'message_ids', 'display_name'], "should only read the mail fields"); } return this._super.apply(this, arguments); @@ -686,11 +686,11 @@ QUnit.test('form activity widget: mark as done and remove', function (assert) { if (nbReads === 1) { // first read assert.strictEqual(args.args[1].length, 4, 'should read all fiels the first time'); } else if (nbReads === 2) { // second read: after the unlink - assert.ok(_.isEqual(args.args[1], ['activity_ids']), - 'should only read the activities after an unlink'); + assert.ok(_.isEqual(args.args[1], ['activity_ids', 'display_name']), + 'should only read the activities (+ display_name) after an unlink'); } else { // third read: after marking an activity done - assert.ok(_.isEqual(args.args[1], ['activity_ids', 'message_ids']), - 'should read the activities and messages after marking an activity done'); + assert.ok(_.isEqual(args.args[1], ['activity_ids', 'message_ids', 'display_name']), + 'should read the activities and messages (+ display_name) after marking an activity done'); } } return this._super.apply(this, arguments); @@ -809,8 +809,8 @@ QUnit.test('followers widget: follow/unfollow, edit subtypes', function (assert) assert.strictEqual(args.args[1].length, 3, 'should read "foo", "message_follower_ids" and "display_name"'); } else { // three next reads: only read 'message_follower_ids' field - assert.ok(_.isEqual(args.args[1], ['message_follower_ids']), - 'should only read "message_follower_ids"'); + assert.deepEqual(args.args[1], ['message_follower_ids', 'display_name'], + 'should only read "message_follower_ids" and "display_name"'); } } return this._super.apply(this, arguments); diff --git a/addons/web/static/src/js/services/data_manager.js b/addons/web/static/src/js/services/data_manager.js index 999a9d8df01..77857ef2d31 100644 --- a/addons/web/static/src/js/services/data_manager.js +++ b/addons/web/static/src/js/services/data_manager.js @@ -398,11 +398,6 @@ return core.Class.extend({ */ _processFieldsView: function (viewInfo) { var viewFields = this._processFields(viewInfo.type, viewInfo.arch, viewInfo.fields); - // by default fetch display_name and id - if (!viewInfo.fields.display_name) { - viewInfo.fields.display_name = {type: 'char'}; - viewFields.display_name = {}; - } viewInfo.fieldsInfo = {}; viewInfo.fieldsInfo[viewInfo.type] = viewFields; utils.deepFreeze(viewInfo.fields); 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 d54f609f002..aa0aab7aa64 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -1629,8 +1629,8 @@ var BasicModel = AbstractModel.extend({ */ _fetchRecord: function (record, options) { var self = this; - var fieldNames = options && options.fieldNames || - _.uniq(record.getFieldNames().concat(['display_name'])); + var fieldNames = options && options.fieldNames || record.getFieldNames(); + fieldNames = _.uniq(fieldNames.concat(['display_name'])); return this._rpc({ model: record.model, method: 'read', @@ -2337,6 +2337,16 @@ var BasicModel = AbstractModel.extend({ _generateOnChangeData: function (record) { var commands = this._generateX2ManyCommands(record, {withReadonly: true}); var data = _.extend(this.get(record.id, {raw: true}).data, commands); + // 'display_name' is automatically added to the list of fields to fetch, + // when fetching a record, even if it doesn't appear in the view. However, + // only the fields in the view must be passed to the onchange RPC, so we + // remove it from the data sent by RPC if it isn't in the view. + var hasDisplayName = _.some(record.fieldsInfo, function (fieldsInfo) { + return 'display_name' in fieldsInfo; + }); + if (!hasDisplayName) { + delete data.display_name; + } // one2many records have a parentID if (record.parentID) { diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js index a2504af38bb..92a24066408 100644 --- a/addons/web/static/tests/fields/relational_fields_tests.js +++ b/addons/web/static/tests/fields/relational_fields_tests.js @@ -2521,6 +2521,7 @@ QUnit.module('relational_fields', { arch:'
' + '' + '' + + '' + '' + '' + '
' + @@ -3499,7 +3500,7 @@ QUnit.module('relational_fields', { rpcCount++; if (args.method === 'write') { assert.deepEqual(args.args[1].p, [[0, false, { - display_name: false, int_field: 123, product_id: 41 + int_field: 123, product_id: 41 }]]); } return this._super(route, args); @@ -3736,7 +3737,7 @@ QUnit.module('relational_fields', { if (args.method === 'default_get') { var expected = counter === 0 ? [[4, 2, false]] : - [[4, 2, false], [0, false, {display_name: false, turtle_foo: 'hammer'}]]; + [[4, 2, false], [0, false, {turtle_foo: 'hammer'}]]; assert.deepEqual(args.kwargs.context.turtles, expected, "should have properly evaluated turtles key in context"); counter++; @@ -5544,6 +5545,7 @@ QUnit.module('relational_fields', { arch:'' + '' + '' + + '' + '' + '' + '
' + diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js index 15147c22d1d..6bd2b4ca7b5 100644 --- a/addons/web/static/tests/views/form_tests.js +++ b/addons/web/static/tests/views/form_tests.js @@ -1134,7 +1134,7 @@ QUnit.module('Views', { mockRPC: function (route, args) { if (args.method === "onchange") { assert.deepEqual(args.args[3], - {"foo": "1", "p": "", "p.bar": "", "p.display_name": "", "p.product_id": "", "timmy": "", "timmy.name": ""}, + {"foo": "1", "p": "", "p.bar": "", "p.product_id": "", "timmy": "", "timmy.name": ""}, "should send only the fields used in the views"); } return this._super(route, args); @@ -1282,7 +1282,6 @@ QUnit.module('Views', { if (args.method === 'onchange') { assert.deepEqual(args.args[3], { p: '', - 'p.display_name': '', 'p.foo': '1' }, "onchangeSpec should be correct (with sub fields)"); } @@ -2827,6 +2826,92 @@ QUnit.module('Views', { form.destroy(); }); + QUnit.test('display_name not sent for onchanges if not in view', function (assert) { + assert.expect(7); + + this.data.partner.records[0].timmy = [12]; + this.data.partner.onchanges = { + foo: function () {}, + }; + this.data.partner_type.onchanges = { + name: function () {}, + }; + var readInModal = false; + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '', + mockRPC: function (route, args) { + if (args.method === 'read' && args.model === 'partner') { + assert.deepEqual(args.args[1], ['foo', 'timmy', 'display_name'], + "should read display_name even if not in the view"); + } + if (args.method === 'read' && args.model === 'partner_type') { + if (!readInModal) { + assert.deepEqual(args.args[1], ['name'], + "should not read display_name for records in the list"); + } else { + assert.deepEqual(args.args[1], ['name', 'color', 'display_name'], + "should read display_name when opening the subrecord"); + } + } + if (args.method === 'onchange' && args.model === 'partner') { + assert.deepEqual(args.args[1], { + id: 1, + foo: 'coucou', + timmy: [[6, false, [12]]], + }, "should only send the value of fields in the view (+ id)"); + assert.deepEqual(args.args[3], { + foo: '1', + timmy: '', + 'timmy.name': '1', + 'timmy.color': '', + }, "only the fields in the view should be in the onchange spec"); + } + if (args.method === 'onchange' && args.model === 'partner_type') { + assert.deepEqual(args.args[1], { + id: 12, + name: 'new name', + color: 2, + }, "should only send the value of fields in the view (+ id)"); + assert.deepEqual(args.args[3], { + name: '1', + color: '', + }, "only the fields in the view should be in the onchange spec"); + } + return this._super.apply(this, arguments); + }, + res_id: 1, + viewOptions: { + mode: 'edit', + }, + }); + + // trigger the onchange + form.$('.o_field_widget[name=foo]').val("coucou").trigger('input'); + + // open a subrecord and trigger an onchange + readInModal = true; + form.$('.o_data_row .o_data_cell:first').click(); + $('.modal .o_field_widget[name=name]').val("new name").trigger('input'); + + form.destroy(); + }); + QUnit.test('onchanges on date(time) fields', function (assert) { assert.expect(6); @@ -3725,7 +3810,6 @@ QUnit.module('Views', { if (args.method === 'create') { var command = args.args[0].p; assert.deepEqual(command, [[0, false, { - display_name: false, timmy: [[6, false, [12]]], }]], "the default partner_type_id should be equal to 12"); }