-
-
Notifications
You must be signed in to change notification settings - Fork 617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[14.0][FIX] pos_fixed_discount: fix float conversion #1275
[14.0][FIX] pos_fixed_discount: fix float conversion #1275
Conversation
Hi @eLBati, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional ok
This solves an issue with global discount which is not taking decimals into consideration, see video
@eLBati could you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and function review, LGTM.
Even when trying to insert a number like 1.445
in the discount popup, it's correctly rounded to 1.45
@@ -17,8 +17,7 @@ odoo.define("pos_fixed_discount.FixedDiscountButton", function (require) { | |||
startingValue: 0, | |||
}); | |||
if (confirmed) { | |||
var val = Math.round(parseFloat(payload)); | |||
this.apply_discount(val); | |||
this.apply_discount(payload.replace(",", ".")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: Tested this, couldn't find a reason for the replace
, even if I type ,
in the discount, I still get a .
in the payload. Is it due to language settings in Odoo or the browser? Either way, this seems fine to me.
I'd perhaps keep the call to parseFloat
just to be safe with Javascript?
Really what was the breaking the flow was only the call to Math.round
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment: Tested this, couldn't find a reason for the
replace
, even if I type,
in the discount, I still get a.
in the payload. Is it due to language settings in Odoo or the browser? Either way, this seems fine to me.
Probably on language settings or a combination of this one and numpad dot; btw i got "," on payload
I'd perhaps keep the call to
parseFloat
just to be safe with Javascript?
it is ok, will re.add; removed following other modules, like:
await self.apply_discount(payload.replace(",", ".")); |
Really what was the breaking the flow was only the call to
Math.round
Yeah, the issue was Math.round
This PR has the |
ab71936
to
6623ce6
Compare
@eLBati good to go? thanks! |
@OCA/pos-maintainers can this be merged please? |
/ocabot merge patch |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at fd4b965. Thanks a lot for contributing to OCA. ❤️ |
No description provided.