OBPIH-6689 Rely on unit price instead amount while calculating inverse items#4855
OBPIH-6689 Rely on unit price instead amount while calculating inverse items#4855awalkowiak merged 3 commits intorelease/0.9.2-hotfix1from
Conversation
awalkowiak
requested review from
alannadolny,
drodzewicz,
ewaterman and
kchelstowski
github-actions
bot
added
domain: frontend
| 10 | -3 | 7 | ||
| -5 | 2 | -3 | ||
| -5 | 5 | 0 | ||
| preapymentItemUnitPrice | inversedUnitPrice | unitPriceAvailableToInverse |
There was a problem hiding this comment.
minor: prepaymentItemUnitPrice
| // how much of prepayment item's amount is available for inversing. | ||
| return prepaymentItem.amount + orderAdjustment.inversedAmount | ||
| BigDecimal getUnitPriceAvailableToInverse(BigDecimal unitPrice, BigDecimal inversedUnitPrice) { | ||
| return Math.abs(unitPrice) > Math.abs(inversedUnitPrice) ? unitPrice - inversedUnitPrice : 0 |
There was a problem hiding this comment.
reintroducing this discussion from the other PR: are we allowing a scenario where unit price and inversed unit price have different signs? Like I said previously, that case doesn't really make sense, but if a user gets confused and puts in the wrong sign, we might get into undesirable scenarios here.
Ex: unit price is 1 and inversed unit price is -1. We should return 2 but we return 0.
Maybe because of this we shouldn't allow the signs to differ.
There was a problem hiding this comment.
or is the idea that using unit price guarantees we always have the same sign? If so, can you add a comment clarifying this somewhere?
There was a problem hiding this comment.
are we allowing a scenario where unit price and inversed unit price have different signs?
No. Both will have the same sign (can be negative or positive but both will have the same, and this is already validated here https://github.com/openboxes/openboxes/blob/release/0.9.2-hotfix1/grails-app/services/org/pih/warehouse/invoice/PrepaymentInvoiceService.groovy#L400). That's why I changed to the unit price from amount, because with the amount it was a wild ride here because amount could have different signs.
|
I'm still wrapping my head around this change. Is this going to impact how the migration works? |
@ewaterman I think it should not affect the migration, because this in majority affects/targets editing invoice items and inverse items. I'll try to double check that while testing your migration locally |
4 participants
✨ Description of Change
Link to GitHub issue or Jira ticket:
https://pihemr.atlassian.net/browse/OBPIH-6689
Description:
This is basically a changing approach to determining inverse items. Previously we were relying on the amount field, which while comparing to unit prices could have different sign and could be reduced by prepayment percentage. To simplify things we should always rely on the unit price all the time while determining inverse items.