From 26fd276c45bac192e7d814fb0055bd2eb1920f56 Mon Sep 17 00:00:00 2001 From: Joseph Caburnay Date: Wed, 11 Aug 2021 14:51:13 +0000 Subject: [PATCH] [FIX] point_of_sale: properly handler errors after wowl refactoring * Refactor the use of Mutex. We remove the unnecessary resolves and rejects. The excess rejects trigger the unhandledrejection event handler. * Make sure that the validation of order is not broken when the error is about connections (aborted or lost). * In Chrome, we introduce a set of methods to handle "unexpected errors". Prior to wowl, POS just augments the CrashManager to show the right popup. But since CrashManager is no longer supported, we have to introduce a way to handle those unhandled errors. closes odoo/odoo#73725 Signed-off-by: pimodoo --- addons/point_of_sale/static/src/js/Chrome.js | 89 ++++++++++--- .../js/Screens/PaymentScreen/PaymentScreen.js | 31 ++++- .../static/src/js/chrome_adapter.js | 11 ++ addons/point_of_sale/static/src/js/models.js | 126 +++++++----------- addons/point_of_sale/static/src/js/utils.js | 22 ++- addons/pos_coupon/static/src/js/coupon.js | 2 +- 6 files changed, 171 insertions(+), 110 deletions(-) diff --git a/addons/point_of_sale/static/src/js/Chrome.js b/addons/point_of_sale/static/src/js/Chrome.js index b65f6b50eda..0082a53c712 100644 --- a/addons/point_of_sale/static/src/js/Chrome.js +++ b/addons/point_of_sale/static/src/js/Chrome.js @@ -12,7 +12,9 @@ odoo.define('point_of_sale.Chrome', function(require) { const Registries = require('point_of_sale.Registries'); const IndependentToOrderScreen = require('point_of_sale.IndependentToOrderScreen'); const contexts = require('point_of_sale.PosContext'); - const { registry } = require("@web/core/registry"); + const { identifyError } = require('point_of_sale.utils'); + const { odooExceptionTitleMap } = require("@web/core/errors/error_dialogs"); + const { ConnectionLostError, ConnectionAbortedError, RPCError } = require('@web/core/network/rpc_service'); // This is kind of a trick. // We get a reference to the whole exports so that @@ -20,15 +22,6 @@ odoo.define('point_of_sale.Chrome', function(require) { // we instantiate the extended one. const models = require('point_of_sale.models'); - let chromeInstance; - function posErrorHandler() { - if (chromeInstance) { - return chromeInstance.handleError(...arguments); - } - return false; - } - registry.category("error_handlers").add("posErrorHandler", posErrorHandler, { sequence: 0 }); - /** * Chrome is the root component of the PoS App. */ @@ -195,17 +188,6 @@ odoo.define('point_of_sale.Chrome', function(require) { }); } } - handleError(env, error, originalError) { - if (this.env.pos && originalError && originalError.message) { - const { type, message, data } = originalError.message; - this.showPopup('ErrorTracebackPopup', { - title: type, - body: message + '\n' + data.debug + '\n', - }); - return true; - } - return false; - } // EVENT HANDLERS // @@ -413,7 +395,6 @@ odoo.define('point_of_sale.Chrome', function(require) { } this._disableBackspaceBack(); - chromeInstance = this; } // prevent backspace from performing a 'back' navigation _disableBackspaceBack() { @@ -452,6 +433,70 @@ odoo.define('point_of_sale.Chrome', function(require) { showCashMoveButton() { return this.env.pos && this.env.pos.config && this.env.pos.config.cash_control; } + + // UNEXPECTED ERROR HANDLING // + + /** + * This method is used to handle unexpected errors. It is registered to + * the `error_handlers` service when this component is properly mounted. + * See `onMounted` hook of the `ChromeAdapter` component. + * @param {*} env + * @param {UncaughtClientError | UncaughtPromiseError} error + * @param {*} originalError + * @returns {boolean} + */ + errorHandler(env, error, originalError) { + if (!env.pos) return false; + const errorToHandle = identifyError(originalError); + // Assume that the unhandled falsey rejections can be ignored. + if (errorToHandle) { + this._errorHandler(error, errorToHandle); + } + return true; + } + + _errorHandler(error, errorToHandle) { + if (errorToHandle instanceof RPCError) { + const { message, data } = errorToHandle; + if (odooExceptionTitleMap.has(errorToHandle.exceptionName)) { + const title = odooExceptionTitleMap.get(errorToHandle.exceptionName).toString(); + this.showPopup('ErrorPopup', { title, body: data.message }); + } else { + this.showPopup('ErrorTracebackPopup', { + title: message, + body: data.message + '\n' + data.debug + '\n', + }); + } + } else if (errorToHandle instanceof ConnectionLostError) { + this.showPopup('OfflineErrorPopup', { + title: this.env._t('Connection is lost'), + body: this.env._t('Check the internet connection then try again.'), + }); + } else if (errorToHandle instanceof ConnectionAbortedError) { + this.showPopup('OfflineErrorPopup', { + title: this.env._t('Connection is aborted'), + body: this.env._t('Check the internet connection then try again.'), + }); + } else if (errorToHandle instanceof Error) { + // If `errorToHandle` is a normal Error (such as TypeError), + // the annotated traceback can be found from `error`. + this.showPopup('ErrorTracebackPopup', { + // Hopefully the message is translated. + title: `${errorToHandle.name}: ${errorToHandle.message}`, + body: error.traceback, + }); + } else { + // Hey developer. It's your fault that the error reach here. + // Please, throw an Error object in order to get stack trace of the error. + // At least we can find the file that throws the error when you look + // at the console. + this.showPopup('ErrorPopup', { + title: this.env._t('Unknown Error'), + body: this.env._t('Unable to show information about this error.'), + }); + console.error('Unknown error. Unable to show information about this error.', errorToHandle); + } + } } Chrome.template = 'Chrome'; diff --git a/addons/point_of_sale/static/src/js/Screens/PaymentScreen/PaymentScreen.js b/addons/point_of_sale/static/src/js/Screens/PaymentScreen/PaymentScreen.js index 1216e3b8032..2d873c0873b 100644 --- a/addons/point_of_sale/static/src/js/Screens/PaymentScreen/PaymentScreen.js +++ b/addons/point_of_sale/static/src/js/Screens/PaymentScreen/PaymentScreen.js @@ -8,6 +8,8 @@ odoo.define('point_of_sale.PaymentScreen', function (require) { const { useListener } = require('web.custom_hooks'); const Registries = require('point_of_sale.Registries'); const { onChangeOrder } = require('point_of_sale.custom_hooks'); + const { identifyError } = require('point_of_sale.utils'); + const { ConnectionLostError, ConnectionAbortedError } = require('@web/core/network/rpc_service'); class PaymentScreen extends PosComponent { constructor() { @@ -182,10 +184,27 @@ odoo.define('point_of_sale.PaymentScreen', function (require) { } catch (error) { if (error.code == 700) this.error = true; - if (error instanceof Error) { - throw error; - } else { + + if ('code' in error) { + // We started putting `code` in the rejected object for invoicing error. + // We can continue with that convention such that when the error has `code`, + // then it is an error when invoicing. Besides, _handlePushOrderError was + // introduce to handle invoicing error logic. await this._handlePushOrderError(error); + } else { + // We don't block for connection error. But we rethrow for any other errors. + const errorIdentity = identifyError(error); + if ( + errorIdentity instanceof ConnectionLostError || + errorIdentity instanceof ConnectionAbortedError + ) { + this.showPopup('OfflineErrorPopup', { + title: this.env._t('Connection Error'), + body: this.env._t('Order is not synced. Check your internet connection'), + }); + } else { + throw error; + } } } if (syncedOrderBackendIds.length && this.currentOrder.wait_for_push_order()) { @@ -195,10 +214,8 @@ odoo.define('point_of_sale.PaymentScreen', function (require) { ); if (!result) { await this.showPopup('ErrorPopup', { - title: 'Error: no internet connection.', - // FIXME: the variable 'error' is not declared - // eslint-disable-next-line no-undef - body: error, + title: this.env._t('Error: no internet connection.'), + body: this.env._t('Some, if not all, post-processing after syncing order failed.') }); } } diff --git a/addons/point_of_sale/static/src/js/chrome_adapter.js b/addons/point_of_sale/static/src/js/chrome_adapter.js index 12ee15f055f..68a0e886de3 100644 --- a/addons/point_of_sale/static/src/js/chrome_adapter.js +++ b/addons/point_of_sale/static/src/js/chrome_adapter.js @@ -30,6 +30,17 @@ export class ChromeAdapter extends Component { useBus(this.env.qweb, "update", () => this.render()); const chrome = owl.hooks.useRef("chrome"); owl.hooks.onMounted(async () => { + // Add the pos error handler when the chrome component is available. + registry.category('error_handlers').add( + 'posErrorHandler', + (env, ...noEnvArgs) => { + if (chrome.comp) { + return chrome.comp.errorHandler(this.env, ...noEnvArgs); + } + return false; + }, + { sequence: 0 } + ); // Little trick to avoid displaying the block ui during the POS models loading const BlockUiFromRegistry = registry.category("main_components").get("BlockUI"); registry.category("main_components").remove("BlockUI"); diff --git a/addons/point_of_sale/static/src/js/models.js b/addons/point_of_sale/static/src/js/models.js index f937d23ce1a..ada9e2feb9e 100644 --- a/addons/point_of_sale/static/src/js/models.js +++ b/addons/point_of_sale/static/src/js/models.js @@ -935,13 +935,13 @@ exports.PosModel = Backbone.Model.extend({ this.db.add_order(order.export_as_JSON()); } - return new Promise(function (resolve, reject) { - self.flush_mutex.exec(function () { - var flushed = self._flush_orders(self.db.get_orders(), opts); - - flushed.then(resolve, reject); - - return flushed; + return new Promise((resolve, reject) => { + self.flush_mutex.exec(async () => { + try { + resolve(await self._flush_orders(self.db.get_orders(), opts)); + } catch (error) { + reject(error); + } }); }); }, @@ -951,17 +951,14 @@ exports.PosModel = Backbone.Model.extend({ const self = this; const order_id = self.db.add_order(order.export_as_JSON()); - return new Promise(function (resolve, reject) { - self.flush_mutex.exec(function () { - var order = self.db.get_order(order_id); - if (order){ - var flushed = self._flush_orders([order], opts); - } else { - var flushed = Promise.resolve([]); + return new Promise((resolve, reject) => { + self.flush_mutex.exec(async () => { + const order = self.db.get_order(order_id); + try { + resolve(await self._flush_orders([order], opts)); + } catch (error) { + reject(error); } - flushed.then(resolve, reject); - - return flushed; }); }); }, @@ -976,71 +973,43 @@ exports.PosModel = Backbone.Model.extend({ push_and_invoice_order: function (order) { var self = this; - var invoiced = new Promise(function (resolveInvoiced, rejectInvoiced) { - if(!order.get_client()){ - rejectInvoiced({code:400, message:'Missing Customer', data:{}}); - } - else { + return new Promise((resolve, reject) => { + if (!order.get_client()) { + reject({ code: 400, message: 'Missing Customer', data: {} }); + } else { var order_id = self.db.add_order(order.export_as_JSON()); - - self.flush_mutex.exec(function () { - var done = new Promise(function (resolveDone, rejectDone) { - // send the order to the server - // we have a 30 seconds timeout on this push. - // FIXME: if the server takes more than 30 seconds to accept the order, - // the client will believe it wasn't successfully sent, and very bad - // things will happen as a duplicate will be sent next time - // so we must make sure the server detects and ignores duplicated orders - - var transfer = self._flush_orders([self.db.get_order(order_id)], {timeout:30000, to_invoice:true}); - - transfer.catch(function (error) { - rejectInvoiced(error); - rejectDone(); + self.flush_mutex.exec(async () => { + try { + const server_ids = await self._flush_orders([self.db.get_order(order_id)], { + timeout: 30000, + to_invoice: true, }); - - // on success, get the order id generated by the server - transfer.then(async function(order_server_id){ - // generate the pdf and download it - if (order_server_id.length) { - const [orderWithInvoice] = await self.rpc({ - method: 'read', - model: 'pos.order', - args: [order_server_id, ['account_move']], - kwargs: { load: false }, + if (server_ids.length) { + const [orderWithInvoice] = await self.rpc({ + method: 'read', + model: 'pos.order', + args: [server_ids, ['account_move']], + kwargs: { load: false }, + }); + await self + .do_action('account.account_invoices', { + additional_context: { + active_ids: [orderWithInvoice.account_move], + }, + }) + .catch(() => { + reject({ code: 401, message: 'Backend Invoice', data: { order: order } }); }); - if (orderWithInvoice && orderWithInvoice.account_move) { - self.do_action('account.account_invoices',{additional_context:{ - active_ids:[orderWithInvoice.account_move], - }}).then(function () { - resolveInvoiced(order_server_id); - resolveDone(); - }).catch(error => { - rejectInvoiced({code:401, message:'Backend Invoice', data:{order: order}}); - rejectDone(); - }); - } else { - rejectInvoiced({code:401, message:'Backend Invoice', data:{order: order}}); - rejectDone(); - } - } else if (order_server_id.length) { - resolveInvoiced(order_server_id); - resolveDone(); - } else { - // The order has been pushed separately in batch when - // the connection came back. - // The user has to go to the backend to print the invoice - rejectInvoiced({code:401, message:'Backend Invoice', data:{order: order}}); - rejectDone(); - } - }); - return done; - }); + } else { + reject({ code: 401, message: 'Backend Invoice', data: { order: order } }); + } + resolve(server_ids); + } catch (error) { + reject(error); + } }); } }); - - return invoiced; }, // wrapper around the _save_to_server that updates the synch status widget @@ -1057,7 +1026,7 @@ exports.PosModel = Backbone.Model.extend({ return _.pluck(server_ids, 'id'); }).catch(function(error){ self.set_synch(self.get('failed') ? 'error' : 'disconnected'); - return Promise.reject(error); + throw error; }); }, @@ -1111,8 +1080,7 @@ exports.PosModel = Backbone.Model.extend({ }); self.set('failed',false); return server_ids; - }).catch(function (reason){ - var error = reason.message; + }).catch(function (error){ console.warn('Failed to send orders:', orders); if(error.code === 200 ){ // Business Logic Error, not a connection problem // Hide error if already shown before ... diff --git a/addons/point_of_sale/static/src/js/utils.js b/addons/point_of_sale/static/src/js/utils.js index 7aa7b35e93d..6ac1ec88f73 100644 --- a/addons/point_of_sale/static/src/js/utils.js +++ b/addons/point_of_sale/static/src/js/utils.js @@ -2,6 +2,7 @@ odoo.define('point_of_sale.utils', function (require) { 'use strict'; const { EventBus } = owl.core; + const { ConnectionAbortedError } = require('@web/core/network/rpc_service'); function getFileAsText(file) { return new Promise((resolve, reject) => { @@ -45,5 +46,24 @@ odoo.define('point_of_sale.utils', function (require) { ); } - return { getFileAsText, nextFrame, isRpcError, posbus: new EventBus() }; + function identifyError(error) { + if (!error) return error; + let errorToHandle; + if (error.legacy) { + // error.message is either RPCError or ConnectionLostError + errorToHandle = error.message; + } else if (error.event && error.event.type == 'abort') { + // Check if there is event and if the event type is abort. + // If so, then it's supposed to be a ConnectionAbortedError, + // however, it was stripped in the patch of rpc in `mapLegacyEnvToWowlEnv`. + // We recreate the error object here so that in the actual handler, + // ConnectionAbortedError and ConnectionLostError are handled properly. + errorToHandle = new ConnectionAbortedError(error.message); + } else if (error instanceof Error) { + errorToHandle = error; + } + return errorToHandle || error; + } + + return { getFileAsText, nextFrame, isRpcError, posbus: new EventBus(), identifyError }; }); diff --git a/addons/pos_coupon/static/src/js/coupon.js b/addons/pos_coupon/static/src/js/coupon.js index e11ac6c787f..7780eaf46f7 100644 --- a/addons/pos_coupon/static/src/js/coupon.js +++ b/addons/pos_coupon/static/src/js/coupon.js @@ -264,7 +264,7 @@ odoo.define('pos_coupon.pos', function (require) { this.rewardsContainer = rewardsContainer; // Send a signal that the rewardsContainer are updated. this.trigger('rewards-updated'); - }); + }).catch(() => { /* catch the reject of dp when calling `add` to avoid unhandledrejection */ }); }, res );