Skip to content

Commit

Permalink
Bugfix FXIOS-10898 [Bookmarks Evolution] Inconsistent bookmarks order…
Browse files Browse the repository at this point in the history 
…ing (#24049)
  • Loading branch information
@MattLichtenstein
MattLichtenstein authored Jan 8, 2025
1 parent 90d18e3 commit d73683b
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 27 deletions.
9 changes: 7 additions & 2 deletions firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ class BookmarksPanelViewModel: BookmarksRefactorFeatureFlagProvider {
let bookmarkFolderGUID: GUID
var bookmarkFolder: FxBookmarkNode?
var bookmarkNodes = [FxBookmarkNode]()
private var hasDesktopFolders = false
private var bookmarksHandler: BookmarksHandler
private var flashLastRowOnNextReload = false
private var logger: Logger
Expand Down Expand Up @@ -90,8 +91,9 @@ class BookmarksPanelViewModel: BookmarksRefactorFeatureFlagProvider {
/// we need to account for this when saving bookmark index in A-S. This is done by subtracting
/// the Local Desktop Folder number of rows it takes to the actual index.
func getNewIndex(from index: Int) -> Int {
guard bookmarkFolderGUID == BookmarkRoots.MobileFolderGUID else {
return index
guard bookmarkFolderGUID == BookmarkRoots.MobileFolderGUID, isBookmarkRefactorEnabled ?
hasDesktopFolders : true else {
return max(index, 0)
}

// Ensure we don't return lower than 0
Expand Down Expand Up @@ -120,8 +122,11 @@ class BookmarksPanelViewModel: BookmarksRefactorFeatureFlagProvider {
switch result {
case .success(let bookmarkCount):
if bookmarkCount > 0 || !self.isBookmarkRefactorEnabled {
self.hasDesktopFolders = true
let desktopFolder = LocalDesktopFolder()
self.bookmarkNodes.insert(desktopFolder, at: 0)
} else {
self.hasDesktopFolders = false
}
case .failure(let error):
self.logger.log("Error counting bookmarks: \(error)", level: .debug, category: .library)
Expand Down
109 changes: 84 additions & 25 deletions ...s/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/BookmarkPanelViewModelTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,31 +87,10 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {
featureFlags.set(feature: .bookmarksRefactor, to: true)
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)

let expectation = expectation(description: "Subject reloaded")

profile.places.createBookmark(
parentGUID: BookmarkRoots.MenuFolderGUID,
url: "https://www.firefox.com",
title: "Firefox",
position: 0
).uponQueue(.main) { _ in
self.profile.places.countBookmarksInTrees(folderGuids: [BookmarkRoots.MenuFolderGUID]) { result in
switch result {
case .success(let bookmarkCount):
XCTAssertEqual(bookmarkCount, 1, "Menu folder contains one bookmark")

subject.reloadData {
XCTAssertNotNil(subject.bookmarkFolder)
XCTAssertEqual(subject.bookmarkNodes.count, 1, "Mobile folder contains the local desktop folder")
expectation.fulfill()
}
case .failure(let error):
XCTFail("Failed to count bookmarks: \(error)")
expectation.fulfill()
}
}
createDesktopBookmark(subject: subject) {
XCTAssertNotNil(subject.bookmarkFolder)
XCTAssertEqual(subject.bookmarkNodes.count, 1, "Mobile folder contains the local desktop folder")
}
waitForExpectations(timeout: 5)
}

func testShouldReload_whenLocalDesktopFolder() {
Expand Down Expand Up @@ -152,7 +131,7 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {

func testMoveRowAtGetNewIndex_NotMobileGuid_minusIndex() {
let subject = createSubject(guid: BookmarkRoots.MenuFolderGUID)
let expectedIndex = -1
let expectedIndex = 0
let index = subject.getNewIndex(from: expectedIndex)
XCTAssertEqual(index, expectedIndex)
}
Expand Down Expand Up @@ -181,6 +160,60 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {
let index = subject.getNewIndex(from: 5)
XCTAssertEqual(index, 4)
}

func testMoveRowAtGetNewIndex_MobileGuid_showingDesktopFolder_zeroIndex_bookmarksRefactor() {
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
featureFlags.set(feature: .bookmarksRefactor, to: true)

createDesktopBookmark(subject: subject) {
let index = subject.getNewIndex(from: 0)
XCTAssertEqual(index, 0)
}
}

func testMoveRowAtGetNewIndex_MobileGuid_showingDesktopFolder_minusIndex_bookmarksRefactor() {
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
featureFlags.set(feature: .bookmarksRefactor, to: true)

createDesktopBookmark(subject: subject) {
let index = subject.getNewIndex(from: -1)
XCTAssertEqual(index, 0)
}
}

func testMoveRowAtGetNewIndex_MobileGuid_showingDesktopFolder_atFive_bookmarksRefactor() {
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
featureFlags.set(feature: .bookmarksRefactor, to: true)

createDesktopBookmark(subject: subject) {
let index = subject.getNewIndex(from: 5)
XCTAssertEqual(index, 4)
}
}

func testMoveRowAtGetNewIndex_MobileGuid_hidingDesktopFolder_zeroIndex_bookmarksRefactor() {
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
featureFlags.set(feature: .bookmarksRefactor, to: true)

let index = subject.getNewIndex(from: 0)
XCTAssertEqual(index, 0)
}

func testMoveRowAtGetNewIndex_MobileGuid_hidingDesktopFolder_minusIndex_bookmarksRefactor() {
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
featureFlags.set(feature: .bookmarksRefactor, to: true)

let index = subject.getNewIndex(from: -1)
XCTAssertEqual(index, 0)
}

func testMoveRowAtGetNewIndex_MobileGuid_hidingDesktopFolder_atFive_bookmarksRefactor() {
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
featureFlags.set(feature: .bookmarksRefactor, to: true)

let index = subject.getNewIndex(from: 5)
XCTAssertEqual(index, 4)
}
}

extension BookmarksPanelViewModelTests {
Expand All @@ -200,6 +233,32 @@ extension BookmarksPanelViewModelTests {
}
return nodes
}

private func createDesktopBookmark(subject: BookmarksPanelViewModel, completion: @escaping () -> Void) {
let expectation = expectation(description: "Subject reloaded")

profile.places.createBookmark(
parentGUID: BookmarkRoots.MenuFolderGUID,
url: "https://www.firefox.com",
title: "Firefox",
position: 0
).uponQueue(.main) { _ in
self.profile.places.countBookmarksInTrees(folderGuids: [BookmarkRoots.MenuFolderGUID]) { result in
switch result {
case .success:
subject.reloadData {
completion()
expectation.fulfill()
}
case .failure(let error):
XCTFail("Failed to count bookmarks: \(error)")
expectation.fulfill()
}
}
}

waitForExpectations(timeout: 5)
}
}

class MockBookmarkNode: FxBookmarkNode {
Expand Down

0 comments on commit d73683b

Please sign in to comment.