diff --git a/DuckDuckGo/Bookmarks/Model/BookmarkManager.swift b/DuckDuckGo/Bookmarks/Model/BookmarkManager.swift index ee19960da1..7e249c3b48 100644 --- a/DuckDuckGo/Bookmarks/Model/BookmarkManager.swift +++ b/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 { diff --git a/DuckDuckGo/Bookmarks/Services/BookmarkStore.swift b/DuckDuckGo/Bookmarks/Services/BookmarkStore.swift index 6b7981c96a..3466d1a7fa 100644 --- a/DuckDuckGo/Bookmarks/Services/BookmarkStore.swift +++ b/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 diff --git a/DuckDuckGo/Bookmarks/Services/BookmarkStoreMock.swift b/DuckDuckGo/Bookmarks/Services/BookmarkStoreMock.swift index d67a877bfc..d098e434e8 100644 --- a/DuckDuckGo/Bookmarks/Services/BookmarkStoreMock.swift +++ b/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 diff --git a/DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift b/DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift index 307d16cc5d..166eb51c02 100644 --- a/DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift +++ b/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,10 +569,6 @@ 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 @@ -577,28 +576,8 @@ final class LocalBookmarkStore: BookmarkStore { 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 { diff --git a/DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkFolderDialogViewModel.swift b/DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkFolderDialogViewModel.swift index 18ac4b30c3..a1da3c5262 100644 --- a/DuckDuckGo/Bookmarks/ViewModel/AddEditBookmarkFolderDialogViewModel.swift +++ b/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?) { diff --git a/UnitTests/Bookmarks/Model/LocalBookmarkManagerTests.swift b/UnitTests/Bookmarks/Model/LocalBookmarkManagerTests.swift index b82636a7c5..96e1c7d4e3 100644 --- a/UnitTests/Bookmarks/Model/LocalBookmarkManagerTests.swift +++ b/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 { diff --git a/UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift b/UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift index b165b87d16..9e7cc1b169 100644 --- a/UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift +++ b/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 { diff --git a/UnitTests/Bookmarks/ViewModels/AddEditBookmarkFolderDialogViewModelTests.swift b/UnitTests/Bookmarks/ViewModels/AddEditBookmarkFolderDialogViewModelTests.swift index 13f0358bd1..135fc69cbf 100644 --- a/UnitTests/Bookmarks/ViewModels/AddEditBookmarkFolderDialogViewModelTests.swift +++ b/UnitTests/Bookmarks/ViewModels/AddEditBookmarkFolderDialogViewModelTests.swift @@ -369,8 +369,8 @@ 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 @@ -378,8 +378,8 @@ final class AddEditBookmarkFolderDialogViewModelTests: XCTestCase { // 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,8 +389,8 @@ 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 @@ -398,8 +398,8 @@ final class AddEditBookmarkFolderDialogViewModelTests: XCTestCase { // THEN XCTAssertFalse(bookmarkStoreMock.saveFolderCalled) - XCTAssertTrue(bookmarkStoreMock.moveObjectUUIDCalled) - XCTAssertEqual(bookmarkStoreMock.capturedObjectUUIDs, [folder.id]) + XCTAssertTrue(bookmarkStoreMock.updateFolderAndMoveToParentCalled) + XCTAssertEqual(bookmarkStoreMock.capturedFolder, folder) XCTAssertEqual(bookmarkStoreMock.capturedParentFolderType, .root) } diff --git a/UnitTests/HomePage/Mocks/MockBookmarkManager.swift b/UnitTests/HomePage/Mocks/MockBookmarkManager.swift index c6a0840629..0d719e1a87 100644 --- a/UnitTests/HomePage/Mocks/MockBookmarkManager.swift +++ b/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 }