From f4c711ef73ffc9a49661b0c2894c509c490b2195 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 6 Nov 2025 14:59:04 +0000 Subject: [PATCH 1/2] Use upsert for incremental updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes an issue with incremental sync Previously, if variations changed, any unchanged variations for the same product would be deleted during an incremental sync. This was because we also get the parent product back, and we used `insert(onConflict: .replace)` – this triggered a cascade delete of variations (and other relations.) Then we inserted the changed variations, so any that were unchanged remained deleted. To avoid these issues, we move to `save`, which is GRDB’s upsert method. Additionally, with the fix deleted variations would not be deleted during an incremental sync. To improve that, we check the `variationIDs` array for which variations to leave in, after an incremental sync. --- .../Sources/Fakes/Networking.generated.swift | 3 +- .../Sources/Fakes/Yosemite.generated.swift | 9 ++++ .../Copiable/Models+Copiable.generated.swift | 7 ++- .../Sources/Networking/Model/POSProduct.swift | 13 ++++- .../Storage/GRDB/Model/PersistedProduct.swift | 6 +++ .../Model/PersistedProductVariation.swift | 3 +- .../PersistedProduct+Conversions.swift | 14 ++++-- .../POS/POSCatalogPersistenceService.swift | 50 ++++++++----------- .../Storage/PersistedProductTests.swift | 6 ++- 9 files changed, 69 insertions(+), 42 deletions(-) diff --git a/Modules/Sources/Fakes/Networking.generated.swift b/Modules/Sources/Fakes/Networking.generated.swift index 59d765ba9ad..45f3a9b179b 100644 --- a/Modules/Sources/Fakes/Networking.generated.swift +++ b/Modules/Sources/Fakes/Networking.generated.swift @@ -812,7 +812,8 @@ extension Networking.POSProduct { attributes: .fake(), manageStock: .fake(), stockQuantity: .fake(), - stockStatusKey: .fake() + stockStatusKey: .fake(), + variationIDs: .fake() ) } } diff --git a/Modules/Sources/Fakes/Yosemite.generated.swift b/Modules/Sources/Fakes/Yosemite.generated.swift index b58654291cd..4b4a8b785b3 100644 --- a/Modules/Sources/Fakes/Yosemite.generated.swift +++ b/Modules/Sources/Fakes/Yosemite.generated.swift @@ -76,6 +76,15 @@ extension Yosemite.ProductReviewFromNoteParcel { ) } } +extension Yosemite.StoredBookingFilters { + /// Returns a "ready to use" type filled with fake values. + /// + public static func fake() -> Yosemite.StoredBookingFilters { + .init( + filters: .fake() + ) + } +} extension Yosemite.SystemInformation { /// Returns a "ready to use" type filled with fake values. /// diff --git a/Modules/Sources/Networking/Model/Copiable/Models+Copiable.generated.swift b/Modules/Sources/Networking/Model/Copiable/Models+Copiable.generated.swift index 68c69a8fbf5..c20e19dc571 100644 --- a/Modules/Sources/Networking/Model/Copiable/Models+Copiable.generated.swift +++ b/Modules/Sources/Networking/Model/Copiable/Models+Copiable.generated.swift @@ -1367,7 +1367,8 @@ extension Networking.POSProduct { attributes: CopiableProp<[ProductAttribute]> = .copy, manageStock: CopiableProp = .copy, stockQuantity: NullableCopiableProp = .copy, - stockStatusKey: CopiableProp = .copy + stockStatusKey: CopiableProp = .copy, + variationIDs: CopiableProp<[Int64]> = .copy ) -> Networking.POSProduct { let siteID = siteID ?? self.siteID let productID = productID ?? self.productID @@ -1385,6 +1386,7 @@ extension Networking.POSProduct { let manageStock = manageStock ?? self.manageStock let stockQuantity = stockQuantity ?? self.stockQuantity let stockStatusKey = stockStatusKey ?? self.stockStatusKey + let variationIDs = variationIDs ?? self.variationIDs return Networking.POSProduct( siteID: siteID, @@ -1402,7 +1404,8 @@ extension Networking.POSProduct { attributes: attributes, manageStock: manageStock, stockQuantity: stockQuantity, - stockStatusKey: stockStatusKey + stockStatusKey: stockStatusKey, + variationIDs: variationIDs ) } } diff --git a/Modules/Sources/Networking/Model/POSProduct.swift b/Modules/Sources/Networking/Model/POSProduct.swift index b9ad128874c..dc0702cf2c9 100644 --- a/Modules/Sources/Networking/Model/POSProduct.swift +++ b/Modules/Sources/Networking/Model/POSProduct.swift @@ -43,6 +43,8 @@ public struct POSProduct: Codable, Equatable, GeneratedCopiable, GeneratedFakeab public let stockQuantity: Decimal? public let stockStatusKey: String + public let variationIDs: [Int64] + public init(siteID: Int64, productID: Int64, name: String, @@ -58,7 +60,8 @@ public struct POSProduct: Codable, Equatable, GeneratedCopiable, GeneratedFakeab attributes: [ProductAttribute], manageStock: Bool, stockQuantity: Decimal?, - stockStatusKey: String) { + stockStatusKey: String, + variationIDs: [Int64]) { self.siteID = siteID self.productID = productID self.name = name @@ -81,6 +84,8 @@ public struct POSProduct: Codable, Equatable, GeneratedCopiable, GeneratedFakeab self.manageStock = manageStock self.stockQuantity = stockQuantity self.stockStatusKey = stockStatusKey + + self.variationIDs = variationIDs } public init(from decoder: any Decoder) throws { @@ -124,6 +129,8 @@ public struct POSProduct: Codable, Equatable, GeneratedCopiable, GeneratedFakeab let stockQuantity = container.failsafeDecodeIfPresent(decimalForKey: .stockQuantity) let stockStatusKey = try container.decode(String.self, forKey: .stockStatusKey) + let variationIDs = try container.decodeIfPresent([Int64].self, forKey: .variationIDs) ?? [] + self.init(siteID: siteID, productID: productID, name: name, @@ -139,7 +146,8 @@ public struct POSProduct: Codable, Equatable, GeneratedCopiable, GeneratedFakeab attributes: attributes, manageStock: manageStock, stockQuantity: stockQuantity, - stockStatusKey: stockStatusKey) + stockStatusKey: stockStatusKey, + variationIDs: variationIDs) } static let requestFields: [String] = { @@ -172,5 +180,6 @@ private extension POSProduct { case manageStock = "manage_stock" case stockQuantity = "stock_quantity" case stockStatusKey = "stock_status" + case variationIDs = "variations" } } diff --git a/Modules/Sources/Storage/GRDB/Model/PersistedProduct.swift b/Modules/Sources/Storage/GRDB/Model/PersistedProduct.swift index fc2123d2692..f126e09e2ba 100644 --- a/Modules/Sources/Storage/GRDB/Model/PersistedProduct.swift +++ b/Modules/Sources/Storage/GRDB/Model/PersistedProduct.swift @@ -88,6 +88,12 @@ extension PersistedProduct: FetchableRecord, PersistableRecord { using: ForeignKey([PersistedProductAttribute.CodingKeys.siteID.stringValue, PersistedProductAttribute.CodingKeys.productID.stringValue], to: primaryKey)) + + public static let variations = hasMany(PersistedProductVariation.self, + key: "variations", + using: ForeignKey([PersistedProductVariation.CodingKeys.siteID.stringValue, + PersistedProductVariation.CodingKeys.productID.stringValue], + to: primaryKey)) } // MARK: - Point of Sale Requests diff --git a/Modules/Sources/Storage/GRDB/Model/PersistedProductVariation.swift b/Modules/Sources/Storage/GRDB/Model/PersistedProductVariation.swift index ad6ca5d8ed9..af42b54d911 100644 --- a/Modules/Sources/Storage/GRDB/Model/PersistedProductVariation.swift +++ b/Modules/Sources/Storage/GRDB/Model/PersistedProductVariation.swift @@ -108,8 +108,7 @@ public extension PersistedProductVariation { } } -// periphery:ignore - TODO: remove ignore when populating database -private extension PersistedProductVariation { +extension PersistedProductVariation { enum CodingKeys: String, CodingKey { case id case siteID diff --git a/Modules/Sources/Yosemite/Model/Storage/PersistedProduct+Conversions.swift b/Modules/Sources/Yosemite/Model/Storage/PersistedProduct+Conversions.swift index 3f5fe127ad0..16807f0a893 100644 --- a/Modules/Sources/Yosemite/Model/Storage/PersistedProduct+Conversions.swift +++ b/Modules/Sources/Yosemite/Model/Storage/PersistedProduct+Conversions.swift @@ -23,7 +23,7 @@ extension PersistedProduct { ) } - func toPOSProduct(images: [ProductImage] = [], attributes: [ProductAttribute] = []) -> POSProduct { + func toPOSProduct(images: [ProductImage] = [], attributes: [ProductAttribute] = [], variationIDs: [Int64] = []) -> POSProduct { return POSProduct( siteID: siteID, productID: id, @@ -40,20 +40,24 @@ extension PersistedProduct { attributes: attributes, manageStock: manageStock, stockQuantity: stockQuantity, - stockStatusKey: stockStatusKey + stockStatusKey: stockStatusKey, + variationIDs: variationIDs ) } func toPOSProduct(db: GRDBDatabaseConnection) throws -> POSProduct { - let (images, attributes) = try db.read { db in + let (images, attributes, variationIDs) = try db.read { db in let images = try request(for: PersistedProduct.images).fetchAll(db) let attributes = try request(for: PersistedProduct.attributes).fetchAll(db) - return (images, attributes) + let variations = try request(for: PersistedProduct.variations).fetchAll(db) + let variationIDs = variations.map(\.id) + return (images, attributes, variationIDs) } return toPOSProduct( images: images.map { $0.toProductImage() }, - attributes: attributes.map { $0.toProductAttribute(siteID: siteID) } + attributes: attributes.map { $0.toProductAttribute(siteID: siteID) }, + variationIDs: variationIDs ) } diff --git a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift index e449b93a073..263b52472c7 100644 --- a/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift +++ b/Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift @@ -87,52 +87,46 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol { DDLogInfo("💾 Persisting incremental catalog with \(catalog.products.count) updated products and \(catalog.variations.count) updated variations") try await grdbManager.databaseConnection.write { db in - for product in catalog.productsToPersist { - try product.insert(db, onConflict: .replace) - - // Delete old join table entries for this product - try PersistedProductImage - .filter { $0.siteID == siteID && $0.productID == product.id } - .deleteAll(db) - - try PersistedProductAttribute - .filter { $0.siteID == siteID && $0.productID == product.id } - .deleteAll(db) + for product in catalog.products { + try PersistedProduct(from: product).save(db) + + // Delete variations that are no longer associated with this product + let existingVariations = try PersistedProductVariation + .filter(PersistedProductVariation.Columns.siteID == siteID) + .filter(PersistedProductVariation.Columns.productID == product.productID) + .fetchAll(db) + + for variation in existingVariations { + if !product.variationIDs.contains(variation.id) { + try variation.delete(db) + } + } } for variation in catalog.variationsToPersist { - try variation.insert(db, onConflict: .replace) - - // Delete old join table entries for this variation - try PersistedProductVariationImage - .filter { $0.siteID == siteID && $0.productVariationID == variation.id } - .deleteAll(db) - - try PersistedProductVariationAttribute - .filter { $0.siteID == siteID && $0.productVariationID == variation.id } - .deleteAll(db) + try variation.save(db) } - // Insert/update actual image data (shared by products and variations) + // Upsert actual image data (shared by products and variations) for image in catalog.imagesToPersist { - try image.insert(db, onConflict: .replace) + try image.save(db) } - // Insert new join table entries + // Upsert new join table entries for image in catalog.productImagesToPersist { - try image.insert(db, onConflict: .replace) + try image.save(db) } for image in catalog.variationImagesToPersist { - try image.insert(db, onConflict: .replace) + try image.save(db) } for var attribute in catalog.productAttributesToPersist { - try attribute.insert(db, onConflict: .replace) + try attribute.save(db) } for var attribute in catalog.variationAttributesToPersist { - try attribute.insert(db, onConflict: .replace) + try attribute.save(db) } var site = try PersistedSite.fetchOne(db, key: siteID) diff --git a/Modules/Tests/YosemiteTests/Storage/PersistedProductTests.swift b/Modules/Tests/YosemiteTests/Storage/PersistedProductTests.swift index 464376cb72a..aeef327b164 100644 --- a/Modules/Tests/YosemiteTests/Storage/PersistedProductTests.swift +++ b/Modules/Tests/YosemiteTests/Storage/PersistedProductTests.swift @@ -27,7 +27,8 @@ struct PersistedProductTests { attributes: [], manageStock: true, stockQuantity: 5, - stockStatusKey: "instock" + stockStatusKey: "instock", + variationIDs: [] ) // When @@ -508,7 +509,8 @@ struct PersistedProductTests { ], manageStock: true, stockQuantity: 50, - stockStatusKey: "instock" + stockStatusKey: "instock", + variationIDs: [] ) // When saving and loading back From 085214add30ff315d49311ca24d997b560bfcb52 Mon Sep 17 00:00:00 2001 From: Josh Heald Date: Thu, 6 Nov 2025 16:56:10 +0000 Subject: [PATCH 2/2] Update tests to reflect upsert approach --- .../POSCatalogPersistenceServiceTests.swift | 129 +++++++++++++++--- 1 file changed, 113 insertions(+), 16 deletions(-) diff --git a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift index 58b22c21a45..8c1f64df95c 100644 --- a/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift +++ b/Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift @@ -275,7 +275,7 @@ struct POSCatalogPersistenceServiceTests { } } - @Test func persistIncrementalCatalogData_replaces_attributes_for_updated_product() async throws { + @Test func persistIncrementalCatalogData_upserts_attributes_for_updated_product() async throws { // Given let attribute1 = Yosemite.ProductAttribute.fake().copy(name: "Color", options: ["Indigo", "Blue"]) let attribute2 = Yosemite.ProductAttribute.fake().copy(name: "Size") @@ -291,16 +291,17 @@ struct POSCatalogPersistenceServiceTests { // Then try await grdbManager.databaseConnection.read { db in + // Should have 4 attributes: original 2 + updated 2 (upsert adds new ones, keeps old ones) let attributeCount = try PersistedProductAttribute.fetchCount(db) - #expect(attributeCount == 2) + #expect(attributeCount == 4) let attributes = try PersistedProductAttribute.fetchAll(db).sorted(by: { $0.name < $1.name }) #expect(attributes[0].name == "Color") - #expect(attributes[0].options == ["Cardinal", "Blue"]) - #expect(attributes[0].productID == 1) - #expect(attributes[1].name == "Material") - #expect(attributes[1].options == []) - #expect(attributes[1].productID == 1) + #expect(attributes[0].options == ["Indigo", "Blue"]) // Original unchanged + #expect(attributes[1].name == "Color") + #expect(attributes[1].options == ["Cardinal", "Blue"]) // Updated version + #expect(attributes[2].name == "Material") // New attribute + #expect(attributes[3].name == "Size") // Original unchanged } } @@ -320,24 +321,26 @@ struct POSCatalogPersistenceServiceTests { // Then try await grdbManager.databaseConnection.read { db in - // Check join table has correct count + // Check join table has correct count - should have 3 (original 2 + new 1, upsert keeps all) let joinCount = try PersistedProductImage.fetchCount(db) - #expect(joinCount == 2) + #expect(joinCount == 3) // Check join table entries let joins = try PersistedProductImage.fetchAll(db).sorted(by: { $0.imageID < $1.imageID }) - #expect(joins.count == 2) + #expect(joins.count == 3) #expect(joins[0].productID == 1) #expect(joins[0].imageID == 1) #expect(joins[1].productID == 1) - #expect(joins[1].imageID == 3) + #expect(joins[1].imageID == 2) // Original image 2 join remains + #expect(joins[2].productID == 1) + #expect(joins[2].imageID == 3) - // Check actual images + // Check actual images - image 1 updated, image 2 unchanged, image 3 added let images = try PersistedImage.fetchAll(db).sorted(by: { $0.id < $1.id }) - #expect(images.count == 3) // `image2` remains, but is unlinked. - #expect(images[0].src == "https://example.com/image1-1.jpg") - #expect(images[1].src == "https://example.com/image2.jpg") - #expect(images[2].src == "https://example.com/image3.jpg") + #expect(images.count == 3) + #expect(images[0].src == "https://example.com/image1-1.jpg") // Updated + #expect(images[1].src == "https://example.com/image2.jpg") // Unchanged + #expect(images[2].src == "https://example.com/image3.jpg") // New } } @@ -522,6 +525,100 @@ struct POSCatalogPersistenceServiceTests { } } + @Test func persistIncrementalCatalogData_deletes_variations_not_in_updated_product_variationIDs() async throws { + // Given - a variable product with 3 variations + let product = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 1, + productTypeKey: "variable", + variationIDs: [10, 20, 30] + ) + let variation1 = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 1, productVariationID: 10) + let variation2 = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 1, productVariationID: 20) + let variation3 = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 1, productVariationID: 30) + + let catalog = POSCatalog(products: [product], variations: [variation1, variation2, variation3], syncDate: .now) + try await sut.replaceAllCatalogData(catalog, siteID: sampleSiteID) + + // Verify initial state + try await db.read { db in + let variationCount = try PersistedProductVariation.fetchCount(db) + #expect(variationCount == 3) + } + + // When - incremental sync with updated product that only has 2 variations (removed variation 20) + let updatedProduct = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 1, + productTypeKey: "variable", + variationIDs: [10, 30] // variation 20 removed + ) + let incrementalCatalog = POSCatalog(products: [updatedProduct], variations: [], syncDate: .now) + try await sut.persistIncrementalCatalogData(incrementalCatalog, siteID: sampleSiteID) + + // Then - variation 20 should be deleted, variations 10 and 30 should remain + try await db.read { db in + let variationCount = try PersistedProductVariation.fetchCount(db) + #expect(variationCount == 2) + + let remainingVariations = try PersistedProductVariation.fetchAll(db).sorted(by: { $0.id < $1.id }) + #expect(remainingVariations.count == 2) + #expect(remainingVariations[0].id == 10) + #expect(remainingVariations[1].id == 30) + } + } + + @Test func persistIncrementalCatalogData_preserves_variations_not_mentioned_in_incremental_sync() async throws { + // Given - two variable products with variations + let product1 = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 1, + productTypeKey: "variable", + variationIDs: [10, 20] + ) + let product2 = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 2, + productTypeKey: "variable", + variationIDs: [30, 40] + ) + let variation10 = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 1, productVariationID: 10) + let variation20 = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 1, productVariationID: 20) + let variation30 = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 2, productVariationID: 30) + let variation40 = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 2, productVariationID: 40) + + let catalog = POSCatalog( + products: [product1, product2], + variations: [variation10, variation20, variation30, variation40], + syncDate: .now + ) + try await sut.replaceAllCatalogData(catalog, siteID: sampleSiteID) + + // When - incremental sync only updates product 1, product 2 not mentioned + let updatedProduct1 = POSProduct.fake().copy( + siteID: sampleSiteID, + productID: 1, + productTypeKey: "variable", + variationIDs: [10] // removed variation 20 + ) + let incrementalCatalog = POSCatalog(products: [updatedProduct1], variations: [], syncDate: .now) + try await sut.persistIncrementalCatalogData(incrementalCatalog, siteID: sampleSiteID) + + // Then - variation 20 deleted, but product 2's variations (30, 40) remain untouched + try await db.read { db in + let variationCount = try PersistedProductVariation.fetchCount(db) + #expect(variationCount == 3) + + let remainingVariations = try PersistedProductVariation.fetchAll(db).sorted(by: { $0.id < $1.id }) + #expect(remainingVariations[0].id == 10) + #expect(remainingVariations[0].productID == 1) + #expect(remainingVariations[1].id == 30) + #expect(remainingVariations[1].productID == 2) + #expect(remainingVariations[2].id == 40) + #expect(remainingVariations[2].productID == 2) + } + } + @Test func persistIncrementalCatalogData_stores_incremental_sync_date() async throws { // Given - site with existing full sync date let fullSyncDate = Date().addingTimeInterval(-3600) // 1 hour ago