[WOOMOB-382] Update Coupons Fetching/Loading Business Logic

[malinajirka]

Closes: #WOOMOB-382

Do not merge label - #14015 needs to be merged first.

Description

This PR updates the behavior of the coupons list in the POS.

• Show cached data when available.
• Show PTR loading indicator when fetching from remote and cached data shown to indicate a request is in progress.
• Prevent users from starting another PTR when the previous one is in progres.

Testing information

1. Clean coupons cache
2. Open POS
3. Switch to Coupons tab
4. Notice "fullscreen" loading is shown (since cache is empty)
5. Notice the data are shown when the request finishes

---

1. Exit POS
2. Open POS
3. Switch to Coupons tab
4. Notice cached data are shown and PTR indicator is shown since the remote request is in progress
5. Notice the coupons cache is replaced with the fetched data

---

1. Pull to refresh
2. Notice a loading indicator is shown
3. Notice you can't pull to refresh again until the request finishes
4. Notice the loading indicator disappears when the request finishes and the data are replaced with the fetched data

The tests that have been performed

I've tested the above.

Images/gif

Screen.Recording.2025-05-06.at.8.59.33.mov

• 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.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

• The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
• Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
• Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

[wpmobilebot]

📲 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 Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	ada3e0b
	Direct Download	woocommerce-wear-prototype-build-pr14019-ada3e0b.apk

[wpmobilebot]

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

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	ada3e0b
	Direct Download	woocommerce-prototype-build-pr14019-ada3e0b.apk

[kidinov]

Why it needs to be an extension of WooPosCouponsListViewStateManager?

[kidinov]

Also, the naming is a bit confusing. The method itself doesn't show anything and can create it from any coupons, not necessary from cached. Maybe simply createContentViewState?

[malinajirka]

Agree, I'll update this.

[malinajirka]

Fixed in c9d9efb

[kidinov]

A few things I noticed:

• The PTR is shown when it's not triggered by the user in a few cases

05-06--15-27.mp4

05-06--15-12.mp4

• On the empty state the PTR is not working properly as it's closed before the remote request comes back

05-06--15-34.mp4

• Not related to the PR - empty state is at the top

[malinajirka]

Thanks for the review @kidinov! All the issues you mentioned don't feel like issues to me - I mean I implemented the behavior like this intentionally (except the vertical alignment). Wdyt @samiuelson, how does the behavior feel to you? Long story short, the app displays Loading more indicator in progress when we show cache data and a remote request is in progress. If we are not showing cached data and remote request is in progress we display the shimmering effect rows.

The PTR is shown when it's not triggered by the user in a few cases

I added it intentionally, isn't it desired to show we are loading something? Or how do we show that the remote request is in progress?

Not related to the PR - empty state is at the top

This is already fixed in one of the other open PRs.

On the empty state the PTR is not working properly as it's closed before the remote request comes back

Again, I believe this is desired - we are showing "fullscreen" loading in this state and showing two loading approaches on top of each other feels off, doesn't it?

[kidinov]

@malinajirka 👋

Aside from that, it feels off to me (this may not be important at all 😀); more important is that it's at least partially against the guidelines and not consistent with the behavior of the products and variations in the POS

The refresh indicator appears only in conjunction with a refresh gesture or action. Syncing does not display a refresh indicator.

The refresh indicator remains visible until the refresh activity completes and any new content is visible, or the user navigates away from the refreshing content.

https://m2.material.io/design/platform-guidance/android-swipe-to-refresh.html#usage

[malinajirka]

Thanks for sharing these references!

On the empty state the PTR is not working properly as it's closed before the remote request comes back

👍 I'll update this one to continue showing the indicator until the request finishes to match material 3 guidelines.

Update: This is fixed now.

The refresh indicator appears only in conjunction with a refresh gesture or action. Syncing does not display a refresh indicator.

I believe tapping on retry button doesn't go against the guideline. It's still a refresh gesture or action. Wdyt?

more important is that it's at least partially against the guidelines

Not showing any indicator when action is in progress and then out of no-where replacing the displayed data or even worse replacing them with network error is very likely against the guidelines as well. (btw we show the PTR indicator on order list and product lists as well when remote request is in progress - I'm not saying that's a reason we should match designs in the POS, just saying that it's not something new in our app and not something users or developers reported AFAIK)

It seems that guidelines say we should be showing a horizontal indicator at the top of the list -https://m3.material.io/components/progress-indicators/guidelines#0804b871-234a-44d3-be91-0ec85e161ee5. I think we can consider updating the lists to match this. (AFAIK this doesn't affect product list since it's loaded on POS load.)

However, regarding and not consistent with the behavior of the products and variations in the POS: those list are different in more aspects - not observable, data fetched on POS load, coupons are stored in persistent storage, ... . We could consider bringing them to consistency, however, not showing any indicator when load is in progress is imo a bug not something we should try to implement across the POS.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.

Project coverage is 38.34%. Comparing base (b4df658) to head (ada3e0b).
Report is 62 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...oopos/home/items/coupons/WooPosCouponsViewModel.kt	0.00%	4 Missing ⚠️
...items/coupons/WooPosCouponsListViewStateManager.kt	96.87%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #14019      +/-   ##
============================================
+ Coverage     38.33%   38.34%   +0.01%     
- Complexity     9540     9545       +5     
============================================
  Files          2122     2122              
  Lines        116797   116817      +20     
  Branches      14986    14989       +3     
============================================
+ Hits          44776    44796      +20     
  Misses        67913    67913              
  Partials       4108     4108              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[malinajirka]

I plan to test this a bit more, but I've updated the behavior as you suggested. Thanks for sharing the examples of the gmail/whatsapp on Slack - those truly don't show any loading indicator when they are updating the cache. I still think we should either show it or we should continue showing the cached data when remote request fails, but we iterate on that later.

I'll re-request review after I test all the changes.

[malinajirka]

I've rested it and it seems it all works now as you suggested. Would appreciate if you could re-review it, thanks!

[kidinov]

@malinajirka 👋

I believe tapping on retry button doesn't go against the guideline. It's still a refresh gesture or action. Wdyt?

I don't see that they say anything specific about this case, and they allow it to be displayed when data is requested to be refreshed, e.g. this video:

mio-staging_mio-design_1579302979877_assets_1OKTKpogOxe023_RcmqB6OBYquG6sRyc7_patterns-swipetorefresh-tap.mp4

Refreshing content generated by an app bar action.

But IMO, it's better to display a skeleton, as the primary goal of the PTR is to be a swipe gesture that combines the gesture with the indicator. In this case, it's just the indicator, without the gesture. Another point is the consistency with the rest of the lists - I believe they don't have PTR enabled for the error states at all.

That said, if the chosen architecture makes it difficult to implement this, I think it's ok to leave it as it is, since this error state, which hopefully nobody will see anyway

Not showing any indicator when action is in progress and then out of nowhere replacing the displayed data or even worse replacing them with network error is very likely against the guidelines as well.

I believe this was already discussed on Slack. We verified that numerous major apps all display cached data without any loading indicators for new data fetching, updating the UI only when the data arrives. In my opinion, this approach is sensible, particularly for our situation where the data does not change frequently. Therefore, in most cases, merchants will begin using the app without seeing for any background processes we may be running

It seems that guidelines say we should be showing a horizontal indicator at the top of the list -https://m3.material.io/components/progress-indicators/guidelines#0804b871-234a-44d3-be91-0ec85e161ee5. I think we can consider updating the lists to match this. (AFAIK this doesn't affect product list since it's loaded on POS load.)

I don't see anywhere that it says that. Both video and text say that:

Progress indicators inform users about the status of ongoing processes, such as loading an app, submitting a form, or saving updates.

But nothing like this is happening here. We just sync the data. In the video examples, they show that they have no data to display; therefore, there is a loading indicator. By the way, I completely don't think that we need to introduce a third loading indicator on top of the skeleton and PTR here.

However, regarding and not consistent with the behavior of the products and variations in the POS: those list are different in more aspects - not observable, data fetched on POS load, coupons are stored in persistent storage, ... .

The lists themselves are the same from the user's point of view, and I understand that's the technical approach you've decided on, but it shouldn't truly affect the overall user experience. Overall, if the architecture has limitations on such small things like when to display the PTR or when to show a skeleton, I believe it's possibly not the best choice for this particular case.

[malinajirka]

@kidinov Thanks for the reply!

I'm a bit confused though - I updated the app to behave as you suggested, do you still see some issues with the current approach?

[kidinov]

@malinajirka 👋

Sorry, I didn't test the code yet, just answered you 😀

I missed this comment 😮‍💨

I've rested it and it seems it all works now as you suggested. Would appreciate if you could re-review it, thanks!

Testing it now!

[kidinov]

Looks nice, thanks!

The only things I noticed:

• Instead of the skeleton, we show the cached data when there is a retry on the error screen
• I think when it's PTR on the empty screen, we should just show PTR. No need to show two loading indicators, but I guess this is more like NP
• Not sure about that, but it looks like both empty and error screens have a larger margin at the top than at the bottom. I didn't measure that, just how it looks to my eye, so maybe it's not correct
• I think I mentioned this already before, but it looks like we use color for the empty state icon that has not have enough of contrast with the background

05-08--11-32.mp4

05-08--11-30.mp4

[malinajirka]

Thanks for the re-review @kidinov !

Instead of the skeleton, we show the cached data when there is a retry on the error screen

I originally didn't update this since it was consistent with the product list, but looking at it it's not great UX. I've fixed this state on the coupon list - it's still happening on the product list though. When the internet is down, the error arrives so quickly that it's not visible, but on poor internet connection the cached data are shown. Do you think it's worth updating there? If so, could you please update it?

No Data in Cache	Data in cache
Screen.Recording.2025-05-08.at.14.49.02.mov	Screen.Recording.2025-05-08.at.14.52.03.mov

I think when it's PTR on the empty screen, we should just show PTR. No need to show two loading indicators, but I guess this is more like NP

Make sense, however, this feels like a state that would make the code more complicated => more costly to maintain => the value for the user is IMO zero, the value for the company is therefor likely negative.

I think I mentioned this already before, but it looks like we use color for the empty state icon that has not have enough of contrast with the background

A couple week ago Wagner mentioned he plans to polish all the error/empty screen and we should ignore them for now.

Not sure about that, but it looks like both empty and error screens have a larger margin at the top than at the bottom. I didn't measure that, just how it looks to my eye, so maybe it's not correct

It seems to be center correctly (340px from top and bottom) - assuming we are centering it within the list not center of the screen.

Top	Bottom

[kidinov]

@malinajirka

I originally didn't update this since it was consistent with the product list, but looking at it it's not great UX. I've fixed this state on the coupon list - it's still happening on the product list though. When the internet is down, the error arrives so quickly that it's not visible, but on poor internet connection the cached data are shown. Do you think it's worth updating there? If so, could you please update it?

Apologies, but I'm somewhat unclear about that. Update what exactly? We have two ways to address remote request failures while utilizing a local cache:

• We display an error screen when a remote request fails because we want to prioritize the accuracy of the remote data, even if we have cached data. Therefore, the cache is invalidated if the remote fails. In this case, when we click retry, we show a skeleton to indicate that we are loading data. That's how the product list is currently implemented.
• We don't display the error screen when a remote request fails and we have cached data, as we show cached data in this case; thus, we clearly don't have a retry button. However, we do show the error screen when a remote request fails and we lack cached data; therefore, we cannot display cached data when the retry button is clicked, as we don't have it.

So in both cases, the retry button cannot show cached data, only a skeleton. Here, it seems that a mix of both was implemented. If we have cached data, we should not show the error screen at all

Make sense, however, this feels like a state that would make the code more complicated => more costly to maintain => the value for the user is IMO zero, the value for the company is therefor likely negative.

I won't say that it's zero. For me, small things like that create an impression of a good product where people did their best. However, I agree that it's probably not the case for most people, and the impact is really really low.

As per implementation, it largely depends on the selected architectural approach. In the products, this is managed by those lines of code:

            _viewState.value = if (withPullToRefresh) {
                buildProductsReloadingState()
            } else {
                WooPosProductsViewState.Loading()
            }

Which, in my opinion, isn't very complicated. However, if you find it complicated in your situation, it's perfectly acceptable to leave it as is

A couple week ago Wagner mentioned he plans to polish all the error/empty screen and we should ignore them for now.

👍

It seems to be center correctly (340px from top and bottom) - assuming we are centering it within the list not center of the screen.

👍

[kidinov]

LGTM. Please go ahead with mering if you feel it's good to go

[malinajirka]

Apologies, but I'm somewhat unclear about that. Update what exactly? We have two ways to address remote request failures while utilizing a local cache:

We display an error screen when a remote request fails because we want to prioritize the accuracy of the remote data, even if we have cached data. Therefore, the cache is invalidated if the remote fails. In this case, when we click retry, we show a skeleton to indicate that we are loading data. That's how the product list is currently implemented.
We don't display the error screen when a remote request fails and we have cached data, as we show cached data in this case; thus, we clearly don't have a retry button. However, we do show the error screen when a remote request fails and we lack cached data; therefore, we cannot display cached data when the retry button is clicked, as we don't have it.

Interesting, now I'm even more confused 🤔 . How do we decide whether to invalidate the cache after a request failure? What do we display on the UI when a request fails and we continue showing the cached data? (I mean, I'm aware only about the load more failure).

Btw, I have never seen an app which would clear cached data upon a request-failure. It feels like we are trying too be too smart, ending up with worse UX. I think the user can decide whether they are fine continue using the product if the refresh failed. Especially considering the order creation is always remote, so all the calculations are guaranteed to be correct.

Which, in my opinion, isn't very complicated. However, if you find it complicated in your situation, it's perfectly acceptable to leave it as is

Yes, if you don't look at the big picture, I agree handling such cases is simple. It's similar with VM size - at the beginning one might feel like we can put everything there, since it's just 80 lines of code. However, if we continue applying this we end up with 500+ lines of code and the way back is very expensive.

In this case, it might be one if statement, but if we handle 15 such cases, we have 15 if statements in a better case, in the worse case even more since the states affect each other. Every single if statement needs to be understood by whoever wants to change the class => it is often difficult to understand them when they fix random edge cases which often don't matter.

[kidinov]

Interesting, now I'm even more confused 🤔 . How do we decide whether to invalidate the cache after a request failure? What do we display on the UI when a request fails and we continue showing the cached data? (I mean, I'm aware only about the load more failure).

Oh, sorry, I meant that we (now) invalidate it when we do PTR in general, not only when the request fails. We display an error when requests fails.

Yes, if you don't look at the big picture, I agree handling such cases is simple. It's similar with VM size - at the beginning one might feel like we can put everything there, since it's just 80 lines of code. However, if we continue applying this we end up with 500+ lines of code and the way back is very expensive.

In this case, it might be one if statement, but if we handle 15 such cases, we have 15 if statements in a better case, in the worse case even more since the states affect each other. Every single if statement needs to be understood by whoever wants to change the class => it is often difficult to understand them when they fix random edge cases which often don't matter.

I am talking about the specific case where all these cases are handled, on the products list and its relativilty simple VM. As the code suggests, it handles all the cases to show either loading state or PTR... we don't need 15 cases for this.

[malinajirka]

Oh, sorry, I meant that we (now) invalidate it when we do PTR in general, not only when the request fails. We display an error when requests fails.

Ohh I see. The usual practice AFAIK is to clear the cache only when the request succeeds - right before we store the new data into the DB. Either case, since we don't have designs for remote request failed but cached data available state, I'd just go with the current solution which shows fullscreen error when PTR fails.