Skip to content

Refund by items: Rounding errors - Step 4#1629

Merged
anitaa1990 merged 10 commits intofeature/refund-by-items-masterfrom
0nko/bigdecimal-rounding
Dec 12, 2019
Merged

Refund by items: Rounding errors - Step 4#1629
anitaa1990 merged 10 commits intofeature/refund-by-items-masterfrom
0nko/bigdecimal-rounding

Conversation

@0nko
Copy link
Copy Markdown
Contributor

@0nko 0nko commented Dec 3, 2019

We're using BigDecimals to minimize the rounding error during calculations. Recently I noticed that the Issue refund button is being shown even though there is no more money to be refunded (total - refund total was supposed to be 0). It turned out that there was rounding error already present that came from the API. This PR makes sure the errors are removed during model creation.

Note: #1623 must be merged before this PR.

@0nko 0nko added the feature: refunds Related to order refunds. label Dec 3, 2019
@0nko 0nko added this to the 3.3 milestone Dec 3, 2019
@0nko 0nko added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 3, 2019
@peril-woocommerce
Copy link
Copy Markdown

peril-woocommerce bot commented Dec 3, 2019

You can test the changes on this Pull Request by downloading the APK here.

@0nko 0nko changed the base branch from 0nko/items-refactoring to feature/refund-by-items-master December 12, 2019 09:29
@0nko 0nko removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 12, 2019
Copy link
Copy Markdown
Contributor

@anitaa1990 anitaa1990 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! Just left one non blocking comment in the PR. :shipit:

Comment on lines +5 to +7
fun min(a: BigDecimal, b: BigDecimal): BigDecimal = if (a < b) a else b

fun max(a: BigDecimal, b: BigDecimal): BigDecimal = if (a > b) a else b
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

np: I'm not seeing this used anywhere in the app? Is it used in another PR? If not, can we remove it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's used in the next PR. Thanks for checking!

@anitaa1990 anitaa1990 self-assigned this Dec 12, 2019
@anitaa1990 anitaa1990 merged commit 1ad3efc into feature/refund-by-items-master Dec 12, 2019
@anitaa1990 anitaa1990 deleted the 0nko/bigdecimal-rounding branch March 29, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: refunds Related to order refunds.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants