[Woo POS][Local Catalog] Check catalog size in sync coordinator

[joshheald]

Description

This PR adds a 1000 item limit for full and incremental syncs.

1. When we check whether we can perform a sync, we perform two minimal GET requests (products and variations, 1 item each, ids only.)
2. We sum those, and if they're over 1000 we don't return shouldPerform (for full syncs) and block incremental syncs.

Originally I used HEAD requests, which work for site credentials but for some reason don't make it through our networking stack with Jetpack.

Steps to reproduce

1. Launch the app and switch to a large catalog site
2. Observe that syncs don't occur, and the reason is logged.
3. Switch to a smaller site
4. Observe that syncs happen as normal

Screenshots

---

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

[wpmobilebot]

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

	App Name	WooCommerce iOS Prototype
	Build Number	pr16119-cea0e8e
	Version	23.2
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	cea0e8e
	Installation URL	5srtg2g8s3ii8

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

[joshheald]

@jaclync Sorry this is so long, it's a bit ridiculous for checking two numbers. I could remove some tests, but perhaps it's better to keep them.

[jaclync]

❓ A high-level question: in the sync coordinator, how about fetching the eligibility state async, caching it throughout the app session (until logout or app relaunch), and exposing it so that the POS item data source can determine whether to display data from the local catalog or the pre-existing remote approach? This way, the local catalog feature availability is consistent, and we don't have to handle edge cases when the availability changes from any of the conditions in the middle of any catalog tasks. Merchants also have the same POS UX throughout the app session.

In the PR, the eligibility check is made for each full/incremental sync trigger. If the store was first eligible (for the catalog feature) with full sync completed, then store size grew to be ineligible in the next sync, adding in the variable that the merchant could be using POS at the moment, it felt like there could be quite many scenarios to handle. It also adds the number of network requests to check the catalog size + remote feature flag as a secondary consideration.

WDYT?

---

The rest of the PR changes (Networking and POSCatalogSizeChecker + tests) look good 👍

[jaclync]

nit: the catalog size checker seems like an internal tool within Yosemite, do you foresee this being used in the app layer?

[joshheald]

Yes. For example, we'll want to check the size when we decide whether to use the existing approach or the local catalog, which needs to be decided in the app layer.

[jaclync]

Given that there are other sync/async conditions for the local catalog feature as in p1757587795454369/1757587788.433699-slack-C070SJRA8DP, I thought we might have another high-level checker for all the conditions (and implement caching if we decide to). So that this class is for internal use in Yosemite. WDYT?

[jaclync]

nit: are variationCountDelay and productCountDelay used in tests?

[joshheald]

Thanks for the review @jaclync – please could you mark it as approved though? I think any caching would need to happen outside this PR because it's already huge.

❓ A high-level question: in the sync coordinator, how about fetching the eligibility state async, caching it throughout the app session (until logout or app relaunch), and exposing it so that the POS item data source can determine whether to display data from the local catalog or the pre-existing remote approach? This way, the local catalog feature availability is consistent, and we don't have to handle edge cases when the availability changes from any of the conditions in the middle of any catalog tasks. Merchants also have the same POS UX throughout the app session.

In the PR, the eligibility check is made for each full/incremental sync trigger. If the store was first eligible (for the catalog feature) with full sync completed, then store size grew to be ineligible in the next sync, adding in the variable that the merchant could be using POS at the moment, it felt like there could be quite many scenarios to handle. It also adds the number of network requests to check the catalog size + remote feature flag as a secondary consideration.

WDYT?

I think we could cache the eligibility state, but I think an app session might be too long. Especially for a POS where the iPad may be set to never sleep (Povilas has experience of companies doing this from previous work.)

I agree there are some edge cases around the catalog size growing. I'm just not sure we that solve them all perfectly is a good use of time, for this temporary limit. It'd still be confusing with the cache, just at a different time.

Adding a cache usually adds different edge cases around cache invalidation and expiry. The onboarding state cache is an example of one which appears to work well. In onboarding checks, we ignore the cached value if it's not success... I believe this was to minimise the edge case handling.

If we add a cache, it should probably cover all the criteria for eligibility in the same place – at the moment, that will either require a refactor, or putting the cache in the app layer. It can still use this size checker in Yosemite though, and the more of that business logic we can move to Yosemite, the better, IMO.

[jaclync]

Thanks for the review @jaclync – please could you mark it as approved though? I think any caching would need to happen outside this PR because it's already huge.

Yup makes sense, just finished reviewing the coordinator changes.

I think we could cache the eligibility state, but I think an app session might be too long. Especially for a POS where the iPad may be set to never sleep (Povilas has experience of companies doing this from previous work.)

What do you think is a good duration for the same local catalog feature availability? I don't have a good answer myself, just thought that an app session is the simplest in terms of implementation, and the UX stays the same for merchants until they manually relaunch the app or log out.

I agree there are some edge cases around the catalog size growing. I'm just not sure we that solve them all perfectly is a good use of time, for this temporary limit. It'd still be confusing with the cache, just at a different time.

Adding a cache usually adds different edge cases around cache invalidation and expiry. The onboarding state cache is an example of one which appears to work well. In onboarding checks, we ignore the cached value if it's not success... I believe this was to minimise the edge case handling.

If we add a cache, it should probably cover all the criteria for eligibility in the same place – at the moment, that will either require a refactor, or putting the cache in the app layer. It can still use this size checker in Yosemite though, and the more of that business logic we can move to Yosemite, the better, IMO.

I was thinking a simple cache like the GeneralStoreSettings which is used for POS tab visibility caching. During site initialization (app launch in logged-in state / after logging in / after switching stores), it (the high-level eligibility checker) always checks eligibility and caches this value for background tasks. The feature availability remains the same value until the next check. I agree that this cached value should include all the checks.

Feel free to go with the current implementation, and I can work on the high-level eligibility checker (with or without caching depending on the decision) later in WOOMOB-1287.

[jaclync]

nit: the use case in MainTabBarController still calls the existing shouldPerformFullSync, what use cases do you think will call this new method with maxCatalogSize? If for unit testing, we could consider passing the max size in the coordinator initializer.

Similar question for the performIncrementalSyncIfApplicable method.

[joshheald]

It was mostly for unit tests, but yes, those could come in from the co-ordinator. Another potential use case would be making it a setting to allow users to override our limits... but I'll remove these for now.

[jaclync]

nit: we could keep the 1000 default as a constant. Same in performIncrementalSyncIfApplicable.

[joshheald]

What do you think is a good duration for the same local catalog feature availability? I don't have a good answer myself, just thought that an app session is the simplest in terms of implementation, and the UX stays the same for merchants until they manually relaunch the app or log out.

I don't think it needs to be particularly long. 24h maximum, potentially as low as 1hr, but I'd agree it could also reset for app launch or log out.

I was thinking a simple cache like the GeneralStoreSettings which is used for POS tab visibility caching. During site initialization (app launch in logged-in state / after logging in / after switching stores), it (the high-level eligibility checker) always checks eligibility and caches this value for background tasks. The feature availability remains the same value until the next check. I agree that this cached value should include all the checks.

Feel free to go with the current implementation, and I can work on the high-level eligibility checker (with or without caching depending on the decision) later in WOOMOB-1287.

I'll merge it with the current approach, let's iterate as we go. Thanks!