Only publish a post after all media has been uploaded

WordPress/Classes/Models/AbstractPost.swift

@@ -95,4 +95,23 @@ extension AbstractPost {
             return nil
         }
     }
+
+    // MARK: - Updating the Remote Status
+    /// Updates the post after an upload failure.
+    ///
+    /// - Important: This logic could have been placed in the setter for `remoteStatus`, but it's my belief
+    ///     that our code will be much more resilient if we decouple the act of setting the `remoteStatus` value
+    ///     and the logic behind processing an upload failure.  In fact I think the `remoteStatus` setter should
+    ///     eventually be made private.
+    ///
+    @objc
+    func failedToUpload() {
+        remoteStatus = .failed
+
+        if !hasRemote() {
+            // If the post was not created on the server yet we convert the post to a local draft post with the current date.
+            status = .draft
+            dateModified = Date()
+        }
+    }
 }

WordPress/Classes/Services/PostCoordinator.swift

@@ -6,13 +6,9 @@ class PostCoordinator: NSObject {
 
     @objc static let shared = PostCoordinator()
 
-    private(set) var backgroundContext: NSManagedObjectContext = {
-        let context = ContextManager.sharedInstance().newDerivedContext()
-        context.automaticallyMergesChangesFromParent = true
-        return context
-    }()
+    private let backgroundContext: NSManagedObjectContext
 
-    private let mainContext = ContextManager.sharedInstance().mainContext
+    private let mainContext: NSManagedObjectContext
 
     private let queue = DispatchQueue(label: "org.wordpress.postcoordinator")
 
@@ -22,6 +18,23 @@ class PostCoordinator: NSObject {
         return MediaCoordinator.shared
     }()
 
+    private let backgroundService: PostService
+
+    private let mainService: PostService
+
+    init(mainContext: NSManagedObjectContext = ContextManager.sharedInstance().mainContext,
+         backgroundContext: NSManagedObjectContext = ContextManager.sharedInstance().newDerivedContext(),
+         mainService: PostService? = nil,
+         backgroundService: PostService? = nil) {
+
+        self.mainContext = mainContext
+        self.backgroundContext = backgroundContext
+        backgroundContext.automaticallyMergesChangesFromParent = true
+
+        self.backgroundService = backgroundService ?? PostService(managedObjectContext: backgroundContext)
+        self.mainService = mainService ?? PostService(managedObjectContext: mainContext)
+    }
+
     /// Saves the post to both the local database and the server if available.
     /// If media is still uploading it keeps track of the ongoing media operations and updates the post content when they finish
     ///
@@ -34,9 +47,18 @@ class PostCoordinator: NSObject {
             post.deleteRevision()
         }
 
+        if post.hasFailedMedia {
+            for media in post.media {
+                guard media.remoteStatus == .failed else {
+                    continue
+                }
+                mediaCoordinator.retryMedia(media)
+            }
+        }
+
         change(post: post, status: .pushing)
 
-        if mediaCoordinator.isUploadingMedia(for: post) {
+        if mediaCoordinator.isUploadingMedia(for: post) || post.hasFailedMedia {
             change(post: post, status: .pushingMedia)
             // Only observe if we're not already
             guard !isObserving(post: post) else {
@@ -147,26 +169,18 @@ class PostCoordinator: NSObject {
     /// - Parameter post: the post to retry the upload
     ///
     @objc func retrySave(of post: AbstractPost) {
-        for media in post.media {
-            guard media.remoteStatus == .failed else {
-                continue
-            }
-            mediaCoordinator.retryMedia(media)
-        }
         save(post: post)
     }
 
     /// This method checks the status of all post objects and updates them to the correct status if needed.
     /// The main cause of wrong status is the app being killed while uploads of posts are happening.
     ///
     @objc func refreshPostStatus() {
-        let service = PostService(managedObjectContext: backgroundContext)
-        service.refreshPostStatus()
+        backgroundService.refreshPostStatus()
     }
 
     private func upload(post: AbstractPost) {
-        let postService = PostService(managedObjectContext: mainContext)
-        postService.uploadPost(post, success: { uploadedPost in
+        mainService.uploadPost(post, success: { uploadedPost in
             print("Post Coordinator -> upload succesfull: \(String(describing: uploadedPost.content))")
 
             SearchManager.shared.indexItem(uploadedPost)
@@ -240,17 +254,20 @@ class PostCoordinator: NSObject {
 
     private func change(post: AbstractPost, status: AbstractPostRemoteStatus) {
         post.managedObjectContext?.perform {
-            post.remoteStatus = status
+            if status == .failed {
+                post.failedToUpload()
+            } else {
+                post.remoteStatus = status
+            }
+
             try? post.managedObjectContext?.save()
         }
     }
 }
 
 extension PostCoordinator: Uploader {
     func resume() {
-        let service = PostService(managedObjectContext: mainContext)
-
-        service.getFailedPosts { [weak self] posts in
+        mainService.getFailedPosts { [weak self] posts in
             guard let self = self else {
                 return
             }

WordPress/WordPress.xcodeproj/project.pbxproj

@@ -999,6 +999,7 @@
 		85F8E19D1B018698000859BB /* PushAuthenticationServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 85F8E19C1B018698000859BB /* PushAuthenticationServiceTests.swift */; };
 		8B8C814D2318073300A0E620 /* BasePostTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8B8C814C2318073300A0E620 /* BasePostTests.swift */; };
 		8B93856E22DC08060010BF02 /* PageListSectionHeaderView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8B93856D22DC08060010BF02 /* PageListSectionHeaderView.swift */; };
+		8BC12F72231FEBA1004DDA72 /* PostCoordinatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8BC12F71231FEBA1004DDA72 /* PostCoordinatorTests.swift */; };
 		8BFE36FD230F16580061EBA8 /* AbstractPost+fixLocalMediaURLs.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8BFE36FC230F16580061EBA8 /* AbstractPost+fixLocalMediaURLs.swift */; };
 		8BFE36FF230F1C850061EBA8 /* AbstractPost+fixLocalMediaURLsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8BFE36FE230F1C850061EBA8 /* AbstractPost+fixLocalMediaURLsTests.swift */; };
 		91138455228373EB00FB02B7 /* GutenbergVideoUploadProcessor.swift in Sources */ = {isa = PBXBuildFile; fileRef = 91138454228373EB00FB02B7 /* GutenbergVideoUploadProcessor.swift */; };
@@ -3133,6 +3134,7 @@
 		85F8E19C1B018698000859BB /* PushAuthenticationServiceTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PushAuthenticationServiceTests.swift; sourceTree = "<group>"; };
 		8B8C814C2318073300A0E620 /* BasePostTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BasePostTests.swift; sourceTree = "<group>"; };
 		8B93856D22DC08060010BF02 /* PageListSectionHeaderView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PageListSectionHeaderView.swift; sourceTree = "<group>"; };
+		8BC12F71231FEBA1004DDA72 /* PostCoordinatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PostCoordinatorTests.swift; sourceTree = "<group>"; };
 		8BFE36FC230F16580061EBA8 /* AbstractPost+fixLocalMediaURLs.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "AbstractPost+fixLocalMediaURLs.swift"; sourceTree = "<group>"; };
 		8BFE36FE230F1C850061EBA8 /* AbstractPost+fixLocalMediaURLsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "AbstractPost+fixLocalMediaURLsTests.swift"; sourceTree = "<group>"; };
 		8CE5BBD00FF1470AC4B88247 /* Pods_WordPressTodayWidget.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_WordPressTodayWidget.framework; sourceTree = BUILT_PRODUCTS_DIR; };
@@ -7957,6 +7959,7 @@
 				59FBD5611B5684F300734466 /* ThemeServiceTests.m */,
 				40E4698E2017E0700030DB5F /* PluginDirectoryEntryStateTests.swift */,
 				7E8980B822E73F4000C567B0 /* EditorSettingsServiceTests.swift */,
+				8BC12F71231FEBA1004DDA72 /* PostCoordinatorTests.swift */,
 			);
 			name = Services;
 			sourceTree = "<group>";
@@ -11808,6 +11811,7 @@
 				400A2C952217B68D000A8A59 /* TopViewedVideoStatsRecordValueTests.swift in Sources */,
 				D81C2F6620F8ACCD002AE1F1 /* FormattableContentFormatterTests.swift in Sources */,
 				F93735F822D53C3B00A3C312 /* LoggingURLRedactorTests.swift in Sources */,
+				8BC12F72231FEBA1004DDA72 /* PostCoordinatorTests.swift in Sources */,
 				D8B6BEB7203E11F2007C8A19 /* Bundle+LoadFromNib.swift in Sources */,
 				E135965D1E7152D1006C6606 /* RecentSitesServiceTests.swift in Sources */,
 				D88A64AC208D9B09008AE9BC /* StockPhotosPageableTests.swift in Sources */,

WordPress/WordPressTest/PostBuilder.swift

@@ -79,7 +79,7 @@ class PostBuilder {
         return self
     }
 
-    func with(image: String) -> PostBuilder {
+    func with(image: String, status: MediaRemoteStatus? = nil) -> PostBuilder {
         guard let context = post.managedObjectContext else {
             return self
         }
@@ -89,6 +89,7 @@ class PostBuilder {
         }
         media.localURL = image
         media.localThumbnailURL = "thumb-\(image)"
+        if let status = status { media.remoteStatus = status }
 
         media.addPostsObject(post)
         post.addMediaObject(media)

WordPress/WordPressTest/PostCoordinatorTests.swift

@@ -0,0 +1,39 @@
+import UIKit
+import XCTest
+import Nimble
+
+@testable import WordPress
+
+class PostCoordinatorTests: XCTestCase {
+
+    private let context = TestContextManager().newDerivedContext()
+
+    func testDoNotUploadAPostWithFailedMedia() {
+        let post = PostBuilder(context).with(image: "test.jpeg", status: .failed).build()
+        let postServiceMock = PostServiceMock()
+        let postCoordinator = PostCoordinator(mainService: postServiceMock, backgroundService: postServiceMock)
+
+        postCoordinator.save(post: post)
+
+        expect(post.remoteStatus).toEventually(equal(.failed))
+        expect(postServiceMock.didCallUploadPost).to(beFalse())
+    }
+
+    func testUploadAPostWithNoFailedMedia() {
+        let post = PostBuilder(context).with(image: "test.jpeg").build()
+        let postServiceMock = PostServiceMock()
+        let postCoordinator = PostCoordinator(mainService: postServiceMock, backgroundService: postServiceMock)
+
+        postCoordinator.save(post: post)
+
+        expect(postServiceMock.didCallUploadPost).to(beTrue())
+    }
+}
+
+class PostServiceMock: PostService {
+    var didCallUploadPost = false
+
+    override func uploadPost(_ post: AbstractPost, success: ((AbstractPost) -> Void)?, failure: @escaping (Error?) -> Void) {
+        didCallUploadPost = true
+    }
+}