From f9a1241721d1457dd7ddd0eebb1ec0ba4fba3225 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Wed, 31 May 2017 08:55:24 +0200 Subject: [PATCH] [FIX] web: editable list: don't create invalid records Clicking multiple times on 'Create' (in main list views) or 'Add an item' (in o2m lists) might create invalid rows (i.e. rows with required fields unset). Creating a new record requires two sequential RPCs: a default_get and an onchange (only if there is an onchange of one of the record's field). When clicking twice to create a new record, that sequence of RPCs is done twice in parallel, after unselecting the current row (i.e. saving it if it is valid). However, in both cases, there is no line to unselect yet (as both operations are done basically simultaneously). When the first onchange returns, a first new record is added to the list. When the second returns, the second one is added and takes the edition, leaving the first one, invalid, in the list. This rev. ensures this doesn't happen by preventing concurrent record creation. --- .../static/src/js/fields/relational_fields.js | 22 ++++- .../src/js/views/basic/basic_controller.js | 20 +++++ .../src/js/views/form/form_controller.js | 46 +++++----- .../src/js/views/list/list_controller.js | 12 ++- .../js/views/list/list_editable_renderer.js | 90 +++++++++---------- .../tests/fields/relational_fields_tests.js | 45 ++++++++++ addons/web/static/tests/views/list_tests.js | 37 ++++++++ 7 files changed, 196 insertions(+), 76 deletions(-) diff --git a/addons/web/static/src/js/fields/relational_fields.js b/addons/web/static/src/js/fields/relational_fields.js index f223fca68a4..a5f9b443ae0 100644 --- a/addons/web/static/src/js/fields/relational_fields.js +++ b/addons/web/static/src/js/fields/relational_fields.js @@ -914,6 +914,16 @@ var FieldOne2Many = FieldX2Many.extend({ className: 'o_field_one2many', supportedFieldTypes: ['one2many'], + /** + * @override + */ + init: function () { + this._super.apply(this, arguments); + + // boolean used to prevent concurrent record creation + this.creatingRecord = false; + }, + //-------------------------------------------------------------------------- // Public //-------------------------------------------------------------------------- @@ -931,6 +941,7 @@ var FieldOne2Many = FieldX2Many.extend({ var index = self.editable === 'top' ? 0 : self.value.data.length - 1; var newID = self.value.data[index].id; self.renderer.editRecord(newID); + self.creatingRecord = false; } } }); @@ -984,10 +995,13 @@ var FieldOne2Many = FieldX2Many.extend({ ev.stopPropagation(); if (this.editable) { - this._setValue({ - operation: 'CREATE', - position: this.editable, - }); + if (!this.creatingRecord) { + this.creatingRecord = true; + this._setValue({ + operation: 'CREATE', + position: this.editable, + }); + } } else { var self = this; this._openFormDialog({ diff --git a/addons/web/static/src/js/views/basic/basic_controller.js b/addons/web/static/src/js/views/basic/basic_controller.js index 9f767799851..53e5be8ff6e 100644 --- a/addons/web/static/src/js/views/basic/basic_controller.js +++ b/addons/web/static/src/js/views/basic/basic_controller.js @@ -309,6 +309,26 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { doIt(); } }, + /** + * Disables buttons so that they can't be clicked anymore. + * + * @private + */ + _disableButtons: function () { + if (this.$buttons) { + this.$buttons.find('button').attr('disabled', true); + } + }, + /** + * Enables buttons so they can be clicked again. + * + * @private + */ + _enableButtons: function () { + if (this.$buttons) { + this.$buttons.find('button').removeAttr('disabled'); + } + }, /** * Returns the new sidebar env * diff --git a/addons/web/static/src/js/views/form/form_controller.js b/addons/web/static/src/js/views/form/form_controller.js index 8ae66848b75..6adb474303d 100644 --- a/addons/web/static/src/js/views/form/form_controller.js +++ b/addons/web/static/src/js/views/form/form_controller.js @@ -72,26 +72,6 @@ var FormController = BasicController.extend({ return self._setMode('edit'); }); }, - /** - * Disable buttons so that they can't be clicked anymore - * - */ - disableButtons: function () { - if (this.$buttons) { - this.$buttons.find('button').attr('disabled', true); - } - this.renderer.disableButtons(); - }, - /** - * Enable buttons so they can be clicked again - * - */ - enableButtons: function () { - if (this.$buttons) { - this.$buttons.find('button').removeAttr('disabled'); - } - this.renderer.enableButtons(); - }, /** * Returns the current res_id, wrapped in a list. This is only used by the * sidebar (and the debugmanager) @@ -252,6 +232,26 @@ var FormController = BasicController.extend({ return this.renderer.confirmChange(record, record.id, [fieldsChanged]); } }, + /** + * Override to disable buttons in the renderer. + * + * @override + * @private + */ + _disableButtons: function () { + this._super.apply(this, arguments); + this.renderer.disableButtons(); + }, + /** + * Override to enable buttons in the renderer. + * + * @override + * @private + */ + _enableButtons: function () { + this._super.apply(this, arguments); + this.renderer.enableButtons(); + }, /** * Hook method, called when record(s) has been deleted. * @@ -347,7 +347,7 @@ var FormController = BasicController.extend({ var self = this; var def; - this.disableButtons(); + this._disableButtons(); var attrs = event.data.attrs; if (attrs.confirm) { @@ -380,9 +380,7 @@ var FormController = BasicController.extend({ }); } - def.always(function() { - self.enableButtons(); - }); + def.always(this._enableButtons.bind(this)); }, /** * Called when the user wants to create a new record -> @see createRecord diff --git a/addons/web/static/src/js/views/list/list_controller.js b/addons/web/static/src/js/views/list/list_controller.js index 3c86b7c222f..7c83022cd60 100644 --- a/addons/web/static/src/js/views/list/list_controller.js +++ b/addons/web/static/src/js/views/list/list_controller.js @@ -194,19 +194,25 @@ var ListController = BasicController.extend({ } }, /** - * Add a record to the list + * Adds a record to the list. + * Disables the buttons to prevent concurrent record creation or edition. * * @todo make record creation a basic controller feature * @private */ _addRecord: function () { var self = this; - this.model.addDefaultRecord(this.handle, {position: this.editable}).then(function (recordID) { + this._disableButtons(); + return this.renderer.unselectRow().then(function () { + return self.model.addDefaultRecord(self.handle, { + position: self.editable, + }); + }).then(function (recordID) { self._toggleNoContentHelper(false); var state = self.model.get(self.handle); self.renderer.updateState(state, {}); self.renderer.editRecord(recordID); - }); + }).always(this._enableButtons.bind(this)); }, /** * Archive the current selection diff --git a/addons/web/static/src/js/views/list/list_editable_renderer.js b/addons/web/static/src/js/views/list/list_editable_renderer.js index a65d4f685b7..a037c12a96e 100644 --- a/addons/web/static/src/js/views/list/list_editable_renderer.js +++ b/addons/web/static/src/js/views/list/list_editable_renderer.js @@ -224,6 +224,44 @@ ListRenderer.include({ return $.when.apply($, defs); }, + /** + * This method is called whenever we click/move outside of a row that was + * in edit mode. This is the moment we save all accumulated changes on that + * row, if needed (@see BasicController.saveRecord). + * + * Note that we have to disable the focusable elements (inputs, ...) to + * prevent subsequent editions. These edits would be lost, because the list + * view only saves records when unselecting a row. + * + * @returns {Deferred} The deferred resolves if the row was unselected (and + * possibly removed). If may be rejected, when the row is dirty and the + * user refuses to discard its changes. + */ + unselectRow: function () { + // Protect against calling this method when no row is selected + if (this.currentRow === null) { + return $.when(); + } + + var record = this.state.data[this.currentRow]; + var recordWidgets = this.allFieldWidgets[record.id]; + toggleWidgets(true); + + var def = $.Deferred(); + this.trigger_up('save_line', { + recordID: record.id, + onSuccess: def.resolve.bind(def), + onFailure: def.reject.bind(def), + }); + return def.fail(toggleWidgets.bind(null, false)); + + function toggleWidgets(disabled) { + _.each(recordWidgets, function (widget) { + var $el = widget.getFocusableElement(); + $el.prop('disabled', disabled); + }); + } + }, //-------------------------------------------------------------------------- // Private @@ -263,7 +301,7 @@ ListRenderer.include({ if (this.currentRow > 0) { this._selectCell(this.currentRow - 1, this.columns.length - 1); } else { - this._unselectRow().then(this.trigger_up.bind(this, 'add_record')); + this.unselectRow().then(this.trigger_up.bind(this, 'add_record')); } }, /** @@ -282,7 +320,7 @@ ListRenderer.include({ if (this.currentRow < this.state.data.length - 1) { this._selectCell(this.currentRow + 1, 0); } else { - this._unselectRow().then(this.trigger_up.bind(this, 'add_record')); + this.unselectRow().then(this.trigger_up.bind(this, 'add_record')); } }, /** @@ -427,7 +465,7 @@ ListRenderer.include({ // To select a row, the currently selected one must be unselected first var self = this; - return this._unselectRow().then(function () { + return this.unselectRow().then(function () { // Notify the controller we want to make a record editable var record = self.state.data[rowIndex]; var def = $.Deferred(); @@ -438,44 +476,6 @@ ListRenderer.include({ return def; }); }, - /** - * This method is called whenever we click/move outside of a row that was - * in edit mode. This is the moment we save all accumulated changes on that - * row, if needed (@see BasicController.saveRecord). - * - * Note that we have to disable the focusable elements (inputs, ...) to - * prevent subsequent editions. These edits would be lost, because the list - * view only saves records when unselecting a row. - * - * @returns {Deferred} The deferred resolves if the row was unselected (and - * possibly removed). If may be rejected, when the row is dirty and the - * user refuses to discard its changes. - */ - _unselectRow: function () { - // Protect against calling this method when no row is selected - if (this.currentRow === null) { - return $.when(); - } - - var record = this.state.data[this.currentRow]; - var recordWidgets = this.allFieldWidgets[record.id]; - toggleWidgets(true); - - var def = $.Deferred(); - this.trigger_up('save_line', { - recordID: record.id, - onSuccess: def.resolve.bind(def), - onFailure: def.reject.bind(def), - }); - return def.fail(toggleWidgets.bind(null, false)); - - function toggleWidgets(disabled) { - _.each(recordWidgets, function (widget) { - var $el = widget.getFocusableElement(); - $el.prop('disabled', disabled); - }); - } - }, //-------------------------------------------------------------------------- // Handlers @@ -497,7 +497,7 @@ ListRenderer.include({ // but we do want to unselect current row var self = this; - this._unselectRow().then(function () { + this.unselectRow().then(function () { self.trigger_up('add_record'); // TODO write a test, the deferred was not considered }); }, @@ -523,7 +523,7 @@ ListRenderer.include({ * We need to manually unselect row, because noone else would do it */ _onEmptyRowClick: function () { - this._unselectRow(); + this.unselectRow(); }, /** * Clicking on a footer should unselect (and save) the currently selected @@ -531,7 +531,7 @@ ListRenderer.include({ * and _onWindowClicked ignore those clicks. */ _onFooterClick: function () { - this._unselectRow(); + this.unselectRow(); }, /** * Handles the keyboard navigation according to events triggered by field @@ -677,7 +677,7 @@ ListRenderer.include({ return; } - this._unselectRow(); + this.unselectRow(); }, }); diff --git a/addons/web/static/tests/fields/relational_fields_tests.js b/addons/web/static/tests/fields/relational_fields_tests.js index 916325b64b7..73aa28730e2 100644 --- a/addons/web/static/tests/fields/relational_fields_tests.js +++ b/addons/web/static/tests/fields/relational_fields_tests.js @@ -3699,6 +3699,51 @@ QUnit.module('relational_fields', { form.destroy(); }); + QUnit.test('editable list: multiple clicks on Add an item do not create invalid rows', function (assert) { + assert.expect(3); + + this.data.turtle.onchanges = { + turtle_trululu: function () {}, + }; + + var def; + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: '
' + + '' + + '' + + '' + + '' + + '' + + '
', + mockRPC: function (route, args) { + var result = this._super.apply(this, arguments); + if (args.method === 'onchange') { + return $.when(def).then(_.constant(result)); + } + return result; + }, + }); + + // click twice to add a new line + def = $.Deferred(); + form.$('.o_field_x2many_list_row_add a').click(); + form.$('.o_field_x2many_list_row_add a').click(); + assert.strictEqual(form.$('.o_data_row').length, 0, + "no row should have been created yet (waiting for the onchange)"); + + // resolve the onchange def + def.resolve(); + assert.strictEqual(form.$('.o_data_row').length, 1, + "only one row should have been created"); + assert.ok(form.$('.o_data_row:first').hasClass('o_selected_row'), + "the created row should be in edition"); + + form.destroy(); + }); + QUnit.module('FieldMany2Many'); QUnit.test('many2many kanban: edition', function (assert) { diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js index 601e7a23533..0418158465b 100644 --- a/addons/web/static/tests/views/list_tests.js +++ b/addons/web/static/tests/views/list_tests.js @@ -2262,6 +2262,43 @@ QUnit.module('Views', { list.destroy(); }); + QUnit.test('multiple clicks on Add do not create invalid rows', function (assert) { + assert.expect(2); + + this.data.foo.onchanges = { + m2o: function () {}, + }; + + var def = $.Deferred(); + var list = createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: '', + mockRPC: function (route, args) { + var result = this._super.apply(this, arguments); + if (args.method === 'onchange') { + return $.when(def).then(_.constant(result)); + } + return result; + }, + }); + + assert.strictEqual(list.$('.o_data_row').length, 4, + "should contain 4 records"); + + // click on Add twice, and delay the onchange + list.$buttons.find('.o_list_button_add').click(); + list.$buttons.find('.o_list_button_add').click(); + + def.resolve(); + + assert.strictEqual(list.$('.o_data_row').length, 5, + "only one record should have been created"); + + list.destroy(); + }); + }); });