From ceff4d4608c1a07bf9935e5682824ef506a3e695 Mon Sep 17 00:00:00 2001 From: Adrien Dieudonne Date: Tue, 27 Jun 2017 11:13:41 +0200 Subject: [PATCH] [FIX] web: basic_controller: wait for the mutex before discard changes Before this commit, the discard confirm dialog was directly shown without waiting for the write RPC. For instance, it the user clicked on 'Save' after editing a record, and then directly clicked on another menu or on the breadcrumb before the write RPC returned, the record was still considered as dirty and thus the confirm dialog was displayed. We now wait for the RPC to return before checking if the record is dirty, so that the confirm dialog is only displayed when the record is actually dirty. --- .../src/js/views/basic/basic_controller.js | 77 +++++++++++-------- .../src/js/views/form/form_controller.js | 2 +- .../src/js/views/list/list_controller.js | 45 +++++------ addons/web/static/tests/views/form_tests.js | 61 +++++++++++++-- 4 files changed, 123 insertions(+), 62 deletions(-) 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 67a4fa504d8..82dc6f90d26 100644 --- a/addons/web/static/src/js/views/basic/basic_controller.js +++ b/addons/web/static/src/js/views/basic/basic_controller.js @@ -104,39 +104,15 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { return true; }, /** - * Discards the changes made to the record whose ID is given, if necessary. - * Automatically leaves to default mode for the given record. + * Waits for the mutex to be unlocked and then calls _.discardChanges. + * This ensures that the confirm dialog isn't displayed directly if there is + * a pending 'write' rpc. * - * @param {string} [recordID] - default to main recordID - * @param {Object} [options] - * @param {boolean} [options.readonlyIfRealDiscard=false] - * After discarding record changes, the usual option is to make the - * record readonly. However, the view manager calls this function - * at inappropriate times in the current code and in that case, we - * don't want to go back to readonly if there is nothing to discard - * (e.g. when switching record in edit mode in form view, we expect - * the new record to be in edit mode too, but the view manager calls - * this function as the URL changes...) @todo get rid of this when - * the view manager is improved. - * @returns {Deferred} + * @see _.discardChanges */ discardChanges: function (recordID, options) { - var self = this; - recordID = recordID || this.handle; - return this.canBeDiscarded(recordID).then(function (needDiscard) { - if (options && options.readonlyIfRealDiscard && !needDiscard) { - return; - } - - if (needDiscard) { // Just some optimization - self.model.discardChanges(recordID); - } - if (self.model.isNew(recordID)) { - self._abandonRecord(recordID); - return; - } - return self._confirmSave(recordID); - }); + return this.mutex.exec(function () {}) + .then(this._discardChanges.bind(this, recordID || this.handle, options)); }, /** * Method that will be overriden by the views with the ability to have selected ids @@ -326,6 +302,42 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { this.$buttons.find('button').attr('disabled', true); } }, + /** + * Discards the changes made to the record whose ID is given, if necessary. + * Automatically leaves to default mode for the given record. + * + * @private + * @param {string} [recordID] - default to main recordID + * @param {Object} [options] + * @param {boolean} [options.readonlyIfRealDiscard=false] + * After discarding record changes, the usual option is to make the + * record readonly. However, the view manager calls this function + * at inappropriate times in the current code and in that case, we + * don't want to go back to readonly if there is nothing to discard + * (e.g. when switching record in edit mode in form view, we expect + * the new record to be in edit mode too, but the view manager calls + * this function as the URL changes...) @todo get rid of this when + * the view manager is improved. + * @returns {Deferred} + */ + _discardChanges: function (recordID, options) { + var self = this; + recordID = recordID || this.handle; + return this.canBeDiscarded(recordID) + .then(function (needDiscard) { + if (options && options.readonlyIfRealDiscard && !needDiscard) { + return; + } + if (needDiscard) { // Just some optimization + self.model.discardChanges(recordID); + } + if (self.model.isNew(recordID)) { + self._abandonRecord(recordID); + return; + } + return self._confirmSave(recordID); + }); + }, /** * Enables buttons so they can be clicked again. * @@ -501,11 +513,8 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { var self = this; ev.stopPropagation(); var recordID = ev.data.recordID; - this.discardChanges(recordID) + this._discardChanges(recordID) .done(function () { - if (self.model.isNew(recordID)) { - self._abandonRecord(recordID); - } // TODO this will tell the renderer to rerender the widget that // asked for the discard but will unfortunately lose the click // made on another row if any 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 8a42168a098..2b607496e85 100644 --- a/addons/web/static/src/js/views/form/form_controller.js +++ b/addons/web/static/src/js/views/form/form_controller.js @@ -409,7 +409,7 @@ var FormController = BasicController.extend({ * @private */ _onDiscard: function () { - this.discardChanges(); + this._discardChanges(); }, /** * Called when the user clicks on 'Duplicate Record' in the sidebar 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 ee0fd323ce1..2ec9cf3181e 100644 --- a/addons/web/static/src/js/views/list/list_controller.js +++ b/addons/web/static/src/js/views/list/list_controller.js @@ -49,27 +49,6 @@ var ListController = BasicController.extend({ // Public //-------------------------------------------------------------------------- - /** - * To improve performance, list view must not be rerendered if it is asked - * to discard all its changes. Indeed, only the in-edition row needs to be - * discarded in that case. - * - * @override - * @param {string} [recordID] - default to main recordID - * @returns {Deferred} - */ - discardChanges: function (recordID) { - if ((recordID || this.handle) === this.handle) { - recordID = this.renderer.getEditableRecordID(); - if (recordID === null) { - return $.when(); - } - } - var self = this; - return this._super(recordID).then(function () { - self._updateButtons('readonly'); - }); - }, /** * Calculate the active domain of the list view. This should be done only * if the header checkbox has been checked. This is done by evaluating the @@ -245,6 +224,28 @@ var ListController = BasicController.extend({ return this.renderer.updateState(state, {noRender: true}) .then(this._setMode.bind(this, 'readonly', id)); }, + /** + * To improve performance, list view must not be rerendered if it is asked + * to discard all its changes. Indeed, only the in-edition row needs to be + * discarded in that case. + * + * @override + * @private + * @param {string} [recordID] - default to main recordID + * @returns {Deferred} + */ + _discardChanges: function (recordID) { + if ((recordID || this.handle) === this.handle) { + recordID = this.renderer.getEditableRecordID(); + if (recordID === null) { + return $.when(); + } + } + var self = this; + return this._super(recordID).then(function () { + self._updateButtons('readonly'); + }); + }, /** * @override * @private @@ -362,7 +363,7 @@ var ListController = BasicController.extend({ */ _onDiscard: function (ev) { ev.stopPropagation(); // So that it is not considered as a row leaving - this.discardChanges(); + this._discardChanges(); }, /** * Called when the user asks to edit a row -> Updates the controller buttons diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js index b15e341adb6..e9716603d60 100644 --- a/addons/web/static/tests/views/form_tests.js +++ b/addons/web/static/tests/views/form_tests.js @@ -2883,7 +2883,7 @@ QUnit.module('Views', { }); QUnit.test('onchanges that complete after discarding', function (assert) { - assert.expect(4); + assert.expect(6); var def1 = $.Deferred(); @@ -2922,14 +2922,65 @@ QUnit.module('Views', { // discard changes form.$buttons.find('.o_form_button_cancel').click(); + assert.strictEqual(form.$('.o_field_widget[name="foo"]').val(), "1234", + "field foo should still contain new value"); + assert.strictEqual($('.modal').length, 0, + "Confirm dialog should not be displayed yet"); + + // complete the onchange + def1.resolve(); + assert.strictEqual($('.modal').length, 1, + "Confirm dialog should be displayed"); $('.modal .modal-footer .btn-primary').click(); assert.strictEqual(form.$('span[name="foo"]').text(), "blip", "field foo should still be displayed to initial value"); - // complete the onchange - def1.resolve(); - assert.strictEqual(form.$('span[name="foo"]').text(), "blip", - "field foo should still be displayed to initial value"); + form.destroy(); + }); + + QUnit.test('discarding before save returns', function (assert) { + assert.expect(4); + + var def = $.Deferred(); + + var form = createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: '
' + + '' + + '
', + res_id: 2, + mockRPC: function (route, args) { + var result = this._super.apply(this, arguments); + if (args.method === 'write') { + return def.then(_.constant(result)); + } + return result; + }, + viewOptions: { + mode: 'edit', + }, + }); + + form.$('input').val("1234").trigger('input'); + + // save the value and discard directly + form.$buttons.find('.o_form_button_save').click(); + form.discardChanges(); // Simulate click on breadcrumb + + assert.strictEqual(form.$('.o_field_widget[name="foo"]').val(), "1234", + "field foo should still contain new value"); + assert.strictEqual($('.modal').length, 0, + "Confirm dialog should not be displayed"); + + // complete the write + def.resolve(); + + assert.strictEqual($('.modal').length, 0, + "Confirm dialog should not be displayed"); + assert.strictEqual(form.$('.o_field_widget[name="foo"]').text(), "1234", + "value should have been saved and rerendered in readonly"); form.destroy(); });