Skip to content

Commit 504ee64

Browse files
committed
Add new unit test for removing bookmarks to ensure bookmarks panel VM backing arrays are updated.
Consolidate 2 bookmarks handler mocks into one. Fix retain issue with the quick actions extension default implementation by making it a static method.
1 parent 9198732 commit 504ee64

7 files changed

Lines changed: 188 additions & 102 deletions

File tree

firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2360,7 +2360,7 @@ class BrowserViewController: UIViewController,
23602360
// FXIOS-13228 It should be safe to assumeIsolated here because of `.main` queue above
23612361
MainActor.assumeIsolated {
23622362
guard result.isSuccess else { return }
2363-
self.removeBookmarkShortcut()
2363+
Self.removeBookmarkShortcut(withBookmarksHandler: self.bookmarksHandler)
23642364
}
23652365
}
23662366
}

firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1102,7 +1102,7 @@ final class TabManagerMiddleware: FeatureFlaggable,
11021102
// FXIOS-13228 It should be safe to assumeIsolated here because of `.main` queue above
11031103
MainActor.assumeIsolated {
11041104
guard result.isSuccess else { return }
1105-
self.removeBookmarkShortcut()
1105+
Self.removeBookmarkShortcut(withBookmarksHandler: self.bookmarksHandler)
11061106
}
11071107
}
11081108
}

firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ final class BookmarksPanelViewModel: BookmarksPanelViewModelProtocol {
8080

8181
var bookmarksHandler: BookmarksHandler
8282
private var bookmarksSaver: BookmarksSaver
83+
private var quickActions: QuickActions
8384
private var hasDesktopFolders = false
8485
private var flashLastRowOnNextReload = false
8586
private let logger: Logger
@@ -89,11 +90,13 @@ final class BookmarksPanelViewModel: BookmarksPanelViewModelProtocol {
8990
bookmarksHandler: BookmarksHandler,
9091
bookmarkFolderGUID: GUID = BookmarkRoots.MobileFolderGUID,
9192
bookmarksSaver: BookmarksSaver? = nil,
93+
quickActions: QuickActions = QuickActionsImplementation(),
9294
logger: Logger = DefaultLogger.shared) {
9395
self.profile = profile
9496
self.bookmarksHandler = bookmarksHandler
9597
self.bookmarkFolderGUID = bookmarkFolderGUID
9698
self.bookmarksSaver = bookmarksSaver ?? DefaultBookmarksSaver(profile: profile)
99+
self.quickActions = quickActions
97100
self.logger = logger
98101
}
99102

@@ -441,14 +444,18 @@ final class BookmarksPanelViewModel: BookmarksPanelViewModelProtocol {
441444
self.reloadData {
442445
completion()
443446
}
447+
} else {
448+
completion()
444449
}
445450
}
446451
// Remove the bookmark from places (async background work)
447-
profile.places.deleteBookmarkNode(guid: bookmark.guid).uponQueue(.main) { _ in
452+
bookmarksHandler.deleteBookmarkNode(guid: bookmark.guid).uponQueue(DispatchQueue.main) { _ in
448453
// FXIOS-13228 It should be safe to assumeIsolated here because of `.main` queue above
449-
MainActor.assumeIsolated {
454+
MainActor.assumeIsolated { [weak self] in
455+
guard let self else { return }
456+
450457
// Remove this bookmark from quick actions
451-
self.removeBookmarkShortcut()
458+
Self.removeBookmarkShortcut(withBookmarksHandler: self.bookmarksHandler, withQuickActions: self.quickActions)
452459

453460
// Remove this bookmark out of recent places
454461
if let recentBookmarkFolderGuid = self.profile.prefs.stringForKey(PrefsKeys.RecentBookmarkFolder) {

firefox-ios/Client/Protocols/CanRemoveQuickActionBookmark.swift

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,35 +13,35 @@ protocol CanRemoveQuickActionBookmark: Sendable {
1313
var bookmarksHandler: BookmarksHandler { get }
1414

1515
@MainActor
16-
func removeBookmarkShortcut(quickAction: QuickActions)
16+
static func removeBookmarkShortcut(
17+
withBookmarksHandler bookmarksHandler: BookmarksHandler,
18+
withQuickActions quickActions: QuickActions
19+
)
1720
}
1821

1922
// Extension to easily remove a bookmark from the quick actions
2023
extension CanRemoveQuickActionBookmark {
2124
@MainActor
22-
func removeBookmarkShortcut(quickAction: QuickActions = QuickActionsImplementation()) {
25+
static func removeBookmarkShortcut(
26+
withBookmarksHandler bookmarksHandler: BookmarksHandler,
27+
withQuickActions quickActions: QuickActions = QuickActionsImplementation()
28+
) {
2329
// Get most recent bookmark
2430
bookmarksHandler.getRecentBookmarks(limit: 1) { bookmarkItems in
2531
ensureMainThread {
26-
self.removeBookmarks(quickAction: quickAction,
27-
bookmarkItems: bookmarkItems)
32+
if bookmarkItems.isEmpty {
33+
// Remove the openLastBookmark shortcut
34+
quickActions.removeDynamicApplicationShortcutItemOfType(.openLastBookmark,
35+
fromApplication: .shared)
36+
} else {
37+
// Update the last bookmark shortcut
38+
let userData = [QuickActionInfos.tabURLKey: bookmarkItems[0].url,
39+
QuickActionInfos.tabTitleKey: bookmarkItems[0].title]
40+
quickActions.addDynamicApplicationShortcutItemOfType(.openLastBookmark,
41+
withUserData: userData,
42+
toApplication: .shared)
43+
}
2844
}
2945
}
3046
}
31-
32-
@MainActor
33-
private func removeBookmarks(quickAction: QuickActions, bookmarkItems: [BookmarkItemData]) {
34-
if bookmarkItems.isEmpty {
35-
// Remove the openLastBookmark shortcut
36-
quickAction.removeDynamicApplicationShortcutItemOfType(.openLastBookmark,
37-
fromApplication: .shared)
38-
} else {
39-
// Update the last bookmark shortcut
40-
let userData = [QuickActionInfos.tabURLKey: bookmarkItems[0].url,
41-
QuickActionInfos.tabTitleKey: bookmarkItems[0].title]
42-
quickAction.addDynamicApplicationShortcutItemOfType(.openLastBookmark,
43-
withUserData: userData,
44-
toApplication: .shared)
45-
}
46-
}
4747
}

firefox-ios/firefox-ios-tests/Tests/ClientTests/CanRemoveQuickActionBookmarkTests.swift

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,11 @@ class CanRemoveQuickActionBookmarkTests: XCTestCase {
2929

3030
@MainActor
3131
func testWithoutBookmarks() {
32-
subject.removeBookmarkShortcut(quickAction: mockQuickActions)
33-
mockBookmarksHandler.callGetRecentBookmarksCompletion(with: [])
32+
mockBookmarksHandler.getRecentBookmarksResult = []
33+
MockCanRemoveQuickActionBookmark.removeBookmarkShortcut(
34+
withBookmarksHandler: mockBookmarksHandler,
35+
withQuickActions: mockQuickActions
36+
)
3437

3538
XCTAssertEqual(mockBookmarksHandler.getRecentBookmarksCallCount, 1)
3639
XCTAssertEqual(mockQuickActions.removeWasCalled, 1)
@@ -40,8 +43,11 @@ class CanRemoveQuickActionBookmarkTests: XCTestCase {
4043

4144
@MainActor
4245
func testWithBookmarks() {
43-
subject.removeBookmarkShortcut(quickAction: mockQuickActions)
44-
mockBookmarksHandler.callGetRecentBookmarksCompletion(with: getMockBookmarks())
46+
mockBookmarksHandler.getRecentBookmarksResult = getMockBookmarks()
47+
MockCanRemoveQuickActionBookmark.removeBookmarkShortcut(
48+
withBookmarksHandler: mockBookmarksHandler,
49+
withQuickActions: mockQuickActions
50+
)
4551

4652
XCTAssertEqual(mockBookmarksHandler.getRecentBookmarksCallCount, 1)
4753
XCTAssertEqual(mockQuickActions.removeWasCalled, 0)

0 commit comments

Comments
 (0)