[POS][Local Catalog] Add top level catalog sync coordinator #16098
Conversation
joshheald
added
type: task
|
|
| self.settingsStore = settingsStore ?? SiteSpecificAppSettingsStoreMethods(fileStorage: PListFileStorage()) | ||
| } | ||
|
|
||
| public func performFullSync(for siteID: Int64) async throws -> POSCatalog { |
There was a problem hiding this comment.
nit: I think we could remove the POSCatalog as the return value, and in the protocol. In POSCatalogFullSyncService.startFullSync, the returned catalog is based on the API response, not the final catalog in the database which is the source of truth for POS. As we discovered on the Large fun testing test site, the API response could include duplicate items.
There was a problem hiding this comment.
Yeah, agreed. I wasn't sure whether it would break anything for you if I did that, but if you're happy I'll take it out.
|
|
||
| // Trigger POS catalog sync if tab is visible and feature flag is enabled | ||
| if isPOSTabVisible, ServiceLocator.featureFlagService.isFeatureFlagEnabled(.pointOfSaleLocalCatalogi1) { | ||
| await triggerPOSCatalogSyncIfNeeded(for: siteID) |
There was a problem hiding this comment.
nit: WDYT about moving posCatalogSyncCoordinator initialization here before this line to consolidate the catalog sync code? So that if the updateTabViewControllers call ever gets moved after this code by accident, it won't just silently skip the full sync.
There was a problem hiding this comment.
❓ Have we decided to trigger the initial full sync if the store is eligible for POS, or if the POS tab is shown (not necessarily eligible)? If the former, we might need to check for eligibility from posEligibilityChecker. Though, it might be simpler to perform a full sync for stores with POS tab visible so that once they fix the ineligibility issue it enters POS right away. I'm not sure if we made a decision cross-platform. In any case, we can make any eligibility updates separately.
There was a problem hiding this comment.
I'm not aware of a decision at that level of detail. I can see arguments for either. How quickly after the tab being shown do we usually know if the store's actually eligible?
If those are generally very close together, we can leave the sync until we know for sure. If it takes some time, e.g. they need to open the tab, I think it's better to sync preemptively.
There was a problem hiding this comment.
How quickly after the tab being shown do we usually know if the store's actually eligible?
If those are generally very close together, we can leave the sync until we know for sure. If it takes some time, e.g. they need to open the tab, I think it's better to sync preemptively.
Currently, we only check the country eligibility & POS remote feature flag on site launch for the POS tab visibility. The rest of the POS eligibility (iOS version, currency, WC plugin version, feature switch in core) are checked when the user taps on the POS tab.
I think we can include the POS tab visibility check in the Local Catalog feature check. Asked a question in p1757465589133769/1757414787.869759-slack-C070SJRA8DP to sync with Android.
|
|
||
|
|
There was a problem hiding this comment.
super nit: could remove extra empty lines
| var startFullSyncResult: POSCatalog = POSCatalog(products: [], variations: []) | ||
| var startFullSyncError: Error? |
There was a problem hiding this comment.
nit: how about a Result<POSCatalog, Error> variable for mocking either success or failure?
| } | ||
|
|
||
| public final class POSCatalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol { | ||
| private let syncService: POSCatalogFullSyncServiceProtocol |
There was a problem hiding this comment.
nit: could be named fullSyncService to differentiate from incremental sync when both syncs are supported.
Part of: WOOMOB-1252
Closes: WOOMOB-1094
Merge after: #16097
Description
This PR adds a basic catalog sync coordinator. For now, it only supports full syncs.
I've removed the previous code for creating the database – this will happen automatically at the end of a successful sync.
Steps to reproduce
Testing information
Note that if you switch during a sync, it will carry on, and start syncing the other site if needed.
This doesn't actually cause a problem, it's just a bit wasteful. Only the last one to save will be persisted in the end, because we currently only persist one site at a time. In future, we will change the primary keys to properly support multiple sites, and then they'll be saved as expected.
RELEASE-NOTES.txtif necessary.