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

Background image upload for products without attaching images to the products #15277

Open
wants to merge 31 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
682e932
feat: initial implementation of background image upload for products …
pmusolino Feb 12, 2025
60f1f77
Merge commit '4fadf3f4d301424fcb5168e4fc85fac20aed31b4' into feat/rea…
pmusolino Feb 12, 2025
3bdc3ea
fix: indentation
pmusolino Feb 12, 2025
828f80b
feat: handle background upload images in `AppDelegate`
pmusolino Feb 12, 2025
f6fdbd9
refactor: `uploadMediaRequest` in `MediaRemote` to handle both WPCom …
pmusolino Feb 12, 2025
aed77cd
Merge commit '1bdbcb35ece301bab3f12eab89a3e6d531229cdc' into feat/rea…
pmusolino Feb 13, 2025
08bb136
Merge commit '29d1a4919c44b412aef9fac8f07cf87bafa269d7' into feat/rea…
pmusolino Feb 13, 2025
ca08551
feat: new configuration to allow cellular access in background image …
pmusolino Feb 13, 2025
0534deb
fix: remove private method, since the extension it's already private
pmusolino Feb 13, 2025
c611e5d
refactor: MediaUploadSessionManager and all related classes to manage…
pmusolino Feb 13, 2025
3736482
Merge commit 'a477857633cf5ddd3bef136f977d17786e622284' into feat/rea…
pmusolino Feb 14, 2025
677c91b
feat: new `backgroundProductImageUpload` feature flag
pmusolino Feb 16, 2025
6ab6900
update: hidden `uploadMediaInBackground` method in `ProductImageActio…
pmusolino Feb 16, 2025
8e5e3f8
Merge commit 'e4a32754714512358d4235997147d7984fe3a24e' into feat/rea…
pmusolino Feb 17, 2025
1a13232
fix: unit tests for background image upload
pmusolino Feb 17, 2025
a8017d4
Merge commit 'bfafb75aacb1354f698c6734fc292677eb2aeef2' into feat/rea…
pmusolino Feb 18, 2025
3103925
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Mar 4, 2025
0c2559d
Refactor logging in MediaUploadSessionManager
pmusolino Mar 4, 2025
909f9bb
fix: fallback on old upload media if `backgroundProductImageUpload` f…
pmusolino Mar 4, 2025
eca879b
feat: support optional feature flag parameter in convenience init met…
pmusolino Mar 4, 2025
82c7d6b
update: `ProductImageActionHandler` failing unit tests
pmusolino Mar 5, 2025
24ed03c
fix: lint violations
pmusolino Mar 5, 2025
237e7e9
fix: indentation
pmusolino Mar 7, 2025
beb20fe
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Mar 7, 2025
d31b7c8
update: moved `MediaUploadSessionManager` under a different path
pmusolino Mar 7, 2025
5dd0d54
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Mar 11, 2025
47e2505
Merge branch 'feat/save-product-image-upload-statuses-in-user-default…
pmusolino Mar 17, 2025
06e7962
Merge commit '5aa86247e3291fcb3468939789db619c45d83b42' into feat/rea…
pmusolino Mar 28, 2025
0c7be43
Implement bundle-based identifiers for default sessionIdentifier usin…
pmusolino Mar 28, 2025
3fcb776
Always disable the feature flag `backgroundProductImageUpload`, also …
pmusolino Mar 28, 2025
f0c71cb
fix: lint issue
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
4 changes: 4 additions & 0 deletions Networking/Networking.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@
45B204B82489095100FE6526 /* ProductCategoryMapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45B204B72489095100FE6526 /* ProductCategoryMapper.swift */; };
45B204BA24890A8C00FE6526 /* ProductCategoryMapperTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45B204B924890A8C00FE6526 /* ProductCategoryMapperTests.swift */; };
45B204BC24890B1200FE6526 /* category.json in Resources */ = {isa = PBXBuildFile; fileRef = 45B204BB24890B1200FE6526 /* category.json */; };
45B383252D5CC00700F8CB1A /* MediaUploadSessionManager.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45B383242D5CC00700F8CB1A /* MediaUploadSessionManager.swift */; };
45B79AC62C9355F800DCCB2C /* meta-data-products-and-orders_nested_in_data.json in Resources */ = {isa = PBXBuildFile; fileRef = 45B79AC52C9355F800DCCB2C /* meta-data-products-and-orders_nested_in_data.json */; };
45C6D0E429B9F327009CE29C /* CookieNonceAuthenticatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45C6D0E329B9F327009CE29C /* CookieNonceAuthenticatorTests.swift */; };
45CCFCE227A2C9BF0012E8CB /* InboxNote.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45CCFCE127A2C9BF0012E8CB /* InboxNote.swift */; };
Expand Down Expand Up @@ -1725,6 +1726,7 @@
45B204B72489095100FE6526 /* ProductCategoryMapper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ProductCategoryMapper.swift; sourceTree = "<group>"; };
45B204B924890A8C00FE6526 /* ProductCategoryMapperTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ProductCategoryMapperTests.swift; sourceTree = "<group>"; };
45B204BB24890B1200FE6526 /* category.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = category.json; sourceTree = "<group>"; };
45B383242D5CC00700F8CB1A /* MediaUploadSessionManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MediaUploadSessionManager.swift; sourceTree = "<group>"; };
45B79AC52C9355F800DCCB2C /* meta-data-products-and-orders_nested_in_data.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = "meta-data-products-and-orders_nested_in_data.json"; sourceTree = "<group>"; };
45C6D0E329B9F327009CE29C /* CookieNonceAuthenticatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CookieNonceAuthenticatorTests.swift; sourceTree = "<group>"; };
45CCFCE127A2C9BF0012E8CB /* InboxNote.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InboxNote.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2815,6 +2817,7 @@
B518662320A099BF00037A38 /* AlamofireNetwork.swift */,
B518662620A09BCC00037A38 /* MockNetwork.swift */,
D87F6150226591E10031A13B /* NullNetwork.swift */,
45B383242D5CC00700F8CB1A /* MediaUploadSessionManager.swift */,
);
path = Network;
sourceTree = "<group>";
Expand Down Expand Up @@ -5496,6 +5499,7 @@
DE50295D28C6068B00551736 /* JetpackUserMapper.swift in Sources */,
B524194121AC60A700D6FC0A /* DotcomDevice.swift in Sources */,
D8EDFE2225EE88C9003D2213 /* ReaderConnectionToken.swift in Sources */,
45B383252D5CC00700F8CB1A /* MediaUploadSessionManager.swift in Sources */,
4599FC5824A624BD0056157A /* ProductTagListMapper.swift in Sources */,
45A4B86225D3086600776FB4 /* ShippingLabelAddressValidationError.swift in Sources */,
D823D90D2237784A00C90817 /* ShipmentTrackingProvider.swift in Sources */,
Expand Down
24 changes: 24 additions & 0 deletions Networking/Networking/Model/Media/WordPressMedia.swift
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,27 @@ private extension WordPressMedia {
case title
}
}

extension WordPressMedia {
/// Converts a `WordPressMedia` to `Media`.
public func toMedia() -> Media {
.init(mediaID: mediaID,
date: date,
fileExtension: fileExtension,
filename: details?.fileName ?? title?.rendered ?? slug,
mimeType: mimeType,
src: src,
thumbnailURL: details?.sizes?["thumbnail"]?.src,
name: slug,
alt: alt,
height: details?.height,
width: details?.width)
}

private var fileExtension: String {
guard let fileName = details?.fileName else {
return ""
}
return URL(fileURLWithPath: fileName).pathExtension
}
}
3 changes: 3 additions & 0 deletions Networking/Networking/Network/AlamofireNetwork.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@ public class AlamofireNetwork: Network {

public var session: URLSession { Session.default.session }

public let credentials: Credentials?

/// Public Initializer
///
///
public required init(credentials: Credentials?, sessionManager: Alamofire.Session? = nil) {
self.credentials = credentials
self.requestConverter = RequestConverter(credentials: credentials)
self.requestAuthenticator = RequestProcessor(requestAuthenticator: DefaultRequestAuthenticator(credentials: credentials))
if let sessionManager {
Expand Down
164 changes: 164 additions & 0 deletions Networking/Networking/Network/MediaUploadSessionManager.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
import Foundation

public protocol MediaUploadSessionManagerDelegate: AnyObject {
func mediaUploadSessionManager(_ manager: MediaUploadSessionManager,
didCompleteUpload uploadID: String,
withResult result: Result<Media, Error>)
}

/// Background upload specific errors
enum BackgroundUploadError: Error {
case invalidRequestBody
case invalidResponse
case decodingError
}

/// Session Manager for media upload in background
///
public final class MediaUploadSessionManager: NSObject {

public let backgroundSessionIdentifier: String
private lazy var backgroundSession: URLSession = {
let config = URLSessionConfiguration.background(withIdentifier: backgroundSessionIdentifier)
config.sharedContainerIdentifier = "group.com.automattic.woocommerce"
config.sessionSendsLaunchEvents = true
config.isDiscretionary = false
config.allowsCellularAccess = true
return URLSession(configuration: config, delegate: self, delegateQueue: nil)
}()

private var completionHandlers: [String: (Result<Media, Error>) -> Void] = [:]
private var taskResponseData: [Int: Data] = [:]
private var backgroundCompletionHandler: (() -> Void)?
weak var delegate: MediaUploadSessionManagerDelegate?

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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'
}

public init(sessionIdentifier: String = "com.automattic.woocommerce.background.upload") {
self.backgroundSessionIdentifier = sessionIdentifier
super.init()
}

public func uploadMedia(request: URLRequest,
mediaItem: UploadableMedia,
uploadID: String,
completion: @escaping (Result<Media, Error>) -> Void) {
completionHandlers[uploadID] = completion

guard let httpBody = request.httpBody else {
let error = BackgroundUploadError.invalidRequestBody
completion(.failure(error))
return
}

do {
// Create temp file with proper extension from mediaItem
let tempDirectory = FileManager.default.temporaryDirectory
let tempFileURL = tempDirectory.appendingPathComponent(mediaItem.filename)
try httpBody.write(to: tempFileURL)

// Upload using temp file
var modifiedRequest = request
modifiedRequest.httpBody = nil

let task = backgroundSession.uploadTask(with: modifiedRequest, fromFile: tempFileURL)
task.taskDescription = uploadID
task.resume()

// Cleanup temp file
DispatchQueue.main.async {
try? FileManager.default.removeItem(at: tempFileURL)
}
} catch {
DDLogError("⛔️ MediaUploadSessionManager- Failed image upload while creating temp file: \(error)")
completion(.failure(error))
}
}

public func handleBackgroundSessionCompletion(_ completionHandler: @escaping () -> Void) {
backgroundCompletionHandler = completionHandler
}
}

extension MediaUploadSessionManager: URLSessionDataDelegate {
public func urlSession(_ session: URLSession,
dataTask: URLSessionDataTask,
didReceive data: Data) {
if let existingData = taskResponseData[dataTask.taskIdentifier] {
taskResponseData[dataTask.taskIdentifier] = existingData + data
} else {
taskResponseData[dataTask.taskIdentifier] = data
}
}

public func urlSession(_ session: URLSession,
task: URLSessionTask,
didCompleteWithError error: Error?) {
guard let uploadID = task.taskDescription else {
DDLogDebug("MediaUploadSessionManager- task completed without an upload identifier. Task identifier: \(task.taskIdentifier)")
return
}
defer {
taskResponseData.removeValue(forKey: task.taskIdentifier)
}

if let error = error {
DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): encountered error: \(error.localizedDescription)")
notifyCompletion(.failure(error), for: uploadID)
return
}

guard let httpResponse = task.response as? HTTPURLResponse else {
DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): " +
"response is not a valid HTTPURLResponse. Actual response: " +
"\(String(describing: task.response))")
notifyCompletion(.failure(BackgroundUploadError.invalidResponse), for: uploadID)
return
}

guard let data = taskResponseData[task.taskIdentifier] else {
DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): " +
"missing response data for task with identifier \(task.taskIdentifier)")
notifyCompletion(.failure(BackgroundUploadError.invalidResponse), for: uploadID)
return
}

if !(200...299).contains(httpResponse.statusCode) {
DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): " +
"unexpected HTTP status code \(httpResponse.statusCode). " +
"Full response: \(httpResponse) Headers: \(httpResponse.allHeaderFields)")
notifyCompletion(.failure(BackgroundUploadError.invalidResponse), for: uploadID)
return
}

// Use MediaMapper to parse response
if let jsonString = String(data: data, encoding: .utf8) {
} else {
DDLogError("⛔️ MediaUploadSessionManager- Failed to convert response data to JSON string for task (\(uploadID))")
}
let mapper = WordPressMediaMapper()
do {
let media = try mapper.map(response: data)
notifyCompletion(.success(media.toMedia()), for: uploadID)
} catch {
DDLogError("⛔️ MediaUploadSessionManager- Upload failure for task (\(uploadID)): error mapping media: \(error.localizedDescription)")
notifyCompletion(.failure(error), for: uploadID)
}
}

public func urlSessionDidFinishEvents(forBackgroundURLSession session: URLSession) {
DispatchQueue.main.async { [weak self] in
DDLogDebug("MediaUploadSessionManager- Background URL session did finish events. Invoking completion handler.")
self?.backgroundCompletionHandler?()
self?.backgroundCompletionHandler = nil
}
}

private func notifyCompletion(_ result: Result<Media, Error>, for uploadID: String) {
DispatchQueue.main.async { [weak self] in
DDLogError("⛔️ MediaUploadSessionManager- Notifying completion for task (\(uploadID)) with result: \(result)")
guard let self = self else { return }
self.completionHandlers[uploadID]?(result)
self.completionHandlers.removeValue(forKey: uploadID)
self.delegate?.mediaUploadSessionManager(self, didCompleteUpload: uploadID, withResult: result)
}
}
}
68 changes: 68 additions & 0 deletions Networking/Networking/Remote/MediaRemote.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Foundation
import Alamofire

/// Protocol for `MediaRemote` mainly used for mocking.
public protocol MediaRemoteProtocol {
Expand All @@ -19,6 +20,11 @@ public protocol MediaRemoteProtocol {
productID: Int64,
mediaID: Int64,
completion: @escaping (Result<WordPressMedia, Error>) -> Void)

/// Creates a URLRequest for uploading media in background
func uploadMediaRequest(siteID: Int64,
productID: Int64,
mediaItem: UploadableMedia) async throws -> URLRequest
}

/// Media: Remote Endpoints
Expand Down Expand Up @@ -163,6 +169,68 @@ public class MediaRemote: Remote, MediaRemoteProtocol {
completion(.failure(error))
}
}

public func uploadMediaRequest(siteID: Int64,
productID: Int64,
mediaItem: UploadableMedia) async throws -> URLRequest {

let boundary = UUID().uuidString
let path = "sites/\(siteID)/media"

let dotcomRequest = try DotcomRequest(wordpressApiVersion: .wpMark2,
method: .post,
path: path,
parameters: nil,
availableAsRESTRequest: true)

guard let network = network as? AlamofireNetwork else {
throw NetworkError.unacceptableStatusCode(statusCode: 500, response: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you kindly explain why we have to throw 500 error code?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

let converter = RequestConverter(credentials: network.credentials)
var request = try converter.convert(dotcomRequest).asURLRequest()

// Authenticate the request if we have credentials
if let credentials = network.credentials {
request = try DefaultRequestAuthenticator(credentials: credentials).authenticate(request)
} else {
throw NetworkError.unacceptableStatusCode(statusCode: 401, response: nil)
}

// Add multipart content type
request.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type")

// Add form data
var body = Data()
func append(_ string: String) {
body.append(string.data(using: .utf8)!)
}

let params: [String: String] = [
ParameterKey.wordPressMediaPostID: "\(productID)",
ParameterKey.fieldsWordPressSite: ParameterValue.wordPressMediaFields,
ParameterKey.wordPressAltText: mediaItem.altText ?? ""
]

//Add parameters
for (key, value) in params {
append("--\(boundary)\r\n")
append("Content-Disposition: form-data; name=\"\(key)\"\r\n\r\n")
append("\(value)\r\n")
}

// Add file data
append("--\(boundary)\r\n")
append("Content-Disposition: form-data; name=\"file\"; filename=\"\(mediaItem.filename)\"\r\n")
append("Content-Type: \(mediaItem.mimeType)\r\n\r\n")
body.append(try Data(contentsOf: mediaItem.localURL))
append("\r\n")
append("--\(boundary)--\r\n")

request.httpBody = body
request.httpMethod = "POST"
return request
}
}


Expand Down
9 changes: 9 additions & 0 deletions WooCommerce/Classes/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import UIKit
import Combine
import Storage
import class Networking.UserAgent
import class Networking.MediaUploadSessionManager
import Experiments
import class WidgetKit.WidgetCenter
import protocol WooFoundation.Analytics
Expand Down Expand Up @@ -265,6 +266,14 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
DDLogDebug("Received memory warning: Available memory - \(size)")
ServiceLocator.imageService.clearMemoryCache()
}

func application(_ application: UIApplication,
handleEventsForBackgroundURLSession identifier: String,
completionHandler: @escaping () -> Void) {
if identifier == ServiceLocator.backgroundMediaUploadSessionManager.backgroundSessionIdentifier {
ServiceLocator.backgroundMediaUploadSessionManager.handleBackgroundSessionCompletion(completionHandler)
}
}
}

// MARK: - Initialization Methods
Expand Down
11 changes: 11 additions & 0 deletions WooCommerce/Classes/ServiceLocator/ServiceLocator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import Experiments
import Storage
import Yosemite
import Hardware
import class Networking.MediaUploadSessionManager
import WooFoundation
import WordPressShared

Expand Down Expand Up @@ -97,6 +98,10 @@ final class ServiceLocator {
///
private static var _generalAppSettings: GeneralAppSettingsStorage = GeneralAppSettingsStorage()

/// Background image service
///
private static var _backgroundMediaUploadSessionManager = MediaUploadSessionManager()

private static var _cardPresentPaymentsOnboardingIPPUsersRefresher: CardPresentPaymentsOnboardingIPPUsersRefresher =
CardPresentPaymentsOnboardingIPPUsersRefresher()

Expand Down Expand Up @@ -266,6 +271,12 @@ final class ServiceLocator {
static var startupWaitingTimeTracker: AppStartupWaitingTimeTracker {
_startupWaitingTimeTracker
}

/// Provides access point to the `MediaUploadSessionManager`.
///
static var backgroundMediaUploadSessionManager: MediaUploadSessionManager {
return _backgroundMediaUploadSessionManager
}
}


Expand Down
Loading