Background image upload: Fix issue showing uploaded images while saving is in progress (#15107)

itsmeichigo · web-flow · commit 3fe785074b56 · 2025-02-14T10:02:56.000+07:00
diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
@@ -7,6 +7,7 @@
 - [*] Now "Suggested by AI" label is visible in dark mode in Blaze campaign creation flow. [https://github.com/woocommerce/woocommerce-ios/pull/15088]
 - [*] Improved image loading in Blaze Campaign Creation: displays a redacted and shimmering effects when loading product image and falls back to a placeholder if no image is available. [https://github.com/woocommerce/woocommerce-ios/pull/15098]
 - [*] Product List: Display syncing animation on items with image upload in progress [https://github.com/woocommerce/woocommerce-ios/pull/15052]
+- [*] Background image upload: Fix issue showing uploaded images while saving is in progress [https://github.com/woocommerce/woocommerce-ios/pull/15107]
 - [*] Background image upload: Fix missing error notice in iPhones [https://github.com/woocommerce/woocommerce-ios/pull/15117]
 - [*] Filters applied in product selector no longer affect the main product list screen. [https://github.com/woocommerce/woocommerce-ios/pull/14764]
 
diff --git a/WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift b/WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift
@@ -110,7 +110,6 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
 
     private var actionHandlersByProduct: [Key: ProductImageActionHandler] = [:]
     private var imagesSaverByProduct: [Key: ProductImagesSaver] = [:]
-    private var initialStatusesByProduct: [Key: [ProductImageStatus]] = [:]
 
     @Published private var activeUploadsPublisher: [ProductImageUploaderKey] = []
 
@@ -125,12 +124,11 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
 
     func actionHandler(key: ProductImageUploaderKey, originalStatuses: [ProductImageStatus]) -> ProductImageActionHandler {
         let actionHandler: ProductImageActionHandler
-        if let handler = actionHandlersByProduct[key], handler.productImageStatuses.hasPendingUpload {
+        if let handler = actionHandlersByProduct[key] {
             actionHandler = handler
         } else {
             actionHandler = ProductImageActionHandler(siteID: key.siteID, productID: key.productOrVariationID, imageStatuses: originalStatuses, stores: stores)
             actionHandlersByProduct[key] = actionHandler
-            initialStatusesByProduct[key] = originalStatuses
             observeStatusUpdates(key: key, actionHandler: actionHandler)
         }
 
@@ -171,14 +169,24 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
         guard let handler = actionHandlersByProduct[key] else {
             return false
         }
-        if let productImagesSaver = imagesSaverByProduct[key], productImagesSaver.imageStatusesToSave.isNotEmpty {
+        let productImagesSaver = imagesSaverByProduct[key]
+
+        if let productImagesSaver, productImagesSaver.imageStatusesToSave.isNotEmpty {
             // If there are images scheduled to be saved, there are no unsaved changes if the image statuses to save match the latest image statuses.
             return handler.productImageStatuses != productImagesSaver.imageStatusesToSave
         } else {
-            // Otherwise, there are unsaved changes if there is any pending image upload or any difference in the remote image IDs between the
+            if handler.productImageStatuses.hasPendingUpload {
+                return true
+            }
+
+            /// If there's a product saved in background, compare the images to determine unsaved changes.
+            if let savedProduct = productImagesSaver?.savedProduct {
+                return handler.productImageStatuses.images.map { $0.imageID } != savedProduct.images.map { $0.imageID }
+            }
+
+            // Otherwise, there are unsaved changes if there is any difference in the remote image IDs between the
             // original and latest product.
-            return handler.productImageStatuses.hasPendingUpload ||
-            handler.productImageStatuses.images.map { $0.imageID } != originalImages.map { $0.imageID }
+            return handler.productImageStatuses.images.map { $0.imageID } != originalImages.map { $0.imageID }
         }
     }
 
@@ -187,12 +195,10 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
         // The product has to exist remotely in order to save its images remotely.
         // In product creation, this save function should be called after a new product is saved remotely for the first time.
         guard key.isLocalID == false else {
-            removeProductFromActiveUploads(key: key)
             return onProductSave(.failure(ProductImageUploaderError.noRemoteProductIDFound))
         }
 
         guard let handler = actionHandlersByProduct[key] else {
-            removeProductFromActiveUploads(key: key)
             return onProductSave(.failure(ProductImageUploaderError.noActionHandlerFound))
         }
 
@@ -208,7 +214,6 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
 
         imagesSaver.saveProductImagesWhenNoneIsPendingUploadAnymore(imageActionHandler: handler) { [weak self] result in
             guard let self = self else { return }
-            removeProductFromActiveUploads(key: key)
             onProductSave(result)
             if case let .failure(error) = result {
                 self.errorsSubject.send(.init(siteID: key.siteID,
@@ -229,7 +234,6 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
 
         actionHandlersByProduct = [:]
         imagesSaverByProduct = [:]
-        initialStatusesByProduct = [:]
     }
 }
 
@@ -254,10 +258,9 @@ private extension ProductImageUploader {
 
             if !activeUploadsPublisher.contains(key), productImageStatuses.hasPendingUpload {
                 activeUploadsPublisher.append(key)
-            } else if let initialStatuses = initialStatusesByProduct[key],
-                initialStatuses == productImageStatuses,
-                activeUploadsPublisher.contains(key) {
-                /// When upload is reset, remove the key from active uploads
+            } else if activeUploadsPublisher.contains(key), !productImageStatuses.hasPendingUpload {
+                /// When all pending uploads are completed or removed,
+                /// remove the key from active uploads
                 removeProductFromActiveUploads(key: key)
             }
 
@@ -274,7 +277,6 @@ private extension ProductImageUploader {
 
     func removeProductFromActiveUploads(key: Key) {
         activeUploadsPublisher.removeAll(where: { $0 == key })
-        initialStatusesByProduct.removeValue(forKey: key)
     }
 }
 
diff --git a/WooCommerce/Classes/ViewRelated/Products/Cells/ProductsTabProductTableViewCell.swift b/WooCommerce/Classes/ViewRelated/Products/Cells/ProductsTabProductTableViewCell.swift
@@ -58,6 +58,14 @@ final class ProductsTabProductTableViewCell: UITableViewCell {
         super.updateConfiguration(using: state)
         updateDefaultBackgroundConfiguration(using: state)
     }
+
+    override func prepareForReuse() {
+        super.prepareForReuse()
+        syncingOverlayView?.removeFromSuperview()
+        selectedProductImageOverlayView?.removeFromSuperview()
+        syncingOverlayView = nil
+        selectedProductImageOverlayView = nil
+    }
 }
 
 extension ProductsTabProductTableViewCell: SearchResultCell {
diff --git a/WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesSaver.swift b/WooCommerce/Classes/ViewRelated/Products/Media/ProductImagesSaver.swift
@@ -10,6 +10,9 @@ final class ProductImagesSaver {
             imageStatusesToSaveSubject.send(imageStatusesToSave)
         }
     }
+
+    private(set) var savedProduct: Product?
+
     // The use of a `PassthroughSubject` sent on variable `didSet` instead of a `@Published` is because `@Published`
     // subscription happens in `willSet` while we want to update the statuses after `didSet`.
     private let imageStatusesToSaveSubject: PassthroughSubject<[ProductImageStatus], Never> = .init()
@@ -38,6 +41,9 @@ final class ProductImagesSaver {
     ///   - onProductSave: called after the product is updated remotely with the uploaded images.
     func saveProductImagesWhenNoneIsPendingUploadAnymore(imageActionHandler: ProductImageActionHandlerProtocol,
                                                          onProductSave: @escaping (Result<[ProductImage], Error>) -> Void) {
+        /// Reset previously saved product
+        savedProduct = nil
+
         let imageStatuses = imageActionHandler.productImageStatuses
         guard imageStatuses.hasPendingUpload else {
             return saveProductImages(imageStatuses.images, onProductSave: onProductSave)
@@ -72,6 +78,7 @@ private extension ProductImagesSaver {
                 guard let self = self else { return }
                 switch result {
                 case .success(let product):
+                    savedProduct = product
                     onProductSave(.success(product.images))
                     self.analytics.track(event:
                             .ImageUpload.savingProductAfterBackgroundImageUploadSuccess(productOrVariation: .product))
diff --git a/WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift
@@ -594,26 +594,16 @@ final class ProductImageUploaderTests: XCTestCase {
         let uploadedMedia = Media.fake().copy(mediaID: 645)
         stores.whenReceivingAction(ofType: MediaAction.self) { action in
             if case let .uploadMedia(_, _, _, _, _, onCompletion) = action {
+                XCTAssertEqual(activeUploads, [key])
                 onCompletion(.success(uploadedMedia))
-            }
-        }
-        actionHandler.uploadMediaAssetToSiteMediaLibrary(asset: .phAsset(asset: asset))
-
-        // Then
-        waitUntil {
-            activeUploads == [key]
-        }
 
-        // When
-        stores.whenReceivingAction(ofType: ProductAction.self) { action in
-            if case let .updateProductImages(_, _, images, onCompletion) = action {
-                onCompletion(.success(.fake().copy(images: images)))
+                // Then
+                self.waitUntil {
+                    activeUploads == []
+                }
             }
         }
-        imageUploader.saveProductImagesWhenNoneIsPendingUploadAnymore(key: key) { _ in
-            // Then
-            XCTAssertEqual(activeUploads, [])
-        }
+        actionHandler.uploadMediaAssetToSiteMediaLibrary(asset: .phAsset(asset: asset))
     }
 
     func test_product_is_removed_from_activeUploads_when_upload_is_cancelled() {
diff --git a/WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImagesSaverTests.swift b/WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImagesSaverTests.swift
@@ -181,6 +181,63 @@ final class ProductImagesSaverTests: XCTestCase {
         XCTAssertEqual(savedImages, [image])
     }
 
+    func test_savedProduct_is_updated_correctly() {
+        // Given
+        let stores = MockStoresManager(sessionManager: .testingInstance)
+        let actionHandler = ProductImageActionHandler(siteID: siteID, productID: .product(id: productID), imageStatuses: [], stores: stores)
+        let asset: ProductImageAssetType = .phAsset(asset: PHAsset())
+        let imagesSaver = ProductImagesSaver(siteID: siteID, productOrVariationID: .product(id: productID), stores: stores)
+        let expectedProduct = Product.fake().copy(images: [])
+
+        // Mocks successful product images update.
+        stores.whenReceivingAction(ofType: ProductAction.self) { action in
+            if case let .updateProductImages(_, _, _, onCompletion) = action {
+                onCompletion(.success(expectedProduct))
+            }
+        }
+
+        // Uploads an image and waits for the image upload completion closure to be called later.
+        let imageUploadCompletion: ((Result<Media, Error>) -> Void) = waitFor { promise in
+            stores.whenReceivingAction(ofType: MediaAction.self) { action in
+                if case let .uploadMedia(_, _, _, _, _, onCompletion) = action {
+                    promise(onCompletion)
+                }
+            }
+            actionHandler.uploadMediaAssetToSiteMediaLibrary(asset: asset)
+        }
+
+        waitFor { promise in
+            // Saves product images.
+            imagesSaver.saveProductImagesWhenNoneIsPendingUploadAnymore(imageActionHandler: actionHandler) { _ in
+                promise(())
+            }
+            XCTAssertNil(imagesSaver.savedProduct)
+
+            // When
+            // Mocks successful image upload.
+            imageUploadCompletion(.success(.fake().copy(mediaID: 645)))
+        }
+
+        // Then
+        XCTAssertEqual(imagesSaver.savedProduct, expectedProduct)
+
+        // When
+        // Uploads another image and save.
+        let imageUploadCompletionAfterSave: ((Result<Media, Error>) -> Void) = waitFor { promise in
+            stores.whenReceivingAction(ofType: MediaAction.self) { action in
+                if case let .uploadMedia(_, _, _, _, _, onCompletion) = action {
+                    promise(onCompletion)
+                }
+            }
+            actionHandler.uploadMediaAssetToSiteMediaLibrary(asset: asset)
+        }
+        imageUploadCompletionAfterSave(.success(.fake().copy(mediaID: 606)))
+        imagesSaver.saveProductImagesWhenNoneIsPendingUploadAnymore(imageActionHandler: actionHandler) { _ in }
+
+        // Then the saved product is reset immediately
+        XCTAssertNil(imagesSaver.savedProduct)
+    }
+
     // MARK: - Analytics
 
     func test_savingProductAfterBackgroundImageUploadSuccess_is_tracked_on_variation_update_success() throws {

Original file line number	Diff line number	Diff line change
`@@ -58,6 +58,14 @@ final class ProductsTabProductTableViewCell: UITableViewCell {`
`58`	`58`	`super.updateConfiguration(using: state)`
`59`	`59`	`updateDefaultBackgroundConfiguration(using: state)`
`60`	`60`	`}`
	`61`	`+`
	`62`	`+ override func prepareForReuse() {`
	`63`	`+ super.prepareForReuse()`
	`64`	`+ syncingOverlayView?.removeFromSuperview()`
	`65`	`+ selectedProductImageOverlayView?.removeFromSuperview()`
	`66`	`+ syncingOverlayView = nil`
	`67`	`+ selectedProductImageOverlayView = nil`
	`68`	`+ }`
`61`	`69`	`}`
`62`	`70`
`63`	`71`	`extension ProductsTabProductTableViewCell: SearchResultCell {`