From 54ea9564905f6c140b4892357657e763d21b4fa5 Mon Sep 17 00:00:00 2001 From: "Michael Mattiello (mcm)" Date: Mon, 25 Jan 2021 13:05:02 +0000 Subject: [PATCH] [FIX] web,base: save when tab/browser closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR [1] introduced the auto save feature (changes in form and list views are automatically saved when the view is left, e.g. with the pager, breadcrumbs, stat buttons, menus...). However, there was one case that had been left aside: when we close the tab or browser. This commit ensures that we also save the changes in this case. The record is saved only if it is valid, otherwise the changes are simply lost (we don't block the tab/browser from closing itself). Moreover, the beforeunload handler must be *almost* sync (a few ms setTimeout seems ok, but definitely not an rpc roundtrip). For that reason, we cannot wait for onchanges to apply before saving. part of task 2330101 [1] https://github.com/odoo/odoo/pull/60693 closes odoo/odoo#65561 X-original-commit: 9949ea14e4f27ca8c0fc84d726ad0434e2e216b8 Signed-off-by: Géry Debongnie (ged) Signed-off-by: Aaron Bohy (aab) Co-authored-by: Aaron Bohy --- .../src/js/views/basic/basic_controller.js | 95 ++++- .../static/src/js/views/basic/basic_model.js | 31 +- .../src/js/views/form/form_controller.js | 8 + .../src/js/views/list/list_controller.js | 11 + addons/web/static/tests/views/form_tests.js | 387 ++++++++++++++++++ addons/web/static/tests/views/list_tests.js | 103 +++++ .../base/static/src/js/res_config_settings.js | 7 + 7 files changed, 637 insertions(+), 5 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 0093a9e3425..55a8d978bd8 100644 --- a/addons/web/static/src/js/views/basic/basic_controller.js +++ b/addons/web/static/src/js/views/basic/basic_controller.js @@ -48,6 +48,13 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { // discard when discardChanges is called this.savingDef = Promise.resolve(); this.viewId = params.viewId; + + // In this controller, '_applyChanges' is overridden s.t. '_notifyChanges' + // of the model is called in a mutex. The following structure is used to + // accumulate change requests that haven't been sent to the model yet, + // because of the mutex. This is useful when we want to quickly save a + // record before leaving Odoo (see @_urgentSave). + this.pendingChanges = []; }, /** * @override @@ -59,6 +66,32 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { this.$el.toggleClass('o_cannot_create', !this.activeActions.create); await this._super(...arguments); }, + /** + * @override + */ + destroy: function () { + this._super(...arguments); + if (this._boundOnBeforeUnload) { + window.removeEventListener("beforeunload", this._boundOnBeforeUnload); + } + }, + /** + * @override + */ + on_attach_callback: function () { + this._super(...arguments); + this._boundOnBeforeUnload = this._onBeforeUnload.bind(this); + window.addEventListener("beforeunload", this._boundOnBeforeUnload); + }, + /** + * @override + */ + on_detach_callback: function () { + this._super(...arguments); + if (this._boundOnBeforeUnload) { + window.removeEventListener("beforeunload", this._boundOnBeforeUnload); + } + }, //-------------------------------------------------------------------------- // Public @@ -232,13 +265,19 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { }, /** * We override applyChanges (from the field manager mixin) to protect it - * with a mutex. + * with a mutex. As we do so, we need to accumulate change requests that + * haven't been sent to the model yet, just in case we would leave Odoo + * (close tab/browser). If this happens, we will bypass the mutex and notify + * the model directly of those changes, to save them if possible (see + * @_urgentSave). * * @override */ _applyChanges: function (dataPointID, changes, event) { + this.pendingChanges.push({ dataPointID, changes, event }); var _super = FieldManagerMixin._applyChanges.bind(this); - return this.mutex.exec(function () { + return this.mutex.exec(() => { + this.pendingChanges.shift(); return _super(dataPointID, changes, event); }); }, @@ -656,6 +695,48 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { }); return this.updateControlPanel(props); }, + /** + * To be called **only** when Odoo is about to be closed, and we want to + * save potential changes on a given record. + * + * We can't follow the normal flow (onchange(s) + save, mutexified), + * because the 'beforeunload' handler must be *almost* sync (< 10 ms + * setTimeout seems fine, but an rpc roundtrip is definitely too long), + * so here we bypass the standard mechanism of notifying changes and + * saving them: + * - we ask the model to bypass its mutex for upcoming 'notifyChanges' and + * 'save' requests + * - we ask all widgets to commit their changes (in case there would + * be a focused field with a fresh value) + * - we take all pendingChanges (changes that have been reported to the + * controller, but not yet sent to the model because of the mutex), + * and directly notify the model about them + * - we reset the widgets with all those changes, s.t. a further call + * to 'canBeRemoved' uses the correct data (it asks the widgets if + * they are set/valid, based on their internal state) + * - if the record is dirty, we save directly + * + * @param {string} recordID + * @private + */ + _urgentSave(recordID) { + this.model.executeDirectly(() => { + this.renderer.commitChanges(recordID); + for (const key in this.pendingChanges) { + const { changes, dataPointID, event } = this.pendingChanges[key]; + const options = { + context: event.data.context, + viewType: event.data.viewType, + notifyChange: false, + }; + this.model.notifyChanges(dataPointID, changes, options); + this._confirmChange(dataPointID, Object.keys(changes), event); + } + if (this.isDirty()) { + this._saveRecord(recordID, { reload: false, stayInEdit: true }); + } + }); + }, //-------------------------------------------------------------------------- // Handlers @@ -731,6 +812,16 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { this.trigger_up('scrollTo', { top: 0 }); } }, + /** + * Called when the user closes the tab or browser. To be overriden by + * specific controllers to execute some code (e.g. save pending changes) + * just before leaving Odoo. + * + * @abstract + * @private + */ + _onBeforeUnload: function () { + }, /** * When a reload event triggers up, we need to reload the full view. * For example, after a form view dialog saved some data. 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 8beebba7df6..d90c7ff5892 100644 --- a/addons/web/static/src/js/views/basic/basic_model.js +++ b/addons/web/static/src/js/views/basic/basic_model.js @@ -159,6 +159,7 @@ var BasicModel = AbstractModel.extend({ // sequentially, for example, an onchange needs to be completed before a // save is performed. this.mutex = new concurrency.Mutex(); + this.bypassMutex = false; // never set this to true manually (see @executeDirectly) // this array is used to accumulate RPC requests done in the same call // stack, so that they can be batched in the minimum number of RPCs @@ -458,6 +459,21 @@ var BasicModel = AbstractModel.extend({ }); }); }, + /** + * This method allows to execute a callback for which '_notifyChanges' and + * 'save' will bypass the mutex. This is useful when we are leaving Odoo + * (closing tab/browser), and we want to quickly save pending changes (in + * an 'onbeforeunload' handler, which is mostly sync). + * + * This function should never be called except when we are leaving Odoo. + * + * @param {Function} callback + */ + executeDirectly(callback) { + this.bypassMutex = true; + callback(); + this.bypassMutex = false; + }, /** * For list resources, this freezes the current records order. * @@ -936,7 +952,11 @@ var BasicModel = AbstractModel.extend({ * @returns {Promise} list of changed fields */ notifyChanges: function (record_id, changes, options) { - return this.mutex.exec(this._applyChange.bind(this, record_id, changes, options)); + const notifyChanges = () => this._applyChange(record_id, changes, options); + if (this.bypassMutex) { + return notifyChanges(); + } + return this.mutex.exec(notifyChanges); }, /** * Reload all data for a given resource. At any time there is at most one @@ -1087,7 +1107,7 @@ var BasicModel = AbstractModel.extend({ */ save: function (recordID, options) { var self = this; - return this.mutex.exec(function () { + function _save() { options = options || {}; var record = self.localData[recordID]; if (options.savePoint) { @@ -1179,7 +1199,12 @@ var BasicModel = AbstractModel.extend({ record._isDirty = false; }); return prom; - }); + } + if (this.bypassMutex) { + return _save(); + } else { + return this.mutex.exec(_save); + } }, /** * Manually sets a resource as dirty. This is used to notify that a field 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 3fbb43a3413..f9f5357ff6c 100644 --- a/addons/web/static/src/js/views/form/form_controller.js +++ b/addons/web/static/src/js/views/form/form_controller.js @@ -478,6 +478,14 @@ var FormController = BasicController.extend({ // Handlers //-------------------------------------------------------------------------- + /** + * Save the record when we are about to leave Odoo. + * + * @override + */ + _onBeforeUnload: function () { + this._urgentSave(this.handle); + }, /** * @private * @param {OdooEvent} ev 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 c806cadc735..c240ea13149 100644 --- a/addons/web/static/src/js/views/list/list_controller.js +++ b/addons/web/static/src/js/views/list/list_controller.js @@ -690,6 +690,17 @@ var ListController = BasicController.extend({ ev.data.onFail(); } }, + /** + * Save the row in edition, if any, when we are about to leave Odoo. + * + * @override + */ + _onBeforeUnload: function () { + const recordId = this.renderer.getEditableRecordID(); + if (recordId) { + this._urgentSave(recordId); + } + }, /** * Handles a click on a button by performing its action. * diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js index 570fc238f5a..852ef11dfc7 100644 --- a/addons/web/static/tests/views/form_tests.js +++ b/addons/web/static/tests/views/form_tests.js @@ -9853,6 +9853,393 @@ QUnit.module('Views', { actionManager.destroy(); }); + QUnit.test('Auto save: save on closing tab/browser', async function (assert) { + assert.expect(2); + + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: ` +
+ + + +
`, + res_id: 1, + mockRPC(route, { args, method, model }) { + if (method === 'write' && model === 'partner') { + assert.deepEqual(args, [ + [1], + { display_name: 'test' }, + ]); + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + assert.notStrictEqual(form.$('.o_field_widget[name="display_name"]').val(), 'test'); + + await testUtils.fields.editInput(form.$('.o_field_widget[name="display_name"]'), 'test'); + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + form.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (invalid field)', async function (assert) { + assert.expect(1); + + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: ` +
+ + + +
`, + res_id: 1, + mockRPC(route, { args, method, model }) { + if (method === 'write' && model === 'partner') { + assert.step('save'); // should not be called + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + await testUtils.fields.editInput(form.$('.o_field_widget[name="display_name"]'), ''); + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + assert.verifySteps([], 'should not save because of invalid field'); + + form.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (not dirty)', async function (assert) { + assert.expect(1); + + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: ` +
+ + + +
`, + res_id: 1, + mockRPC(route, { args, method, model }) { + if (method === 'write' && model === 'partner') { + assert.step('save'); // should not be called + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + assert.verifySteps([], 'should not save because we do not change anything'); + + form.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (detached form)', async function (assert) { + assert.expect(3); + + const actions = [{ + id: 1, + name: 'Partner', + res_model: 'partner', + type: 'ir.actions.act_window', + views: [[false, 'list'], [false, 'form']], + }]; + + const actionManager = await createActionManager({ + actions, + data: this.data, + archs: { + 'partner,false,list': ` + + + + `, + 'partner,false,form': ` +
+ + + +
+ `, + 'partner,false,search': '', + }, + mockRPC(route, { args, method }) { + if (method === 'write') { + assert.step('save'); + } + return this._super(...arguments); + }, + }); + await actionManager.doAction(1); + + // Click on a row to open a record + await testUtils.dom.click(actionManager.$('.o_data_row:first')); + assert.strictEqual(actionManager.$('.breadcrumb').text(), 'Partnerfirst record'); + + // Return in the list view to detach the form view + await testUtils.dom.click(actionManager.$('.o_back_button')); + assert.strictEqual(actionManager.$('.breadcrumb').text(), 'Partner'); + + // Simulate tab/browser close in the list + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + // write rpc should not trigger because form view has been detached + // and list has nothing to save + assert.verifySteps([]); + + actionManager.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (onchanges)', async function (assert) { + assert.expect(1); + + this.data.partner.onchanges = { + display_name: function (obj) { + obj.name = `copy: ${obj.display_name}`; + }, + }; + + const def = testUtils.makeTestPromise(); + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: ` +
+ + + + +
`, + res_id: 1, + mockRPC(route, { args, method, model }) { + if (method === 'onchange' && model === 'partner') { + return def; + } + if (method === 'write' && model === 'partner') { + assert.deepEqual(args, [ + [1], + { display_name: 'test' }, + ]); + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + await testUtils.fields.editInput(form.$('.o_field_widget[name="display_name"]'), 'test'); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + form.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (onchanges 2)', async function (assert) { + assert.expect(1); + + this.data.partner.onchanges = { + display_name: function () {}, + }; + + const def = testUtils.makeTestPromise(); + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: ` +
+ + + + +
`, + res_id: 1, + mockRPC(route, { args, method }) { + if (method === 'onchange') { + return def; + } + if (method === 'write') { + assert.deepEqual(args, [ + [1], + { display_name: 'test', name: 'test' }, + ]); + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + await testUtils.fields.editInput(form.$('.o_field_widget[name="display_name"]'), 'test'); + await testUtils.fields.editInput(form.$('.o_field_widget[name="name"]'), 'test'); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + form.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (pending change)', async function (assert) { + assert.expect(4); + + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + fieldDebounce: 1000, + arch: `
`, + res_id: 1, + mockRPC(route, { args, method }) { + assert.step(method); + if (method === 'write') { + assert.deepEqual(args, [[1], { foo: 'test' }]); + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + + // edit 'foo' but do not focusout -> the model isn't aware of the change + // until the 'beforeunload' event is triggered + form.$('.o_field_widget[name="foo"]').val('test'); + await testUtils.dom.triggerEvent(form.$('.o_field_widget[name="foo"]'), 'input'); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + assert.verifySteps(['read', 'write']); + + form.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (onchanges + pending change)', async function (assert) { + assert.expect(5); + + this.data.partner.onchanges = { + display_name: function (obj) { + obj.name = `copy: ${obj.display_name}`; + }, + }; + + const def = testUtils.makeTestPromise(); + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + fieldDebounce: 1000, + arch: ` +
+ + + + `, + res_id: 1, + mockRPC(route, { args, method }) { + assert.step(method); + if (method === 'onchange') { + return def; + } + if (method === 'write') { + assert.deepEqual(args, [ + [1], + { display_name: 'test', name: 'test', foo: 'test' }, + ]); + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + // edit 'display_name' and simulate a focusout (trigger the 'change' event) + // -> notifies the model of the change and performs the onchange + form.$('.o_field_widget[name="display_name"]').val('test'); + await testUtils.dom.triggerEvent(form.$('.o_field_widget[name="display_name"]'), 'change'); + + // edit 'name' and simulate a focusout (trigger the 'change' event) + // -> waits for the mutex (i.e. the onchange) to notify the model + form.$('.o_field_widget[name="name"]').val('test'); + await testUtils.dom.triggerEvent(form.$('.o_field_widget[name="name"]'), 'change'); + + // edit 'foo' but do not focusout -> the model isn't aware of the change + // until the 'beforeunload' event is triggered + form.$('.o_field_widget[name="foo"]').val('test'); + await testUtils.dom.triggerEvent(form.$('.o_field_widget[name="foo"]'), 'input'); + + // trigger the 'beforeunload' event -> notifies the model directly and saves + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + assert.verifySteps(['read', 'onchange', 'write']); + + form.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (onchanges + invalid field)', async function (assert) { + assert.expect(3); + + this.data.partner.onchanges = { + display_name: function (obj) { + obj.name = `copy: ${obj.display_name}`; + }, + }; + + const def = testUtils.makeTestPromise(); + const form = await createView({ + View: FormView, + model: 'partner', + data: this.data, + arch: ` +
+ + + + +
`, + res_id: 1, + mockRPC(route, { method }) { + assert.step(method); + if (method === 'onchange') { + return def; + } + if (method === 'write') { + throw new Error('Should not save the record'); + } + return this._super(...arguments); + }, + }); + + await testUtils.form.clickEdit(form); + await testUtils.fields.editInput(form.$('.o_field_widget[name="display_name"]'), 'test'); + await testUtils.fields.editInput(form.$('.o_field_widget[name="name"]'), ''); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + assert.verifySteps(['read', 'onchange']); + + form.destroy(); + }); + QUnit.test('Quick Edition: click on a quick editable field', async function (assert) { assert.expect(3); diff --git a/addons/web/static/tests/views/list_tests.js b/addons/web/static/tests/views/list_tests.js index b93b47e3d4f..9263c449eb8 100644 --- a/addons/web/static/tests/views/list_tests.js +++ b/addons/web/static/tests/views/list_tests.js @@ -11332,6 +11332,109 @@ QUnit.module('Views', { list.destroy(); }); + + QUnit.test("Auto save: save on closing tab/browser", async function (assert) { + assert.expect(1); + + const list = await createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: ` + + + `, + mockRPC(route, { args, method, model }) { + if (model === 'foo' && method === 'write') { + assert.deepEqual(args, [ + [1], + { foo: 'test' }, + ]); + } + return this._super(...arguments); + }, + }); + + await testUtils.dom.click(list.$('.o_field_cell[name="foo"]:first')); + await testUtils.fields.editInput(list.$('.o_field_widget[name="foo"]'), "test"); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + list.destroy(); + }); + + QUnit.test("Auto save: save on closing tab/browser (invalid field)", async function (assert) { + assert.expect(1); + + const list = await createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: ` + + + `, + mockRPC(route, { args, method, model }) { + if (model === 'foo' && method === 'write') { + assert.step('save'); // should not be called + } + return this._super(...arguments); + }, + }); + + await testUtils.dom.click(list.$('.o_field_cell[name="foo"]:first')); + await testUtils.fields.editInput(list.$('.o_field_widget[name="foo"]'), ''); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + assert.verifySteps([], 'should not save because of invalid field'); + + list.destroy(); + }); + + QUnit.test('Auto save: save on closing tab/browser (onchanges)', async function (assert) { + assert.expect(1); + + this.data.foo.onchanges = { + int_field: function (obj) { + obj.foo = `${obj.int_field}`; + }, + }; + + const def = testUtils.makeTestPromise(); + const list = await createView({ + View: ListView, + model: 'foo', + data: this.data, + arch: ` + + + + `, + mockRPC(route, { args, method, model }) { + if (model === 'foo' && method === 'onchange') { + return def; + } + if (model === 'foo' && method === 'write') { + assert.deepEqual(args, [ + [1], + { int_field: 2021 }, + ]); + } + return this._super(...arguments); + }, + }); + + await testUtils.dom.click(list.$('.o_field_cell[name="int_field"]:first')); + await testUtils.fields.editInput(list.$('.o_field_widget[name="int_field"]'), '2021'); + + window.dispatchEvent(new Event("beforeunload")); + await testUtils.nextTick(); + + list.destroy(); + }); }); }); diff --git a/odoo/addons/base/static/src/js/res_config_settings.js b/odoo/addons/base/static/src/js/res_config_settings.js index 6205ddec7a0..0dfdfea7df7 100644 --- a/odoo/addons/base/static/src/js/res_config_settings.js +++ b/odoo/addons/base/static/src/js/res_config_settings.js @@ -414,6 +414,13 @@ var BaseSettingController = FormController.extend({ this._super.apply(this, arguments); } }, + /** + * @override + * @private + */ + _onBeforeUnload: function () { + // We should not save when leaving Odoo in the settings + }, });