Background image upload for products without attaching images to the products #15277
Conversation
…using background URL Session (still just working on WPCom stores)
…l-background-image-upload
…and application password requests
…l-background-image-upload
…l-background-image-upload
… image background upload
…l-background-image-upload
…nHandled` under feature flag
…l-background-image-upload
…l-background-image-upload # Conflicts: # WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift
…s-part-5' into feat/real-background-image-upload
- Remove all print statements used for debugging - Add DDLogError for error-related messages - Add DDLogDebug for debug-related messages
…eature flag is disabled
…hod in unit tests for `ProductImageActionHandler`
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 pull request adds background media upload functionality to the project. It introduces a new file for managing media uploads in the background and updates several components to integrate this functionality. Changes include new extensions and methods for converting media types, modifications to networking classes for handling credentials, and updates in app integration, testing, and documentation. Feature flags now determine whether to use background uploads, and the media store is enhanced with a background uploader dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ProductImageActionHandler
participant Store as MediaStore
participant Manager as MediaUploadSessionManager
participant Session as URLSession
participant Delegate as UploadDelegate
UI->>Store: uploadMediaInBackground(siteID, productID, mediaAsset, uploadID)
Store->>Manager: uploadMedia(request, mediaItem, uploadID, completion)
Manager->>Session: Start background upload task
Session-->>Manager: Upload progress/completion
Manager->>Delegate: Notify upload completion
Manager-->>Store: Return upload result (success/failure)
Store-->>UI: Return upload result
sequenceDiagram
participant Handler as ProductImageActionHandler
participant Flag as FeatureFlagService
participant Action as MediaAction
Handler->>Flag: isBackgroundProductImageUploadEnabled?
alt Enabled
Handler->>Action: uploadMediaInBackground(..., uploadID, onCompletion)
else Disabled
Handler->>Action: uploadMedia(..., onCompletion)
end
Suggested labels
🪧 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
|
Generated by 🚫 Danger |
|
|
|
@coderabbitai review |
✅ Actions performedReview 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: 1
🧹 Nitpick comments (13)
Yosemite/Yosemite/Actions/MediaAction.swift (1)
43-51: Implementation looks good for background upload capability.The addition of the
uploadMediaInBackgroundcase to theMediaActionenum provides a clear and well-structured entry point for background media uploads. It properly extends the existing upload functionality with the necessary parameters for background processing.I'd suggest one minor adjustment to maintain consistent indentation with other enum cases:
case uploadMediaInBackground(siteID: Int64, - productID: Int64, - mediaAsset: ExportableAsset, - altText: String?, - filename: String?, - uploadID: String, - onCompletion: (Result<Media, Error>) -> Void) + productID: Int64, + mediaAsset: ExportableAsset, + altText: String?, + filename: String?, + uploadID: String, + onCompletion: (Result<Media, Error>) -> Void)WooCommerce/Classes/AppDelegate.swift (1)
270-276: Well-implemented background session handling.The implementation of
application(_:handleEventsForBackgroundURLSession:completionHandler:)correctly handles background URL session events by checking the session identifier and delegating to the appropriate manager. This is essential for the background upload functionality to work properly when the app is relaunched after a background upload completes.Consider adding a brief comment explaining what happens if the identifier doesn't match:
func application(_ application: UIApplication, handleEventsForBackgroundURLSession identifier: String, completionHandler: @escaping () -> Void) { if identifier == ServiceLocator.backgroundMediaUploadSessionManager.backgroundSessionIdentifier { ServiceLocator.backgroundMediaUploadSessionManager.handleBackgroundSessionCompletion(completionHandler) } + // If the identifier doesn't match our background media upload session, + // we're not handling it since it must belong to another subsystem }WooCommerce/Classes/ServiceLocator/ServiceLocator.swift (2)
101-103: Consider adding documentation for the background media manager's purposeWhile the property has basic documentation, consider expanding it to explain its role in the background upload process.
/// Background image service /// -private static var _backgroundMediaUploadSessionManager = MediaUploadSessionManager() +/// Manages background media uploads that continue even when the app is suspended or terminated. +private static var _backgroundMediaUploadSessionManager = MediaUploadSessionManager()
275-279: Missing test setter for background media managerUnlike other services in this class, there's no test setter method for
_backgroundMediaUploadSessionManagerin the testability extension. This could make it harder to provide a mock for testing.Consider adding a test setter:
static func setBackgroundMediaUploadSessionManager(_ mock: MediaUploadSessionManager) { guard isRunningTests() else { return } _backgroundMediaUploadSessionManager = mock }Yosemite/YosemiteTests/Mocks/Networking/Remote/MockMediaRemote.swift (1)
106-117: Implementation of the uploadMediaRequest method looks goodThe method correctly simulates creating an upload request with appropriate multipart form data headers and records the invocation. Since this is a mock, error handling is sufficient.
One minor improvement could be to make the return value customizable, similar to how other mock methods work:
private var uploadMediaRequestResultsBySiteID = [Int64: URLRequest]() func whenUploadingMediaRequest(siteID: Int64, thenReturn request: URLRequest) { uploadMediaRequestResultsBySiteID[siteID] = request }Then you could return the predefined request instead of constructing one in the method, offering more flexibility in tests.
Networking/Networking/Remote/MediaRemote.swift (3)
173-233: Comprehensive implementation of background media upload request creation.The implementation constructs a proper multipart form data request for background uploading, with several positive aspects:
- Generates a unique boundary for multipart form data
- Properly authenticates the request when credentials are available
- Includes appropriate error handling
- Correctly builds the multipart form data manually
Consider potential improvements for robustness:
- Consider using an enumeration for HTTP methods instead of hardcoding "POST"
- Extract the multipart form data creation to a separate helper method for better maintainability
- request.httpMethod = "POST" + request.httpMethod = HTTPMethod.post.rawValue
204-207: Minor optimization opportunity in string data conversion.The helper function for appending data uses forced unwrapping which is generally safe for UTF-8 encoding of strings, but could be improved.
Consider using a more robust approach:
-func append(_ string: String) { - body.append(string.data(using: .utf8)!) -} +func append(_ string: String) { + if let data = string.data(using: .utf8) { + body.append(data) + } +}
209-213: Consider handling nil altText more explicitly.The code uses null coalescing to set an empty string when altText is nil, which is good, but it may be clearer to handle this more explicitly.
- ParameterKey.wordPressAltText: mediaItem.altText ?? "" + ParameterKey.wordPressAltText: mediaItem.altText.map { $0 } ?? ""WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift (2)
229-237: Avoid code duplication between background and standard uploads.Although the logic here is valid, there's a repeating pattern for handling
phAssetunder the background/foreground logic. Consider extracting a helper to reduce duplication and improve maintainability.- if featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) { - action = MediaAction.uploadMediaInBackground(..., onCompletion: onCompletion) - } else { - action = MediaAction.uploadMedia(..., onCompletion: onCompletion) - } + action = featureFlagService.isFeatureFlagEnabled(.backgroundProductImageUpload) + ? MediaAction.uploadMediaInBackground(..., onCompletion: onCompletion) + : MediaAction.uploadMedia(..., onCompletion: onCompletion)
246-254: Reduce repetition in .uiImage branch logic.Similar to the
.phAssetcase, the.uiImagebranch also has near-identical code for choosing between background and standard uploads. Extracting a shared function or using a conditional expression in a single line can simplify maintenance.Networking/Networking/Network/MediaUploadSessionManager.swift (1)
40-74: Temp file cleanup timing.The code removes the temp file on the main queue (
DispatchQueue.main.async), which might momentarily queue up behind various UI tasks. For efficiency, consider removing the file on a background queue unless there's a specific UI-related reason to do so.Yosemite/Yosemite/Stores/MediaStore.swift (2)
18-26: Constructor dependency injection improvements.
- Having multiple initializers for different requirements is flexible but can become unwieldy.
- Ensure that each initializer delegates to a single designated initializer to avoid code drift between them.
224-261: Background upload integration appears sound.
- The
Task { @MainActor in ... }block for export is appropriate since it likely interacts with UI-bound code or needs main-actor constraints.- Cleanup of local media on completion is well-handled.
- Consider adding cancellation support in the future, allowing users to halt a background upload if needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Networking/Networking.xcodeproj/project.pbxproj(4 hunks)Networking/Networking/Model/Media/WordPressMedia.swift(1 hunks)Networking/Networking/Network/AlamofireNetwork.swift(1 hunks)Networking/Networking/Network/MediaUploadSessionManager.swift(1 hunks)Networking/Networking/Remote/MediaRemote.swift(3 hunks)WooCommerce/Classes/AppDelegate.swift(2 hunks)WooCommerce/Classes/ServiceLocator/ServiceLocator.swift(3 hunks)WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift(4 hunks)WooCommerce/Classes/Yosemite/AuthenticatedState.swift(1 hunks)WooCommerce/WooCommerceTests/Mocks/MockMediaStoresManager.swift(1 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Edit Product/ProductFormViewModel+UpdatesTests.swift(1 hunks)WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageActionHandlerTests.swift(11 hunks)Yosemite/Yosemite/Actions/MediaAction.swift(1 hunks)Yosemite/Yosemite/Stores/MediaStore.swift(3 hunks)Yosemite/YosemiteTests/Mocks/Networking/Remote/MockMediaRemote.swift(2 hunks)Yosemite/YosemiteTests/Stores/MediaStoreTests.swift(9 hunks)docs/NETWORKING.md(1 hunks)
🔇 Additional comments (35)
WooCommerce/WooCommerceTests/Mocks/MockMediaStoresManager.swift (1)
59-64: The new case correctly handles background media upload mocking.This implementation properly follows the established pattern in the class for handling media actions. It correctly checks for the presence of media and returns either a success or failure result accordingly.
Since this is a mock for testing, are there any specific error scenarios for background uploads that might need separate test cases? The current implementation only handles the basic nil media case, but background uploads might have unique failure modes (like app termination during upload or connectivity issues) that could be worth simulating in more advanced test scenarios.
Networking/Networking.xcodeproj/project.pbxproj (4)
509-509: LGTM: New file reference correctly added to the build phase.The
MediaUploadSessionManager.swiftfile is properly referenced in the build phase, which is a necessary step for including it in the compilation process.
1729-1729: LGTM: File reference correctly configured.The file reference for
MediaUploadSessionManager.swiftis properly configured with the correct type (sourcecode.swift), which ensures Xcode will treat it as a Swift source file.
2820-2820: LGTM: Appropriate placement in project structure.The
MediaUploadSessionManager.swiftfile is correctly placed in the Network group alongside other networking-related files likeAlamofireNetwork.swift,MockNetwork.swift, andNullNetwork.swift. This organization makes sense given its purpose of managing background media upload sessions.
5502-5502: Don't forget to include unit tests in the follow-up PR.The
MediaUploadSessionManager.swiftfile is properly included in the source compilation. As mentioned in the PR description, unit tests for this new functionality will be added in a follow-up PR. Please ensure these tests cover all critical paths of the background upload functionality, including:
- Session initialization
- Background upload handling
- Error cases and recovery
- Interaction with other components (ServiceLocator, MediaStore)
Networking/Networking/Model/Media/WordPressMedia.swift (2)
129-143: Extension provides a clean conversion method.The
toMedia()method is well-implemented and handles data mapping between related types efficiently. The code properly deals with optional values and provides sensible fallbacks for the filename.
145-150: File extension handling is clean and robust.The
fileExtensionproperty safely extracts the extension from a filename and handles the null case appropriately. UsingURL(fileURLWithPath:).pathExtensionis an elegant approach to extract the file extension.docs/NETWORKING.md (1)
40-48: Clear documentation for the new background upload functionality.The added documentation clearly explains the purpose and features of the
MediaUploadSessionManager. This will be valuable for developers working with this component. The documentation is concise and follows the established style of the existing NETWORKING.md file.WooCommerce/Classes/AppDelegate.swift (2)
5-5: Import is correctly added for the new functionality.The import of
MediaUploadSessionManageris properly placed at the top of the file with other imports.
270-276:❓ Verification inconclusive
Verify integration with other background URL sessions.
The current implementation only handles sessions for the
backgroundMediaUploadSessionManager. If the app uses other background URL sessions, you should ensure the method properly handles those as well or allows for extensibility.
🏁 Script executed:
#!/bin/bash # Search for other URLSession background configurations in the codebase echo "Searching for background URLSession configurations..." rg -i "\.background|urlsessionconfiguration.*background" --type swiftLength of output: 66136
Action Required: Confirm Background Session Handling
The implementation in
WooCommerce/Classes/AppDelegate.swift(lines 270–276) only handles the background session forbackgroundMediaUploadSessionManager. However, our code search reveals that other modules—including the networking layer and WordPressAuthenticator components—also configure background URL sessions (for example, inNetworking/Networking/Network/MediaUploadSessionManager.swiftand various files underWooCommerce/WordPressAuthenticator/WordPressKit/).
- Review Required: Verify whether these additional background sessions are managed via separate mechanisms. If they need to be routed through the AppDelegate, consider extending this handler (or implementing a more generic dispatcher) to cover all background sessions.
- Documentation Note: If multiple background session handlers are intentional, please add clarifying comments to ensure future maintainers understand the design.
Yosemite/YosemiteTests/Stores/MediaStoreTests.swift (3)
53-54: Consistent integration of background uploader parameterThe
backgroundUploaderparameter has been correctly added to theMediaStoreinitializer to support the new background upload functionality.
80-81: Consistent application of new dependency across test casesThe
backgroundUploaderparameter is consistently applied to ensure test cases continue to work with the updatedMediaStoreAPI.
233-234: Thorough update of all MediaStore instancesAll initializations of the
MediaStoreclass have been properly updated with the newbackgroundUploaderparameter, ensuring test coverage for the new functionality.Also applies to: 261-262, 291-292, 322-323, 654-655, 751-752, 757-758
WooCommerce/Classes/ServiceLocator/ServiceLocator.swift (1)
7-7: LGTM: Clear import declaration for the new class dependencyThe import statement correctly imports just the specific class needed rather than the entire module.
WooCommerce/WooCommerceTests/ViewRelated/Products/Edit Product/ProductFormViewModel+UpdatesTests.swift (1)
361-367: Well-designed feature flag integration in the convenience initializerThe initializer properly handles both cases (with and without feature flag), ensuring backward compatibility while allowing for new functionality.
Yosemite/YosemiteTests/Mocks/Networking/Remote/MockMediaRemote.swift (1)
52-52: Good addition of a new invocation case for uploadMediaRequestThis new enum case allows for tracking calls to the new method, which is essential for test verification.
Networking/Networking/Network/AlamofireNetwork.swift (2)
30-30: Credentials property exposed in a controlled way, good implementation.Adding the public credentials property allows the network layer to share authentication information with other components that need it for background processing.
36-36: Properly initialized credentials field.The credentials property is correctly initialized from the constructor parameter, maintaining the object's integrity.
WooCommerce/Classes/Yosemite/AuthenticatedState.swift (1)
49-52: Dependency injection of backgroundUploader properly implemented.The MediaStore initialization has been updated to include the backgroundUploader dependency, which enables the background image upload functionality. This follows good dependency injection practices and maintains separation of concerns.
Networking/Networking/Remote/MediaRemote.swift (4)
2-2: Import for Alamofire correctly added.The Alamofire import is necessary as the updated code relies on Alamofire types.
24-28: Well-defined protocol extension for background upload support.The function signature properly includes async/throws for error handling and returns a URLRequest that can be used for background uploads.
186-188: Proper type checking and error handling.The code correctly verifies that the network is an AlamofireNetwork before proceeding, which prevents runtime errors.
194-198: Good authentication and error handling.The implementation properly authenticates the request when credentials are available and throws an appropriate error when they're not.
WooCommerce/WooCommerceTests/ViewRelated/Products/Media/ProductImageActionHandlerTests.swift (5)
14-15: Necessary test dependencies added.The addition of mockFeatureFlagService and storage as private properties allows for proper testing of the new feature flag functionality.
17-22: Well-structured test setup method.The setup method properly initializes the test dependencies, including removing any existing test data to ensure a clean state.
24-29: Thorough test teardown to prevent test pollution.The teardown method correctly cleans up all test resources, ensuring that tests don't interfere with each other.
50-52: Test updated to accommodate new dependency requirement.The ProductImageActionHandler instantiation now includes the mockFeatureFlagService parameter to match the updated constructor signature.
108-109: Consistent constructor parameter updates across all test cases.All instances of ProductImageActionHandler instantiation have been updated to include the new featureFlag parameter, maintaining test consistency.
Also applies to: 151-152, 179-180, 208-209, 250-251, 301-302, 339-340, 385-386, 435-436
WooCommerce/Classes/ViewRelated/Products/Media/ProductImageActionHandler.swift (3)
4-4: Good addition for feature-flag-driven logic.Importing the
FeatureFlagServiceprotocol at this location is clear and satisfies the need to decouple feature-flag logic from the handler.
49-50: Great approach to dependency injection.Using a dedicated property for
featureFlagServicehelps keep feature-flag checks explicit, and makes it easier to mock during tests.
79-80: Constructor design is consistent and test-friendly.
- Providing default parameters from
ServiceLocatoris perfectly valid, and it avoids propagation of global singletons beyond this type.- Having an explicit parameter for
featureFlagServicealso ensures that tests can inject a mock or stub.Also applies to: 85-85
Networking/Networking/Network/MediaUploadSessionManager.swift (2)
3-7: Interface is clearly defined.The use of a protocol for delegate callbacks is commendable. This design cleanly separates upload logic from the consumer and makes testing more straightforward.
81-163: Robust error handling and success callbacks.
- Detailed logging of success/failure states is helpful for troubleshooting.
- The fallback for non-2xx status codes is correct, returning a failure.
- Consider clarifying which thread the
didReceive datamethod runs on. If it isn't always the main thread, ensure dictionary writes remain thread-safe.Yosemite/Yosemite/Stores/MediaStore.swift (2)
9-9: Injecting a dedicated background uploader is a welcome extension.This approach avoids monolithic solutions and maintains the single-responsibility principle by delegating background upload tasks to a specialized component.
14-15: Convenience initializer keeps initialization consistent.It’s good to see that you’re using a convenience initializer to cleanly inject the necessary dependencies in one place.
| private var completionHandlers: [String: (Result<Media, Error>) -> Void] = [:] | ||
| private var taskResponseData: [Int: Data] = [:] | ||
| private var backgroundCompletionHandler: (() -> Void)? | ||
| weak var delegate: MediaUploadSessionManagerDelegate? | ||
|
|
There was a problem hiding this comment.
Potential concurrent access to dictionaries.
completionHandlers and taskResponseData may be accessed from different threads (e.g., during didReceive data callbacks vs. the main thread). Swift dictionaries are not inherently thread-safe. Consider using a dedicated serial queue or locks to avoid data races.
-class MediaUploadSessionManager: NSObject {
- private var completionHandlers: [String: (Result<Media, Error>) -> Void] = [:]
- private var taskResponseData: [Int: Data] = [:]
+class MediaUploadSessionManager: NSObject {
+ private let syncQueue = DispatchQueue(label: "com.woocommerce.MediaUploadSessionManager.sync")
+ private var completionHandlers: [String: (Result<Media, Error>) -> Void] = [:]
+ private var taskResponseData: [Int: Data] = [:]
+
+ private func setCompletionHandler(_ handler: @escaping (Result<Media, Error>) -> Void, for uploadID: String) {
+ syncQueue.async {
+ self.completionHandlers[uploadID] = handler
+ }
+ }
+
+ // ... similarly wrap dictionary read/writes for 'taskResponseData'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private var completionHandlers: [String: (Result<Media, Error>) -> Void] = [:] | |
| private var taskResponseData: [Int: Data] = [:] | |
| private var backgroundCompletionHandler: (() -> Void)? | |
| weak var delegate: MediaUploadSessionManagerDelegate? | |
| class MediaUploadSessionManager: NSObject { | |
| private let syncQueue = DispatchQueue(label: "com.woocommerce.MediaUploadSessionManager.sync") | |
| private var completionHandlers: [String: (Result<Media, Error>) -> Void] = [:] | |
| private var taskResponseData: [Int: Data] = [:] | |
| private var backgroundCompletionHandler: (() -> Void)? | |
| weak var delegate: MediaUploadSessionManagerDelegate? | |
| private func setCompletionHandler(_ handler: @escaping (Result<Media, Error>) -> Void, for uploadID: String) { | |
| syncQueue.async { | |
| self.completionHandlers[uploadID] = handler | |
| } | |
| } | |
| // TODO: Similarly wrap dictionary read/writes for 'taskResponseData' | |
| } |
|
Version |
pmusolino
changed the title
…s-part-5' into feat/real-background-image-upload
There was a problem hiding this comment.
Thanks for working on this, Paolo! Background testing worked as described. 👏
I left a few questions about the code to get a better understanding.
I noticed the following issue while testing background image uploads.
After testing the background upload case when I open the app I see the loading indicator over the product image in the product list screen.
ScreenRecording_03-11-2025.16-48-21_1.MP4
| @@ -46,7 +46,10 @@ class AuthenticatedState: StoresManagerState { | |||
| InboxNotesStore(dispatcher: dispatcher, storageManager: storageManager, network: network), | |||
| JetpackSettingsStore(dispatcher: dispatcher, storageManager: storageManager, network: network), | |||
| JustInTimeMessageStore(dispatcher: dispatcher, storageManager: storageManager, network: network), | |||
| MediaStore(dispatcher: dispatcher, storageManager: storageManager, network: network), | |||
| MediaStore(dispatcher: dispatcher, | |||
There was a problem hiding this comment.
Can we inject the credentials into MediaStore here instead of making it a public property in AlamofireNetwork? We can pass the credentials to Remote later to create the request.
| availableAsRESTRequest: true) | ||
|
|
||
| guard let network = network as? AlamofireNetwork else { | ||
| throw NetworkError.unacceptableStatusCode(statusCode: 500, response: nil) |
There was a problem hiding this comment.
Could you kindly explain why we have to throw 500 error code?
There was a problem hiding this comment.
Good question. I identified it as a generic error, but the HTTP error 500 is more for server-side errors, while for a client error, it's probably best to use a generic error code like 400 (Bad Request). Wdyt?
There was a problem hiding this comment.
while for a client error, it's probably best to use a generic error code like 400 (Bad Request).
Thanks! This sounds good to me.
| var modifiedRequest = request | ||
| modifiedRequest.httpBody = nil | ||
|
|
||
| let task = backgroundSession.uploadTask(with: modifiedRequest, fromFile: tempFileURL) |
There was a problem hiding this comment.
I see that we have already appended the data of the media item to the request body in this line. Do we still have to create a temporary file and pass it to this uploadTask method?
Is this the reason for using a temp file? If yes, can you add a comment to explain?
There was a problem hiding this comment.
That's exactly the reason. I will add a comment to clarify that for the future 👍
| // Cleanup temp file | ||
| DispatchQueue.main.async { | ||
| try? FileManager.default.removeItem(at: tempFileURL) | ||
| } |
There was a problem hiding this comment.
I don't understand why we removed the temporary file here. Shouldn't we wait for the upload task to finish/fail?
There was a problem hiding this comment.
So, as you understand from reading that article, it is necessary to have the file on disk. However, when you create and start an upload task using a file URL, URLSession immediately begins accessing the file to prepare for the upload. The system reads the file data synchronously or copies the file data into its own buffer to manage the upload independently of the app's process. At that moment, it is no longer needed.
Since the upload task has already been started and the system has a reference to the file, deleting it from the app's sandboxed file system doesn't interrupt the upload.
There's a slight chance that if the file is deleted before URLSession has begun reading it, the upload could fail. That's why I use DispatchQueue.main.async, which helps mitigate this risk by ensuring the deletion happens after the current code execution block.
Thinking aloud about it, for maximum safety, I tried to defer deleting the temp file until we receive confirmation that the upload task has successfully started or even until the upload completes. However, this might not be practical because:
- Waiting until the upload completes could mean the temp file remains on disk for an extended period, especially if the upload takes a long time.
- If the app is terminated (not background suspended) before we get a chance to delete the file (e.g., user force-quits the app), the temp file might remain on disk indefinitely.
So I think the current solution offers a good balance between cleanup and reliability.
There was a problem hiding this comment.
As it is a temporary directory, we shouldn't have to worry about file remaining in disk for extended period.
We can delete the file once the upload completes and let the system deal with cleaning it up in case the app is force-quit.
What do you think?
There was a problem hiding this comment.
If we delete it right away, we somehow prevent the system from cleaning up the directory at its discretion if the device has little available disk space. Also, I believe it will be easy to test in the future, right?
| } | ||
|
|
||
| // Use MediaMapper to parse response | ||
| if let jsonString = String(data: data, encoding: .utf8) { |
There was a problem hiding this comment.
I see that jsonString is unused. I believe we will use this JSON response in a future PR.
| private var backgroundCompletionHandler: (() -> Void)? | ||
| weak var delegate: MediaUploadSessionManagerDelegate? | ||
|
|
||
| public init(sessionIdentifier: String = "com.automattic.woocommerce.background.upload") { |
There was a problem hiding this comment.
Should we use bundle based identifiers as suggested here?
…s-part-5' into feat/real-background-image-upload
|
Thanks @selanthiraiyan for the review!
Yes, this is completely normal in this PR. When the feature flag is disabled, this issue will not occur. The problem arises because the upload statuses are not yet managed in the current PR. You begin uploading the images while the app is running, and the statuses are properly managed in UserDefaults as |
…s-part-5' into feat/real-background-image-upload
|
Thanks for the explanations, Paolo! I believe you are working on addressing this comment. Kindly let me know by tapping the "Request Re-review" button once this PR is ready for another round of review. 
|
|
Version |
…l-background-image-upload # Conflicts: # WooCommerce/WooCommerceTests/Mocks/MockFeatureFlagService.swift
|
@selanthiraiyan this is ready for another review. Changes applied here: 0c7be43 I also disabled the feature flag |
There was a problem hiding this comment.
Thanks, Paolo! Could you kindly add your thoughts about my previous comment about injecting credentials in MediaStore?
Part 6 of the changes needed for product image upload in the background.
Before merging this PR, make sure to merge this one.
Description
This pull request introduces the capability for background image uploads for products using a new background URL session. Please let me know if this PR is too big to be reviewed and if you prefer I split it into two PRs.
- Feature Introduction:
- Availability:
- Functionality:
- Testing:
MediaUploadSessionManagerwill be added after implementation is finalized.ProductImageActionHandlerTestsonly address conditions when the feature flag is disabled.Testing information
There are several scenarios with both the feature flag
backgroundProductImageUploadenabled and disabled that should be tested. All of them should work as they did previously for authenticated user with WP.com but also with application password.Testing information in background
Now that everything works like before, let's test the background upload:
Here you can find a detailed guide on how to send the app in background and understand the behavior.
The main points are:
AppDelegate, under the methodapplicationDidEnterBackground(_ application: UIApplication), addexit(0). This will terminate the app and send it into a background state (suspended) without forcing a quit.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: