Improve view model to update and move bookmark folders in one transac…

…tion

DuckDuckGo/Bookmarks/Model/BookmarkManager.swift

@@ -36,6 +36,7 @@ protocol BookmarkManager: AnyObject {
     func remove(objectsWithUUIDs uuids: [String])
     func update(bookmark: Bookmark)
     func update(folder: BookmarkFolder)
+    func update(folder: BookmarkFolder, andMoveToParent parent: ParentFolderType)
     @discardableResult func updateUrl(of bookmark: Bookmark, to newUrl: URL) -> Bookmark?
     func add(bookmark: Bookmark, to parent: BookmarkFolder?, completion: @escaping (Error?) -> Void)
     func add(objectsWithUUIDs uuids: [String], to parent: BookmarkFolder?, completion: @escaping (Error?) -> Void)
@@ -217,6 +218,12 @@ final class LocalBookmarkManager: BookmarkManager {
         requestSync()
     }
 
+    func update(folder: BookmarkFolder, andMoveToParent parent: ParentFolderType) {
+        bookmarkStore.update(folder: folder, andMoveToParent: parent)
+        loadBookmarks()
+        requestSync()
+    }
+
     func updateUrl(of bookmark: Bookmark, to newUrl: URL) -> Bookmark? {
         guard list != nil else { return nil }
         guard getBookmark(forUrl: bookmark.url) != nil else {

DuckDuckGo/Bookmarks/Services/BookmarkStore.swift

@@ -52,6 +52,7 @@ protocol BookmarkStore {
     func remove(objectsWithUUIDs: [String], completion: @escaping (Bool, Error?) -> Void)
     func update(bookmark: Bookmark)
     func update(folder: BookmarkFolder)
+    func update(folder: BookmarkFolder, andMoveToParent parent: ParentFolderType)
     func add(objectsWithUUIDs: [String], to parent: BookmarkFolder?, completion: @escaping (Error?) -> Void)
     func update(objectsWithUUIDs uuids: [String], update: @escaping (BaseBookmarkEntity) -> Void, completion: @escaping (Error?) -> Void)
     func canMoveObjectWithUUID(objectUUID uuid: String, to parent: BookmarkFolder) -> Bool

DuckDuckGo/Bookmarks/Services/BookmarkStoreMock.swift

@@ -48,6 +48,7 @@ public final class BookmarkStoreMock: BookmarkStore {
 
     var capturedFolder: BookmarkFolder?
     var capturedParentFolder: BookmarkFolder?
+    var capturedParentFolderType: ParentFolderType?
 
     var loadAllCalled = false
     var bookmarks: [BaseBookmarkEntity]?
@@ -105,6 +106,13 @@ public final class BookmarkStoreMock: BookmarkStore {
         capturedFolder = folder
     }
 
+    var updateFolderAndMoveToParentCalled = false
+    func update(folder: BookmarkFolder, andMoveToParent parent: ParentFolderType) {
+        updateFolderAndMoveToParentCalled = true
+        capturedFolder = folder
+        capturedParentFolderType = parent
+    }
+
     var addChildCalled = false
     func add(objectsWithUUIDs: [String], to parent: BookmarkFolder?, completion: @escaping (Error?) -> Void) {
         addChildCalled = true
@@ -129,7 +137,6 @@ public final class BookmarkStoreMock: BookmarkStore {
 
     var moveObjectUUIDCalled = false
     var capturedObjectUUIDs: [String]?
-    var capturedParentFolderType: ParentFolderType?
     func move(objectUUIDs: [String], toIndex: Int?, withinParentFolder: ParentFolderType, completion: @escaping (Error?) -> Void) {
         moveObjectUUIDCalled = true
         capturedObjectUUIDs = objectUUIDs
@@ -146,7 +153,7 @@ public final class BookmarkStoreMock: BookmarkStore {
 }
 
 extension ParentFolderType: Equatable {
-    public static func ==(lhs: Self, rhs: Self) -> Bool {
+    public static func == (lhs: Self, rhs: Self) -> Bool {
         switch (lhs, rhs) {
         case (.root, .root):
             return true

DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift

@@ -391,18 +391,21 @@ final class LocalBookmarkStore: BookmarkStore {
     }
 
     func update(folder: BookmarkFolder) {
-
         do {
             _ = try applyChangesAndSave(changes: { context in
-                let folderFetchRequest = BaseBookmarkEntity.singleEntity(with: folder.id)
-                let folderFetchRequestResults = try? context.fetch(folderFetchRequest)
-
-                guard let bookmarkFolderMO = folderFetchRequestResults?.first else {
-                    assertionFailure("LocalBookmarkStore: Failed to get BookmarkEntity from the context")
-                    throw BookmarkStoreError.missingEntity
-                }
+                try update(folder: folder, in: context)
+            })
+        } catch {
+            let error = error as NSError
+            commonOnSaveErrorHandler(error)
+        }
+    }
 
-                bookmarkFolderMO.update(with: folder)
+    func update(folder: BookmarkFolder, andMoveToParent parent: ParentFolderType) {
+        do {
+            _ = try applyChangesAndSave(changes: { context in
+                let folderEntity = try update(folder: folder, in: context)
+                try move(entities: [folderEntity], toIndex: nil, withinParentFolderType: parent, in: context)
             })
         } catch {
             let error = error as NSError
@@ -566,39 +569,15 @@ final class LocalBookmarkStore: BookmarkStore {
                 throw BookmarkStoreError.storeDeallocated
             }
 
-            guard let rootFolder = self.bookmarksRoot(in: context) else {
-                throw BookmarkStoreError.missingRoot
-            }
-
             // Guarantee that bookmarks are fetched in the same order as the UUIDs. In the future, this should fetch all objects at once with a
             // batch fetch request and have them sorted in the correct order.
             let bookmarkManagedObjects: [BookmarkEntity] = objectUUIDs.compactMap { uuid in
                 let entityFetchRequest = BaseBookmarkEntity.singleEntity(with: uuid)
                 return (try? context.fetch(entityFetchRequest))?.first
             }
 
-            let newParentFolder: BookmarkEntity
-
-            switch type {
-            case .root: newParentFolder = rootFolder
-            case .parent(let newParentUUID):
-                let bookmarksFetchRequest = BaseBookmarkEntity.singleEntity(with: newParentUUID)
-
-                if let fetchedParent = try context.fetch(bookmarksFetchRequest).first, fetchedParent.isFolder {
-                    newParentFolder = fetchedParent
-                } else {
-                    throw BookmarkStoreError.missingEntity
-                }
-            }
+            try move(entities: bookmarkManagedObjects, toIndex: index, withinParentFolderType: type, in: context)
 
-            if let index = index, index < newParentFolder.childrenArray.count {
-                self.move(entities: bookmarkManagedObjects, to: index, within: newParentFolder)
-            } else {
-                for bookmarkManagedObject in bookmarkManagedObjects {
-                    bookmarkManagedObject.parent = nil
-                    newParentFolder.addToChildren(bookmarkManagedObject)
-                }
-            }
         }, onError: { [weak self] error in
             self?.commonOnSaveErrorHandler(error)
             DispatchQueue.main.async { completion(error) }
@@ -996,6 +975,53 @@ final class LocalBookmarkStore: BookmarkStore {
 
 }
 
+private extension LocalBookmarkStore {
+
+    @discardableResult
+    func update(folder: BookmarkFolder, in context: NSManagedObjectContext) throws -> BookmarkEntity {
+        let folderFetchRequest = BaseBookmarkEntity.singleEntity(with: folder.id)
+        let folderFetchRequestResults = try? context.fetch(folderFetchRequest)
+
+        guard let bookmarkFolderMO = folderFetchRequestResults?.first else {
+            assertionFailure("LocalBookmarkStore: Failed to get BookmarkEntity from the context")
+            throw BookmarkStoreError.missingEntity
+        }
+
+        bookmarkFolderMO.update(with: folder)
+        return bookmarkFolderMO
+    }
+
+    func move(entities: [BookmarkEntity], toIndex index: Int?, withinParentFolderType type: ParentFolderType, in context: NSManagedObjectContext) throws {
+        guard let rootFolder = bookmarksRoot(in: context) else {
+            throw BookmarkStoreError.missingRoot
+        }
+
+        let newParentFolder: BookmarkEntity
+
+        switch type {
+        case .root: newParentFolder = rootFolder
+        case .parent(let newParentUUID):
+            let bookmarksFetchRequest = BaseBookmarkEntity.singleEntity(with: newParentUUID)
+
+            if let fetchedParent = try context.fetch(bookmarksFetchRequest).first, fetchedParent.isFolder {
+                newParentFolder = fetchedParent
+            } else {
+                throw BookmarkStoreError.missingEntity
+            }
+        }
+
+        if let index = index, index < newParentFolder.childrenArray.count {
+            self.move(entities: entities, to: index, within: newParentFolder)
+        } else {
+            for bookmarkManagedObject in entities {
+                bookmarkManagedObject.parent = nil
+                newParentFolder.addToChildren(bookmarkManagedObject)
+            }
+        }
+    }
+
+}
+
 extension LocalBookmarkStore.BookmarkStoreError: CustomNSError {
 
     var errorCode: Int {

DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkFolderDialogViewModel.swift

@@ -84,6 +84,8 @@ final class AddEditBookmarkFolderDialogViewModel: ObservableObject {
     }
 
     func addOrSave(dismiss: () -> Void) {
+        defer { dismiss() }
+
         guard !folderName.isEmpty else {
             assertionFailure("folderName is empty, button should be disabled")
             return
@@ -93,12 +95,13 @@ final class AddEditBookmarkFolderDialogViewModel: ObservableObject {
 
         switch mode {
         case let .edit(folder, originalParent):
-            update(folder: folder, parent: originalParent)
+            // If there are no pending changes dismiss
+            guard folder.title != folderName || selectedFolder?.id != originalParent?.id else { return }
+            // Otherwise update Folder.
+            update(folder: folder, originalParent: originalParent, newParent: selectedFolder)
         case .add:
             add(folderWithName: folderName, to: selectedFolder)
         }
-
-        dismiss()
     }
 
 }
@@ -107,22 +110,17 @@ final class AddEditBookmarkFolderDialogViewModel: ObservableObject {
 
 private extension AddEditBookmarkFolderDialogViewModel {
 
-    func update(folder: BookmarkFolder, parent: BookmarkFolder?) {
-        // Update the title of the folder
-        if folder.title != folderName {
+    func update(folder: BookmarkFolder, originalParent: BookmarkFolder?, newParent: BookmarkFolder?) {
+        // If the original location of the folder changed move it to the new folder.
+        if selectedFolder?.id != originalParent?.id {
+            // Update the title anyway.
+            folder.title = folderName
+            let parentFolderType: ParentFolderType = newParent.flatMap { ParentFolderType.parent(uuid: $0.id) } ?? .root
+            bookmarkManager.update(folder: folder, andMoveToParent: parentFolderType)
+        } else if folder.title != folderName { // If only title changed just update the folder title without updating its parent.
             folder.title = folderName
             bookmarkManager.update(folder: folder)
         }
-        // If the original location of the folder changed move it to the new folder.
-        if selectedFolder?.id != parent?.id {
-            let parentFolderType: ParentFolderType = selectedFolder.flatMap { ParentFolderType.parent(uuid: $0.id) } ?? .root
-            bookmarkManager.move(
-                objectUUIDs: [folder.id],
-                toIndex: nil,
-                withinParentFolder: parentFolderType,
-                completion: { _ in }
-            )
-        }
     }
 
     func add(folderWithName name: String, to parent: BookmarkFolder?) {

UnitTests/Bookmarks/Model/LocalBookmarkManagerTests.swift

@@ -16,6 +16,7 @@
 //  limitations under the License.
 //
 
+import Combine
 import Foundation
 
 import XCTest
@@ -157,6 +158,26 @@ final class LocalBookmarkManagerTests: XCTestCase {
         XCTAssert(bookmarkStoreMock.updateBookmarkCalled)
     }
 
+    func testWhenBookmarkFolderIsUpdatedAndMoved_ThenManagerUpdatesItAlsoInStore() throws {
+        let (bookmarkManager, bookmarkStoreMock) = LocalBookmarkManager.aManager
+        let parent = BookmarkFolder(id: "1", title: "Parent")
+        let folder = BookmarkFolder(id: "2", title: "Child")
+        var bookmarkList: BookmarkList?
+        let cancellable = bookmarkManager.listPublisher
+            .dropFirst()
+            .sink { list in
+            bookmarkList = list
+        }
+
+        bookmarkManager.update(folder: folder, andMoveToParent: .parent(uuid: parent.id))
+
+        let topLevelEntities = try XCTUnwrap(bookmarkList?.topLevelEntities)
+        XCTAssertTrue(bookmarkStoreMock.updateFolderAndMoveToParentCalled)
+        XCTAssertEqual(bookmarkStoreMock.capturedFolder, folder)
+        XCTAssertEqual(bookmarkStoreMock.capturedParentFolderType, .parent(uuid: parent.id))
+        XCTAssertNotNil(bookmarkList)
+    }
+
 }
 
 fileprivate extension LocalBookmarkManager {

UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift

@@ -468,6 +468,45 @@ final class LocalBookmarkStoreTests: XCTestCase {
         XCTAssertEqual(topLevelEntityIDs, [testState.initialParentFolder.id, testState.bookmark3.id])
     }
 
+    func testWhenUpdatingAndMovingBookmarkFolder_ThenBookmarkFolderIsMovedAndTitleUpdated() async throws {
+        let context = container.viewContext
+        let bookmarkStore = LocalBookmarkStore(context: context)
+
+        let folder1 = BookmarkFolder(id: UUID().uuidString, title: "Folder 1")
+        let folder2 = BookmarkFolder(id: UUID().uuidString, title: "Folder 2")
+        let folder3 = BookmarkFolder(id: UUID().uuidString, title: "Folder 3")
+
+        // Save the initial bookmarks state:
+
+        _ = await bookmarkStore.save(folder: folder1, parent: nil)
+        _ = await bookmarkStore.save(folder: folder2, parent: nil)
+        _ = await bookmarkStore.save(folder: folder3, parent: nil)
+
+        // Fetch persisted bookmark folders back from the store:
+
+        let folders = try await bookmarkStore.loadAll(type: .topLevelEntities).get().compactMap { $0 as? BookmarkFolder }
+
+        XCTAssertEqual(folders.count, 3)
+        XCTAssertEqual(folders[0], folder1)
+        XCTAssertEqual(folders[1], folder2)
+        XCTAssertEqual(folders[2], folder3)
+
+        // Update the folder title and parent:
+
+        var folderToMove = folder1
+        folderToMove.title = #function
+        bookmarkStore.update(folder: folder1, andMoveToParent: .parent(uuid: folder2.id))
+
+        // Check the new bookmark folders order:
+
+        let newFolders = try await bookmarkStore.loadAll(type: .topLevelEntities).get().compactMap { $0 as? BookmarkFolder }
+
+        XCTAssertEqual(newFolders.count, 2)
+        XCTAssertEqual(newFolders[0].id, folder2.id)
+        XCTAssertEqual(newFolders[0].children, [folder1])
+        XCTAssertEqual(newFolders[1], folder3)
+    }
+
     // MARK: Favorites
 
     func testThatTopLevelEntitiesDoNotContainFavoritesFolder() async {

UnitTests/Bookmarks/ViewModels/AddEditBookmarkFolderDialogViewModelTests.swift

@@ -369,17 +369,17 @@ final class AddEditBookmarkFolderDialogViewModelTests: XCTestCase {
         let folder = BookmarkFolder.mock
         let sut = AddEditBookmarkFolderDialogViewModel(mode: .edit(folder: folder, parentFolder: nil), bookmarkManager: bookmarkManager)
         sut.selectedFolder = location
-        XCTAssertFalse(bookmarkStoreMock.moveObjectUUIDCalled)
-        XCTAssertNil(bookmarkStoreMock.capturedObjectUUIDs)
+        XCTAssertFalse(bookmarkStoreMock.updateFolderAndMoveToParentCalled)
+        XCTAssertNil(bookmarkStoreMock.capturedFolder)
         XCTAssertNil(bookmarkStoreMock.capturedParentFolderType)
 
         // WHEN
         sut.addOrSave {}
 
         // THEN
         XCTAssertFalse(bookmarkStoreMock.saveFolderCalled)
-        XCTAssertTrue(bookmarkStoreMock.moveObjectUUIDCalled)
-        XCTAssertEqual(bookmarkStoreMock.capturedObjectUUIDs, [folder.id])
+        XCTAssertTrue(bookmarkStoreMock.updateFolderAndMoveToParentCalled)
+        XCTAssertEqual(bookmarkStoreMock.capturedFolder, folder)
         XCTAssertEqual(bookmarkStoreMock.capturedParentFolderType, .parent(uuid: #file))
     }
 
@@ -389,17 +389,17 @@ final class AddEditBookmarkFolderDialogViewModelTests: XCTestCase {
         let folder = BookmarkFolder.mock
         let sut = AddEditBookmarkFolderDialogViewModel(mode: .edit(folder: folder, parentFolder: .mock), bookmarkManager: bookmarkManager)
         sut.selectedFolder = nil
-        XCTAssertFalse(bookmarkStoreMock.moveObjectUUIDCalled)
-        XCTAssertNil(bookmarkStoreMock.capturedObjectUUIDs)
+        XCTAssertFalse(bookmarkStoreMock.updateFolderAndMoveToParentCalled)
+        XCTAssertNil(bookmarkStoreMock.capturedFolder)
         XCTAssertNil(bookmarkStoreMock.capturedParentFolderType)
 
         // WHEN
         sut.addOrSave {}
 
         // THEN
         XCTAssertFalse(bookmarkStoreMock.saveFolderCalled)
-        XCTAssertTrue(bookmarkStoreMock.moveObjectUUIDCalled)
-        XCTAssertEqual(bookmarkStoreMock.capturedObjectUUIDs, [folder.id])
+        XCTAssertTrue(bookmarkStoreMock.updateFolderAndMoveToParentCalled)
+        XCTAssertEqual(bookmarkStoreMock.capturedFolder, folder)
         XCTAssertEqual(bookmarkStoreMock.capturedParentFolderType, .root)
     }
 

UnitTests/HomePage/Mocks/MockBookmarkManager.swift

@@ -61,6 +61,8 @@ class MockBookmarkManager: BookmarkManager {
 
     func update(folder: DuckDuckGo_Privacy_Browser.BookmarkFolder) {}
 
+    func update(folder: DuckDuckGo_Privacy_Browser.BookmarkFolder, andMoveToParent parent: DuckDuckGo_Privacy_Browser.ParentFolderType) {}
+
     func updateUrl(of bookmark: DuckDuckGo_Privacy_Browser.Bookmark, to newUrl: URL) -> DuckDuckGo_Privacy_Browser.Bookmark? {
         return nil
     }