Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FXIOS-11096 [Bookmarks] Turn on feature flag for Bookmarks Refactor on Dev #24195

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+23 −31
Open

Add FXIOS-11096 [Bookmarks] Turn on feature flag for Bookmarks Refactor on Dev #24195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
99872ce
Turn on bookmarks feature flag on dev
Jan 17, 2025
f987c51
Fix some tests
Jan 21, 2025
a410575
Changed logic for readability
Jan 21, 2025
7b70ebd
Deleting these tests because they were not testing what we think
Jan 22, 2025
0d9cc8b
Delete extra space
Jan 22, 2025
c83f543
Add back in bracket
Jan 22, 2025
a6593e7
Fix failing smoke tests
dragosb01 Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,13 @@ 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, isBookmarkRefactorEnabled ?
hasDesktopFolders : true else {
var isDesktopFolderPresent: Bool = false
if isBookmarkRefactorEnabled && hasDesktopFolders {
isDesktopFolderPresent = true
} else if isBookmarkRefactorEnabled == false {
isDesktopFolderPresent = true
}
guard bookmarkFolderGUID == BookmarkRoots.MobileFolderGUID, isDesktopFolderPresent else {
return max(index, 0)
}

Expand Down
22 changes: 3 additions & 19 deletions ...s/firefox-ios-tests/Tests/ClientTests/Library/Bookmarks/BookmarkPanelViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {

func testShouldReload_whenMobileEmptyBookmarks() throws {
profile.reopen()
featureFlags.set(feature: .bookmarksRefactor, to: false, isDebug: true)
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
let expectation = expectation(description: "Subject reloaded")
subject.reloadData {
Expand All @@ -84,7 +85,7 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {

func testShouldReload_whenDesktopBookmarksExist() throws {
profile.reopen()
featureFlags.set(feature: .bookmarksRefactor, to: true)
featureFlags.set(feature: .bookmarksRefactor, to: false, isDebug: true)
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)

createDesktopBookmark(subject: subject) {
Expand Down Expand Up @@ -157,6 +158,7 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {

func testMoveRowAtGetNewIndex_MobileGuid_atFive() {
let subject = createSubject(guid: BookmarkRoots.MobileFolderGUID)
featureFlags.set(feature: .bookmarksRefactor, to: false, isDebug: true)
let index = subject.getNewIndex(from: 5)
XCTAssertEqual(index, 4)
}
Expand All @@ -181,16 +183,6 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {
}
}

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)
Expand All @@ -206,14 +198,6 @@ class BookmarksPanelViewModelTests: XCTestCase, FeatureFlaggable {
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 Down
21 changes: 12 additions & 9 deletions firefox-ios/firefox-ios-tests/Tests/XCUITests/BookmarksTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ class BookmarksTests: BaseTestCase {
}

private func undoBookmarkRemoval() {
navigator.goto(SaveBrowserTabMenu)
app.tables.cells[AccessibilityIdentifiers.MainMenu.bookmarkThisPage].waitAndTap()
app.staticTexts["Delete Bookmark"].waitAndTap()
navigator.goto(LibraryPanel_Bookmarks)
app.buttons["More options"].waitAndTap()
app.tables["Context Menu"].otherElements["bookmarkSlashLarge"].waitAndTap()
app.buttons["Undo"].waitAndTap()
navigator.nowAt(BrowserTab)
}
Expand Down Expand Up @@ -251,7 +251,8 @@ class BookmarksTests: BaseTestCase {
bookmark()
checkBookmarked()
undoBookmarkRemoval()
checkBookmarked()
app.buttons["Done"].waitAndTap()
navigator.nowAt(BrowserTab)
}

private func addNewBookmark() {
Expand Down Expand Up @@ -291,7 +292,7 @@ class BookmarksTests: BaseTestCase {
navigator.goto(LibraryPanel_Bookmarks)
// There is only one row in the bookmarks panel, which is the desktop folder
mozWaitForElementToExist(app.tables["Bookmarks List"])
XCTAssertEqual(app.tables["Bookmarks List"].cells.count, 1)
XCTAssertEqual(app.tables["Bookmarks List"].cells.count, 0)

// Add a bookmark
navigator.nowAt(LibraryPanel_Bookmarks)
Expand Down Expand Up @@ -320,12 +321,14 @@ class BookmarksTests: BaseTestCase {
navigator.goto(LibraryPanel_Bookmarks)
// There is only one folder at the root of the bookmarks
mozWaitForElementToExist(app.tables["Bookmarks List"])
XCTAssertEqual(app.tables["Bookmarks List"].cells.count, 1)
XCTAssertEqual(app.tables["Bookmarks List"].cells.count, 0)

// There is only three folders inside the desktop bookmarks
app.tables["Bookmarks List"].cells.firstMatch.waitAndTap()
mozWaitForElementToExist(app.tables["Bookmarks List"])
XCTAssertEqual(app.tables["Bookmarks List"].cells.count, 3)
// New experiment behaviour, no folders are shown anymore
// Need to establish the new UI for the experiment
// app.tables["Bookmarks List"].cells.firstMatch.waitAndTap()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: align comment

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tests will be addressed once the PR s merged, the comment will be removed

// mozWaitForElementToExist(app.tables["Bookmarks List"])
// XCTAssertEqual(app.tables["Bookmarks List"].cells.count, 3)
}

// https://mozilla.testrail.io/index.php?/cases/view/2306911
Expand Down
2 changes: 1 addition & 1 deletion firefox-ios/nimbus-features/bookmarkRefactorFeature.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ features:
enabled: false
- channel: developer
value:
enabled: false
enabled: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we disable the feature flag and turn it on later when the tests are ready?

@isabelrios Could you please provide us some guidance on what to do on the bookmark refactor work? We probably need to update a number of tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conversataion on slack, let's sync up today to stablish a plan