Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[IMP] pos: Negative payment (refund) on touchscreen #31054

Closed
wants to merge 1 commit into from
Closed

[IMP] pos: Negative payment (refund) on touchscreen #31054

wants to merge 1 commit into from

Conversation

mgh-odoo
Copy link
Contributor

@mgh-odoo mgh-odoo commented Feb 13, 2019 •
edited by qle-odoo
Loading

currently, if a user wants to return fund to the customer.
we don't have a button to bill negatively. that will fixed.

Task-ID: 1929650
Closes: #31054

Task:https://www.odoo.com/web#id=1929650&action=333&active_id=131&model=project.task&view_type=form&menu_id=4720
Pad:https://pad.odoo.com/p/r.5745606edc51c9662beff81d277af3e3

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Feb 13, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from a2410b1 to 40526ef Compare February 13, 2019 08:49
@C3POdoo C3POdoo added the RD research & development, internal work label Feb 13, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from 40526ef to 3ab96fa Compare March 12, 2019 04:51
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 12, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from 3ab96fa to c672c07 Compare March 12, 2019 06:59
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 12, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from c672c07 to 6420aea Compare March 12, 2019 09:34
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Mar 12, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from 6420aea to a8a8d4e Compare March 12, 2019 09:41
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Mar 12, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from a8a8d4e to 0dd64de Compare March 14, 2019 07:13
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 14, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from 0dd64de to a51cf94 Compare March 19, 2019 06:00
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Mar 19, 2019
switch87
switch87 suggested changes Apr 2, 2019
View reviewed changes
Copy link
Contributor

@switch87 switch87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, only some small changes.

addons/point_of_sale/static/src/xml/pos.xml Outdated
@@ -1553,7 +1553,7 @@
</tr>
</t>
<tr>
<t t-if="order.get_total_discount() > 0">
<t t-if="order.get_total_discount()">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this look better, but this is totally out of scope for this task, could you remove this out of the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello, @switch87
if we remove this then not need to use Math.abs() and Math.sign() in models.js a51cf94#diff-1c14accbe350f7a177b3e81e5846861bR2506
I have added to show returned product price with a previously applied discount minus.
only thing is left now is, replace "Clear" to "+/-" is it ok?
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgh-odoo can you record an example pre- and post-commit on runbot of this? The task was indeed as simple as replacing this key apparently. But if this extra work is indeed an issue that should be fixed, you can split the commit into one [IMP] and one [FIX] and your work would not be for nothing :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello, @switch87
I changed commit based on your suggestion and
also recorded a video about pre-commit and post-commit
if it's out of scope, let me know I'll remove that one :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgh-odoo yes it's out of scope for this task. I think it is not a bad idea, so make it a new pr for it and ask @lap-odoo for her opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@switch87 done. 👍

addons/point_of_sale/static/src/js/models.js Outdated
@@ -2501,8 +2501,9 @@ exports.Order = Backbone.Model.extend({
},
get_total_discount: function() {
return round_pr(this.orderlines.reduce((function(sum, orderLine) {
sum += (orderLine.get_unit_price() * (orderLine.get_discount()/100) * orderLine.get_quantity());
sum += ((orderLine.get_lst_price() - orderLine.get_unit_price()) * orderLine.get_quantity());
var unit_price = orderLine.get_unit_price();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_quantity and Math.abs(unit_price) could also be set in a separate variable.

@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from a51cf94 to afd60be Compare April 3, 2019 12:10
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Apr 3, 2019
@mgh-odoo
[FIX] pos: Negative payment (refund) on touchscreen
ad4a479 
currently, if a user wants to return fund to the customer.
we don't have a button to bill negatively. that will fixed.

Task-ID: 1929650
Closes: odoo#31054
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from afd60be to d71ff10 Compare April 3, 2019 12:14
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Apr 3, 2019
@mgh-odoo mgh-odoo force-pushed the master-pos-negative-payment-fix-mgh branch from d71ff10 to ad4a479 Compare April 4, 2019 04:42
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses and removed CI 🤖 Robodoo has seen passing statuses labels Apr 4, 2019
@switch87
Copy link
Contributor

@qle-odoo r+ please

@qle-odoo
Copy link
Contributor

@robodoo r+

robodoo pushed a commit that referenced this pull request Apr 17, 2019
@mgh-odoo
[FIX] pos: Negative payment (refund) on touchscreen
c159735 
currently, if a user wants to return fund to the customer.
we don't have a button to bill negatively. that will fixed.

Task-ID: 1929650
Closes: #31054

Signed-off-by: Quentin Lejeune (qle) <qle@odoo.com>
@robodoo
Copy link
Contributor

robodoo commented Apr 17, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 17, 2019
@mgh-odoo mgh-odoo deleted the master-pos-negative-payment-fix-mgh branch July 15, 2019 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@switch87 switch87 switch87 requested changes

Assignees
No one assigned
Labels
CI 🤖 Robodoo has seen passing statuses Point of Sale RD research & development, internal work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mgh-odoo @switch87 @qle-odoo @robodoo @C3POdoo