[Local catalog] Fix observation of variation attributes (#16400)

Author: joshheald

Modules/Sources/Yosemite/PointOfSale/Items/GRDBObservableDataSource.swift

@@ -160,9 +160,47 @@ public final class GRDBObservableDataSource: POSObservableDataSourceProtocol {
 
     private func setupVariationObservation(parentProduct: POSVariableParentProduct) {
         let currentPage = currentVariationPage
+        let parentProductID = parentProduct.productID
+
+        struct ObservationResult {
+            let variations: [POSProductVariation]
+            let parentProduct: POSVariableParentProduct
+        }
+
         let observation = ValueObservation
-            .tracking { [weak self] database -> [POSProductVariation] in
-                guard let self else { return [] }
+            .tracking { [weak self] database -> ObservationResult in
+                guard let self else { return ObservationResult(variations: [], parentProduct: parentProduct) }
+
+                // Fetch parent product with updated attributes
+                struct ParentProductWithAttributes: Decodable, FetchableRecord {
+                    let product: PersistedProduct
+                    let attributes: [PersistedProductAttribute]
+                }
+
+                let parentWithAttributes = try PersistedProduct
+                    .filter(PersistedProduct.Columns.siteID == self.siteID)
+                    .filter(PersistedProduct.Columns.id == parentProductID)
+                    .including(all: PersistedProduct.attributes)
+                    .asRequest(of: ParentProductWithAttributes.self)
+                    .fetchOne(database)
+
+                // Create updated parent product with fresh attributes
+                let updatedParentProduct: POSVariableParentProduct
+                if let parentWithAttributes {
+                    let attributes = (parentWithAttributes.attributes).map {
+                        $0.toProductAttribute(siteID: parentWithAttributes.product.siteID)
+                    }
+                    let product = parentWithAttributes.product
+                    updatedParentProduct = POSVariableParentProduct(
+                        id: parentProduct.id,
+                        name: product.name,
+                        productImageSource: nil, // Image not needed for variation name generation
+                        productID: product.id,
+                        allAttributes: attributes
+                    )
+                } else {
+                    updatedParentProduct = parentProduct
+                }
 
                 struct VariationWithRelations: Decodable, FetchableRecord {
                     let persistedProductVariation: PersistedProductVariation
@@ -171,36 +209,42 @@ public final class GRDBObservableDataSource: POSObservableDataSourceProtocol {
                 }
 
                 let variationsWithRelations = try PersistedProductVariation
-                    .posVariationsRequest(siteID: self.siteID, parentProductID: parentProduct.productID)
+                    .posVariationsRequest(siteID: self.siteID, parentProductID: parentProductID)
                     .limit(self.pageSize * currentPage)
                     .including(all: PersistedProductVariation.attributes)
                     .including(optional: PersistedProductVariation.image)
                     .asRequest(of: VariationWithRelations.self)
                     .fetchAll(database)
 
-                return variationsWithRelations.map { record in
+                let variations = variationsWithRelations.map { record in
                     record.persistedProductVariation.toPOSProductVariation(
                         attributes: (record.attributes ?? []).map { $0.toProductVariationAttribute() },
                         image: record.image?.toProductImage()
                     )
                 }
+
+                return ObservationResult(variations: variations, parentProduct: updatedParentProduct)
             }
 
         variationObservationCancellable = observation
             .publisher(in: grdbManager.databaseConnection)
             .receive(on: DispatchQueue.main)
             .sink(
-                receiveCompletion: { [weak self] completion in
+                receiveCompletion: { [weak self] (completion: Subscribers.Completion<Error>) in
                     if case .failure(let error) = completion {
                         self?.variationError = error
                         self?.isLoadingVariations = false
                     }
                 },
-                receiveValue: { [weak self] observedVariations in
+                receiveValue: { [weak self] (result: ObservationResult) in
                     guard let self else { return }
+
+                    // Update current parent product with fresh attributes
+                    self.currentParentProduct = result.parentProduct
+
                     let posItems = itemMapper.mapVariationsToPOSItems(
-                        variations: observedVariations,
-                        parentProduct: parentProduct
+                        variations: result.variations,
+                        parentProduct: result.parentProduct
                     )
                     variationItems = posItems
                     variationError = nil

Modules/Sources/Yosemite/Tools/POS/POSCatalogPersistenceService.swift

@@ -116,6 +116,11 @@ final class POSCatalogPersistenceService: POSCatalogPersistenceServiceProtocol {
             }
 
             for variation in catalog.variationsToPersist {
+                // Delete attributes for updated variations, the remaining set will be recreated later in the save
+                try PersistedProductVariationAttribute
+                    .filter(PersistedProductVariationAttribute.Columns.productVariationID == variation.id)
+                    .deleteAll(db)
+
                 try variation.save(db)
             }
 

Modules/Tests/YosemiteTests/Tools/POS/POSCatalogPersistenceServiceTests.swift

@@ -472,6 +472,36 @@ struct POSCatalogPersistenceServiceTests {
         }
     }
 
+    @Test func persistIncrementalCatalogData_prevents_duplicate_variation_attributes_on_multiple_syncs() async throws {
+        // Given - variation with attributes
+        let parentProduct = POSProduct.fake().copy(siteID: sampleSiteID, productID: 10)
+        let attribute1 = Yosemite.ProductVariationAttribute.fake().copy(name: "Color", option: "Red")
+        let attribute2 = Yosemite.ProductVariationAttribute.fake().copy(name: "Size", option: "L")
+        let variation = POSProductVariation.fake().copy(siteID: sampleSiteID, productID: 10, productVariationID: 1, attributes: [attribute1, attribute2])
+        try await insertProduct(parentProduct)
+        try await insertVariation(variation)
+
+        // When - perform multiple incremental syncs with the same variation/attributes
+        let catalog = POSCatalog(products: [parentProduct], variations: [variation], syncDate: .now)
+        try await sut.persistIncrementalCatalogData(catalog, siteID: sampleSiteID)
+        try await sut.persistIncrementalCatalogData(catalog, siteID: sampleSiteID)
+        try await sut.persistIncrementalCatalogData(catalog, siteID: sampleSiteID)
+
+        // Then - should have exactly 2 attributes, not duplicates
+        try await grdbManager.databaseConnection.read { db in
+            let attributeCount = try PersistedProductVariationAttribute.fetchCount(db)
+            #expect(attributeCount == 2)
+
+            let attributes = try PersistedProductVariationAttribute.fetchAll(db).sorted(by: { $0.name < $1.name })
+            #expect(attributes[0].name == "Color")
+            #expect(attributes[0].option == "Red")
+            #expect(attributes[0].productVariationID == 1)
+            #expect(attributes[1].name == "Size")
+            #expect(attributes[1].option == "L")
+            #expect(attributes[1].productVariationID == 1)
+        }
+    }
+
     @Test func persistIncrementalCatalogData_replaces_image_for_updated_variation() async throws {
         // Given
         let parentProduct = POSProduct.fake().copy(siteID: sampleSiteID, productID: 10)