From 6f7f235799128dc650b3f94994aa9a1a55a1f033 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Tue, 8 Jan 2019 11:06:50 +0000 Subject: [PATCH 1/6] [FIX] web_tour: remove delay in test mode For historical reasons, the tour manager executed each step of tour in a setTimeout (10ms). When a step could be executed (i.e. when its trigger selector has a match in the DOM), the element matching its trigger was saved as a jQuery element (the $anchor), and in the setTimeout, the action (e.g. click) was performed on that element. However, it could happen that, after the delay, the $anchor was no longer in the DOM, either because 1) the tip selector had no match anymore, meaning that the element had been removed from the DOM meanwhile 2) the tip selector still had a match in the DOM, but that element had been rerendered meanwhile Case 2) occured sometimes when the main_flow_tour was executed in community, when trying to open a Manufacturing Order after having reloaded the Manufacturing Order list view (because the row of the order to open that was saved as $anchor was the one of the list before the reload, and that list was re-rendered during the delay). This rev. removes the default delay of 10ms, but the feature is kept such that one can still manually run a tour slowly (e.g. to debug or to make a demo). --- addons/web_tour/static/src/js/tour_manager.js | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/addons/web_tour/static/src/js/tour_manager.js b/addons/web_tour/static/src/js/tour_manager.js index 60b36ddc5c9..7b50c2de75a 100644 --- a/addons/web_tour/static/src/js/tour_manager.js +++ b/addons/web_tour/static/src/js/tour_manager.js @@ -32,7 +32,7 @@ return core.Class.extend(mixins.EventDispatcherMixin, ServicesMixin, { this.tours = {}; this.consumed_tours = consumed_tours || []; this.running_tour = local_storage.getItem(get_running_key()); - this.running_step_delay = parseInt(local_storage.getItem(get_running_delay_key()), 10) || 10; + this.running_step_delay = parseInt(local_storage.getItem(get_running_delay_key()), 10) || 0; this.edition = (_.last(session.server_version_info) === 'e') ? 'enterprise' : 'community'; this._log = []; console.log('Tour Manager is ready. running_tour=' + this.running_tour); @@ -366,11 +366,23 @@ return core.Class.extend(mixins.EventDispatcherMixin, ServicesMixin, { }, _to_next_running_step: function (tip, tour_name) { if (this.running_tour !== tour_name) return; + var self = this; this._stop_running_tour_timeout(); + if (this.running_step_delay) { + // warning: due to the delay, it may happen that the $anchor isn't + // in the DOM anymore when exec is called, either because: + // - it has been removed from the DOM meanwhile and the tip's + // selector doesn't match anything anymore + // - it has been re-rendered and thus the selector still has a match + // in the DOM, but executing the step with that $anchor won't work + _.delay(exec, this.running_step_delay); + } else { + exec(); + } - var action_helper = new RunningTourActionHelper(tip.widget); - _.delay((function () { - do_before_unload(this._consume_tip.bind(this, tip, tour_name)); + function exec() { + var action_helper = new RunningTourActionHelper(tip.widget); + do_before_unload(self._consume_tip.bind(self, tip, tour_name)); if (typeof tip.run === "function") { tip.run.call(tip.widget, action_helper); @@ -380,7 +392,7 @@ return core.Class.extend(mixins.EventDispatcherMixin, ServicesMixin, { } else { action_helper.auto(); } - }).bind(this), this.running_step_delay); + } }, /** From b4387108de968e3e80cd43d6df746049b0e3fbc8 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Fri, 11 Jan 2019 09:14:13 +0000 Subject: [PATCH 2/6] [FIX] web_tour: clear call stack between each step This rev. concerns automatic executions of tours. It makes each step executed in a setTimeout, thus ensuring that the call stack has been emptied before executing the next step (which is the case when the user manually executes the flow). --- addons/web_tour/static/src/js/tour_manager.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/addons/web_tour/static/src/js/tour_manager.js b/addons/web_tour/static/src/js/tour_manager.js index 7b50c2de75a..32e7af075bf 100644 --- a/addons/web_tour/static/src/js/tour_manager.js +++ b/addons/web_tour/static/src/js/tour_manager.js @@ -171,7 +171,10 @@ return core.Class.extend(mixins.EventDispatcherMixin, ServicesMixin, { if (this.running_tour && this.running_tour_timeout === undefined) { this._set_running_tour_timeout(this.running_tour, this.active_tooltips[this.running_tour]); } - this._check_for_tooltip(this.active_tooltips[tour_name], tour_name); + var self = this; + setTimeout(function () { + self._check_for_tooltip(self.active_tooltips[tour_name], tour_name); + }); } else { for (var tourName in this.active_tooltips) { var tip = this.active_tooltips[tourName]; From 141c9a0ced5280aa9d6a7068c880f1e949e52fee Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Wed, 9 Jan 2019 11:21:42 +0000 Subject: [PATCH 3/6] [FIX] web: kanban: quick create columns and records This rev. fixes a concurrency issue in the kanban view spotted by the Project tour and the removal of the delay in the tour system. Let's assume the following scenario in a kanban view grouped by a many2one field with at least one column and with the quick create feature enabled: - quick create a new column - before the column is actually created, click on CREATE in the control panel to create a new record Before this rev., the record quick create widget was opened directly, but as soon as the column was created, the view was re-rendered, and the quick create widget was lost. This rev. forces the column to be created (and the view to be re-rendered) before opening the record quick create. --- .../src/js/views/kanban/kanban_controller.js | 29 +++++---- addons/web/static/tests/views/kanban_tests.js | 61 +++++++++++++++++++ 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/addons/web/static/src/js/views/kanban/kanban_controller.js b/addons/web/static/src/js/views/kanban/kanban_controller.js index 6ffbad9612a..cf3404ae420 100644 --- a/addons/web/static/src/js/views/kanban/kanban_controller.js +++ b/addons/web/static/src/js/views/kanban/kanban_controller.js @@ -210,15 +210,17 @@ var KanbanController = BasicController.extend({ */ _onAddColumn: function (event) { var self = this; - this.model.createGroup(event.data.value, this.handle).then(function () { - var state = self.model.get(self.handle, {raw: true}); - var ids = _.pluck(state.data, 'res_id').filter(_.isNumber); - return self._resequenceColumns(ids); - }).then(function () { - return self.update({}, {reload: false}); - }).then(function () { - self._updateButtons(); - self.renderer.quickCreateToggleFold(); + this.mutex.exec(function () { + return self.model.createGroup(event.data.value, self.handle).then(function () { + var state = self.model.get(self.handle, {raw: true}); + var ids = _.pluck(state.data, 'res_id').filter(_.isNumber); + return self._resequenceColumns(ids); + }).then(function () { + return self.update({}, {reload: false}); + }).then(function () { + self._updateButtons(); + self.renderer.quickCreateToggleFold(); + }); }); }, /** @@ -271,11 +273,16 @@ var KanbanController = BasicController.extend({ * @private */ _onButtonNew: function () { + var self = this; var state = this.model.get(this.handle, {raw: true}); var quickCreateEnabled = this.quickCreateEnabled && viewUtils.isQuickCreateEnabled(state); if (this.on_create === 'quick_create' && quickCreateEnabled && state.data.length) { - // Activate the quick create in the first column - this.renderer.addQuickCreate(); + // activate the quick create in the first column when the mutex is + // unlocked, to ensure that there is no pending re-rendering that + // would remove it (e.g. if we are currently adding a new column) + this.mutex.getUnlockedDef().then(function () { + self.renderer.addQuickCreate(); + }); } else if (this.on_create && this.on_create !== 'quick_create') { // Execute the given action this.do_action(this.on_create, { diff --git a/addons/web/static/tests/views/kanban_tests.js b/addons/web/static/tests/views/kanban_tests.js index 87e4ebd2748..dbe8e748d33 100644 --- a/addons/web/static/tests/views/kanban_tests.js +++ b/addons/web/static/tests/views/kanban_tests.js @@ -2161,6 +2161,67 @@ QUnit.module('Views', { kanban.destroy(); }); + QUnit.test('quick create record while adding a new column', function (assert) { + assert.expect(10); + + var def = $.Deferred(); + var kanban = createView({ + View: KanbanView, + model: 'partner', + data: this.data, + arch: '' + + '' + + '
' + + '' + + '', + groupBy: ['product_id'], + mockRPC: function (route, args) { + var result = this._super.apply(this, arguments); + if (args.method === 'name_create' && args.model === 'product') { + return def.then(_.constant(result)); + } + return result; + }, + }); + + assert.containsN(kanban, '.o_kanban_group', 2); + assert.containsN(kanban, '.o_kanban_group:first .o_kanban_record', 2); + + // add a new column + assert.containsOnce(kanban, '.o_column_quick_create'); + assert.isNotVisible(kanban.$('.o_column_quick_create input')); + + testUtils.dom.click(kanban.$('.o_quick_create_folded')); + + assert.isVisible(kanban.$('.o_column_quick_create input')); + + testUtils.fields.editInput(kanban.$('.o_column_quick_create input'), 'new column'); + testUtils.dom.click(kanban.$('.o_column_quick_create button.o_kanban_add')); + + assert.containsN(kanban, '.o_kanban_group', 2); + + // click to add a new record + testUtils.dom.click(kanban.$buttons.find('.o-kanban-button-new')); + + // should wait for the column to be created (and view to be re-rendered + // before opening the quick create + assert.containsNone(kanban, '.o_kanban_quick_create'); + + // unlock column creation + def.resolve(); + + assert.containsN(kanban, '.o_kanban_group', 3); + assert.containsOnce(kanban, '.o_kanban_quick_create'); + + // quick create record in first column + testUtils.fields.editInput(kanban.$('.o_kanban_quick_create input'), 'new record'); + testUtils.dom.click(kanban.$('.o_kanban_quick_create .o_kanban_add')); + + assert.containsN(kanban, '.o_kanban_group:first .o_kanban_record', 3); + + kanban.destroy(); + }); + QUnit.test('many2many_tags in kanban views', function (assert) { assert.expect(12); From b3f1081082ce33afe2513c2712bd0204fc5d8a22 Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Fri, 11 Jan 2019 07:42:05 +0000 Subject: [PATCH 4/6] [FIX] web: form: call discardChanges while saving Before this rev., there was a race condition when the user did some changes on a form view (triggering an onchange), saved and clicked on the breadcrumbs to leave the form view (before the onchange returned). As a consequence, the dialog telling the user that there are pending changes was displayed. This mostly occured on the main_flow_tour since the removal of an unnecessary check, and could be reproduced manually on a slow network. Basically, the check if there were pending changes was done too early (before the view was actually saved). --- .../src/js/views/basic/basic_controller.js | 12 +++- addons/web/static/tests/views/form_tests.js | 63 +++++++++++++++++++ 2 files changed, 72 insertions(+), 3 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 76adef0a154..01bbd9cd2fe 100644 --- a/addons/web/static/src/js/views/basic/basic_controller.js +++ b/addons/web/static/src/js/views/basic/basic_controller.js @@ -38,6 +38,10 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { FieldManagerMixin.init.call(this, this.model); this.handle = params.initialState.id; this.mode = params.mode || 'readonly'; + // savingDef is used to ensure that we always wait for pending save + // operations to complete before checking if there are changes to + // discard when discardChanges is called + this.savingDef = $.when(); }, /** * @override @@ -102,14 +106,15 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { return true; }, /** - * Waits for the mutex to be unlocked and then calls _.discardChanges. + * Waits for the mutex to be unlocked and for changes to be saved, then + * calls _.discardChanges. * This ensures that the confirm dialog isn't displayed directly if there is * a pending 'write' rpc. * * @see _.discardChanges */ discardChanges: function (recordID, options) { - return this.mutex.exec(function () {}) + return $.when(this.mutex.getUnlockedDef(), this.savingDef) .then(this._discardChanges.bind(this, recordID || this.handle, options)); }, /** @@ -183,11 +188,12 @@ var BasicController = AbstractController.extend(FieldManagerMixin, { // mutex-protected as commitChanges function of x2m has to be aware of // all final changes made to a row. var self = this; - return this.mutex.getUnlockedDef().then(function () { + this.savingDef = this.mutex.getUnlockedDef().then(function () { return self.renderer.commitChanges(recordID || self.handle).then(function () { return self.mutex.exec(self._saveRecord.bind(self, recordID, options)); }); }); + return this.savingDef; }, /** * @override diff --git a/addons/web/static/tests/views/form_tests.js b/addons/web/static/tests/views/form_tests.js index 8099c58092a..e5b5f0cf36f 100644 --- a/addons/web/static/tests/views/form_tests.js +++ b/addons/web/static/tests/views/form_tests.js @@ -7254,6 +7254,69 @@ QUnit.module('Views', { form.destroy(); }); + QUnit.test('call canBeRemoved while saving', function (assert) { + assert.expect(10); + + this.data.partner.onchanges = { + foo: function (obj) { + obj.display_name = obj.foo === 'trigger onchange' ? 'changed' : 'default'; + }, + }; + + var onchangeDef; + var createDef = $.Deferred(); + 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(onchangeDef).then(_.constant(result)); + } + if (args.method === 'create') { + return $.when(createDef).then(_.constant(result)); + } + return result; + }, + }); + + // edit foo to trigger a delayed onchange + onchangeDef = $.Deferred(); + testUtils.fields.editInput(form.$('.o_field_widget[name=foo]'), 'trigger onchange'); + + assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'default'); + + // save (will wait for the onchange to return), and will be delayed as well + testUtils.dom.click(form.$buttons.find('.o_form_button_save')); + + assert.hasClass(form.$('.o_form_view'), 'o_form_editable'); + assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'default'); + + // simulate a click on the breadcrumbs to leave the form view + form.canBeRemoved(); + + assert.hasClass(form.$('.o_form_view'), 'o_form_editable'); + assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'default'); + + // unlock the onchange + onchangeDef.resolve(); + + assert.hasClass(form.$('.o_form_view'), 'o_form_editable'); + assert.strictEqual(form.$('.o_field_widget[name=display_name]').val(), 'changed'); + + // unlock the create + createDef.resolve(); + + assert.hasClass(form.$('.o_form_view'), 'o_form_readonly'); + assert.strictEqual(form.$('.o_field_widget[name=display_name]').text(), 'changed'); + assert.containsNone(document.body, '.modal', + "should not display the 'Changes will be discarded' dialog"); + + form.destroy(); + }); + QUnit.module('FormViewTABMainButtons'); QUnit.test('using tab in an empty required string field should not move to the next field',function(assert) { From 18cb7192b78cca03bf2a0e5d410106c5d502329b Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Wed, 9 Jan 2019 11:46:48 +0000 Subject: [PATCH 5/6] [FIX] test_main_flows: remove patches due to tour manager delay Since the removal of the delay in the tour system, some patches that had been done to make the tour pass (all the time) can be removed. --- .../test_main_flows/static/src/js/tour.js | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/odoo/addons/test_main_flows/static/src/js/tour.js b/odoo/addons/test_main_flows/static/src/js/tour.js index 047b0c40543..c49ae1cb4eb 100644 --- a/odoo/addons/test_main_flows/static/src/js/tour.js +++ b/odoo/addons/test_main_flows/static/src/js/tour.js @@ -211,7 +211,6 @@ tour.register('main_flow_tour', { position: 'bottom', }, { trigger: ".breadcrumb-item:first", - extra_trigger: ".o_form_readonly", // FIXME: this is required due to an issue in tour_manager (see [*]) content: _t("Use the breadcrumbs to go back to products."), position: "bottom" }, { @@ -542,12 +541,10 @@ tour.register('main_flow_tour', { trigger: ".o_menu_sections a[data-menu-xmlid='mrp.menu_mrp_manufacturing']", content: _t('Click on Operations menuitem'), position: 'bottom', - edition: 'enterprise', }, { trigger: ".o_menu_sections a[data-menu-xmlid='mrp.menu_mrp_production_action']", content: _t('Open manufacturing orders'), position: 'bottom', - edition: 'enterprise', }, { trigger: '.o_data_row:has(.o_data_cell:contains("the_flow.product")):first', content: _t('Select the generated manufacturing order'), @@ -755,7 +752,6 @@ tour.register('main_flow_tour', { }, { edition: "enterprise", trigger: ".o_selected_row .o_field_widget[name=name]", - extra_trigger: ".o_selected_row .o_field_widget[name=partner_id] .o_external_button", // FIXME: this is required due to an issue in tour_manager (see [*]) content: _t('Let\'s enter a name.'), position: "bottom", run: "text the_flow.statement.line", @@ -781,16 +777,3 @@ tour.register('main_flow_tour', { position: "bottom", }]); }); - -/* - * [*] FIXME: issue in tour_manager: - * The JQuery element of a step is registered as soon as its trigger and - * extra_trigger elements are visible in the DOM, but it's action is delayed - * to handle the 'running_step_delay' option (0 by default, but even with 0, - * the action is executed after finishing the current execution block, which - * may produce a re-rendering of the part of the DOM containing the element). - * When this happens, the action is executed on an jquery element not present - * in the DOM anymore. - * To avoid this, we add an 'extra_trigger' to wait for the updated jquery - * element before activating the step (and thus registering the JQuery element). - */ From 070bd40f64da3b16fbfce42de867aaf9e149d64a Mon Sep 17 00:00:00 2001 From: Aaron Bohy Date: Thu, 10 Jan 2019 09:35:46 +0000 Subject: [PATCH 6/6] [FIX] website_sale(_wishlist): adapt tours to delay removal After the delay removal, those two tours failed for the same reason. They made a research on the /shop page with a specific product name, and then opened that product. With the delay, the page was reloaded with the new search, and we clicked on the product after the reload. Without the delay, we directly click on the product (which is in both case already displayed even before searching), and then the page is reloaded, so the product is never opened and the tours fail. This rev. adapts those tours to either wait for the page to be reloaded before clicking on the product, or simply remove the searching step when the requested product is actually the first one. --- .../static/src/js/website_sale_tour_buy.js | 2 +- .../static/src/js/website_sale_tour_wishlist.js | 11 +++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/addons/website_sale/static/src/js/website_sale_tour_buy.js b/addons/website_sale/static/src/js/website_sale_tour_buy.js index 1797a59ea9b..87ae9564b65 100644 --- a/addons/website_sale/static/src/js/website_sale_tour_buy.js +++ b/addons/website_sale/static/src/js/website_sale_tour_buy.js @@ -21,7 +21,7 @@ tour.register('shop_buy_product', { }, { content: "select conference chair", - trigger: '.oe_product_cart a:contains("Conference Chair")', + trigger: '.oe_product_cart:first a:contains("Conference Chair")', }, { content: "select Conference Chair Aluminium", diff --git a/addons/website_sale_wishlist/static/src/js/website_sale_tour_wishlist.js b/addons/website_sale_wishlist/static/src/js/website_sale_tour_wishlist.js index b03e0108fb2..8089d8fb3e7 100644 --- a/addons/website_sale_wishlist/static/src/js/website_sale_tour_wishlist.js +++ b/addons/website_sale_wishlist/static/src/js/website_sale_tour_wishlist.js @@ -47,14 +47,9 @@ tour.register('shop_wishlist', { }, }, { - content: "check that logged in and search for Customizable Desk", - extra_trigger: "li span:contains('Mitchell Admin')", - trigger: 'form input[name="search"]', - run: "text Customizable Desk", - }, - { - content: "submit search", - trigger: 'form:has(input[name="search"]) .oe_search_button', + content: "check that logged in", + trigger: "li span:contains('Mitchell Admin')", + run: function () {}, }, { content: "click on Customizable Desk",