Fix a bug where the image upload screen was unintentionally dismissed for some failures. (#4414)

Author: pixlwave

ElementX/Sources/Screens/MediaUploadPreviewScreen/MediaUploadPreviewScreenViewModel.swift

@@ -62,31 +62,27 @@ class MediaUploadPreviewScreenViewModel: MediaUploadPreviewScreenViewModelType,
             Task {
                 defer { stopLoading() }
 
-                var shouldDismissOnCompletion = true
                 switch await processingTask.value {
                 case .success(let mediaInfos):
                     for mediaInfo in mediaInfos {
                         switch await sendAttachment(mediaInfo: mediaInfo, caption: caption) {
                         case .success:
                             caption = nil // Set the caption only on the first uploaded file.
                         case .failure(let error):
-                            MXLog.error("Failed processing media to upload with error: \(error)")
-                            showError(label: L10n.screenMediaUploadPreviewErrorFailedProcessing)
+                            MXLog.error("Failed sending media with error: \(error)")
+                            showError(label: L10n.screenMediaUploadPreviewErrorFailedSending)
                         }
                     }
+                    
+                    actionsSubject.send(.dismiss)
                 case .failure(.maxUploadSizeUnknown):
                     showAlert(.maxUploadSizeUnknown)
-                    shouldDismissOnCompletion = false
                 case .failure(.maxUploadSizeExceeded(let limit)):
                     showAlert(.maxUploadSizeExceeded(limit: limit))
                 case .failure(let error):
                     MXLog.error("Failed processing media to upload with error: \(error)")
                     showError(label: L10n.screenMediaUploadPreviewErrorFailedProcessing)
                 }
-                
-                if shouldDismissOnCompletion {
-                    actionsSubject.send(.dismiss)
-                }
             }
         case .cancel:
             requestHandle?.cancel()

UnitTests/Sources/MediaUploadPreviewScreenViewModelTests.swift

@@ -13,6 +13,8 @@ import XCTest
 class MediaUploadPreviewScreenViewModelTests: XCTestCase {
     var timelineProxy: TimelineProxyMock!
     var clientProxy: ClientProxyMock!
+    var userIndicatorController: UserIndicatorControllerMock!
+    
     var viewModel: MediaUploadPreviewScreenViewModel!
     var context: MediaUploadPreviewScreenViewModel.Context { viewModel.context }
 
@@ -33,77 +35,97 @@ class MediaUploadPreviewScreenViewModelTests: XCTestCase {
     }
 
     func testImageUploadWithoutCaption() async throws {
-        setUpViewModel(url: imageURL, expectedCaption: nil)
+        setUpViewModel(urls: [imageURL], expectedCaption: nil)
         context.caption = .init("")
         try await send()
     }
 
     func testImageUploadWithBlankCaption() async throws {
-        setUpViewModel(url: imageURL, expectedCaption: nil)
+        setUpViewModel(urls: [imageURL], expectedCaption: nil)
         context.caption = .init("     ")
         try await send()
     }
 
     func testImageUploadWithCaption() async throws {
         let caption = "This is a really great image!"
-        setUpViewModel(url: imageURL, expectedCaption: caption)
+        setUpViewModel(urls: [imageURL], expectedCaption: caption)
         context.caption = .init(string: caption)
         try await send()
     }
 
     func testVideoUploadWithoutCaption() async throws {
-        setUpViewModel(url: videoURL, expectedCaption: nil)
+        setUpViewModel(urls: [videoURL], expectedCaption: nil)
         context.caption = .init("")
         try await send()
     }
 
     func testVideoUploadWithCaption() async throws {
         let caption = "Check out this video!"
-        setUpViewModel(url: videoURL, expectedCaption: caption)
+        setUpViewModel(urls: [videoURL], expectedCaption: caption)
         context.caption = .init(string: caption)
         try await send()
     }
 
     func testAudioUploadWithoutCaption() async throws {
-        setUpViewModel(url: audioURL, expectedCaption: nil)
+        setUpViewModel(urls: [audioURL], expectedCaption: nil)
         context.caption = .init("")
         try await send()
     }
 
     func testAudioUploadWithCaption() async throws {
         let caption = "Listen to this!"
-        setUpViewModel(url: audioURL, expectedCaption: caption)
+        setUpViewModel(urls: [audioURL], expectedCaption: caption)
         context.caption = .init(string: caption)
         try await send()
     }
 
     func testFileUploadWithoutCaption() async throws {
-        setUpViewModel(url: fileURL, expectedCaption: nil)
+        setUpViewModel(urls: [fileURL], expectedCaption: nil)
         context.caption = .init("")
         try await send()
     }
 
     func testFileUploadWithCaption() async throws {
         let caption = "Please will you check my article."
-        setUpViewModel(url: fileURL, expectedCaption: caption)
+        setUpViewModel(urls: [fileURL], expectedCaption: caption)
         context.caption = .init(string: caption)
         try await send()
     }
 
+    func testProcessingFailure() async throws {
+        // Given an upload screen for a non-existent file.
+        setUpViewModel(urls: [badImageURL], expectedCaption: nil)
+        XCTAssertFalse(context.viewState.shouldDisableInteraction)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 0)
+        
+        // When attempting to send the file
+        let deferredFailure = deferFailure(viewModel.actions, timeout: 1, message: "The screen should remain visible.") { $0 == .dismiss }
+        context.send(viewAction: .send)
+        XCTAssertTrue(context.viewState.shouldDisableInteraction, "The interaction should be disabled while sending.")
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 1) // Loading indicator
+        
+        // Then the failure should occur preventing the screen from being dismissed.
+        try await deferredFailure.fulfill()
+        XCTAssertFalse(context.viewState.shouldDisableInteraction)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 2, "An error indicator should be shown.")
+    }
+    
     func testUploadWithUnknownMaxUploadSize() async throws {
         // Given an upload screen that is unable to fetch the max upload size.
-        setUpViewModel(url: imageURL, expectedCaption: nil, maxUploadSizeResult: .failure(.sdkError(ClientProxyMockError.generic)))
+        setUpViewModel(urls: [imageURL], expectedCaption: nil, maxUploadSizeResult: .failure(.sdkError(ClientProxyMockError.generic)))
         XCTAssertFalse(context.viewState.shouldDisableInteraction)
         XCTAssertNil(context.alertInfo)
 
         // When attempting to send the media.
         let deferredAlert = deferFulfillment(context.observe(\.viewState.bindings.alertInfo)) { $0 != nil }
+        let deferredFailure = deferFailure(viewModel.actions, timeout: 1, message: "The screen should remain visible.") { $0 == .dismiss }
         context.send(viewAction: .send)
 
         XCTAssertTrue(context.viewState.shouldDisableInteraction, "The interaction should be disabled while sending.")
 
         // Then alert should be shown to tell the user it failed.
         try await deferredAlert.fulfill()
+        try await deferredFailure.fulfill()
 
         XCTAssertFalse(context.viewState.shouldDisableInteraction)
         XCTAssertEqual(context.alertInfo?.id, .maxUploadSizeUnknown)
@@ -121,29 +143,90 @@ class MediaUploadPreviewScreenViewModelTests: XCTestCase {
 
     func testUploadExceedingMaxUploadSize() async throws {
         // Given an upload screen with a really small max upload size.
-        setUpViewModel(url: imageURL, expectedCaption: nil, maxUploadSizeResult: .success(100))
+        setUpViewModel(urls: [imageURL], expectedCaption: nil, maxUploadSizeResult: .success(100))
         XCTAssertFalse(context.viewState.shouldDisableInteraction)
         XCTAssertNil(context.alertInfo)
 
         // When attempting to send an image that is larger the limit.
         let deferredAlert = deferFulfillment(context.observe(\.viewState.bindings.alertInfo)) { $0 != nil }
+        let deferredFailure = deferFailure(viewModel.actions, timeout: 1, message: "The screen should remain visible.") { $0 == .dismiss }
         context.send(viewAction: .send)
 
         XCTAssertTrue(context.viewState.shouldDisableInteraction, "The interaction should be disabled while sending.")
 
         // Then an alert should be shown to inform the user of the max upload size.
         try await deferredAlert.fulfill()
+        try await deferredFailure.fulfill()
 
         XCTAssertFalse(context.viewState.shouldDisableInteraction)
         XCTAssertEqual(context.alertInfo?.id, .maxUploadSizeExceeded(limit: 100))
     }
 
+    func testMultipleFiles() async throws {
+        // Given an upload screen with multiple media files.
+        setUpViewModel(urls: [fileURL, imageURL, fileURL], expectedCaption: nil)
+        XCTAssertFalse(context.viewState.shouldDisableInteraction)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 0)
+        
+        // When attempting to send the files.
+        let deferredDismiss = deferFulfillment(viewModel.actions) { $0 == .dismiss }
+        context.send(viewAction: .send)
+        
+        XCTAssertTrue(context.viewState.shouldDisableInteraction, "The interaction should be disabled while sending.")
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 1) // Loading indicator
+        
+        // Then the screen should be dismissed once all of the files have been sent.
+        try await deferredDismiss.fulfill()
+        XCTAssertEqual(timelineProxy.sendImageUrlThumbnailURLImageInfoCaptionRequestHandleCallsCount, 1)
+        XCTAssertEqual(timelineProxy.sendFileUrlFileInfoCaptionRequestHandleCallsCount, 2)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 1, "Only a loading indicator should be shown.")
+    }
+    
+    func testMultipleFilesWithProcessingFailure() async throws {
+        // Given an upload screen for a non-existent file.
+        setUpViewModel(urls: [imageURL, fileURL, badImageURL], expectedCaption: nil)
+        XCTAssertFalse(context.viewState.shouldDisableInteraction)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 0)
+        
+        // When attempting to send the file
+        let deferredFailure = deferFailure(viewModel.actions, timeout: 1, message: "The screen should remain visible.") { $0 == .dismiss }
+        context.send(viewAction: .send)
+        XCTAssertTrue(context.viewState.shouldDisableInteraction, "The interaction should be disabled while sending.")
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 1) // Loading indicator
+        
+        // Then the failure should occur preventing the screen from being dismissed.
+        try await deferredFailure.fulfill()
+        XCTAssertFalse(context.viewState.shouldDisableInteraction)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 2, "An error indicator should be shown.")
+    }
+    
+    func testMultipleFilesWithSendFailure() async throws {
+        // Given an upload screen with multiple media files where one of the files will fail to send.
+        setUpViewModel(urls: [fileURL, imageURL, imageURL, fileURL], expectedCaption: nil, simulateImageSendFailures: true)
+        XCTAssertFalse(context.viewState.shouldDisableInteraction)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 0)
+        
+        // When attempting to send the files.
+        let deferredDismiss = deferFulfillment(viewModel.actions) { $0 == .dismiss }
+        context.send(viewAction: .send)
+        
+        XCTAssertTrue(context.viewState.shouldDisableInteraction, "The interaction should be disabled while sending.")
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 1) // Loading indicator
+        
+        // Then the screen should be dismissed so the user can see which files made it into the timeline.
+        try await deferredDismiss.fulfill()
+        XCTAssertEqual(timelineProxy.sendImageUrlThumbnailURLImageInfoCaptionRequestHandleCallsCount, 2)
+        XCTAssertEqual(timelineProxy.sendFileUrlFileInfoCaptionRequestHandleCallsCount, 2)
+        XCTAssertEqual(userIndicatorController.submitIndicatorDelayCallsCount, 3, "Error indicators for each failure should be shown.")
+    }
+    
     // MARK: - Helpers
 
     private var audioURL: URL { assertResourceURL(filename: "test_audio.mp3") }
     private var fileURL: URL { assertResourceURL(filename: "test_pdf.pdf") }
     private var imageURL: URL { assertResourceURL(filename: "test_animated_image.gif") }
     private var videoURL: URL { assertResourceURL(filename: "landscape_test_video.mov") }
+    private var badImageURL = URL(filePath: "/home/user/this_file_doesn't_exist.jpg")
 
     private func assertResourceURL(filename: String) -> URL {
         guard let url = Bundle(for: Self.self).url(forResource: filename, withExtension: nil) else {
@@ -153,7 +236,10 @@ class MediaUploadPreviewScreenViewModelTests: XCTestCase {
         return url
     }
 
-    private func setUpViewModel(url: URL, expectedCaption: String?, maxUploadSizeResult: Result<UInt, ClientProxyError>? = nil) {
+    private func setUpViewModel(urls: [URL],
+                                expectedCaption: String?,
+                                maxUploadSizeResult: Result<UInt, ClientProxyError>? = nil,
+                                simulateImageSendFailures: Bool = false) {
         timelineProxy = TimelineProxyMock(.init())
         timelineProxy.sendAudioUrlAudioInfoCaptionRequestHandleClosure = { [weak self] _, _, caption, _ in
             self?.verifyCaption(caption, expectedCaption: expectedCaption) ?? .failure(.sdkError(TestError.unknown))
@@ -162,7 +248,8 @@ class MediaUploadPreviewScreenViewModelTests: XCTestCase {
             self?.verifyCaption(caption, expectedCaption: expectedCaption) ?? .failure(.sdkError(TestError.unknown))
         }
         timelineProxy.sendImageUrlThumbnailURLImageInfoCaptionRequestHandleClosure = { [weak self] _, _, _, caption, _ in
-            self?.verifyCaption(caption, expectedCaption: expectedCaption) ?? .failure(.sdkError(TestError.unknown))
+            guard !simulateImageSendFailures else { return .failure(.sdkError(TestError.unknown)) }
+            return self?.verifyCaption(caption, expectedCaption: expectedCaption) ?? .failure(.sdkError(TestError.unknown))
         }
         timelineProxy.sendVideoUrlThumbnailURLVideoInfoCaptionRequestHandleClosure = { [weak self] _, _, _, caption, _ in
             self?.verifyCaption(caption, expectedCaption: expectedCaption) ?? .failure(.sdkError(TestError.unknown))
@@ -173,14 +260,16 @@ class MediaUploadPreviewScreenViewModelTests: XCTestCase {
             clientProxy.underlyingMaxMediaUploadSize = maxUploadSizeResult
         }
 
-        viewModel = MediaUploadPreviewScreenViewModel(mediaURLs: [url],
+        userIndicatorController = UserIndicatorControllerMock()
+        
+        viewModel = MediaUploadPreviewScreenViewModel(mediaURLs: urls,
                                                       title: "Some File",
                                                       isRoomEncrypted: true,
                                                       shouldShowCaptionWarning: true,
                                                       mediaUploadingPreprocessor: MediaUploadingPreprocessor(appSettings: ServiceLocator.shared.settings),
                                                       timelineController: MockTimelineController(timelineProxy: timelineProxy),
                                                       clientProxy: clientProxy,
-                                                      userIndicatorController: UserIndicatorControllerMock())
+                                                      userIndicatorController: userIndicatorController)
     }
 
     private func verifyCaption(_ caption: String?, expectedCaption: String?) -> Result<Void, TimelineProxyError> {