Skip to content

Conversation

@toupper
Copy link
Contributor

@toupper toupper commented Nov 6, 2025

Description

With this PR we fix the case when we were not showing refunds in the POS Orders Details screen if they were not shown locally. To fix this, we fetch them remotely if that is the case.

Decisions

I decided to keep fetching refunds synchronously within the order details flow. While this may occasionally delay rendering when a remote fetch is needed, it simplifies the implementation and ensures consistent, complete order details without extra state handling. If we don’t want to block order-details rendering on a possible remote refund fetch, then we need to separate “build the details” from “enrich the details with refunds.” Happy to hear your thoughts on this one.

1. Simplicity matters right now. Your ViewModel is already doing a lot (search, pagination, selection, mapping, analytics). If we start doing “show details first, then patch in refunds,” we add another update path to the already big state machine.

2. Refunds are a special case. Most orders won’t have refunds (refundTotal == 0), and you already short-circuit those. So the “slow” path only happens on refunded orders — acceptable.

3. Consistency > speed (for details). Order details is the kind of screen where people expect numbers to match. Loading everything in one go guarantees the breakdown, refunds, and net payment are in sync.

4. You can always optimize later. If QA or users report “refunded orders open slowly,” then we do the split: local details first, then enrich with refunds. That refactor is easy because the logic is already isolated in a use case.

Update: Blocking UI is not acceptable. See below for further discussion.

  • I added the refundTotal == 0 guard inside WooPosRetrieveOrderRefunds rather than at the call site. This makes the use case responsible for deciding when refund retrieval is relevant and avoids redundant calls to the store or network for non-refunded orders, simplifying usage for callers.~~

Test Steps

  1. Go to POS
  2. Open Orders
  3. Go to a Refunded order. Make sure the refunds weren't fetched before
  4. Check that refunds and net payment appear in the order details.

Images/gif

Screen_Recording_20251111_152529_Woo.Dev.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@toupper toupper added this to the 23.6 ❄️ milestone Nov 6, 2025
@toupper toupper requested a review from kidinov November 6, 2025 14:13
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 6, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App NameWooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit76683f1
Direct Downloadwoocommerce-wear-prototype-build-pr14915-76683f1.apk

@kidinov kidinov self-assigned this Nov 6, 2025
import javax.inject.Inject

/**
* Use case that retrieves all refunds for the given [Order].
Copy link
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 sure how much value this comment currently adds, as it simply translates to the English code below. However, if the implementation changes in the future, the comment is likely not to be updated and may actually confuse the code reader rather than clarify it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, let's remove it. I wanted to add information that it's not explicitly stated in the API, but the code is indeed sufficiently clear to obtain that info from there. Done in d6b20cd

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 6, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit76683f1
Direct Downloadwoocommerce-prototype-build-pr14915-76683f1.apk

Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

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

@toupper I think we cannot release it like that. Currently, even if all goes well, it can take up to 30 seconds to load the refunds, and all this time an order selection is blocked. Here is a video where I added a 5s delay to WooPosRetrieveOrderRefunds

11-06--15-40.mp4

One option is that we have to have both loading and error states for the details part of the screen. And load or fetch refunds and products for an order when selected.
Another option would be to fetch all the refunds in parallel for all the orders right after a page of orders is fetched. Assuming that most of the orders won't have refunds, maybe this is an even better option.

Also, what we should consider. To show product images, we get products from the in-memory cache right now. But if the store has more than 200 products, then a product may not be in memory, and we'll try to fetch it from the backend as well (WooPosGetProductById) causing the same issue as with the refunds right now. So if we go with the option 2 we'd need to fetch them along with the refunds.

Wdyt?

@kidinov
Copy link
Contributor

kidinov commented Nov 10, 2025

@toupper I played with this and here is the patch:
Refactor_refund_retrieval_to_return_Result_type_and_handle_errors_gracefully.patch

I decided to not touch the products as:

  • It seems like the local catalog is around the corner, so for most cases it will be in there
  • If the product is not there and not fetched via WooPosProductsRemoteDataSource which should be highly unlikely, we just won't show the image of it

@toupper
Copy link
Contributor Author

toupper commented Nov 11, 2025

Thanks for the patch and adding more context @kidinov! I applied and adapted tests. Please take another look

@toupper toupper requested a review from kidinov November 11, 2025 14:30

private suspend fun mapOrderDetails(order: Order): OrderDetailsViewState.Computed.Details = coroutineScope {
val statusText = order.status.localizedLabel(resourceProvider, locale)
// inside WooPosOrdersViewModel
Copy link
Contributor

Choose a reason for hiding this comment

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

np: what is this comment about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I removed it in 76683f1

@kidinov kidinov self-requested a review November 12, 2025 08:55
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

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

LGTM! There is a comment that is not clear if it was intended to be placed, though.

@toupper toupper merged commit 8834f98 into fix/pos-historical-orders-fixes-for-beta Nov 12, 2025
6 of 7 checks passed
@toupper toupper deleted the fix/pos-historical-orders-show-refunds-net-payment branch November 12, 2025 10:16
@toupper toupper mentioned this pull request Nov 13, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants