[Woo POS] Coupons: Sync from remote

[iamgabrielma]

Closes: #15324

Description

This PR adds the ability to fetch coupons from remote to the coupons service. First, we attempt to fetch from the remote. If this fails, we'll attempt to fetch from storage. If fails, we just return an empty list of coupons (for the moment)

We might change the fetching strategy, for now it seemed adequate to attempt remote first and local storage later.

Screen.Recording.2025-03-26.at.13.58.58.mov

Testing information

• On POS, tap the coupons button
• Observe the loading screen and coupons being loaded from remote
• Stop the app and throw an error from CouponsRemote.loadAllCoupons():

    public func loadAllCoupons(for siteID: Int64,
                               pageNumber: Int = CouponsRemote.Default.pageNumber,
                               pageSize: Int = CouponsRemote.Default.pageSize) async throws -> [Coupon] {
        throw NSError(domain: "oops", code: -1)
        try await withCheckedThrowingContinuation { continuation in
   ...
    }

• Observe that we'll still load coupons, but this time they come from the storage fallback:

"Failed to load coupons from remote:" Error Domain=oops Code=-1 "(null)"

---

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

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 all devices (phone/tablet) and no regressions are added.

[iamgabrielma]

We might change the fetching strategy, for now it seemed adequate to attempt remote first and local storage later.

I'm not sure but I think we might have discussed to do the opposite? Storage first and remote later?

[wpmobilebot]

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

	App Name	WooCommerce iOS
	Build Number	pr15423-1a23559
	Version	22.0
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	1a23559
	App Center Build	WooCommerce - Prototype Builds #13446

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[staskus]

I'm not sure but I think we might have discussed to do the opposite? Storage first and remote later?

@iamgabrielma Yes, I imagined we could do local first so the results would appear immediately, but update them with remote ones. How does the current IPP implementation work? As I understand, it's storage-first. My idea was to keep the same strategy as IPP to save time on coming up with alternatives.

Observe the loading screen and coupons being loaded from remote

A full-screen loading doesn't work here. A good point is that we need to ask a designer to consider how we display a loading state. Asked here p1742974197820449-slack-C070SJRA8DP

[staskus]

This is how IPP works.

I added a new coupon and returned to the screen. After a moment, a new coupon appeared. We could sync with the design on this as well. Maybe we could show a small loading indicator somewhere as well while the network request is ongoing. Wdyt?

ScreenRecording_03-26-2025.09-36-12_1.mov

[iamgabrielma]

we could do local first so the results would appear immediately, but update them with remote ones. How does the current IPP implementation work? As I understand, it's storage-first. My idea was to keep the same strategy as IPP to save time on coming up with alternatives.

That sounds good, I'll take a deeper look of how we do it in coupons in the app and make the changes to do local first -> then update with remote results. I'll see if I can change the loading indicator on this PR as well or add the "ghost rows" separately 👍

[iamgabrielma]

I've spent quite a bit of time looking at the IPP implementation and I'm no sure we can do the same, or at least not right away, this is mostly because the current design forces us to perform an action in the UI (load, reload, scroll, ... ) when calling the provider for new items, while IPP plays with a SyncCoordinator delegate that listens and acts upon changes in storage/entities.

The sync + upsert handling also relies on calling the Coupons store, which forces us to do the same if we want to reuse what we have rather than reimplementing it.

I'm still unsure if makes more sense to add it in the controller or the service, but for the moment I think the service makes more sense, as all of these are in Yosemite and will be called every time we fetch items from any action, but we can switch them over as needed, perhaps once we implement PTR and UI state changes this will become clearer.

• In POS, load the coupons view
• In site, create new coupons
• in POS, navigate back and forth (it may take a couple of seconds)
• Observe how the list is updated

Screen.Recording.2025-03-31.at.12.30.31.mov

• We also get PTR as a side-effect, since we sync after fetching

Screen.Recording.2025-03-31.at.13.01.20.mov

[staskus]

Thanks, @iamgabrielma!

I'm no sure we can do the same, or at least not right away, this is mostly because the current design forces us to perform an action in the UI (load, reload, scroll, ... ) when calling the provider for new items, while IPP plays with a SyncCoordinator delegate that listens and acts upon changes in storage/entities.

Yes, it looks like it's not possible to copy the entire solution and we need to add our details.

Do I understand correctly that the challenge is combining local and remote updates so the POS would be aware when both happen?

The sync + upsert handling also relies on calling the Coupons store, which forces us to do the same if we want to reuse what we have rather than reimplementing it.

I think it's fine. Let's reuse the store. 👍

I'm still unsure if makes more sense to add it in the controller or the service, but for the moment I think the service makes more sense, as all of these are in Yosemite and will be called every time we fetch items from any action, but we can switch them over as needed, perhaps once we implement PTR and UI state changes this will become clearer.

Sounds good!

---

If the main challenge is communicating UI updates, I think it should be achievable.

• The service could use AsyncSequence or some more old-school patterns (combine / delegates) to notify the controller, and then remote calls can be finished.
• Controller could update itemsViewState when notified from service.

We'll probably need some additional flags within PointOfSaleItemsControllerProtocol to indicate that the loading is still happening so it could be reflected within the UI.

What do you think? What are your main challenges? Which state do you want to reach in the scope of this PR?

[staskus]

super nit; not loading all coupons here

Suggested change

	public func loadAllCoupons(for siteID: Int64,
	public func loadCoupons(for siteID: Int64,

[iamgabrielma]

I'm not sure to understand: loadAllCoupons loads all while loadCoupons loads a subset which we know the couponIDs of, right? "makes a single request for a list of coupons" but we don't have that list so we have to fetch them all.

[staskus]

I just meant from a readability point of view. loadAllCoupons sounds like we're loading ALL coupons, rather than specific page. But I see now that CouponsRemoteProtocol has loadAllCoupons and loadCoupons which is different. 👍 So we can keep it as it is. 👍

[iamgabrielma]

Ah gotcha, I agree the names are a bit confusing thought, I'll see if I can sneak a task separately to rename these better.

[staskus]

It's fine, not necessary 👍

[staskus]

Idea: If coupons is empty, we could wait for syncCouponsFromRemote to complete before returning PagedItems.

[iamgabrielma]

That sounds good, updated here: 1a23559 . I was a bit wary of using a detached Task attached to a dispatcher action, but from testing seems to work fine, let me know if raises any red flag:

• If non empty, we return storage and fire a sync task which will complete at some point
• If empty, we wait for the sync both remote and storage, then return that

[staskus]

I was a bit wary of using a detached Task attached to a dispatcher action, but from testing seems to work fine

I think it's a right usage of a detached Task. We want the storage to be updated with the newest coupons independent from the providePointOfSaleItems.

[iamgabrielma]

Do I understand correctly that the challenge is combining local and remote updates so the POS would be aware when both happen?

Yes, the question depends a bit on another one: Do we want the UI to be aware of this changes? Or we want to rely on an user action to communicate those (as we do for products currently)? Let me explain:

The current interface for products allows us to fetch items via calling the provider explicitly, and we do so through either:

• loadItems (initial call)
• refreshItems (PTR)
• loadNextItems (pagination)

But we don't update the existing list automatically if the remote is updated, it needs to navigate back, to PTR, or to trigger scrolling, so we fetch/sync on demand.

With coupons we're introducing a different approach: We have something in storage, maybe something different in remote. We fetch and display storage while simultaneously updating the data with the latest remote results, and making the UI update automatically if there are changes.

Cue to:

What are your main challenges? Which state do you want to reach in the scope of this PR?

In both cases (if we want the UI to update automatically or upon an action) I'd say the scope for this PR could be limited to what's now:

• Fetch first page from storage.
• Fetch first page from remote.
• Upon PTR or navigating back, update the list.

I'd leave the UI loading state, and pagination for separate PRs, specially since the current IPP coupons implementation relies heavily on pagination to interact with storage.

What is your position on the UI update for coupons? It feels nicer if we update them automatically if these changes, but at the same time it's a different behaviour that the one we have now 🤔

[staskus]

@iamgabrielma, appreciate the explanation!

What is your position on the UI update for coupons? It feels nicer if we update them automatically if these changes, but at the same time it's a different behaviour that the one we have now 🤔

Well, I think we should update it to the newest state once it's loaded. We could try keeping it simple at first and update the controller itemListState when the service communicates that the remote was refreshed. I don't know how it all connects to pagination and PTR, you're aware of details better. But yes, updating list when remote updates would be good 👍

[staskus]

Looks good 👍 made minor comments

We can add testing, handling remote updates, and improve loading state if another PR if you prefer.

If you make larger changes within this PR, please re-request the review.

[staskus]

Should we use a CouponsRemoteProtocol here to support mocking the remote in unit tests?

Or is it preferred to mock Network instead in the project?

Suggested change

	private let couponsRemote: CouponsRemote
	private let couponsRemote: CouponsRemoteProtocol

[iamgabrielma]

Good question, I'm not sure what we'd want to test better right now: Passing a predefined response from mocking the network, or mocking the remote logic, since a lot of it should be covered by pre-existing tests. Perhaps both! I'll take a look into it asap separately with some unit tests.

[staskus]

I was a bit wary of using a detached Task attached to a dispatcher action, but from testing seems to work fine

I think it's a right usage of a detached Task. We want the storage to be updated with the newest coupons independent from the providePointOfSaleItems.

[staskus]

Theoretically, PointOfSaleCouponService doesn't need or perhaps even shouldn't be PointOfSaleItemServiceProtocol. It's an independent service with its own logic. It doesn't have to fit the same protocol defined for items.

Having a separate protocol allows us to define an interface that would support our needs. E.g., notifying a controller when remote updates.

public func providePointOfSaleItems(pageNumber: Int) could maybe even be AsyncSequence that returns first round of items from storage and another round when storage is updated with remote. I don't know if we have used it anywhere else yet but maybe could be interesting to try out.

[iamgabrielma]

That's a very good point, I initially stuck with conforming to the PointOfSaleItemServiceProtocol to follow the same design as products, but its making things more rigid.

As long as we keep the same protocol for the controller, we can switch them in the dashboard to switch between products and coupons (currentController), but the service that feeds it can be independent, which should give us more flexibility. I'll do that as a next step after merging this one just to keep changes separate, but as you mention, this should clarify/simplify the path for the rest of changes.

[staskus]

It's fine, not necessary 👍