From a869ee87f28c517c67d782b132b65aaf86309d1e Mon Sep 17 00:00:00 2001 From: vlst Date: Fri, 8 Sep 2023 16:18:43 +0200 Subject: [PATCH] [REF] pos: use reactivity in ClosingPopup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "ClosingPopup" relies on an error-prone pattern of calling a "handle" function on each input change. In this commit we refactor it's logic such that the calculation is done reactively.  This leads to much simpler code. In addition, we remove multiple pieces of dead code. closes odoo/odoo#131171 Task: 3499250 Related: odoo/enterprise#47323 Signed-off-by: Joseph Caburnay (jcb) --- .../l10n_fr_pos_cert/static/src/js/Chrome.js | 2 +- addons/point_of_sale/models/pos_session.py | 3 - .../src/app/generic_components/input/input.js | 1 - .../app/navbar/closing_popup/closing_popup.js | 270 ++++++++++-------- .../navbar/closing_popup/closing_popup.xml | 54 ++-- .../static/src/app/navbar/navbar.js | 2 +- .../cash_opening_popup/cash_opening_popup.js | 1 + .../cash_opening_popup/cash_opening_popup.xml | 2 +- .../static/src/app/store/pos_store.js | 46 +-- .../src/app/utils/contextual_utils_service.js | 17 +- 10 files changed, 194 insertions(+), 204 deletions(-) diff --git a/addons/l10n_fr_pos_cert/static/src/js/Chrome.js b/addons/l10n_fr_pos_cert/static/src/js/Chrome.js index e856a3f5e42..12379b96b16 100644 --- a/addons/l10n_fr_pos_cert/static/src/js/Chrome.js +++ b/addons/l10n_fr_pos_cert/static/src/js/Chrome.js @@ -15,7 +15,7 @@ patch(Chrome.prototype, { limitDate.setDate(limitDate.getDate() + 1); if (limitDate.getTime() < now) { const info = await this.pos.getClosePosInfo(); - this.popup.add(ClosePosPopup, { info, keepBehind: true }); + this.popup.add(ClosePosPopup, { ...info, keepBehind: true }); } } }); diff --git a/addons/point_of_sale/models/pos_session.py b/addons/point_of_sale/models/pos_session.py index 80e81844b42..fc564966cf4 100644 --- a/addons/point_of_sale/models/pos_session.py +++ b/addons/point_of_sale/models/pos_session.py @@ -545,7 +545,6 @@ class PosSession(models.Model): self.ensure_one() orders = self.order_ids.filtered(lambda o: o.state == 'paid' or o.state == 'invoiced') payments = orders.payment_ids.filtered(lambda p: p.payment_method_id.type != "pay_later") - pay_later_payments = orders.payment_ids - payments cash_payment_method_ids = self.payment_method_ids.filtered(lambda pm: pm.type == 'cash') default_cash_payment_method_id = cash_payment_method_ids[0] if cash_payment_method_ids else None total_default_cash_payment_amount = sum(payments.filtered(lambda p: p.payment_method_id == default_cash_payment_method_id).mapped('amount')) if default_cash_payment_method_id else 0 @@ -571,8 +570,6 @@ class PosSession(models.Model): 'quantity': len(orders), 'amount': sum(orders.mapped('amount_total')) }, - 'payments_amount': sum(payments.mapped('amount')), - 'pay_later_amount': sum(pay_later_payments.mapped('amount')), 'opening_notes': self.opening_notes, 'default_cash_details': { 'name': default_cash_payment_method_id.name, diff --git a/addons/point_of_sale/static/src/app/generic_components/input/input.js b/addons/point_of_sale/static/src/app/generic_components/input/input.js index bbc5bd47f6d..4179a9e5f4f 100644 --- a/addons/point_of_sale/static/src/app/generic_components/input/input.js +++ b/addons/point_of_sale/static/src/app/generic_components/input/input.js @@ -63,7 +63,6 @@ export class Input extends Component { return obj[key]; } setValue(newValue, tModel = this.props.tModel) { - newValue = this.props.format(newValue); const [obj, key] = tModel; obj[key] = newValue; this.props.callback?.(newValue); diff --git a/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.js b/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.js index f317e9c3fef..58b90a3b01b 100644 --- a/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.js +++ b/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.js @@ -5,19 +5,37 @@ import { SaleDetailsButton } from "@point_of_sale/app/navbar/sale_details_button import { ConfirmPopup } from "@point_of_sale/app/utils/confirm_popup/confirm_popup"; import { MoneyDetailsPopup } from "@point_of_sale/app/utils/money_details_popup/money_details_popup"; import { useService } from "@web/core/utils/hooks"; +import { useState } from "@odoo/owl"; import { AlertPopup } from "@point_of_sale/app/utils/alert_popup/alert_popup"; import { ErrorPopup } from "@point_of_sale/app/errors/popups/error_popup"; -import { useState } from "@odoo/owl"; import { ConnectionLostError } from "@web/core/network/rpc_service"; import { identifyError } from "@point_of_sale/app/errors/error_handlers"; import { _t } from "@web/core/l10n/translation"; import { usePos } from "@point_of_sale/app/store/pos_hook"; import { parseFloat } from "@web/views/fields/parsers"; import { Input } from "@point_of_sale/app/generic_components/input/input"; +import { useAsyncLockedMethod } from "@point_of_sale/app/utils/hooks"; export class ClosePosPopup extends AbstractAwaitablePopup { static components = { SaleDetailsButton, Input }; static template = "point_of_sale.ClosePosPopup"; + static props = [ + "orders_details", + "opening_notes", + "default_cash_details", + "other_payment_methods", + "is_manager", + "amount_authorized_diff", + "state", + // TODO: set the props for all popups + "id", + "keepBehind", + "resolve", + "isActive", + "close", + "confirmKey", + "cancelKey", + ]; setup() { super.setup(); @@ -27,20 +45,33 @@ export class ClosePosPopup extends AbstractAwaitablePopup { this.report = useService("report"); this.hardwareProxy = useService("hardware_proxy"); this.customerDisplay = useService("customer_display"); - this.cashControl = this.pos.config.cash_control; - this.closeSessionClicked = false; - this.moneyDetails = null; - Object.assign(this, this.props.info); - this.state = useState({ - displayMoneyDetailsPopup: false, - }); - Object.assign(this.state, this.props.info.state); + this.state = useState(this.getInitialState()); + this.confirm = useAsyncLockedMethod(this.confirm); } + getInitialState() { + const initialState = { notes: "", payments: {} }; + if (this.pos.config.cash_control) { + initialState.payments[this.props.default_cash_details.id] = { + counted: "0", + }; + } + this.props.other_payment_methods.forEach((pm) => { + if (pm.type === "bank") { + initialState.payments[pm.id] = { + counted: this.env.utils.formatCurrency(pm.amount, false), + }; + } + }); + return initialState; + } + //@override async confirm() { - if (!this.cashControl || !this.hasDifference()) { - this.closeSession(); - } else if (this.hasUserAuthority()) { + if (!this.pos.config.cash_control || this.env.utils.floatIsZero(this.getMaxDifference())) { + await this.closeSession(); + return; + } + if (this.hasUserAuthority()) { const { confirmed } = await this.popup.add(ConfirmPopup, { title: _t("Payments Difference"), body: _t( @@ -48,19 +79,19 @@ export class ClosePosPopup extends AbstractAwaitablePopup { ), }); if (confirmed) { - this.closeSession(); + await this.closeSession(); } - } else { - await this.popup.add(ConfirmPopup, { - title: _t("Payments Difference"), - body: _t( - "The maximum difference allowed is %s.\n\ - Please contact your manager to accept the closing difference.", - this.env.utils.formatCurrency(this.amountAuthorizedDiff) - ), - confirmText: _t("OK"), - }); + return; } + await this.popup.add(ConfirmPopup, { + title: _t("Payments Difference"), + body: _t( + "The maximum difference allowed is %s.\n\ + Please contact your manager to accept the closing difference.", + this.env.utils.formatCurrency(this.props.amount_authorized_diff) + ), + confirmText: _t("OK"), + }); } //@override async cancel() { @@ -68,6 +99,11 @@ export class ClosePosPopup extends AbstractAwaitablePopup { super.cancel(); } } + canConfirm() { + return Object.values(this.state.payments) + .map((v) => v.counted) + .every(this.env.utils.isValidFloat); + } async openDetailsPopup() { const action = _t("Cash control - closing"); this.hardwareProxy.openCashbox(action); @@ -77,12 +113,8 @@ export class ClosePosPopup extends AbstractAwaitablePopup { }); if (confirmed) { const { total, moneyDetailsNotes, moneyDetails } = payload; - this.state.payments[this.defaultCashDetails.id].counted = total; - this.state.payments[this.defaultCashDetails.id].difference = - this.env.utils.roundCurrency( - this.state.payments[[this.defaultCashDetails.id]].counted - - this.defaultCashDetails.amount - ); + this.state.payments[this.props.default_cash_details.id].counted = + this.env.utils.formatCurrency(total, false); if (moneyDetailsNotes) { this.state.notes = moneyDetailsNotes; } @@ -92,114 +124,109 @@ export class ClosePosPopup extends AbstractAwaitablePopup { async downloadSalesReport() { return this.report.download("point_of_sale.sale_details_report", [this.pos.pos_session.id]); } - handleInputChange(paymentId, amount) { - if (!this.env.utils.isValidFloat(amount)) { - return; - } - let expectedAmount; - if (paymentId === this.defaultCashDetails?.id) { - this.moneyDetails = null; + setManualCashInput(amount) { + if (this.env.utils.isValidFloat(amount) && this.moneyDetails) { this.state.notes = ""; - expectedAmount = this.defaultCashDetails.amount; - } else { - expectedAmount = this.otherPaymentMethods.find((pm) => paymentId === pm.id).amount; + this.moneyDetails = null; } - this.state.payments[paymentId].counted = parseFloat(amount); - this.state.payments[paymentId].difference = this.env.utils.roundCurrency( - this.state.payments[paymentId].counted - expectedAmount - ); } - hasDifference() { - return Object.entries(this.state.payments).find((pm) => pm[1].difference != 0); + getDifference(paymentId) { + const counted = this.state.payments[paymentId].counted; + if (!this.env.utils.isValidFloat(counted)) { + return NaN; + } + const expectedAmount = + paymentId === this.props.default_cash_details.id + ? this.props.default_cash_details.amount + : this.props.other_payment_methods.find((pm) => pm.id === paymentId).amount; + + return parseFloat(counted) - expectedAmount; + } + getMaxDifference() { + return Math.max( + ...Object.keys(this.state.payments).map((id) => + Math.abs(this.getDifference(parseInt(id))) + ) + ); } hasUserAuthority() { - const absDifferences = Object.entries(this.state.payments).map((pm) => - Math.abs(pm[1].difference) - ); return ( - this.isManager || - this.amountAuthorizedDiff == null || - Math.max(...absDifferences) <= this.amountAuthorizedDiff + this.props.is_manager || + this.props.amount_authorized_diff == null || + this.getMaxDifference() <= this.props.amount_authorized_diff ); } canCancel() { return true; } async closeSession() { - if (!this.closeSessionClicked) { - this.closeSessionClicked = true; - - if (this.customerDisplay) { - this.customerDisplay.update({ closeUI: true }); - } - - if (this.cashControl) { - const response = await this.orm.call( - "pos.session", - "post_closing_cash_details", - [this.pos.pos_session.id], - { - counted_cash: this.state.payments[this.defaultCashDetails.id].counted, - } - ); - - if (!response.successful) { - return this.handleClosingError(response); + this.customerDisplay?.update({ closeUI: true }); + if (this.pos.config.cash_control) { + const response = await this.orm.call( + "pos.session", + "post_closing_cash_details", + [this.pos.pos_session.id], + { + counted_cash: parseFloat( + this.state.payments[this.props.default_cash_details.id].counted + ), } - } + ); - try { - await this.orm.call("pos.session", "update_closing_control_state_session", [ - this.pos.pos_session.id, - this.state.notes, - ]); - } catch (error) { - // We have to handle the error manually otherwise the validation check stops the script. - // In case of "rescue session", we want to display the next popup with "handleClosingError". - // FIXME - if ( - !error.message && - !error.message.data && - error.message.data.message !== "This session is already closed." - ) { - throw error; - } - } - - try { - const bankPaymentMethodDiffPairs = this.otherPaymentMethods - .filter((pm) => pm.type == "bank") - .map((pm) => [pm.id, this.state.payments[pm.id].difference]); - const response = await this.orm.call("pos.session", "close_session_from_ui", [ - this.pos.pos_session.id, - bankPaymentMethodDiffPairs, - ]); - if (!response.successful) { - return this.handleClosingError(response); - } - window.location = "/web#action=point_of_sale.action_client_pos_menu"; - } catch (error) { - if (identifyError(error) instanceof ConnectionLostError) { - // Cannot redirect to backend when offline, let error handlers show the offline popup - // FIXME POSREF: doing this means closing again when online will redo the beginning of the method - // although it's impossible to close again because this.closeSessionClicked isn't reset to false - // The application state is corrupted. - throw error; - } else { - // FIXME POSREF: why are we catching errors here but not anywhere else in this method? - await this.popup.add(ErrorPopup, { - title: _t("Closing session error"), - body: _t( - "An error has occurred when trying to close the session.\n" + - "You will be redirected to the back-end to manually close the session." - ), - }); - window.location = "/web#action=point_of_sale.action_client_pos_menu"; - } + if (!response.successful) { + return this.handleClosingError(response); } } - this.closeSessionClicked = false; + try { + await this.orm.call("pos.session", "update_closing_control_state_session", [ + this.pos.pos_session.id, + this.state.notes, + ]); + } catch (error) { + // We have to handle the error manually otherwise the validation check stops the script. + // In case of "rescue session", we want to display the next popup with "handleClosingError". + // FIXME + if ( + !error.message && + !error.message.data && + error.message.data.message !== "This session is already closed." + ) { + throw error; + } + } + + try { + const bankPaymentMethodDiffPairs = this.props.other_payment_methods + .filter((pm) => pm.type == "bank") + .map((pm) => [pm.id, this.getDifference(pm.id)]); + const response = await this.orm.call("pos.session", "close_session_from_ui", [ + this.pos.pos_session.id, + bankPaymentMethodDiffPairs, + ]); + if (!response.successful) { + return this.handleClosingError(response); + } + window.location = "/web#action=point_of_sale.action_client_pos_menu"; + } catch (error) { + if (identifyError(error) instanceof ConnectionLostError) { + // Cannot redirect to backend when offline, let error handlers show the offline popup + // FIXME POSREF: doing this means closing again when online will redo the beginning of the method + // although it's impossible to close again because this.closeSessionClicked isn't reset to false + // The application state is corrupted. + throw error; + } else { + // FIXME POSREF: why are we catching errors here but not anywhere else in this method? + await this.popup.add(ErrorPopup, { + title: _t("Closing session error"), + body: _t( + "An error has occurred when trying to close the session.\n" + + "You will be redirected to the back-end to manually close the session." + ), + }); + window.location = "/web#action=point_of_sale.action_client_pos_menu"; + } + } } async handleClosingError(response) { const body = response.message; @@ -213,7 +240,4 @@ export class ClosePosPopup extends AbstractAwaitablePopup { window.location = "/web#action=point_of_sale.action_client_pos_menu"; } } - _getShowDiff(pm) { - return pm.type == "bank" && pm.number !== 0; - } } diff --git a/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.xml b/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.xml index 36731b5baca..5645e04f93a 100644 --- a/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.xml +++ b/addons/point_of_sale/static/src/app/navbar/closing_popup/closing_popup.xml @@ -6,8 +6,8 @@

Closing Session

- Total orders: - + Total orders: +
@@ -21,29 +21,29 @@ Difference - + - - + + -
- + Opening - + - +
@@ -52,37 +52,37 @@ - +
-
- Payments in +
+ Payments in
- + - - + + - + + isValid.bind="env.utils.isValidFloat" /> - +
-
- -
@@ -93,8 +93,8 @@
diff --git a/addons/point_of_sale/static/src/app/store/pos_store.js b/addons/point_of_sale/static/src/app/store/pos_store.js index d4fc0a9d2e4..262a50c7d1d 100644 --- a/addons/point_of_sale/static/src/app/store/pos_store.js +++ b/addons/point_of_sale/static/src/app/store/pos_store.js @@ -926,53 +926,9 @@ export class PosStore extends Reactive { }; } async getClosePosInfo() { - const closingData = await this.orm.call("pos.session", "get_closing_control_data", [ + return await this.orm.call("pos.session", "get_closing_control_data", [ [this.pos_session.id], ]); - const ordersDetails = closingData.orders_details; - const paymentsAmount = closingData.payments_amount; - const payLaterAmount = closingData.pay_later_amount; - const openingNotes = closingData.opening_notes; - const defaultCashDetails = closingData.default_cash_details; - const otherPaymentMethods = closingData.other_payment_methods; - const isManager = closingData.is_manager; - const amountAuthorizedDiff = closingData.amount_authorized_diff; - const cashControl = this.config.cash_control; - - // component state and refs definition - const state = { notes: "", acceptClosing: false, payments: {} }; - if (cashControl) { - state.payments[defaultCashDetails.id] = { - counted: 0, - difference: -defaultCashDetails.amount, - number: 0, - }; - } - if (otherPaymentMethods.length > 0) { - otherPaymentMethods.forEach((pm) => { - if (pm.type === "bank") { - state.payments[pm.id] = { - counted: this.env.utils - .roundCurrency(pm.amount) - .toFixed(this.currency.decimal_places), - difference: 0, - number: pm.number, - }; - } - }); - } - return { - ordersDetails, - paymentsAmount, - payLaterAmount, - openingNotes, - defaultCashDetails, - otherPaymentMethods, - isManager, - amountAuthorizedDiff, - state, - cashControl, - }; } set_start_order() { if (this.orders.length && !this.selectedOrder) { diff --git a/addons/point_of_sale/static/src/app/utils/contextual_utils_service.js b/addons/point_of_sale/static/src/app/utils/contextual_utils_service.js index 15ae4a52f0e..19d75fabe48 100644 --- a/addons/point_of_sale/static/src/app/utils/contextual_utils_service.js +++ b/addons/point_of_sale/static/src/app/utils/contextual_utils_service.js @@ -1,9 +1,14 @@ /** @odoo-module */ import { formatMonetary } from "@web/views/fields/formatters"; -import { formatFloat, roundDecimals } from "@web/core/utils/numbers"; +import { + formatFloat, + roundDecimals, + floatIsZero as genericFloatIsZero, +} from "@web/core/utils/numbers"; import { escapeRegExp } from "@web/core/utils/strings"; import { registry } from "@web/core/registry"; +import { parseFloat } from "@web/views/fields/parsers"; /** * This service introduces `utils` namespace in the `env` which can contain @@ -32,13 +37,19 @@ export const contextualUtilsService = { return formatFloat(qty, { digits: [true, productUoMDecimals] }); }; + const formatStrCurrency = (valueStr, hasSymbol = true) => { + return formatCurrency(parseFloat(valueStr), hasSymbol); + }; + const formatCurrency = (value, hasSymbol = true) => { - value = parseFloat(value); return formatMonetary(value, { currencyId: currency.id, noSymbol: !hasSymbol, }); }; + const floatIsZero = (value) => { + return genericFloatIsZero(value, currency.decimal_places); + }; const roundCurrency = (value) => { return roundDecimals(value, currency.decimal_places); @@ -50,9 +61,11 @@ export const contextualUtilsService = { env.utils = { formatCurrency, + formatStrCurrency, roundCurrency, formatProductQty, isValidFloat, + floatIsZero, }; }, };