Skip to content
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

Store Product Image Upload statuses in UserDefaults #15264

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
31e2a56
Add custom Equatable implementation for ProductImage
pmusolino Feb 26, 2025
4627217
feat: use feature flag in `ProductImageUploader` and implement `Produ…
pmusolino Feb 26, 2025
5f9112b
refactor: Update ProductImageStatusStorage and ProductImageUploader t…
pmusolino Feb 27, 2025
38ae4e6
fix: lint warning
pmusolino Feb 27, 2025
e563a29
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Feb 27, 2025
36efa22
update: variable name
pmusolino Feb 27, 2025
817d080
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Feb 28, 2025
3540180
feat: new unit tests for `ProductImageUploader` managing `backgroundP…
pmusolino Feb 28, 2025
3a14b53
fix: improve error handling for image upload failures in `ProductImag…
pmusolino Mar 3, 2025
c720b67
Update WooCommerce/WooCommerceTests/ViewRelated/Products/Media/Produc…
pmusolino Mar 3, 2025
12f4c1b
rename method name in test
pmusolino Mar 3, 2025
ab508e5
update: remove optional chaining from `userDefaultsStatuses` in `Prod…
pmusolino Mar 3, 2025
927e199
- Remove unused code in `MockProductImageUploader.swift`
pmusolino Mar 4, 2025
a3f7fd4
revert: submodule pushed by mistake
pmusolino Mar 4, 2025
53c87cc
Replace `userDefaultsStatuses` with `imageStatusStorage` throughout `…
pmusolino Mar 4, 2025
5e34cfc
fix: imageStatusStorage.clearAllStatuses()
pmusolino Mar 4, 2025
ff2a952
- Remove storage update for failed image upload when feature flag is …
pmusolino Mar 4, 2025
2607997
refactor: simplify notification scheduling for background image uploa…
pmusolino Mar 4, 2025
782291e
refactor: remove unnecessary status observation for background image …
pmusolino Mar 6, 2025
cd388b6
update: removed unused variable
pmusolino Mar 6, 2025
0d4be44
refactor: improve test for image upload handling and unsaved changes …
pmusolino Mar 6, 2025
b952002
refactor: enhance active upload tracking `test_activeUploads_are_trac…
pmusolino Mar 6, 2025
2a7d9e5
Refactor: the ProductImageUploader initializer to accept imageStatusS…
pmusolino Mar 6, 2025
8235551
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Mar 7, 2025
5b719e4
fix: Exclude error updates for products being edited in ProductImageU…
pmusolino Mar 7, 2025
e20a9c0
fix: Use a unique UserDefaults name for each test in ProductImageUplo…
pmusolino Mar 7, 2025
b81ea1c
fix: failing unit test because of generic PHAsset
pmusolino Mar 7, 2025
9ef4dd7
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Mar 11, 2025
ab5748a
update: removed `test_replaceLocalID_is_called_when_flag_enabled` sin…
pmusolino Mar 11, 2025
1afd492
Merge branch 'trunk' into feat/save-product-image-upload-statuses-in-…
pmusolino Mar 28, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion Networking/Networking/Model/Product/ProductImage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import Codegen

/// Represents a ProductImage entity.
///
public struct ProductImage: Codable, Equatable, Sendable, GeneratedCopiable, GeneratedFakeable {
public struct ProductImage: Codable, Sendable, GeneratedCopiable, GeneratedFakeable {
public let imageID: Int64
public let dateCreated: Date // gmt
public let dateModified: Date? // gmt
Expand Down Expand Up @@ -55,6 +55,20 @@ public struct ProductImage: Codable, Equatable, Sendable, GeneratedCopiable, Gen
}
}

extension ProductImage: Equatable {
public static func == (lhs: ProductImage, rhs: ProductImage) -> Bool {
// Convert timestamps to integers to ignore fractional seconds, ensuring date comparisons
// are accurate to the nearest second and avoiding test discrepancies from millisecond differences.
let lhsTimestamp = Int(lhs.dateCreated.timeIntervalSince1970)
let rhsTimestamp = Int(rhs.dateCreated.timeIntervalSince1970)

return lhs.imageID == rhs.imageID &&
lhsTimestamp == rhsTimestamp &&
lhs.src == rhs.src &&
lhs.name == rhs.name &&
lhs.alt == rhs.alt
}
}

/// Defines all the ProductImage CodingKeys.
///
Expand Down
160 changes: 141 additions & 19 deletions WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import protocol Yosemite.StoresManager
import enum Yosemite.ProductImageStatus
import enum Yosemite.ProductImageAssetType
import enum Yosemite.ProductOrVariationID
import class Networking.ProductImageStatusStorage
import protocol Experiments.FeatureFlagService

/// Information about a background product image upload error.
struct ProductImageUploadErrorInfo {
Expand Down Expand Up @@ -87,12 +89,52 @@ protocol ProductImageUploaderProtocol {

/// Supports background image upload and product images update after the user leaves the product form.
final class ProductImageUploader: ProductImageUploaderProtocol {

let userDefaultsStatuses: ProductImageStatusStorage?

var errors: AnyPublisher<ProductImageUploadErrorInfo, Never> {
errorsSubject.eraseToAnyPublisher()
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
let upstream = userDefaultsStatuses?.errorsPublisher
?? Empty<[(siteID: Int64,
productOrVariationID: ProductOrVariationID?,
assetType: ProductImageAssetType?,
error: Error)], Never>().eraseToAnyPublisher()
return upstream
.flatMap { errorItems in
errorItems.publisher
.compactMap { errorItem in
guard let productOrVariationID = errorItem.productOrVariationID,
let assetType = errorItem.assetType else { return nil }
return ProductImageUploadErrorInfo(
siteID: errorItem.siteID,
productOrVariationID: productOrVariationID,
error: .failedUploadingImage(asset: assetType, error: errorItem.error)
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also missed in the previous reviews that the stream should exclude any keys listed in statusUpdatesExcludedProductKeys. That way, we would not display the error notice when an upload fails while the user is on the same product form.

Notice above that both the error notice and alert are displayed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch Huong, I fixed it here 5b719e4 can you please take another look?

}
.eraseToAnyPublisher()
} else {
return errorsSubject.eraseToAnyPublisher()
}
}

var activeUploads: AnyPublisher<[ProductImageUploaderKey], Never> {
$activeUploadsPublisher.eraseToAnyPublisher()
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
return userDefaultsStatuses?.statusesPublisher
.map { statuses in
statuses.compactMap { status -> ProductImageUploaderKey? in
if status.isUploading {
return ProductImageUploaderKey(siteID: status.siteID,
productOrVariationID: status.productOrVariationID,
isLocalID: status.isLocalID)
}
return nil
}
}
.eraseToAnyPublisher() ?? Empty().eraseToAnyPublisher()
} else {
return $activeUploadsPublisher.eraseToAnyPublisher()
}
}

typealias Key = ProductImageUploaderKey
Expand All @@ -108,12 +150,25 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
@Published private var activeUploadsPublisher: [ProductImageUploaderKey] = []

private let stores: StoresManager
private let featureFlagService: FeatureFlagService
private let imagesProductIDUpdater: ProductImagesProductIDUpdaterProtocol

private var cancellables = Set<AnyCancellable>()

init(stores: StoresManager = ServiceLocator.stores,
featureFlagService: FeatureFlagService = ServiceLocator.featureFlagService,
imagesProductIDUpdater: ProductImagesProductIDUpdaterProtocol = ProductImagesProductIDUpdater()) {
self.stores = stores
self.featureFlagService = featureFlagService
self.imagesProductIDUpdater = imagesProductIDUpdater

if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
self.userDefaultsStatuses = ProductImageStatusStorage()
observeStatuses()
} else {
self.userDefaultsStatuses = nil
}

// Observe when the app enters background.
NotificationCenter.default.addObserver(self,
selector: #selector(appDidEnterBackground),
Expand Down Expand Up @@ -170,9 +225,17 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
}

func sendBackgroundUploadNoticeIfNeeded(key: ProductImageUploaderKey, using noticePresenter: NoticePresenter) {
if activeUploadsPublisher.contains(key) {
let notice = Notice(title: Localization.backgroundUploadNoticeTitle)
noticePresenter.enqueue(notice: notice)
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
if let statuses = userDefaultsStatuses?.getAllStatuses(for: key.siteID, productID: key.productOrVariationID),
statuses.contains(where: { $0.isUploading }) {
let notice = Notice(title: Localization.backgroundUploadNoticeTitle)
noticePresenter.enqueue(notice: notice)
}
} else {
if activeUploadsPublisher.contains(key) {
let notice = Notice(title: Localization.backgroundUploadNoticeTitle)
noticePresenter.enqueue(notice: notice)
}
}
}

Expand Down Expand Up @@ -248,13 +311,31 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
}

private func scheduleUploadInProgressNotificationIfNeeded() {
guard !activeUploadsPublisher.isEmpty else { return }
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
userDefaultsStatuses?.statusesPublisher
.map { statuses in
statuses.filter { $0.isUploading }
}
.sink { uploadingStatuses in
if !uploadingStatuses.isEmpty {
let notification = LocalNotification(scenario: .productImageBackgroundUpload)
let trigger = UNTimeIntervalNotificationTrigger(timeInterval: 1, repeats: false)
Task {
await LocalNotificationScheduler(pushNotesManager: ServiceLocator.pushNotesManager).schedule(notification: notification,
trigger: trigger, remoteFeatureFlag: nil)
}
}
}
.store(in: &cancellables)
} else {
guard !activeUploadsPublisher.isEmpty else { return }

let notification = LocalNotification(scenario: .productImageBackgroundUpload)
let trigger = UNTimeIntervalNotificationTrigger(timeInterval: 1, repeats: false)
Task {
await LocalNotificationScheduler(pushNotesManager: ServiceLocator.pushNotesManager).schedule(notification: notification,
trigger: trigger, remoteFeatureFlag: nil)
let notification = LocalNotification(scenario: .productImageBackgroundUpload)
let trigger = UNTimeIntervalNotificationTrigger(timeInterval: 1, repeats: false)
Task {
await LocalNotificationScheduler(pushNotesManager: ServiceLocator.pushNotesManager).schedule(notification: notification,
trigger: trigger, remoteFeatureFlag: nil)
}
}
}

Expand All @@ -278,16 +359,41 @@ private extension ProductImageUploader {
}
}

private func observeStatuses() {
userDefaultsStatuses?.statusesPublisher
.sink { [weak self] statuses in
guard let self = self else { return }

// Handle errors
if let failureStatus = statuses.first(where: { $0.isUploadFailure }),
let error = failureStatus.error {
let errorInfo = ProductImageUploadErrorInfo(
siteID: failureStatus.siteID,
productOrVariationID: failureStatus.productOrVariationID,
error: .failedUploadingImage(asset: failureStatus.asset!, error: error)
)
self.errorsSubject.send(errorInfo)
}
}
.store(in: &cancellables)
}

func observeStatusUpdates(key: Key, actionHandler: ProductImageActionHandler) {
let observationToken = actionHandler.addUpdateObserver(self) { [weak self] productImageStatuses in
guard let self = self else { return }

if !activeUploadsPublisher.contains(key), productImageStatuses.hasPendingUpload {
activeUploadsPublisher.append(key)
} else if activeUploadsPublisher.contains(key), !productImageStatuses.hasPendingUpload {
/// When all pending uploads are completed or removed,
/// remove the key from active uploads
removeProductFromActiveUploads(key: key)
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
// Update the states in userDefaultsStatuses
self.userDefaultsStatuses?.appendStatuses(productImageStatuses, for: key.siteID, productID: key.productOrVariationID)
}
else {
if !activeUploadsPublisher.contains(key), productImageStatuses.hasPendingUpload {
activeUploadsPublisher.append(key)
} else if activeUploadsPublisher.contains(key), !productImageStatuses.hasPendingUpload {
/// When all pending uploads are completed or removed,
/// remove the key from active uploads
removeProductFromActiveUploads(key: key)
}
}
}
statusUpdatesSubscriptions.insert(observationToken)
Expand All @@ -302,15 +408,31 @@ private extension ProductImageUploader {
productOrVariationID: key.productOrVariationID,
error: .failedUploadingImage(asset: asset, error: error))
if statusUpdatesExcludedProductKeys.contains(key) == false {
errorsSubject.send(infoError)
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
let failedStatus = ProductImageStatus.uploadFailure(asset: asset,
error: error,
siteID: key.siteID,
productID: key.productOrVariationID)
self.userDefaultsStatuses?.updateStatus(failedStatus)
} else {
errorsSubject.send(infoError)
}
}
}
}
imageUploadSubscriptions.insert(observationToken)
}

func removeProductFromActiveUploads(key: Key) {
activeUploadsPublisher.removeAll(where: { $0 == key })
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
userDefaultsStatuses?.removeStatus(where: { status in
status.siteID == key.siteID &&
status.productOrVariationID == key.productOrVariationID &&
status.isUploading
})
} else {
activeUploadsPublisher.removeAll(where: { $0 == key })
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ final class MockFeatureFlagService: FeatureFlagService {
var favoriteProducts: Bool
var isProductGlobalUniqueIdentifierSupported: Bool
var hideSitesInStorePicker: Bool
var backgroundProductImageUpload: Bool

init(isInboxOn: Bool = false,
isShowInboxCTAEnabled: Bool = false,
Expand All @@ -44,7 +45,8 @@ final class MockFeatureFlagService: FeatureFlagService {
viewEditCustomFieldsInProductsAndOrders: Bool = false,
favoriteProducts: Bool = false,
isProductGlobalUniqueIdentifierSupported: Bool = false,
hideSitesInStorePicker: Bool = false) {
hideSitesInStorePicker: Bool = false,
backgroundProductImageUpload: Bool = false) {
self.isInboxOn = isInboxOn
self.isShowInboxCTAEnabled = isShowInboxCTAEnabled
self.isUpdateOrderOptimisticallyOn = isUpdateOrderOptimisticallyOn
Expand All @@ -66,6 +68,7 @@ final class MockFeatureFlagService: FeatureFlagService {
self.favoriteProducts = favoriteProducts
self.isProductGlobalUniqueIdentifierSupported = isProductGlobalUniqueIdentifierSupported
self.hideSitesInStorePicker = hideSitesInStorePicker
self.backgroundProductImageUpload = backgroundProductImageUpload
}

func isFeatureFlagEnabled(_ featureFlag: FeatureFlag) -> Bool {
Expand Down Expand Up @@ -112,6 +115,8 @@ final class MockFeatureFlagService: FeatureFlagService {
return isProductGlobalUniqueIdentifierSupported
case .hideSitesInStorePicker:
return hideSitesInStorePicker
case .backgroundProductImageUpload:
return backgroundProductImageUpload
default:
return false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ final class MockProductImageUploader {
var startEmittingErrorsWasCalled = false
var stopEmittingErrorsWasCalled = false
var resetWasCalled = false
var sendBackgroundUploadNoticeIfNeededWasCalled = false

private var onProductSaveResult: Result<[ProductImage], Error>?
private var hasUnsavedChangesOnImages = false
Expand Down Expand Up @@ -64,7 +65,12 @@ extension MockProductImageUploader: ProductImageUploaderProtocol {
}

func sendBackgroundUploadNoticeIfNeeded(key: ProductImageUploaderKey, using noticePresenter: NoticePresenter) {
// no-op
sendBackgroundUploadNoticeIfNeededWasCalled = true

// It actually sends a notification if there are active uploads for this key.
if activeUploadsKeys.contains(where: { $0 == key }) {
noticePresenter.enqueue(notice: Notice(title: "Test Notice"))
}
}

func reset() {
Expand Down
Loading