[Woo POS][Refunds] Refundable items calculation #15056
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
samiuelson
changed the title
…efundable-items-calculation
…e-refund-dialog-ui
There was a problem hiding this comment.
Pull request overview
This PR introduces foundational components for handling refunds in the WooCommerce POS system. It adds business logic to calculate refundable items from orders by accounting for previous refunds, along with the necessary ViewModel and state management for the refund UI.
Key changes include:
- Implements
WooPosGetRefundableItemsuse case that expands order line items into individual refundable units and calculates remaining quantities after refunds - Adds
WooPosRefundViewModelfor managing refund dialog state with loading, error, and success states - Introduces
getOrderByIdmethod inWooPosOrdersDataSourcewith cache-first retrieval strategy
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| WooPosGetRefundableItems.kt | Core business logic for calculating refundable items with unit price/tax calculations |
| WooPosGetRefundableItemsTest.kt | Comprehensive test suite covering basic cases, refund handling, variations, price calculations, and edge cases |
| WooPosRefundViewModel.kt | ViewModel coordinating order/refund retrieval and building UI state |
| WooPosRefundState.kt | Sealed class defining Loading, Content, Error, and NoRefundableItems states |
| WooPosRefundableItem.kt | Immutable data model representing a single refundable unit |
| WooPosOrdersDataSource.kt | Adds cache-first order retrieval by ID |
| WooPosOrdersDetails.kt | Adds width modifier import for UI layout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosGetRefundableItems.kt
Outdated
Show resolved
Hide resolved
...rce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosGetRefundableItemsTest.kt
Outdated
Show resolved
Hide resolved
...rce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosGetRefundableItemsTest.kt
Outdated
Show resolved
Hide resolved
...rce/src/test/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosGetRefundableItemsTest.kt
Show resolved
Hide resolved
...ommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosGetRefundableItems.kt
Outdated
Show resolved
Hide resolved
...ommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosGetRefundableItems.kt
Outdated
Show resolved
Hide resolved
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
- Create new test class `WooPosRefundViewModelTest`. - Add tests to verify `WooPosRefundViewModel` initialization and state management (`Loading`, `Content`, `Error`, `NoRefundableItems`). - Verify calculations for subtotals, taxes, and totals with multiple refundable items. - Ensure correct handling of order and refund data retrieval failures.
Update the filtering condition when mapping product items to check against `orderItemId` instead of `productId` and `variationId`. This ensures that the correct specific line item is matched when calculating refunded quantities, preventing issues with products sharing IDs across different line items.
…e-refund-dialog-ui
…g-ui [Woo POS][Refunds] Refundable items dialog UI
Generated by 🚫 Danger |
samiuelson
added
the
status: do not merge
|
I'm adding a "do not merge" label to prevent merging until the parent branch is trunk. |
malinajirka
left a comment
malinajirka
left a comment
There was a problem hiding this comment.
Thanks @samiuelson !
I haven't found anything blocking, but I'm personally a bit worried about the local calculations. I think we might want to consider updating/introducing APIs that would give us all data we need, so we can avoid any local calculations.
Doing local calculations has several risks:
- Rounding issues - we have been dealing with these as long as I can remember in Woo.
- Extensions/Hooks - there might be hooks that modify/change what can or can't be refunded (I'm not sure if they exist, but even if they don't, anyone can introduce them at any point).
- API-non-breaking changes in core might break the apps.
Having said that, I don't want to block progress of this project. If this is the approach we decided for, then lets go with it.
| refunds: List<Refund>, | ||
| productItems: List<Order.Item> | ||
| ): Map<Long, Float> { | ||
| val allRefundedItems = refunds.flatMap { it.items } |
There was a problem hiding this comment.
💡 It seems we are iterating over allRefundedItems for each item. I guess performance is not an issue here, but we can consider simplifying this to something like:
private fun calculateMaxRefundQuantities(
refunds: List<Refund>,
productItems: List<Order.Item>
): Map<Long, Float> {
val refundedByItemId = refunds
.flatMap { it.items }
.groupingBy { it.orderItemId }
.fold(0f) { acc, item -> acc + item.quantity }
return productItems
.associate { it.itemId to (it.quantity - (refundedByItemId[it.itemId] ?: 0f)) }
.filterValues { it > 0 }
}
| return if (item.quantity == 0f) { | ||
| item.total | ||
| } else { | ||
| item.total.divide(item.quantity.toBigDecimal(), 2, RoundingMode.HALF_UP) |
There was a problem hiding this comment.
Is there perhaps a solution in which we would avoid doing local calculations like this?
If not, I think we might still want to explore if we could fetch the decimal-places-setting and use that instead of hardcoding 2. We also need to be careful to do the rounding the same way backend does - by row vs by item vs by total => each approach yields different results.
P.S. We have had several rounding issues in the app. I remember at least one case in which it was impossible to refund an order, since the app calculated the total with 0.01 difference compared to backend and the backend refused the refund. So I believe this is not just a theoretical issue.
There was a problem hiding this comment.
Good call, thanks 👍 I updated this to use orderItem.price, which matches the logic in store management: 5092da9
Now, only the tax calculation relies on local calculation (the same as in store management), but this is limited by the current API design.
There was a problem hiding this comment.
Might be easier to discuss on a sync call, but I'd personally consider updating the API. Not doing any local calculations was (and maybe still is) one of the stones we have been building the POS around, so if it's solvable by updating the API it might be worth considering. (If we calculate taxes just for display, but the API uses value that it calculates itself, it's not worth it - but if the API uses the value the app provides, I think it might be worth considering to update the API).
There was a problem hiding this comment.
(the same as in store management)
FWIW I personally believe that the fact it's used in store management shouldn't impact our decision on how to go about it in the POS - I mean it's good to know, but the fact we do local calculations in store management shouldn't be a justification for using it in POS. You were technically not saying that it is a justification, but want to be explicit about this because I honestly believe it's important to keep the quality of the POS high - even if it means slower development, changes on backend, and limitations on out-dated versions of Woo core.
There was a problem hiding this comment.
I personally believe that the fact it's used in store management shouldn't impact our decision on how to go about it in the POS - I mean it's good to know, but the fact we do local calculations in store management shouldn't be a justification for using it in POS.
Good call. II think it's a good idea to raise a discussion internally on how to approach the implementation. Thanks for questioning the current logic.
| return Result.success(cached) | ||
| } | ||
|
|
||
| return refreshOrderById(orderId) |
There was a problem hiding this comment.
❓ If I'm reading the content of refreshOrderById correctly, it won't update the cache with the order we fetch, right? There is updateCachedOrderIfPresent method, but this line is unreachable if the order is already in cache, right?
There was a problem hiding this comment.
Yes. The refreshOrderById returns the fetched order, but it doesn't do an upsert. This is a pattern used in WooPosOrdersViewModel too, and I didn't want to interfere with the existing caching logic in this PR.
| order: Order, | ||
| refundableItems: List<WooPosRefundableItem> | ||
| ): WooPosRefundState.Content { | ||
| val subtotal = refundableItems.sumOf { it.lineTotal } |
There was a problem hiding this comment.
📓 Similar as above - this is technically a local calculation, if there is some solution how we could avoid it that would be best. However, if there isn't, I think we might want to do explicit rounding here (ideally, again based on the setting from Woo core). Wdyt?
There was a problem hiding this comment.
This function matches the logic in store management refunds (fun List<ProductRefundListItem>.calculateTotals(): Pair<BigDecimal, BigDecimal>), so I don't think it's critical. I logged an issue to improve this code: https://linear.app/a8c/issue/WOOMOB-1852/woo-posrefunds-use-decimal-places-rounding-mode-mode-from-api, and will tackle it in a separate PR.
| } | ||
|
|
||
| @Test | ||
| fun `given order with refunds, when init, then calculates remaining refundable items correctly`() = runTest { |
There was a problem hiding this comment.
❓ It feels this test mocks refunds and then verifies whether they are as expected - in order words it tests the mock, wdyt?
There was a problem hiding this comment.
I renamed this method to given order with refunds, when init, then builds content state with refundable items: 036565e
| import java.util.Date | ||
| import kotlin.test.Test | ||
|
|
||
| class WooPosGetRefundableItemsTest { |
There was a problem hiding this comment.
💡 I would consider cleaning up this class. Ideally, each test should set only the data that it needs in order to pass. We could for example introduce a generateOrderItem method with default values - if the test for example needs the total to be $10 it would pass just 10 to the method. This way, it would be clearer what each test is testing and each test would be significantly shorter and easier to maintain. Wdyt?
| rowIndex = 0 | ||
| ), | ||
| WooPosRefundableItem( | ||
| orderItemId = 1L, |
There was a problem hiding this comment.
🔍 It seems this orderItemId is identical to the first one, is this intentional?
P.S. We might want to consider creating a helper generate class here as well. Wdyt?
There was a problem hiding this comment.
Yes, it's explicit to test the scenario of the same orderItemId and different rowIndex. I cleaned up the code to make it more explicit here: 0bebfe6
…efundable-items-calculation
samiuelson
removed
the
status: do not merge
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #15056 +/- ##
============================================
- Coverage 38.68% 38.66% -0.02%
- Complexity 10332 10362 +30
============================================
Files 2164 2168 +4
Lines 122666 123004 +338
Branches 16941 16977 +36
============================================
+ Hits 47450 47561 +111
- Misses 70413 70638 +225
- Partials 4803 4805 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
WOOMOB-1802 part 1/2
This PR introduces foundational components for handling refunds in the Woo POS. It adds business logic to calculate refundable items from orders by accounting for previous refunds, along with the necessary ViewModel and state management for the refund UI.
Key changes:
Test Steps
Images/gif
N/A