From 8ada65ee2265a9b7e499f02a98b79b3af82f0883 Mon Sep 17 00:00:00 2001 From: Joseph Caburnay Date: Mon, 24 Aug 2020 16:16:44 +0000 Subject: [PATCH] [FIX] point_of_sale: random runbot errors due to NumberBuffer abstraction The new randomly occurring runbot errors in pos/* can be reproduced by performing quick successive clicks in numpad widget with the following form: 1. Press 'Price' button. 2. A number input. 3. Press 'Qty' button. 4. A number input. The goal of the above steps is to set the price of the selected orderline followed by setting the quantity. The result however is that the price isn't changed but the quantity is changed to combined digits of the price and quantity inputs. This is due to the implementation of the `NumberBuffer` singleton which is used to abstract and take into account different sources of user inputs -- via numpad widget, keyboard and barcode scanner. Barcode scanner performs fast successive keyboard presses. To differentiate it from normal keyboard input, we used `eventsBuffer` field of `NumberBuffer` and store keyboard events there. Fast keyboard presses that stores more than 2 in the events are neglected by the buffer and is considered a barcode input. In this commit, we save the `handler` that handles the keyboard events saved in the `eventsBuffer`, then instead of only letting it to be invoked by the setTimeout object, we allow the manual invoking of `handler` using the introduced `capture` method. This method is called when the numpad mode is changed (pressing Price, Discount or Qty button). Additionally, we remove the use of 'useState' to prevent unnecessary rerender of the component that uses the `NumberBuffer`. closes odoo/odoo#56493 X-original-commit: a48d5a3599bff8f9f1446a5f7ad0ac9be0723478 Signed-off-by: pimodoo Signed-off-by: Joseph Caburnay (jcb) --- .../static/src/js/Misc/NumberBuffer.js | 26 +++++++- .../js/Screens/ProductScreen/ProductScreen.js | 5 +- .../static/tests/tours/ProductScreen.tour.js | 4 +- .../tours/helpers/ProductScreenTourMethods.js | 1 - .../static/tests/unit/test_NumberBuffer.js | 65 +++++++++++++++++++ .../point_of_sale/views/pos_assets_qunit.xml | 1 + 6 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 addons/point_of_sale/static/tests/unit/test_NumberBuffer.js diff --git a/addons/point_of_sale/static/src/js/Misc/NumberBuffer.js b/addons/point_of_sale/static/src/js/Misc/NumberBuffer.js index 3a358670af5..e43d904a68f 100644 --- a/addons/point_of_sale/static/src/js/Misc/NumberBuffer.js +++ b/addons/point_of_sale/static/src/js/Misc/NumberBuffer.js @@ -1,7 +1,7 @@ odoo.define('point_of_sale.NumberBuffer', function(require) { 'use strict'; - const { Component, useState } = owl; + const { Component } = owl; const { EventBus } = owl.core; const { onMounted, onWillUnmount, useExternalListener } = owl.hooks; const { useListener } = require('web.custom_hooks'); @@ -48,15 +48,17 @@ odoo.define('point_of_sale.NumberBuffer', function(require) { * - set(val) * - reset() * - getFloat() + * - capture() * * Note * ==== - * - No need to instantiate as it is a single created before exporting in this module. + * - No need to instantiate as it is a singleton created before exporting in this module. * * Possible Improvements * ===================== * - Relieve the buffer from responsibility of handling `Enter` and other control keys. * - Make the constants (ALLOWED_KEYS, etc.) more configurable. + * - Write more integration tests. NumberPopup can be used as test component. */ class NumberBuffer extends EventBus { constructor() { @@ -88,6 +90,19 @@ odoo.define('point_of_sale.NumberBuffer', function(require) { this.state.buffer = ''; this.trigger('buffer-update', this.state.buffer); } + /** + * Calling this function, we immediately invoke the `handler` method + * that handles the contents of the input events buffer (`eventsBuffer`). + * This is helpful when we don't want to wait for the timeout that + * is supposed to invoke the handler. + */ + capture() { + if (this.handler) { + clearTimeout(this._timeout); + this.handler(); + delete this.handler; + } + } /** * @returns {number} float equivalent of the value of buffer */ @@ -107,11 +122,15 @@ odoo.define('point_of_sale.NumberBuffer', function(require) { * @param {Object} config Use to setup the buffer * @param {String|null} config.decimalPoint The decimal character. * @param {String|null} config.triggerAtEnter Event triggered when 'Enter' key is pressed. + * @param {String|null} config.triggerAtEsc Event triggered when 'Esc' key is pressed. * @param {String|null} config.triggerAtInput Event triggered for every accepted input. * @param {String|null} config.nonKeyboardInputEvent Also listen to a non-keyboard input event * that carries a payload of { key }. The key is checked if it is a valid input. If valid, * the number buffer is modified just as it is modified when a keyboard key is pressed. * @param {Boolean} config.useWithBarcode Whether this buffer is used with barcode. + * @emits config.triggerAtEnter when 'Enter' key is pressed. + * @emits config.triggerAtEsc when 'Esc' key is pressed. + * @emits config.triggerAtInput when an input is accepted. */ use(config) { this.eventsBuffer = []; @@ -120,7 +139,7 @@ odoo.define('point_of_sale.NumberBuffer', function(require) { onMounted(() => { this.bufferHolderStack.push({ component: currentComponent, - state: config.state ? config.state : useState({ buffer: '' }), + state: config.state ? config.state : { buffer: '' }, config, }); this._setUp(); @@ -160,6 +179,7 @@ odoo.define('point_of_sale.NumberBuffer', function(require) { clearTimeout(this._timeout); this.eventsBuffer.push(event); this._timeout = setTimeout(handler, this.maxTimeBetweenKeys); + this.handler = handler }; } _onInput(keyAccessor) { diff --git a/addons/point_of_sale/static/src/js/Screens/ProductScreen/ProductScreen.js b/addons/point_of_sale/static/src/js/Screens/ProductScreen/ProductScreen.js index f34988065e1..1d83146d557 100644 --- a/addons/point_of_sale/static/src/js/Screens/ProductScreen/ProductScreen.js +++ b/addons/point_of_sale/static/src/js/Screens/ProductScreen/ProductScreen.js @@ -150,10 +150,11 @@ odoo.define('point_of_sale.ProductScreen', function(require) { NumberBuffer.reset(); } - async _setNumpadMode(event) { + _setNumpadMode(event) { const { mode } = event.detail; - this.state.numpadMode = mode; + NumberBuffer.capture(); NumberBuffer.reset(); + this.state.numpadMode = mode; } async _updateSelectedOrderline(event) { if(this.state.numpadMode === 'quantity' && this.env.pos.disallowLineQuantityChange()) { diff --git a/addons/point_of_sale/static/tests/tours/ProductScreen.tour.js b/addons/point_of_sale/static/tests/tours/ProductScreen.tour.js index ae2b9261c5e..dc76cd82307 100644 --- a/addons/point_of_sale/static/tests/tours/ProductScreen.tour.js +++ b/addons/point_of_sale/static/tests/tours/ProductScreen.tour.js @@ -40,9 +40,7 @@ odoo.define('point_of_sale.tour.ProductScreen', function (require) { ProductScreen.check.selectedOrderlineHas('Desk Organizer', '12.0', '61.2'); ProductScreen.do.pressNumpad('3'); ProductScreen.check.selectedOrderlineHas('Desk Organizer', '123.0', '627.3'); - ProductScreen.do.pressNumpad('.'); - ProductScreen.check.selectedOrderlineHas('Desk Organizer', '123.0', '627.3'); - ProductScreen.do.pressNumpad('5 0'); + ProductScreen.do.pressNumpad('. 5'); ProductScreen.check.selectedOrderlineHas('Desk Organizer', '123.5', '629.85'); ProductScreen.do.pressNumpad('Price'); ProductScreen.do.pressNumpad('1'); diff --git a/addons/point_of_sale/static/tests/tours/helpers/ProductScreenTourMethods.js b/addons/point_of_sale/static/tests/tours/helpers/ProductScreenTourMethods.js index 464f8c1c043..69aab18bb6b 100644 --- a/addons/point_of_sale/static/tests/tours/helpers/ProductScreenTourMethods.js +++ b/addons/point_of_sale/static/tests/tours/helpers/ProductScreenTourMethods.js @@ -224,7 +224,6 @@ odoo.define('point_of_sale.tour.ProductScreenTourMethods', function (require) { res.push(...this._do.pressNumpad('Price')); res.push(...this._check.modeIsActive('Price')); res.push(...this._do.pressNumpad(unitPrice.toString().split('').join(' '))); - res.push(...this._check.selectedOrderlineHas(productName, '1.0', unitPrice)); res.push(...this._do.pressNumpad('Qty')); res.push(...this._check.modeIsActive('Qty')); } diff --git a/addons/point_of_sale/static/tests/unit/test_NumberBuffer.js b/addons/point_of_sale/static/tests/unit/test_NumberBuffer.js new file mode 100644 index 00000000000..1e9da1e6b02 --- /dev/null +++ b/addons/point_of_sale/static/tests/unit/test_NumberBuffer.js @@ -0,0 +1,65 @@ +odoo.define('point_of_sale.tests.NumberBuffer', function(require) { + 'use strict'; + + const { Component, useState } = owl; + const { xml } = owl.tags; + const NumberBuffer = require('point_of_sale.NumberBuffer'); + const makeTestEnvironment = require('web.test_env'); + const testUtils = require('web.test_utils'); + + QUnit.module('unit tests for NumberBuffer', { + before() {}, + }); + + QUnit.test('simple fast inputs with capture in between', async function(assert) { + assert.expect(3); + + class Root extends Component { + constructor() { + super(); + this.state = useState({ buffer: '' }); + NumberBuffer.activate(); + NumberBuffer.use({ + nonKeyboardInputEvent: 'numpad-click-input', + state: this.state, + }); + } + resetBuffer() { + NumberBuffer.capture(); + NumberBuffer.reset(); + } + } + Root.env = makeTestEnvironment(); + Root.template = xml/* html */ ` +
+

+ + + +
+ `; + + const root = new Root(); + await root.mount(testUtils.prepareTarget()); + + const oneButton = root.el.querySelector('button.one'); + const twoButton = root.el.querySelector('button.two'); + const resetButton = root.el.querySelector('button.reset'); + const bufferEl = root.el.querySelector('p'); + + testUtils.dom.click(oneButton); + testUtils.dom.click(twoButton); + await testUtils.nextTick(); + assert.strictEqual(bufferEl.textContent, '12'); + testUtils.dom.click(resetButton); + await testUtils.nextTick(); + assert.strictEqual(bufferEl.textContent, ''); + testUtils.dom.click(twoButton); + testUtils.dom.click(oneButton); + await testUtils.nextTick(); + assert.strictEqual(bufferEl.textContent, '21'); + + root.unmount(); + root.destroy(); + }); +}); diff --git a/addons/point_of_sale/views/pos_assets_qunit.xml b/addons/point_of_sale/views/pos_assets_qunit.xml index 41c0a6a1166..e58b3059460 100644 --- a/addons/point_of_sale/views/pos_assets_qunit.xml +++ b/addons/point_of_sale/views/pos_assets_qunit.xml @@ -50,6 +50,7 @@