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 27 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
147 changes: 127 additions & 20 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,65 @@ protocol ProductImageUploaderProtocol {

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

let imageStatusStorage: ProductImageStatusStorage

var errors: AnyPublisher<ProductImageUploadErrorInfo, Never> {
errorsSubject.eraseToAnyPublisher()
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
return imageStatusStorage.errorsPublisher
.flatMap { errorItems in
errorItems.publisher
.compactMap { errorItem in
guard let productOrVariationID = errorItem.productOrVariationID,
let assetType = errorItem.assetType else { return nil }

// Create key to check against excluded keys
let key = Key(siteID: errorItem.siteID,
productOrVariationID: productOrVariationID,
isLocalID: productOrVariationID.id == 0)

// Skip error if it's for a product being edited
guard !self.statusUpdatesExcludedProductKeys.contains(key) else {
return nil
}

return ProductImageUploadErrorInfo(
siteID: errorItem.siteID,
productOrVariationID: productOrVariationID,
error: .failedUploadingImage(asset: assetType, error: errorItem.error)
)
}
}
.eraseToAnyPublisher()
} else {
return errorsSubject
.filter { info in
let key = Key(siteID: info.siteID,
productOrVariationID: info.productOrVariationID,
isLocalID: info.productOrVariationID.id == 0)
return !self.statusUpdatesExcludedProductKeys.contains(key)
}
Comment on lines +124 to +129
Copy link
Contributor

Choose a reason for hiding this comment

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

This filter is redundant because we already check for this in observeImageUploads.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not think it's redundant because errorsSubject can receive error events from other places in ProductImageUploader besides observeImageUploads. For instance, if you look at the saveProductImagesWhenNoneIsPendingUploadAnymore method, it calls errorsSubject.send(...) directly when saving fails, without checking statusUpdatesExcludedProductKeys. Because of that, the filter on errorSubject acts as a "final gatekeeper" ensuring that errors tied to excluded keys are reliably discarded no matter where in ProductImageUploader they're sent.
If the only place errors were published was observeImageUploads and it always checked for excluded keys, then the filter might be redundant, but given errors can come from more than one place, I believe the filter is necessary to ensure excluded keys never emit errors. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was intentional that the save product errors were not filtered out. We were only filtering out upload errors because we also listened to the product form errors. The background saving is handled separately from the product form, so any failures would still be displayed. But this code exists before I joined the project so I'm not entirely sure.

If you feel strongly about hiding the background saving failures when the product form is present, please keep the current change.

.eraseToAnyPublisher()
}
}

var activeUploads: AnyPublisher<[ProductImageUploaderKey], Never> {
$activeUploadsPublisher.eraseToAnyPublisher()
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
return imageStatusStorage.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()
} else {
return $activeUploadsPublisher.eraseToAnyPublisher()
}
}

typealias Key = ProductImageUploaderKey
Expand All @@ -108,12 +163,20 @@ 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,
imagesProductIDUpdater: ProductImagesProductIDUpdaterProtocol = ProductImagesProductIDUpdater()) {
featureFlagService: FeatureFlagService = ServiceLocator.featureFlagService,
imagesProductIDUpdater: ProductImagesProductIDUpdaterProtocol = ProductImagesProductIDUpdater(),
imageStatusStorage: ProductImageStatusStorage = ProductImageStatusStorage()) {
self.stores = stores
self.featureFlagService = featureFlagService
self.imagesProductIDUpdater = imagesProductIDUpdater
self.imageStatusStorage = imageStatusStorage

// Observe when the app enters background.
NotificationCenter.default.addObserver(self,
selector: #selector(appDidEnterBackground),
Expand Down Expand Up @@ -170,9 +233,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) {
let statuses = imageStatusStorage.getAllStatuses(for: key.siteID, productID: key.productOrVariationID)
if 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 @@ -243,18 +314,34 @@ final class ProductImageUploader: ProductImageUploaderProtocol {
imageUploadSubscriptions = []
activeUploadsPublisher = []

imageStatusStorage.clearAllStatuses()

actionHandlersByProduct = [:]
imagesSaverByProduct = [:]
}

private func scheduleUploadInProgressNotificationIfNeeded() {
guard !activeUploadsPublisher.isEmpty else { return }
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
let statuses = imageStatusStorage.getAllStatuses()
let hasUploadingStatuses = statuses.contains { $0.isUploading }

if hasUploadingStatuses {
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)
}
}
} 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 Down Expand Up @@ -282,12 +369,18 @@ private extension ProductImageUploader {
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.imageStatusStorage.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 +395,29 @@ private extension ProductImageUploader {
productOrVariationID: key.productOrVariationID,
error: .failedUploadingImage(asset: asset, error: error))
if statusUpdatesExcludedProductKeys.contains(key) == false {
errorsSubject.send(infoError)
if !self.featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
// Only send the error directly if `backgroundProductImageUpload` feature flag is disabled
errorsSubject.send(infoError)
}
// To keep in mind
// Do not update storage here as the action handler will update its status
// which will trigger `observeStatusUpdates` to do the storage update
}
}
}
imageUploadSubscriptions.insert(observationToken)
}

func removeProductFromActiveUploads(key: Key) {
activeUploadsPublisher.removeAll(where: { $0 == key })
if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) {
imageStatusStorage.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 @@ -64,7 +64,6 @@ extension MockProductImageUploader: ProductImageUploaderProtocol {
}

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

func reset() {
Expand Down
Loading