-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Local Catalog] Save parsed full catalog into database #16087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
joshheald
merged 11 commits into
trunk
from
woomob-1093-woo-poslocal-catalog-save-parsed-full-catalog-into-database
Sep 5, 2025
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
faeb1c5
Ensure variation images are deleted with cascade
joshheald b12bbec
Add persistence helper in Yosemite
joshheald 442f2cc
Use a single transaction to write all entities
joshheald d0b1e6b
Test saving the catalog to the database
joshheald f2b005d
Periphery fixes and ignore
joshheald ae7aab8
Clear data in replaceAllCatalogData transaction
joshheald bad8441
Remove commented out `deleteOne` ideal code
joshheald d3a7978
Use mock from property
joshheald ce9afe1
Make image test match current (flawed) implementation
joshheald 501e954
Add test for variation image/attribute removal
joshheald 76b396b
Merge branch 'trunk' into woomob-1093-woo-poslocal-catalog-save-parse…
joshheald File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
105 changes: 105 additions & 0 deletions
105
Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| // periphery:ignore:all | ||
| import Foundation | ||
| import Storage | ||
|
|
||
| protocol POSCatalogPersistenceServiceProtocol { | ||
| /// Clears existing data and persists new catalog data | ||
| /// - Parameters: | ||
| /// - catalog: The catalog to persist | ||
| /// - siteID: The site ID to associate the catalog with | ||
| func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws | ||
| } | ||
|
|
||
| final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { | ||
| private let grdbManager: GRDBManagerProtocol | ||
|
|
||
| init(grdbManager: GRDBManagerProtocol) { | ||
| self.grdbManager = grdbManager | ||
| } | ||
|
|
||
| func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { | ||
| DDLogInfo("💾 Persisting catalog with \(catalog.products.count) products and \(catalog.variations.count) variations") | ||
|
|
||
| try await grdbManager.databaseConnection.write { db in | ||
| DDLogInfo("🗑️ Clearing catalog data for site \(siteID)") | ||
| // currently, we can't save for more than one site as entity IDs are not namespaced. | ||
| try PersistedSite.deleteAll(db) | ||
|
|
||
| let site = PersistedSite(id: siteID) | ||
| try site.insert(db) | ||
|
|
||
| for product in catalog.productsToPersist { | ||
| try product.insert(db, onConflict: .ignore) | ||
| } | ||
|
|
||
| for image in catalog.productImagesToPersist { | ||
| try image.insert(db, onConflict: .ignore) | ||
| } | ||
|
|
||
| for var attribute in catalog.productAttributesToPersist { | ||
| try attribute.insert(db) | ||
| } | ||
|
|
||
| for variation in catalog.variationsToPersist { | ||
| try variation.insert(db, onConflict: .ignore) | ||
| } | ||
|
|
||
| for image in catalog.variationImagesToPersist { | ||
| try image.insert(db, onConflict: .ignore) | ||
| } | ||
|
|
||
| for var attribute in catalog.variationAttributesToPersist { | ||
| try attribute.insert(db) | ||
| } | ||
| } | ||
|
|
||
| DDLogInfo("✅ Catalog persistence complete") | ||
|
|
||
| try await grdbManager.databaseConnection.read { db in | ||
| let productCount = try PersistedProduct.fetchCount(db) | ||
| let productImageCount = try PersistedProductImage.fetchCount(db) | ||
| let productAttributeCount = try PersistedProductAttribute.fetchCount(db) | ||
| let variationCount = try PersistedProductVariation.fetchCount(db) | ||
| let variationImageCount = try PersistedProductVariationImage.fetchCount(db) | ||
| let variationAttributeCount = try PersistedProductVariationAttribute.fetchCount(db) | ||
|
|
||
| DDLogInfo("Persisted \(productCount) products, \(productImageCount) product images, " + | ||
| "\(productAttributeCount) product attributes, \(variationCount) variations, " + | ||
| "\(variationImageCount) variation images, \(variationAttributeCount) variation attributes") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private extension POSCatalog { | ||
| var productsToPersist: [PersistedProduct] { | ||
| products.map { PersistedProduct(from: $0) } | ||
| } | ||
|
|
||
| var productImagesToPersist: [PersistedProductImage] { | ||
| products.flatMap { product in | ||
| product.images.map { PersistedProductImage(from: $0, productID: product.productID) } | ||
| } | ||
| } | ||
|
|
||
| var productAttributesToPersist: [PersistedProductAttribute] { | ||
| products.flatMap { product in | ||
| product.attributes.map { PersistedProductAttribute(from: $0, productID: product.productID) } | ||
| } | ||
| } | ||
|
|
||
| var variationsToPersist: [PersistedProductVariation] { | ||
| variations.map { PersistedProductVariation(from: $0) } | ||
| } | ||
|
|
||
| var variationImagesToPersist: [PersistedProductVariationImage] { | ||
| variations.compactMap { variation in | ||
| variation.image.map { PersistedProductVariationImage(from: $0, productVariationID: variation.productVariationID) } | ||
| } | ||
| } | ||
|
|
||
| var variationAttributesToPersist: [PersistedProductVariationAttribute] { | ||
| variations.flatMap { variation in | ||
| variation.attributes.map { PersistedProductVariationAttribute(from: $0, productVariationID: variation.productVariationID) } | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,18 @@ import Foundation | |
| import Testing | ||
| @testable import Networking | ||
| @testable import Yosemite | ||
| @testable import Storage | ||
|
|
||
| struct POSCatalogFullSyncServiceTests { | ||
| private let sut: POSCatalogFullSyncService | ||
| private let mockSyncRemote: MockPOSCatalogSyncRemote | ||
| private let mockPersistenceService: MockPOSCatalogPersistenceService | ||
| private let sampleSiteID: Int64 = 134 | ||
|
|
||
| init() { | ||
| self.mockSyncRemote = MockPOSCatalogSyncRemote() | ||
| self.sut = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: 2) | ||
| self.mockPersistenceService = MockPOSCatalogPersistenceService() | ||
| self.sut = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: 2, persistenceService: mockPersistenceService) | ||
| } | ||
|
|
||
| // MARK: - Full Sync Tests | ||
|
|
@@ -128,20 +131,24 @@ struct POSCatalogFullSyncServiceTests { | |
|
|
||
| // MARK: - Initialization Tests | ||
|
|
||
| @Test func init_with_valid_credentials_creates_service() { | ||
| @Test func init_with_valid_credentials_creates_service() throws { | ||
| // Given | ||
| let credentials = Credentials.wpcom(username: "test", authToken: "token", siteAddress: "site.com") | ||
| let grdbManager = try GRDBManager() | ||
|
|
||
| // When | ||
| let service = POSCatalogFullSyncService(credentials: credentials) | ||
| let service = POSCatalogFullSyncService(credentials: credentials, grdbManager: grdbManager) | ||
|
|
||
| // Then | ||
| #expect(service != nil) | ||
| } | ||
|
|
||
| @Test func init_with_nil_credentials_returns_nil() { | ||
| // Given/When | ||
| let service = POSCatalogFullSyncService(credentials: nil) | ||
| @Test func init_with_nil_credentials_returns_nil() throws { | ||
| // Given | ||
| let grdbManager = try GRDBManager() | ||
|
|
||
| // When | ||
| let service = POSCatalogFullSyncService(credentials: nil, grdbManager: grdbManager) | ||
|
|
||
| // Then | ||
| #expect(service == nil) | ||
|
|
@@ -152,7 +159,9 @@ struct POSCatalogFullSyncServiceTests { | |
| let customBatchSize = 5 | ||
|
|
||
| // When | ||
| let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: customBatchSize) | ||
| let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, | ||
| batchSize: customBatchSize, | ||
| persistenceService: mockPersistenceService) | ||
| _ = try await service.startFullSync(for: sampleSiteID) | ||
|
|
||
| // Then | ||
|
|
@@ -231,3 +240,17 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol { | |
| return fallbackVariationResult | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Mock POSCatalogPersistenceService | ||
|
|
||
| final class MockPOSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { | ||
| private(set) var replaceAllCatalogDataCallCount = 0 | ||
| private(set) var lastPersistedCatalog: POSCatalog? | ||
| private(set) var lastPersistedSiteID: Int64? | ||
|
Comment on lines
+247
to
+249
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing test cases that assert on these mock persistence service variables will be added in a future PR? |
||
|
|
||
| func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { | ||
| replaceAllCatalogDataCallCount += 1 | ||
| lastPersistedSiteID = siteID | ||
| lastPersistedCatalog = catalog | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: maybe can import just
POSCatalogPersistenceServiceProtocolandPOSCatalogPersistenceServiceinstead of the wholeStorage?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually for
GRDBManagerProtocol, the persistence service is inYosemiteanyway so doesn't need to be imported.I'm not sure that granular imports help us much here. AIUI, we do it from the main app to make it easier to split POS to a new app, if we ever do that. Storage is much simpler, and we implicitly depend on most of it via the storage model typealiases.
Perhaps a different question: Should GRDB be in a new module of its own? Maybe it would be best to do that now, in a new PR but before we add more to it. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding for granular imports is so that it's clear which (minimum) dependencies are required for the file. It's more useful when the imported module is bigger though, like Yosemite. For Storage I think it's fine, just a nit preference.
Were you thinking of keeping GRDB code in a separate module, and Storage imports it for its usage with potential abstraction in the interface with Yosemite? Was it because the GRDB code might get complex, or any other reasons? Right now, I think it's fine for GRDB to be within Storage as an implementation of the protocols. Would like to hear any strong reasons for moving it to a separate module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I was thinking a new sibling module for Storage, e.g.
GRDBStorage(bad name, but for discussion).Our new persisted models don't rely on anything pre-existing in Storage. Now's the easiest time to separate them so that we don't end up with anything cutting across both types of storage.
By putting them in a separate module, we could call them
Product,ProductVariationetc, at least withinGRDBStorage. If we split POS into a separate app, we might be able to leaveStoragebehind (unlikely, but not impossible.)I don't have super-strong arguments beyond that. Keeping it in Storage is fine... but other than the name, and saving a little effort adding a module, there's no particular reason it has to be in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so the current Storage is more like
CoreDataStorage. Makes sense to me. We could put it up for discussion and fit this after the main tasks are done.From development so far, it's not a strong need, but it might be great to have more abstraction on the interface between the (GRDB)Storage and Yosemite layers.