Store Product Image Upload statuses in UserDefaults #15264
Conversation
…ctImageStatusStorage` for storing images statuses in UserDefaults if feature flag is enabled
…o handle multiple image upload errors
…s-part-4' into feat/save-product-image-upload-statuses-in-user-defaults-part-5 # Conflicts: # Networking/Networking/ProductImageInBackground/ProductImageStatusStorage.swift
pmusolino
added
the
feature: add/edit products
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant U as ProductImageUploader
participant F as FeatureFlagService
participant S as ProductImageStatusStorage
participant N as NoticePresenter
U->>F: isFeatureFlagEnabled(backgroundProductImageUpload)
alt Feature Enabled
U->>S: Initialize & observe statuses
U->>S: Retrieve active uploads & errors
alt Active Upload Exists
U->>N: Enqueue background upload notice
end
else Feature Disabled
U->>U: Execute default upload logic
end
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
|
…s-part-4' into feat/save-product-image-upload-statuses-in-user-defaults-part-5
…roductImageUpload` feature flag, and testing that when `backgroundProductImageUpload == true`, `ProductImageStatusStorage` will be used to store image statuses in UserDefaults.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift (1)
362-379: Monitoring statuses from storage is a good addition.
Consider whether multiple failures should be communicated rather than just the first one, if that is ever a requirement.WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift (2)
794-794: Unused CurrentValueSubject.There's an unused CurrentValueSubject that doesn't appear to serve any purpose in this test.
- _ = CurrentValueSubject<[ProductImageUploaderKey], Never>([])
834-834: Consider verifying notice content.While the test verifies that
sendBackgroundUploadNoticeIfNeededis called, it doesn't verify the content of the notice. Consider capturing the notice and verifying its content to ensure it contains the expected message.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Networking/Networking/Model/Product/ProductImage.swift(2 hunks)WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift(7 hunks)WooCommerce/WooCommerceTests/Mocks/MockFeatureFlagService.swift(4 hunks)WooCommerce/WooCommerceTests/Mocks/MockProductImageUploader.swift(2 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift(18 hunks)woocommerce-ios(1 hunks)
🔇 Additional comments (28)
woocommerce-ios (1)
1-1: Subproject Commit Reference Update ApprovedThe commit reference "2244abb16a73bb0f307b44899c482a5115c89f2b" correctly points to the desired state of the subproject. This update is standard for managing subproject dependencies and ensures version consistency.
WooCommerce/WooCommerceTests/Mocks/MockProductImageUploader.swift (2)
18-18: Good addition for test validation.
This property is neatly introduced to help confirm that thesendBackgroundUploadNoticeIfNeededmethod was invoked.
68-73: Implementation aligns with intended test behavior.
Using the mock to enqueue a notice only ifactiveUploadsKeyscontains the given key is a clean way to simulate real upload behavior in tests.Networking/Networking/Model/Product/ProductImage.swift (2)
6-6: Separation of concerns for Equatable conformance is good.
RemovingEquatablefrom the struct declaration and placing it in an extension keeps the struct declaration lean.
58-71: Confirm the handling ofdateModified.
The extension currently compares onlydateCreated(ignoring milliseconds) but notdateModified. Verify if excludingdateModifiedfrom equality checks is intentional.WooCommerce/WooCommerceTests/Mocks/MockFeatureFlagService.swift (4)
26-26: Feature flag property looks correct.
DeclaringbackgroundProductImageUploadis a straightforward addition for toggling behavior.
48-49: Constructor parameters updated seamlessly.
The new boolean parameter maintains backward compatibility with a default offalse.
71-71: Assignment logic is straightforward.
Storing the constructor parameter into the property is correctly handled.
118-119: Adds explicit case for the new feature flag.
The switch statement properly returns the new property for.backgroundProductImageUpload.WooCommerce/Classes/ServiceLocator/ProductImageUploader.swift (13)
10-11: New imports are appropriate for the feature flag and storage.
No issues with adding these imports.
93-94: Optional storage reference is a sensible approach.
HavinguserDefaultsStatusesoptional simplifies logic when the flag is disabled.
95-119: Error publisher logic is correctly feature-flagged.
Splitting the error pipeline based on the feature flag is a clean solution.
122-137: Active uploads pipeline follows the same pattern as errors.
This maintains consistency between the two streams.
153-153: Feature flag service injection is clear.
Storing a reference tofeatureFlagServiceensures maintainability and clarity.
156-157: Combine cancellables property is necessary.
Tracking subscriptions withSet<AnyCancellable>is the standard approach.
165-170: Conditional initialization ofuserDefaultsStatusesis correct.
Observing user defaults statuses only if the feature flag is enabled avoids overhead otherwise.
228-239: Conditional path for background upload notice.
Both branches handle the notice consistently with the rest of the feature-flag logic.
313-330: Logic for scheduling local notifications is consistent.
The approach of splitting by feature flag usage is cohesive with the rest of the code.
385-389: Status updates push changes to user defaults.
This ensures the new approach remains the source of truth when the flag is on.
390-397: Falling back to local property updates is straightforward.
The code path is consistent with non–feature-flag scenarios.
411-420: Error reporting remains split by feature flag.
This method seamlessly updates user defaults or emits errors directly.
427-436: Method cleanly removes active upload references.
By removing the relevant statuses or active keys, the two approaches remain parallel.WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift (6)
6-6: Import added for Networking module.The new Networking import appears to be needed for the feature flag implementation. This is appropriate as it helps organize module dependencies.
14-16: Properties added for feature flag testing.These new properties will be used to support the feature flag testing. The naming is clear and follows Swift conventions, while maintaining appropriate access control levels.
18-24: Well-structured setUp method.The setUp method properly initializes the test environment by creating the mock feature flag service and ensuring UserDefaults is clean for each test. Following best practices by calling super.setUp() first.
26-31: Proper cleanup in tearDown method.Good practice to clean up resources in tearDown, including setting properties to nil and removing UserDefaults entries. Following best practices by calling super.tearDown() last.
33-33: Good use of MARK for test organization.Adding a MARK comment to distinguish between feature flag disabled and enabled tests improves code readability and organization.
735-881: Comprehensive tests for feature flag functionality.The new test cases thoroughly cover the functionality when the feature flag is enabled, testing:
- Change tracking
- Error publishing
- Active uploads tracking
- Background upload notices
- Reset behavior
- Local ID replacement
All tests follow the Given-When-Then pattern and have clear assertions. Good use of mocks to isolate the code under test.
woocommerce-ios
Outdated
There was a problem hiding this comment.
I think this is an accidental addition and should be removed.
There was a problem hiding this comment.
To what are you referring?
There was a problem hiding this comment.
Oh, I see a new folder named woocommerce-ios added here.
There was a problem hiding this comment.
Oh, good catch! I think it was generated by mistake while moving all this code in this PR, as it was initially part of a larger branch.
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/Mocks/MockProductImageUploader.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift
Outdated
Show resolved
Hide resolved
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageUploaderTests.swift
Outdated
Show resolved
Hide resolved
…eUploader`, considering all the errors and not just the first one.
|
Thank you for the review, @selanthiraiyan! 🙌 I will respond to your feedback step by step. I noticed an issue that was addressed in another class and in a previous PR, so I fixed it here as well. I ensured that all error statuses are processed and reported, not just the first one. 3a14b53 |
…tImageUploaderTests.swift Co-authored-by: Sharma Elanthiraiyan <[email protected]>
…uctImageUploader` and always instantiate `ProductImageStatusStorage` in `ProductImageUploader`
- Refactor `ProductImageUploaderTests.swift` to use actual `ProductImageUploader` instead of mock - Update test logic to check actual upload status and actions - Remove redundant test storage key and update UserDefaults handling in `ProductImageUploaderTests`
|
@selanthiraiyan thanks for all the suggestions! I updated the tests you mentioned since you were right, also removing the unnecessary code in |
…ked_through_storage_with_flag_enabled` using real `ProductImageUploader` instead of a mock.
…torage as a parameter, and avoid using UserDefaults.standard using mock instances. Additionally, address the failing unit test `test_activeUploads_are_tracked_through_storage_with_flag_enabled`, which has been inconsistent when run alongside other tests.
|
Version |
There was a problem hiding this comment.
I tested the app with the changes and found that uploading still works as expected. I saw an issue with the error handling though, as described in the comment below.
The PR description didn't mention testing the upload in the background. Are there any more PR coming to support that or can we test background upload in this PR already?
| } | ||
| } | ||
|
|
||
| func test_replaceLocalID_is_called_when_flag_enabled() { |
There was a problem hiding this comment.
I read through the implementation of replaceLocalID again and still don't see any connection between this method and the status storage. It works solely with actionHandlersByProduct, an in-memory dictionary that holds the action handlers by product upload key. Do you think this implementation still works when the app is in the background?
If you see a connection between replaceLocalID and the status storage, please show me where the code that indicates that, because I cannot see it.
| 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) | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nice catch Huong, I fixed it here 5b719e4 can you please take another look?
|
I'm also seeing that |
…s-part-4' into feat/save-product-image-upload-statuses-in-user-defaults-part-5
This is part of following PRs (one is still currently in draft), nothing to test in background here, but thanks for asking!
It seems that the test is unreliable: it passes when run alone but fails when executed alongside the other tests. I will investigate to determine what is causing the issue. 👍 |
| .filter { info in | ||
| let key = Key(siteID: info.siteID, | ||
| productOrVariationID: info.productOrVariationID, | ||
| isLocalID: info.productOrVariationID.id == 0) | ||
| return !self.statusUpdatesExcludedProductKeys.contains(key) | ||
| } |
There was a problem hiding this comment.
This filter is redundant because we already check for this in observeImageUploads.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| func test_replaceLocalID_is_called_when_flag_enabled() { |
There was a problem hiding this comment.
Since we have existing tests for replaceLocalID already, I think it's better to remove this test when it's unnecessary.
…s-part-4' into feat/save-product-image-upload-statuses-in-user-defaults-part-5
…ce it was a redundant test
|
Version |
…user-defaults-part-5
pmusolino
deleted the
feat/save-product-image-upload-statuses-in-user-defaults-part-5
branch
Part 5 of the changes needed for product image upload in the background.
Before merging this PR, make sure to merge this one.
Description
In this PR:
ProductImageStatusStorageintoProductImageUploader, which is the core of the uploader.ProductImageUploader, the product image statuses will be stored in User Defaults, and stored statuses will become the new source of truth.backgroundProductImageUpload, which currently is enabled only in debug.ProductImageto have a custom Equatable conformance because I noticed that storing dates while encoding/decoding them could lose some precision, so ignoring fractional seconds makes this comparison accurate.ProductImageUploader, also managing the feature flag.After this PR, you can expect to see work on real product image uploads in the background.
Testing information
I tested several scenarios with both the feature flag enabled and disabled. All of them should work as they did previously.
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: