[Woo POS][Local Catalog] Store full sync date on site entity #16115
Conversation
joshheald
added
type: task
|
|
jaclync
left a comment
jaclync
left a comment
There was a problem hiding this comment.
LGTM 🚀 mostly a comment about CIAB files.
| let updatedSite = POSSite( | ||
| siteID: siteID, | ||
| lastIncrementalSyncDate: currentSite?.lastIncrementalSyncDate, | ||
| lastFullSyncDate: Date() | ||
| ) |
There was a problem hiding this comment.
nit: how about the following in case the site properties grow?
| let updatedSite = POSSite( | |
| siteID: siteID, | |
| lastIncrementalSyncDate: currentSite?.lastIncrementalSyncDate, | |
| lastFullSyncDate: Date() | |
| ) | |
| let updatedSite = currentSite.toPOSSite().copy(lastFullSyncDate: Date()) |
| do { | ||
| try await persistenceService.updateSite(updatedSite) | ||
| } catch POSCatalogPersistenceError.siteNotFound { | ||
| // Site doesn't exist yet, this could happen if sync coordinator is called before replaceAllCatalogData |
There was a problem hiding this comment.
nit: fullSyncService.startFullSync above in the same function already calls persistenceService.replaceAllCatalogData, this case seems unexpected and maybe deserves an error log?
| 2DCB54FA2E6AE8E100621F90 /* CIABEligibilityChecker.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2DCB54F92E6AE8D800621F90 /* CIABEligibilityChecker.swift */; }; | ||
| 2DCB54FC2E6AFE6A00621F90 /* CIABAffectedFeature.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2DCB54FB2E6AFE6900621F90 /* CIABAffectedFeature.swift */; }; |
There was a problem hiding this comment.
❓Likely from merge conflicts from trunk, are these 2 files meant to be in this PR?
There was a problem hiding this comment.
Thanks for spotting, fixed
| // Check if we have a stored site from updateSite calls | ||
| if let storedSite = storedSites[siteID] { | ||
| return storedSite | ||
| } | ||
|
|
||
| // Check if we have a specific result for this site | ||
| if let specificSite = siteResults[siteID] { | ||
| return specificSite | ||
| } | ||
|
|
||
| // Fall back to configured result | ||
| switch loadSiteResult { | ||
| case .success(let site): | ||
| return site | ||
| case .failure(let error): | ||
| throw error | ||
| } |
There was a problem hiding this comment.
💭 this feels like a lot of ways to configure the mock setup, and could be tricky to debug. Feel free to leave it as it is and I can make some changes in my PR that will base on this branch.
There was a problem hiding this comment.
I've simplified it a lot by moving the sync date set to the transaction when we insert the catalog.
joshheald
force-pushed
the
woomob-1252-sync-coordinator-store-sync-date-on-site
branch
from
a467350 to
0ffe5d7
Compare
joshheald
deleted the
woomob-1252-sync-coordinator-store-sync-date-on-site
branch
Merge after: #16108
Description
This PR moves the full sync date to the Site entity, to match incremental sync dates.
Steps to reproduce
Something like this should be logged:
RELEASE-NOTES.txtif necessary.