From faeb1c565ef346d35f51b433fd0bd9981056cdbf Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Wed, 3 Sep 2025 16:20:29 +0100 Subject: [PATCH 01/10] Ensure variation images are deleted with cascade --- Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift b/Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift index fe2573fdb0b..32f66163926 100644 --- a/Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift +++ b/Modules/Sources/Storage/GRDB/Migrations/V001InitialSchema.swift @@ -112,7 +112,7 @@ struct V001InitialSchema { private static func createProductVariationImageTable(_ db: Database) throws { try db.create(table: "productVariationImage") { productVariationImageTable in productVariationImageTable.primaryKey("id", .integer).notNull() - productVariationImageTable.belongsTo("productVariation").notNull() + productVariationImageTable.belongsTo("productVariation", onDelete: .cascade).notNull() productVariationImageTable.column("dateCreated", .datetime).notNull() productVariationImageTable.column("dateModified", .datetime) From b12bbecc4d71d26eaf94c83a8224f52e434d5ff0 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 4 Sep 2025 13:50:32 +0100 Subject: [PATCH 02/10] Add persistence helper in Yosemite Add persistence helper in Yosemite --- .../Tools/POS/POSCatalogFullSyncService.swift | 17 +++- .../POS/POSCatalogPersistenceService.swift | 78 +++++++++++++++++++ .../POS/POSCatalogFullSyncServiceTests.swift | 51 ++++++++++-- 3 files changed, 136 insertions(+), 10 deletions(-) create mode 100644 Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogFullSyncService.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogFullSyncService.swift index 66f25415e59..5d5f0fbeb1d 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogFullSyncService.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogFullSyncService.swift @@ -3,6 +3,7 @@ import protocol Networking.POSCatalogSyncRemoteProtocol import class Networking.AlamofireNetwork import class Networking.POSCatalogSyncRemote import CocoaLumberjackSwift +import Storage // TODO - remove the periphery ignore comment when the catalog is integrated with POS. // periphery:ignore @@ -26,20 +27,23 @@ public struct POSCatalog { public final class POSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol { private let syncRemote: POSCatalogSyncRemoteProtocol private let batchSize: Int + private let persistenceService: POSCatalogPersistenceServiceProtocol - public convenience init?(credentials: Credentials?, batchSize: Int = 2) { + public convenience init?(credentials: Credentials?, batchSize: Int = 2, grdbManager: GRDBManagerProtocol) { guard let credentials else { DDLogError("⛔️ Could not create POSCatalogFullSyncService due missing credentials") return nil } let network = AlamofireNetwork(credentials: credentials, ensuresSessionManagerIsInitialized: true) let syncRemote = POSCatalogSyncRemote(network: network) - self.init(syncRemote: syncRemote, batchSize: batchSize) + let persistenceService = POSCatalogPersistenceService(grdbManager: grdbManager) + self.init(syncRemote: syncRemote, batchSize: batchSize, persistenceService: persistenceService) } - init(syncRemote: POSCatalogSyncRemoteProtocol, batchSize: Int) { + init(syncRemote: POSCatalogSyncRemoteProtocol, batchSize: Int, persistenceService: POSCatalogPersistenceServiceProtocol) { self.syncRemote = syncRemote self.batchSize = batchSize + self.persistenceService = persistenceService } // MARK: - Protocol Conformance @@ -48,10 +52,17 @@ public final class POSCatalogFullSyncService: POSCatalogFullSyncServiceProtocol DDLogInfo("🔄 Starting full catalog sync for site ID: \(siteID)") do { + // Sync from network let catalog = try await loadCatalog(for: siteID, syncRemote: syncRemote) DDLogInfo("✅ Loaded \(catalog.products.count) products and \(catalog.variations.count) variations for siteID \(siteID)") + + // Persist to database + try await persistenceService.replaceAllCatalogData(catalog, siteID: siteID) + DDLogInfo("✅ Persisted \(catalog.products.count) products and \(catalog.variations.count) variations to database for siteID \(siteID)") + return catalog } catch { + DDLogError("❌ Failed to sync and persist catalog: \(error)") throw error } } diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift new file mode 100644 index 00000000000..61e22e276d1 --- /dev/null +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift @@ -0,0 +1,78 @@ +import Foundation +import Storage + +public protocol POSCatalogPersistenceServiceProtocol { + /// Clears all catalog data for the specified site + /// - Parameter siteID: The site ID to clear data for + func clearSiteData(for siteID: Int64) async throws + + /// Persists catalog data to the database + /// - Parameters: + /// - catalog: The catalog to persist + /// - siteID: The site ID to associate the catalog with + func persistCatalog(_ catalog: POSCatalog, siteID: Int64) async throws + + /// 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 +} + +public final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { + private let grdbManager: GRDBManagerProtocol + + public init(grdbManager: GRDBManagerProtocol) { + self.grdbManager = grdbManager + } + + public func clearSiteData(for siteID: Int64) async throws { + let db = grdbManager.databaseConnection + try await db.write { db in + DDLogInfo("🗑️ Clearing catalog data for site \(siteID)") + try PersistedSite.deleteOne(db, key: siteID) + } + } + + public func persistCatalog(_ catalog: POSCatalog, siteID: Int64) async throws { + let db = grdbManager.databaseConnection + DDLogInfo("💾 Persisting catalog with \(catalog.products.count) products and \(catalog.variations.count) variations") + + try await withThrowingTaskGroup(of: Void.self) { group in + // Persist site first + group.addTask { + try db.write { db in + let site = PersistedSite(id: siteID) + try site.insert(db) + } + } + + // Wait for site to be persisted before continuing + try await group.next() + + // Persist products + for product in catalog.products { + group.addTask { + try product.insertWithRelationships(in: db) + } + } + + // Persist variations + for variation in catalog.variations { + group.addTask { + try variation.insertWithRelationships(in: db) + } + } + + // Wait for all saves to complete + for try await _ in group {} + } + + DDLogInfo("✅ Catalog persistence complete") + } + + public func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { + try await clearSiteData(for: siteID) + try await persistCatalog(catalog, siteID: siteID) + } +} diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift index 1a0053fc8b8..b2d05bbcff8 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift @@ -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) @@ -150,9 +157,10 @@ struct POSCatalogFullSyncServiceTests { @Test func init_with_custom_batch_size_uses_specified_size() async throws { // Given let customBatchSize = 5 + let mockPersistence = MockPOSCatalogPersistenceService() // When - let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: customBatchSize) + let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: customBatchSize, persistenceService: mockPersistence) _ = try await service.startFullSync(for: sampleSiteID) // Then @@ -231,3 +239,32 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol { return fallbackVariationResult } } + +// MARK: - Mock POSCatalogPersistenceService + +final class MockPOSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { + private(set) var clearSiteDataCallCount = 0 + private(set) var persistCatalogCallCount = 0 + private(set) var replaceAllCatalogDataCallCount = 0 + + private(set) var lastClearedSiteID: Int64? + private(set) var lastPersistedCatalog: POSCatalog? + private(set) var lastPersistedSiteID: Int64? + + func clearSiteData(for siteID: Int64) async throws { + clearSiteDataCallCount += 1 + lastClearedSiteID = siteID + } + + func persistCatalog(_ catalog: POSCatalog, siteID: Int64) async throws { + persistCatalogCallCount += 1 + lastPersistedCatalog = catalog + lastPersistedSiteID = siteID + } + + func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { + replaceAllCatalogDataCallCount += 1 + try await clearSiteData(for: siteID) + try await persistCatalog(catalog, siteID: siteID) + } +} From 442f2cce16f0a5e9c429820112c6d61011890ae5 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 4 Sep 2025 13:52:20 +0100 Subject: [PATCH 03/10] Use a single transaction to write all entities --- .../POS/POSCatalogPersistenceService.swift | 93 ++++++++++++++----- 1 file changed, 70 insertions(+), 23 deletions(-) diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift index 61e22e276d1..cbb72d3f88a 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift @@ -30,45 +30,58 @@ public final class POSCatalogPersistenceService: POSCatalogPersistenceServicePro let db = grdbManager.databaseConnection try await db.write { db in DDLogInfo("🗑️ Clearing catalog data for site \(siteID)") - try PersistedSite.deleteOne(db, key: siteID) + // currently, we can't save for more than one site as entity IDs are not namespaced. + try PersistedSite.deleteAll(db) +// try PersistedSite.deleteOne(db, key: siteID) } } public func persistCatalog(_ catalog: POSCatalog, siteID: Int64) async throws { - let db = grdbManager.databaseConnection DDLogInfo("💾 Persisting catalog with \(catalog.products.count) products and \(catalog.variations.count) variations") - try await withThrowingTaskGroup(of: Void.self) { group in - // Persist site first - group.addTask { - try db.write { db in - let site = PersistedSite(id: siteID) - try site.insert(db) - } + try await grdbManager.databaseConnection.write { db in + let site = PersistedSite(id: siteID) + try site.insert(db) + + for product in catalog.productsToPersist { + try product.insert(db, onConflict: .ignore) } - // Wait for site to be persisted before continuing - try await group.next() + for image in catalog.productImagesToPersist { + try image.insert(db, onConflict: .ignore) + } - // Persist products - for product in catalog.products { - group.addTask { - try product.insertWithRelationships(in: db) - } + for var attribute in catalog.productAttributesToPersist { + try attribute.insert(db) } - // Persist variations - for variation in catalog.variations { - group.addTask { - try variation.insertWithRelationships(in: db) - } + for variation in catalog.variationsToPersist { + try variation.insert(db, onConflict: .ignore) } - // Wait for all saves to complete - for try await _ in group {} + 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") + } } public func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { @@ -76,3 +89,37 @@ public final class POSCatalogPersistenceService: POSCatalogPersistenceServicePro try await persistCatalog(catalog, siteID: siteID) } } + +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) } + } + } +} From d0b1e6be1a0c6855c8bddeb6d17b4be990fcd59a Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 4 Sep 2025 11:46:50 +0100 Subject: [PATCH 04/10] Test saving the catalog to the database --- .../POSCatalogPersistenceServiceTests.swift | 237 ++++++++++++++++++ 1 file changed, 237 insertions(+) create mode 100644 Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift new file mode 100644 index 00000000000..11c607973a6 --- /dev/null +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift @@ -0,0 +1,237 @@ +import Foundation +import Testing +@testable import Storage +@testable import Yosemite + +struct POSCatalogPersistenceServiceTests { + private let grdbManager: GRDBManager + private let sut: POSCatalogPersistenceService + private let sampleSiteID: Int64 = 134 + + init() throws { + self.grdbManager = try GRDBManager() + self.sut = POSCatalogPersistenceService(grdbManager: grdbManager) + } + + // MARK: - Clear Site Data Tests + + @Test func clearSiteData_removes_all_existing_data() async throws { + // Given - existing data in database + let existingCatalog = POSCatalog( + products: [POSProduct.fake().copy(siteID: sampleSiteID, productID: 1)], + variations: [POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 1, productVariationID: 1)] + ) + try await sut.persistCatalog(existingCatalog, siteID: sampleSiteID) + + // When + try await sut.clearSiteData(for: sampleSiteID) + + // Then - all data should be removed + let db = grdbManager.databaseConnection + try await db.read { db in + let siteCount = try PersistedSite.fetchCount(db) + let productCount = try PersistedProduct.fetchCount(db) + let variationCount = try PersistedProductVariation.fetchCount(db) + + #expect(siteCount == 0) + #expect(productCount == 0) + #expect(variationCount == 0) + } + } + + // MARK: - Persist Catalog Tests + + @Test func persistCatalog_saves_site_products_and_variations() async throws { + // Given + let catalog = POSCatalog( + products: [ + POSProduct.fake().copy(siteID: sampleSiteID, productID: 1), + POSProduct.fake().copy(siteID: sampleSiteID, productID: 2, productTypeKey: "variable") + ], + variations: [ + POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 2, productVariationID: 1), + POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 2, productVariationID: 2) + ] + ) + + // When + try await sut.persistCatalog(catalog, siteID: sampleSiteID) + + // Then + let db = grdbManager.databaseConnection + try await db.read { db in + let siteCount = try PersistedSite.fetchCount(db) + let productCount = try PersistedProduct.fetchCount(db) + let variationCount = try PersistedProductVariation.fetchCount(db) + + #expect(siteCount == 1) + #expect(productCount == 2) + #expect(variationCount == 2) + + let site = try PersistedSite.fetchOne(db) + #expect(site?.id == sampleSiteID) + } + } + + @Test func persistCatalog_saves_product_images_and_attributes() async throws { + // Given + let productWithRelations = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 1, + images: [ProductImage.fake().copy(imageID: 100), ProductImage.fake().copy(imageID: 101)], + attributes: [ProductAttribute.fake(), ProductAttribute.fake()] + ) + let catalog = POSCatalog(products: [productWithRelations], variations: []) + + // When + try await sut.persistCatalog(catalog, siteID: sampleSiteID) + + // Then + let db = grdbManager.databaseConnection + try await db.read { db in + let imageCount = try PersistedProductImage.fetchCount(db) + let attributeCount = try PersistedProductAttribute.fetchCount(db) + + #expect(imageCount == 2) + #expect(attributeCount == 2) + } + } + + @Test func persistCatalog_saves_variation_images_and_attributes() async throws { + // Given + let variationWithRelations = POSProductVariation.fake().copy( + siteID: sampleSiteID, + productID: 15, + productVariationID: 1, + attributes: [ProductVariationAttribute.fake(), ProductVariationAttribute.fake()], image: ProductImage.fake().copy(imageID: 200) + ) + let catalog = POSCatalog(products: [POSProduct.fake().copy(siteID: sampleSiteID, productID: 15)], + variations: [variationWithRelations]) + + // When + try await sut.persistCatalog(catalog, siteID: sampleSiteID) + + // Then + let db = grdbManager.databaseConnection + try await db.read { db in + let imageCount = try PersistedProductVariationImage.fetchCount(db) + let attributeCount = try PersistedProductVariationAttribute.fetchCount(db) + + #expect(imageCount == 1) + #expect(attributeCount == 2) + } + } + + @Test func persistCatalog_handles_duplicate_image_ids_gracefully() async throws { + // Given - products with same image ID + let sharedImageID: Int64 = 300 + let product1 = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 1, + images: [ProductImage.fake().copy(imageID: sharedImageID)] + ) + let product2 = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 2, + images: [ProductImage.fake().copy(imageID: sharedImageID)] + ) + let catalog = POSCatalog(products: [product1, product2], variations: []) + + // When + try await sut.persistCatalog(catalog, siteID: sampleSiteID) + + // Then - should not fail and should handle duplicates + let db = grdbManager.databaseConnection + try await db.read { db in + let productCount = try PersistedProduct.fetchCount(db) + let imageCount = try PersistedProductImage.fetchCount(db) + + #expect(productCount == 2) + // One or both images should be saved (depending on conflict resolution) + #expect(imageCount >= 1) + #expect(imageCount <= 2) + } + } + + // MARK: - Replace All Data Tests + + @Test func replaceAllCatalogData_clears_existing_and_persists_new() async throws { + // Given - existing data + let existingCatalog = POSCatalog( + products: [POSProduct.fake().copy(siteID: sampleSiteID, productID: 80)], + variations: [POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 80, productVariationID: 100)] + ) + try await sut.persistCatalog(existingCatalog, siteID: sampleSiteID) + + // When - replace with new data + let newCatalog = POSCatalog( + products: [POSProduct.fake().copy(siteID: sampleSiteID, productID: 180)], + variations: [POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 180, productVariationID: 200)] + ) + try await sut.replaceAllCatalogData(newCatalog, siteID: sampleSiteID) + + // Then - should have only new data + let db = grdbManager.databaseConnection + try await db.read { db in + let productCount = try PersistedProduct.fetchCount(db) + let variationCount = try PersistedProductVariation.fetchCount(db) + + #expect(productCount == 1) + #expect(variationCount == 1) + + let product = try PersistedProduct.fetchOne(db) + let variation = try PersistedProductVariation.fetchOne(db) + + #expect(product?.id == 180) + #expect(variation?.id == 200) + } + } + + @Test func replaceAllCatalogData_removes_related_images_and_attributes() async throws { + // Given - existing data with relations + let existingProduct = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 1, + images: [ProductImage.fake()], + attributes: [ProductAttribute.fake()] + ) + let existingCatalog = POSCatalog(products: [existingProduct], variations: []) + try await sut.persistCatalog(existingCatalog, siteID: sampleSiteID) + + // When - replace with empty catalog + let emptyCatalog = POSCatalog(products: [], variations: []) + try await sut.replaceAllCatalogData(emptyCatalog, siteID: sampleSiteID) + + // Then - all related data should be gone + let db = grdbManager.databaseConnection + try await db.read { db in + let productCount = try PersistedProduct.fetchCount(db) + let imageCount = try PersistedProductImage.fetchCount(db) + let attributeCount = try PersistedProductAttribute.fetchCount(db) + + #expect(productCount == 0) + #expect(imageCount == 0) + #expect(attributeCount == 0) + } + } + + @Test func persistCatalog_handles_empty_catalog_gracefully() async throws { + // Given + let emptyCatalog = POSCatalog(products: [], variations: []) + + // When + try await sut.persistCatalog(emptyCatalog, siteID: sampleSiteID) + + // Then + let db = grdbManager.databaseConnection + try await db.read { db in + let siteCount = try PersistedSite.fetchCount(db) + let productCount = try PersistedProduct.fetchCount(db) + let variationCount = try PersistedProductVariation.fetchCount(db) + + #expect(siteCount == 1) + #expect(productCount == 0) + #expect(variationCount == 0) + } + } +} From f2b005dad6f7a54816dcca3154b7dbc0eca0a35f Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 4 Sep 2025 12:17:42 +0100 Subject: [PATCH 05/10] Periphery fixes and ignore --- .../Tools/POS/POSCatalogPersistenceService.swift | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift index cbb72d3f88a..32fd6147549 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift @@ -1,7 +1,8 @@ +// periphery:ignore:all import Foundation import Storage -public protocol POSCatalogPersistenceServiceProtocol { +protocol POSCatalogPersistenceServiceProtocol { /// Clears all catalog data for the specified site /// - Parameter siteID: The site ID to clear data for func clearSiteData(for siteID: Int64) async throws @@ -19,14 +20,14 @@ public protocol POSCatalogPersistenceServiceProtocol { func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws } -public final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { +final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { private let grdbManager: GRDBManagerProtocol - public init(grdbManager: GRDBManagerProtocol) { + init(grdbManager: GRDBManagerProtocol) { self.grdbManager = grdbManager } - public func clearSiteData(for siteID: Int64) async throws { + func clearSiteData(for siteID: Int64) async throws { let db = grdbManager.databaseConnection try await db.write { db in DDLogInfo("🗑️ Clearing catalog data for site \(siteID)") @@ -36,7 +37,7 @@ public final class POSCatalogPersistenceService: POSCatalogPersistenceServicePro } } - public func persistCatalog(_ catalog: POSCatalog, siteID: Int64) async throws { + func persistCatalog(_ 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 @@ -84,7 +85,7 @@ public final class POSCatalogPersistenceService: POSCatalogPersistenceServicePro } } - public func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { + func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { try await clearSiteData(for: siteID) try await persistCatalog(catalog, siteID: siteID) } From ae7aab85a095b14a26788458b8f9ad7027b879cb Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 4 Sep 2025 12:33:36 +0100 Subject: [PATCH 06/10] Clear data in replaceAllCatalogData transaction --- .../POS/POSCatalogPersistenceService.swift | 28 ++------ .../POS/POSCatalogFullSyncServiceTests.swift | 23 ++---- .../POSCatalogPersistenceServiceTests.swift | 70 +++---------------- 3 files changed, 19 insertions(+), 102 deletions(-) diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift index 32fd6147549..e8758b3e07c 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift @@ -3,16 +3,6 @@ import Foundation import Storage protocol POSCatalogPersistenceServiceProtocol { - /// Clears all catalog data for the specified site - /// - Parameter siteID: The site ID to clear data for - func clearSiteData(for siteID: Int64) async throws - - /// Persists catalog data to the database - /// - Parameters: - /// - catalog: The catalog to persist - /// - siteID: The site ID to associate the catalog with - func persistCatalog(_ catalog: POSCatalog, siteID: Int64) async throws - /// Clears existing data and persists new catalog data /// - Parameters: /// - catalog: The catalog to persist @@ -27,20 +17,15 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { self.grdbManager = grdbManager } - func clearSiteData(for siteID: Int64) async throws { - let db = grdbManager.databaseConnection - try await db.write { db in + 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) // try PersistedSite.deleteOne(db, key: siteID) - } - } - - func persistCatalog(_ 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 let site = PersistedSite(id: siteID) try site.insert(db) @@ -84,11 +69,6 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { "\(variationImageCount) variation images, \(variationAttributeCount) variation attributes") } } - - func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { - try await clearSiteData(for: siteID) - try await persistCatalog(catalog, siteID: siteID) - } } private extension POSCatalog { diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift index b2d05bbcff8..ac3ec97ab7b 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift @@ -146,7 +146,7 @@ struct POSCatalogFullSyncServiceTests { @Test func init_with_nil_credentials_returns_nil() throws { // Given let grdbManager = try GRDBManager() - + // When let service = POSCatalogFullSyncService(credentials: nil, grdbManager: grdbManager) @@ -243,28 +243,13 @@ final class MockPOSCatalogSyncRemote: POSCatalogSyncRemoteProtocol { // MARK: - Mock POSCatalogPersistenceService final class MockPOSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { - private(set) var clearSiteDataCallCount = 0 - private(set) var persistCatalogCallCount = 0 private(set) var replaceAllCatalogDataCallCount = 0 - - private(set) var lastClearedSiteID: Int64? private(set) var lastPersistedCatalog: POSCatalog? private(set) var lastPersistedSiteID: Int64? - - func clearSiteData(for siteID: Int64) async throws { - clearSiteDataCallCount += 1 - lastClearedSiteID = siteID - } - - func persistCatalog(_ catalog: POSCatalog, siteID: Int64) async throws { - persistCatalogCallCount += 1 - lastPersistedCatalog = catalog - lastPersistedSiteID = siteID - } - + func replaceAllCatalogData(_ catalog: POSCatalog, siteID: Int64) async throws { replaceAllCatalogDataCallCount += 1 - try await clearSiteData(for: siteID) - try await persistCatalog(catalog, siteID: siteID) + lastPersistedSiteID = siteID + lastPersistedCatalog = catalog } } diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift index 11c607973a6..7c4a48e2083 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift @@ -13,35 +13,9 @@ struct POSCatalogPersistenceServiceTests { self.sut = POSCatalogPersistenceService(grdbManager: grdbManager) } - // MARK: - Clear Site Data Tests + // MARK: - Replace Catalog Data Tests - @Test func clearSiteData_removes_all_existing_data() async throws { - // Given - existing data in database - let existingCatalog = POSCatalog( - products: [POSProduct.fake().copy(siteID: sampleSiteID, productID: 1)], - variations: [POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 1, productVariationID: 1)] - ) - try await sut.persistCatalog(existingCatalog, siteID: sampleSiteID) - - // When - try await sut.clearSiteData(for: sampleSiteID) - - // Then - all data should be removed - let db = grdbManager.databaseConnection - try await db.read { db in - let siteCount = try PersistedSite.fetchCount(db) - let productCount = try PersistedProduct.fetchCount(db) - let variationCount = try PersistedProductVariation.fetchCount(db) - - #expect(siteCount == 0) - #expect(productCount == 0) - #expect(variationCount == 0) - } - } - - // MARK: - Persist Catalog Tests - - @Test func persistCatalog_saves_site_products_and_variations() async throws { + @Test func replaceAllCatalogData_saves_site_products_and_variations() async throws { // Given let catalog = POSCatalog( products: [ @@ -55,7 +29,7 @@ struct POSCatalogPersistenceServiceTests { ) // When - try await sut.persistCatalog(catalog, siteID: sampleSiteID) + try await sut.replaceAllCatalogData(catalog, siteID: sampleSiteID) // Then let db = grdbManager.databaseConnection @@ -73,7 +47,7 @@ struct POSCatalogPersistenceServiceTests { } } - @Test func persistCatalog_saves_product_images_and_attributes() async throws { + @Test func replaceAllCatalogData_saves_product_images_and_attributes() async throws { // Given let productWithRelations = POSProduct.fake().copy( siteID: sampleSiteID, @@ -84,7 +58,7 @@ struct POSCatalogPersistenceServiceTests { let catalog = POSCatalog(products: [productWithRelations], variations: []) // When - try await sut.persistCatalog(catalog, siteID: sampleSiteID) + try await sut.replaceAllCatalogData(catalog, siteID: sampleSiteID) // Then let db = grdbManager.databaseConnection @@ -97,7 +71,7 @@ struct POSCatalogPersistenceServiceTests { } } - @Test func persistCatalog_saves_variation_images_and_attributes() async throws { + @Test func replaceAllCatalogData_saves_variation_images_and_attributes() async throws { // Given let variationWithRelations = POSProductVariation.fake().copy( siteID: sampleSiteID, @@ -109,7 +83,7 @@ struct POSCatalogPersistenceServiceTests { variations: [variationWithRelations]) // When - try await sut.persistCatalog(catalog, siteID: sampleSiteID) + try await sut.replaceAllCatalogData(catalog, siteID: sampleSiteID) // Then let db = grdbManager.databaseConnection @@ -122,7 +96,7 @@ struct POSCatalogPersistenceServiceTests { } } - @Test func persistCatalog_handles_duplicate_image_ids_gracefully() async throws { + @Test func replaceAllCatalogData_handles_duplicate_image_ids_gracefully() async throws { // Given - products with same image ID let sharedImageID: Int64 = 300 let product1 = POSProduct.fake().copy( @@ -138,7 +112,7 @@ struct POSCatalogPersistenceServiceTests { let catalog = POSCatalog(products: [product1, product2], variations: []) // When - try await sut.persistCatalog(catalog, siteID: sampleSiteID) + try await sut.replaceAllCatalogData(catalog, siteID: sampleSiteID) // Then - should not fail and should handle duplicates let db = grdbManager.databaseConnection @@ -153,15 +127,13 @@ struct POSCatalogPersistenceServiceTests { } } - // MARK: - Replace All Data Tests - @Test func replaceAllCatalogData_clears_existing_and_persists_new() async throws { // Given - existing data let existingCatalog = POSCatalog( products: [POSProduct.fake().copy(siteID: sampleSiteID, productID: 80)], variations: [POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 80, productVariationID: 100)] ) - try await sut.persistCatalog(existingCatalog, siteID: sampleSiteID) + try await sut.replaceAllCatalogData(existingCatalog, siteID: sampleSiteID) // When - replace with new data let newCatalog = POSCatalog( @@ -196,7 +168,7 @@ struct POSCatalogPersistenceServiceTests { attributes: [ProductAttribute.fake()] ) let existingCatalog = POSCatalog(products: [existingProduct], variations: []) - try await sut.persistCatalog(existingCatalog, siteID: sampleSiteID) + try await sut.replaceAllCatalogData(existingCatalog, siteID: sampleSiteID) // When - replace with empty catalog let emptyCatalog = POSCatalog(products: [], variations: []) @@ -214,24 +186,4 @@ struct POSCatalogPersistenceServiceTests { #expect(attributeCount == 0) } } - - @Test func persistCatalog_handles_empty_catalog_gracefully() async throws { - // Given - let emptyCatalog = POSCatalog(products: [], variations: []) - - // When - try await sut.persistCatalog(emptyCatalog, siteID: sampleSiteID) - - // Then - let db = grdbManager.databaseConnection - try await db.read { db in - let siteCount = try PersistedSite.fetchCount(db) - let productCount = try PersistedProduct.fetchCount(db) - let variationCount = try PersistedProductVariation.fetchCount(db) - - #expect(siteCount == 1) - #expect(productCount == 0) - #expect(variationCount == 0) - } - } } From bad84414680155d24771c6999dcae2e06ba149d7 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 5 Sep 2025 10:53:41 +0100 Subject: [PATCH 07/10] Remove commented out `deleteOne` ideal code Co-authored-by: Jaclyn Chen --- .../Yosemite/Tools/POS/POSCatalogPersistenceService.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift index e8758b3e07c..22c991c157f 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift @@ -24,7 +24,6 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { 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) -// try PersistedSite.deleteOne(db, key: siteID) let site = PersistedSite(id: siteID) try site.insert(db) From d3a797885edf638cd4ce8f1c887548ce45c6ca80 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 5 Sep 2025 10:56:52 +0100 Subject: [PATCH 08/10] Use mock from property --- .../Tools/POS/POSCatalogFullSyncServiceTests.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift index ac3ec97ab7b..f973647a2e7 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogFullSyncServiceTests.swift @@ -157,10 +157,11 @@ struct POSCatalogFullSyncServiceTests { @Test func init_with_custom_batch_size_uses_specified_size() async throws { // Given let customBatchSize = 5 - let mockPersistence = MockPOSCatalogPersistenceService() // When - let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, batchSize: customBatchSize, persistenceService: mockPersistence) + let service = POSCatalogFullSyncService(syncRemote: mockSyncRemote, + batchSize: customBatchSize, + persistenceService: mockPersistenceService) _ = try await service.startFullSync(for: sampleSiteID) // Then From ce9afe13f87d9fc564871a88f716bbe062938d49 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 5 Sep 2025 11:09:49 +0100 Subject: [PATCH 09/10] Make image test match current (flawed) implementation --- .../Tools/POS/POSCatalogPersistenceServiceTests.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift index 7c4a48e2083..43d64abb8dc 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift @@ -121,9 +121,9 @@ struct POSCatalogPersistenceServiceTests { let imageCount = try PersistedProductImage.fetchCount(db) #expect(productCount == 2) - // One or both images should be saved (depending on conflict resolution) - #expect(imageCount >= 1) - #expect(imageCount <= 2) + // While there's only one, the current implementation doesn't + // have a join table so only one product has a reference to it + #expect(imageCount == 1) } } From 501e954e985324ae36e013c008f9335e8c98bd0f Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Fri, 5 Sep 2025 11:14:55 +0100 Subject: [PATCH 10/10] Add test for variation image/attribute removal --- .../POSCatalogPersistenceServiceTests.swift | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift index 43d64abb8dc..6777b379277 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift @@ -159,7 +159,7 @@ struct POSCatalogPersistenceServiceTests { } } - @Test func replaceAllCatalogData_removes_related_images_and_attributes() async throws { + @Test func replaceAllCatalogData_removes_related_images_and_attributes_for_products() async throws { // Given - existing data with relations let existingProduct = POSProduct.fake().copy( siteID: sampleSiteID, @@ -186,4 +186,39 @@ struct POSCatalogPersistenceServiceTests { #expect(attributeCount == 0) } } + + @Test func replaceAllCatalogData_removes_related_images_and_attributes_for_variations() async throws { + // Given - existing data with variation relations + let parentProduct = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 10 + ) + let existingVariation = POSProductVariation.fake().copy( + siteID: sampleSiteID, + productID: 10, + productVariationID: 5, + attributes: [ProductVariationAttribute.fake()], + image: ProductImage.fake().copy(imageID: 500) + ) + let existingCatalog = POSCatalog(products: [parentProduct], variations: [existingVariation]) + try await sut.replaceAllCatalogData(existingCatalog, siteID: sampleSiteID) + + // When - replace with catalog containing only parent product (no variations) + let catalogWithoutVariations = POSCatalog(products: [parentProduct], variations: []) + try await sut.replaceAllCatalogData(catalogWithoutVariations, siteID: sampleSiteID) + + // Then - variation and its related data should be gone + let db = grdbManager.databaseConnection + try await db.read { db in + let productCount = try PersistedProduct.fetchCount(db) + let variationCount = try PersistedProductVariation.fetchCount(db) + let variationImageCount = try PersistedProductVariationImage.fetchCount(db) + let variationAttributeCount = try PersistedProductVariationAttribute.fetchCount(db) + + #expect(productCount == 1) // Parent product should remain + #expect(variationCount == 0) // Variation should be gone + #expect(variationImageCount == 0) // Variation image should be gone + #expect(variationAttributeCount == 0) // Variation attributes should be gone + } + } }