IOS-6729: Fix loadPhotos data race crash by protecting mutable states

simonwng · simonwng · commit ce7126b3eeef · 2022-12-20T16:03:15.000+13:00
diff --git a/MEGAUnitTests/Mocks/Sdk/MockNode.swift b/MEGAUnitTests/Mocks/Sdk/MockNode.swift
@@ -9,19 +9,22 @@ final class MockNode: MEGANode {
     private let nodeHandle: HandleEntity
     private let changeType: MEGANodeChangeType
     private var nodeModificationTime: Date?
+    private let _hasThumbnail: Bool
     
     init(handle: HandleEntity,
          name: String = "",
          nodeType: MEGANodeType = .file,
          parentHandle: HandleEntity = .invalid,
          changeType: MEGANodeChangeType = .new,
-         modificationTime: Date? = nil ) {
+         modificationTime: Date? = nil,
+         hasThumbnail: Bool = false) {
         nodeHandle = handle
         nodeName = name
         self.nodeType = nodeType
         nodeParentHandle = parentHandle
         self.changeType = changeType
         nodeModificationTime = modificationTime
+        _hasThumbnail = hasThumbnail
         super.init()
     }
     
@@ -44,4 +47,6 @@ final class MockNode: MEGANode {
     override var parentHandle: HandleEntity { nodeParentHandle }
     
     override var modificationTime: Date? { nodeModificationTime }
+    
+    override func hasThumbnail() -> Bool { _hasThumbnail }
 }
diff --git a/MEGAUnitTests/Photos/PhotosViewModelTests.swift b/MEGAUnitTests/Photos/PhotosViewModelTests.swift
@@ -3,8 +3,9 @@ import MEGADomainMock
 @testable import MEGA
 @testable import MEGADomain
 
+@MainActor
 final class PhotosViewModelTests: XCTestCase {
-    var photosViewModel: PhotosViewModel?
+    var sut: PhotosViewModel!
     
     override func setUp() {
         let publisher = PhotoUpdatePublisher(photosViewController: PhotosViewController())
@@ -15,34 +16,10 @@ final class PhotosViewModelTests: XCTestCase {
                                               allPhotosFromCloudDriveOnly: allPhotosForCloudDrive,
                                               allPhotosFromCameraUpload: allPhotosForCameraUploads)
         
-        photosViewModel = PhotosViewModel(photoUpdatePublisher: publisher, photoLibraryUseCase: usecase, mediaUseCase: MockMediaUseCase())
-    }
-    
-    private func mediaNodesSorted() -> [MEGANode] {
-        let today = Date()
-        let yesterday = Calendar.current.date(byAdding: .day, value: -1, to: today)
-        
-        return [
-            MockNode(handle: 1, name: "TestImage.png", nodeType: .file, parentHandle:0, modificationTime: today),
-            MockNode(handle: 2, name: "TestVideo1.mp4", nodeType: .file, parentHandle:0),
-            MockNode(handle: 2, name: "TestImage2.jpg", nodeType: .file, parentHandle:1, modificationTime: yesterday)
-        ]
-    }
-    
-    private func mediaNodesReverseSorted() -> [MEGANode] {
-        let today = Date()
-        let yesterday = Calendar.current.date(byAdding: .day, value: -1, to: today)
-        
-        return [
-            MockNode(handle: 2, name: "TestImage2.jpg", nodeType: .file, parentHandle:1, modificationTime: yesterday),
-            MockNode(handle: 1, name: "TestImage.png", nodeType: .file, parentHandle:0, modificationTime: today),
-            MockNode(handle: 2, name: "TestVideo1.mp4", nodeType: .file, parentHandle:0)
-        ]
+        sut = PhotosViewModel(photoUpdatePublisher: publisher, photoLibraryUseCase: usecase, mediaUseCase: MockMediaUseCase())
     }
     
     func testLoadSortOrderType() throws {
-        guard let sut = photosViewModel else { return }
-        
         Helper.save(.defaultAsc, for: PhotosViewModel.SortingKeys.cameraUploadExplorerFeed.rawValue)
         let sortTypeUnknown = sut.sortOrderType(forKey: .cameraUploadExplorerFeed)
         XCTAssert(sortTypeUnknown == .newest)
@@ -56,189 +33,111 @@ final class PhotosViewModelTests: XCTestCase {
         XCTAssert(sortTypeNewest == .newest)
     }
     
-    func testReorderPhotos() throws {
-        guard let sut = photosViewModel else { return }
-        
-        sut.mediaNodesArray = mediaNodesSorted()
-        
-        sut.cameraUploadExplorerSortOrderType = .nameAscending
-        XCTAssert(sut.mediaNodesArray == mediaNodesSorted())
-        
-        sut.cameraUploadExplorerSortOrderType = .newest
-        XCTAssert(sut.mediaNodesArray == mediaNodesSorted())
-        
-        sut.cameraUploadExplorerSortOrderType = .oldest
-        XCTAssert(sut.mediaNodesArray == mediaNodesReverseSorted())
-    }
-    
     // MARK: - All locations test cases
     
     func testLoadingPhotos_withAllMediaAllLocations_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedPhotos = sampleNodesForAllLocations()
-        photoVM.filterType = .allMedia
-        photoVM.filterLocation = . allLocations
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedPhotos.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .allMedia
+        sut.filterLocation = . allLocations
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedPhotos)
     }
     
     func testLoadingPhotos_withImagesAllLocations_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedImages = sampleNodesForAllLocations().filter({ $0.name?.mnz_isImagePathExtension == true })
-        photoVM.filterType = .images
-        photoVM.filterLocation = . allLocations
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedImages.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .images
+        sut.filterLocation = . allLocations
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedImages)
     }
     
     func testLoadingVideos_withImagesAllLocations_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedVideos = sampleNodesForAllLocations().filter({ $0.name?.mnz_isVideoPathExtension == true })
-        photoVM.filterType = .videos
-        photoVM.filterLocation = . allLocations
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedVideos.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .videos
+        sut.filterLocation = . allLocations
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedVideos)
     }
     
     // MARK: - Cloud Drive only test cases
     
     func testLoadingPhotos_withAllMediaFromCloudDrive_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedPhotos = sampleNodesForCloudDriveOnly()
-        photoVM.filterType = .allMedia
-        photoVM.filterLocation = .cloudDrive
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedPhotos.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .allMedia
+        sut.filterLocation = .cloudDrive
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedPhotos)
     }
     
     func testLoadingPhotos_withImagesFromCloudDrive_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedImages = sampleNodesForCloudDriveOnly().filter({ $0.name?.mnz_isImagePathExtension == true })
-        photoVM.filterType = .images
-        photoVM.filterLocation = .cloudDrive
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedImages.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .images
+        sut.filterLocation = .cloudDrive
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedImages)
     }
     
     func testLoadingPhotos_withVideosFromCloudDrive_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedVideos = sampleNodesForCloudDriveOnly().filter({ $0.name?.mnz_isVideoPathExtension == true })
-        photoVM.filterType = .videos
-        photoVM.filterLocation = .cloudDrive
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedVideos.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .videos
+        sut.filterLocation = .cloudDrive
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedVideos)
     }
     
     // MARK: - Camera Uploads test cases
     
     func testLoadingPhotos_withAllMediaFromCameraUploads_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedPhotos = sampleNodesForCameraUploads()
-        photoVM.filterType = .allMedia
-        photoVM.filterLocation = .cameraUploads
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedPhotos.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .allMedia
+        sut.filterLocation = .cameraUploads
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedPhotos)
     }
     
     func testLoadingPhotos_withImagesFromCameraUploads_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedImages = sampleNodesForCameraUploads().filter({ $0.name?.mnz_isImagePathExtension == true })
-        photoVM.filterType = .images
-        photoVM.filterLocation = .cameraUploads
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedImages.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .images
+        sut.filterLocation = .cameraUploads
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedImages)
     }
     
     func testLoadingPhotos_withVideosFromCameraUploads_shouldReturnTrue() async throws {
-        guard let photoVM = photosViewModel else { return }
-        
         let expectedVideos = sampleNodesForCameraUploads().filter({ $0.name?.mnz_isVideoPathExtension == true })
-        photoVM.filterType = .videos
-        photoVM.filterLocation = .cameraUploads
-        
-        do {
-            let photos = try await photoVM.loadFilteredPhotos()
-            XCTAssertTrue(photos.count == expectedVideos.count)
-        } catch {
-            assertionFailure("Unexpected exception!")
-        }
+        sut.filterType = .videos
+        sut.filterLocation = .cameraUploads
+        await sut.loadPhotos()
+        XCTAssertEqual(sut.mediaNodesArray, expectedVideos)
     }
     
     private func sampleNodesForAllLocations() ->[MEGANode] {
-        let node1 = MockNode(handle: 1, name: "TestImage1.png", nodeType: .file, parentHandle: 0)
-        let node2 = MockNode(handle: 2, name: "TestImage2.png", nodeType: .file, parentHandle: 1)
-        let node3 = MockNode(handle: 3, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 2)
-        let node4 = MockNode(handle: 4, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 3)
-        let node5 = MockNode(handle: 5, name: "TestImage1.png", nodeType: .file, parentHandle: 4)
-        let node6 = MockNode(handle: 6, name: "TestImage2.png", nodeType: .file, parentHandle: 5)
-        let node7 = MockNode(handle: 7, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 6)
-        let node8 = MockNode(handle: 8, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 7)
+        let node1 = MockNode(handle: 1, name: "TestImage1.png", nodeType: .file, parentHandle: 0, hasThumbnail: true)
+        let node2 = MockNode(handle: 2, name: "TestImage2.png", nodeType: .file, parentHandle: 1, hasThumbnail: true)
+        let node3 = MockNode(handle: 3, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 2, hasThumbnail: true)
+        let node4 = MockNode(handle: 4, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 3, hasThumbnail: true)
+        let node5 = MockNode(handle: 5, name: "TestImage1.png", nodeType: .file, parentHandle: 4, hasThumbnail: true)
+        let node6 = MockNode(handle: 6, name: "TestImage2.png", nodeType: .file, parentHandle: 5, hasThumbnail: true)
+        let node7 = MockNode(handle: 7, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 6, hasThumbnail: true)
+        let node8 = MockNode(handle: 8, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 7, hasThumbnail: true)
         
         return [node1, node2, node3, node4, node5, node6, node7, node8]
     }
     
     private func sampleNodesForCloudDriveOnly() ->[MEGANode] {
-        let node1 = MockNode(handle: 1, name: "TestImage1.png", nodeType: .file, parentHandle: 0)
-        let node2 = MockNode(handle: 2, name: "TestImage2.png", nodeType: .file, parentHandle: 1)
-        let node3 = MockNode(handle: 3, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 2)
-        let node4 = MockNode(handle: 4, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 3)
+        let node1 = MockNode(handle: 1, name: "TestImage1.png", nodeType: .file, parentHandle: 0, hasThumbnail: true)
+        let node2 = MockNode(handle: 2, name: "TestImage2.png", nodeType: .file, parentHandle: 1, hasThumbnail: true)
+        let node3 = MockNode(handle: 3, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 2, hasThumbnail: true)
+        let node4 = MockNode(handle: 4, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 3, hasThumbnail: true)
         
         return [node1, node2, node3, node4]
     }
     
     private func sampleNodesForCameraUploads() ->[MEGANode] {
-        let node5 = MockNode(handle: 5, name: "TestImage1.png", nodeType: .file, parentHandle: 4)
-        let node6 = MockNode(handle: 6, name: "TestImage2.png", nodeType: .file, parentHandle: 5)
-        let node7 = MockNode(handle: 7, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 6)
-        let node8 = MockNode(handle: 8, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 7)
+        let node5 = MockNode(handle: 5, name: "TestImage1.png", nodeType: .file, parentHandle: 4, hasThumbnail: true)
+        let node6 = MockNode(handle: 6, name: "TestImage2.png", nodeType: .file, parentHandle: 5, hasThumbnail: true)
+        let node7 = MockNode(handle: 7, name: "TestVideo1.mp4", nodeType: .file, parentHandle: 6, hasThumbnail: true)
+        let node8 = MockNode(handle: 8, name: "TestVideo2.mp4", nodeType: .file, parentHandle: 7, hasThumbnail: true)
         
         return [node5, node6, node7, node8]
     }
diff --git a/iMEGA/Camera uploads/Extension/PhotosViewController+PhotoLibrary.swift b/iMEGA/Camera uploads/Extension/PhotosViewController+PhotoLibrary.swift
@@ -10,9 +10,8 @@ extension PhotosViewController: PhotoLibraryProvider {
         }
     }
     
-    @objc func objcWrapper_updatePhotoLibrary(by nodes: [MEGANode]) {
-        let sortType = viewModel.cameraUploadExplorerSortOrderType ?? .newest
-        updatePhotoLibrary(by: nodes, withSortType: sortType)
+    @objc func objcWrapper_updatePhotoLibrary() {
+        updatePhotoLibrary(by: viewModel.mediaNodesArray, withSortType: viewModel.cameraUploadExplorerSortOrderType)
     }
     
     @objc func createPhotoLibraryContentViewModel() -> PhotoLibraryContentViewModel {
diff --git a/iMEGA/Camera uploads/PhotosViewController.m b/iMEGA/Camera uploads/PhotosViewController.m
@@ -347,8 +347,7 @@ - (void)reloadPhotos {
     [self reloadHeader];
     [self setupNavigationBarButtons];
     dispatch_async(dispatch_get_main_queue(), ^{
-        [self objcWrapper_updatePhotoLibraryBy: self.viewModel.mediaNodesArray];
-        
+        [self objcWrapper_updatePhotoLibrary];
         if (self.viewModel.mediaNodesArray.count == 0) {
             [self.photosCollectionView reloadEmptyDataSet];
         }
diff --git a/iMEGA/Camera uploads/ViewModel/PhotosViewModel+FilterAndSort.swift b/iMEGA/Camera uploads/ViewModel/PhotosViewModel+FilterAndSort.swift
@@ -9,27 +9,15 @@ extension PhotosViewModel {
         return sortType != .newest && sortType != .oldest ? .newest : sortType
     }
     
-    func reorderPhotos(_ sortType: SortOrderType?, mediaNodes: [MEGANode]) -> [MEGANode] {
-        guard let sortType = sortType,
-              sortType == .newest || sortType == .oldest else { return mediaNodes }
-        
-        return mediaNodes.sorted { node1, node2 in
-            guard let date1 = node1.modificationTime,
-                  let date2 = node2.modificationTime else { return node1.name ?? "" < node2.name ?? "" }
-            
-            return sortType == .newest ? date1 > date2 : date1 < date2
-        }
-    }
-    
     // MARK: - Filter
     
     func filter(nodes: inout [MEGANode], with type: PhotosFilterOptions) {
         guard type != .allMedia else { return }
         
         if type == .images {
-            nodes = nodes.filter({ $0.name?.mnz_isImagePathExtension == true })
+            nodes = nodes.filter({ $0.name?.mnz_isImagePathExtension == true && $0.hasThumbnail() })
         } else {
-            nodes = nodes.filter({ $0.name?.mnz_isVideoPathExtension == true })
+            nodes = nodes.filter({ $0.name?.mnz_isVideoPathExtension == true && $0.hasThumbnail() })
         }
     }
 }
diff --git a/iMEGA/Camera uploads/ViewModel/PhotosViewModel.swift b/iMEGA/Camera uploads/ViewModel/PhotosViewModel.swift

Original file line number	Diff line number	Diff line change
`@@ -10,9 +10,8 @@ extension PhotosViewController: PhotoLibraryProvider {`
`10`	`10`	`}`
`11`	`11`	`}`
`12`	`12`
`13`		`- @objc func objcWrapper_updatePhotoLibrary(by nodes: [MEGANode]) {`
`14`		`- let sortType = viewModel.cameraUploadExplorerSortOrderType ?? .newest`
`15`		`- updatePhotoLibrary(by: nodes, withSortType: sortType)`
	`13`	`+ @objc func objcWrapper_updatePhotoLibrary() {`
	`14`	`+ updatePhotoLibrary(by: viewModel.mediaNodesArray, withSortType: viewModel.cameraUploadExplorerSortOrderType)`
`16`	`15`	`}`
`17`	`16`
`18`	`17`	`@objc func createPhotoLibraryContentViewModel() -> PhotoLibraryContentViewModel {`