Bug 1965832 - Add a `checkAtLeastOneChild` method and inject bookmarksStorage to improve testability of menu item creation. r=android-reviewers,pollymce
authormcarare <48995920+mcarare@users.noreply.github.com>
Mon, 19 May 2025 09:48:31 +0000 (8 weeks ago)
changeset 787735 59c36992320d5551fc6c56ef31676d66b876e08a
parent 787734 1452a3f37b97d281146ee51cb9eebc544fac8dd1
child 787736 806d5c9f9237d2cfac2a4e11f514be1aad549c0c
push id231773
push usermcarare@mozilla.com
push dateMon, 19 May 2025 09:51:37 +0000 (8 weeks ago)
treeherderautoland@59c36992320d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
git commit7f21993b1054179a5d046c545794f79263a21f2d
reviewersandroid-reviewers, pollymce
bugs1965832
milestone140.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1965832 - Add a `checkAtLeastOneChild` method and inject bookmarksStorage to improve testability of menu item creation. r=android-reviewers,pollymce This commit also refactors the code generating the bookmark item menu to be more readable: - Replace `listOfNotNull` with `buildList` for creating the menu options. - Introduce helper variables for each menu option. Differential Revision: https://phabricator.services.mozilla.com/D248936
mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt
mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt
mobile/android/fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt
--- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt
+++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenu.kt
@@ -6,22 +6,23 @@ package org.mozilla.fenix.library.bookma
 
 import android.content.Context
 import androidx.annotation.VisibleForTesting
 import mozilla.components.browser.menu2.BrowserMenuController
 import mozilla.components.concept.menu.MenuController
 import mozilla.components.concept.menu.candidate.TextMenuCandidate
 import mozilla.components.concept.menu.candidate.TextStyle
 import mozilla.components.concept.storage.BookmarkNodeType
+import mozilla.components.concept.storage.BookmarksStorage
 import mozilla.components.support.ktx.android.content.getColorFromAttr
 import org.mozilla.fenix.R
-import org.mozilla.fenix.ext.bookmarkStorage
 
 class BookmarkItemMenu(
     private val context: Context,
+    private val bookmarkStorage: BookmarksStorage,
     var onItemTapped: ((Item) -> Unit)? = null,
 ) {
 
     enum class Item {
         Edit,
         Copy,
         Share,
         OpenInNewTab,
@@ -29,92 +30,92 @@ class BookmarkItemMenu(
         OpenAllInNewTabs,
         OpenAllInPrivateTabs,
         Delete,
     }
 
     val menuController: MenuController by lazy { BrowserMenuController() }
 
     @VisibleForTesting
-    @SuppressWarnings("LongMethod")
     internal suspend fun menuItems(itemType: BookmarkNodeType, itemId: String): List<TextMenuCandidate> {
-        val hasAtLeastOneChild = !context.bookmarkStorage.getTree(itemId, false)?.children.isNullOrEmpty()
+        val editMenuOption =
+            TextMenuCandidate(text = context.getString(R.string.bookmark_menu_edit_button)) {
+                onItemTapped?.invoke(Item.Edit)
+            }
 
-        return listOfNotNull(
-            if (itemType != BookmarkNodeType.SEPARATOR) {
-                TextMenuCandidate(
-                    text = context.getString(R.string.bookmark_menu_edit_button),
-                ) {
-                    onItemTapped?.invoke(Item.Edit)
-                }
-            } else {
-                null
-            },
-            if (itemType == BookmarkNodeType.ITEM) {
-                TextMenuCandidate(
-                    text = context.getString(R.string.bookmark_menu_copy_button),
-                ) {
-                    onItemTapped?.invoke(Item.Copy)
-                }
-            } else {
-                null
-            },
-            if (itemType == BookmarkNodeType.ITEM) {
-                TextMenuCandidate(
-                    text = context.getString(R.string.bookmark_menu_share_button),
-                ) {
-                    onItemTapped?.invoke(Item.Share)
-                }
-            } else {
-                null
-            },
-            if (itemType == BookmarkNodeType.ITEM) {
-                TextMenuCandidate(
-                    text = context.getString(R.string.bookmark_menu_open_in_new_tab_button),
-                ) {
-                    onItemTapped?.invoke(Item.OpenInNewTab)
-                }
-            } else {
-                null
-            },
-            if (itemType == BookmarkNodeType.ITEM) {
-                TextMenuCandidate(
-                    text = context.getString(R.string.bookmark_menu_open_in_private_tab_button),
-                ) {
-                    onItemTapped?.invoke(Item.OpenInPrivateTab)
-                }
-            } else {
-                null
-            },
-            if (hasAtLeastOneChild && itemType == BookmarkNodeType.FOLDER) {
-                TextMenuCandidate(
-                    text = context.getString(R.string.bookmark_menu_open_all_in_tabs_button),
-                ) {
-                    onItemTapped?.invoke(Item.OpenAllInNewTabs)
-                }
-            } else {
-                null
-            },
-            if (hasAtLeastOneChild && itemType == BookmarkNodeType.FOLDER) {
-                TextMenuCandidate(
-                    text = context.getString(R.string.bookmark_menu_open_all_in_private_tabs_button),
-                ) {
-                    onItemTapped?.invoke(Item.OpenAllInPrivateTabs)
-                }
-            } else {
-                null
-            },
+        val copyMenuOption =
+            TextMenuCandidate(text = context.getString(R.string.bookmark_menu_copy_button)) {
+                onItemTapped?.invoke(Item.Copy)
+            }
+
+        val deleteMenuOption =
             TextMenuCandidate(
                 text = context.getString(R.string.bookmark_menu_delete_button),
                 textStyle = TextStyle(color = context.getColorFromAttr(R.attr.textCritical)),
             ) {
                 onItemTapped?.invoke(Item.Delete)
-            },
-        )
+            }
+
+        val shareMenuOption =
+            TextMenuCandidate(text = context.getString(R.string.bookmark_menu_share_button)) {
+                onItemTapped?.invoke(Item.Share)
+            }
+
+        val openInNewTabMenuOption =
+            TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_in_new_tab_button)) {
+                onItemTapped?.invoke(Item.OpenInNewTab)
+            }
+
+        val openInPrivateTabMenuOption =
+            TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_in_private_tab_button)) {
+                onItemTapped?.invoke(Item.OpenInPrivateTab)
+            }
+
+        val openAllInNewTabsMenuOption =
+            TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_all_in_tabs_button)) {
+                onItemTapped?.invoke(Item.OpenAllInNewTabs)
+            }
+
+        val openAllInPrivateTabsMenuOption =
+            TextMenuCandidate(text = context.getString(R.string.bookmark_menu_open_all_in_private_tabs_button)) {
+                onItemTapped?.invoke(Item.OpenAllInPrivateTabs)
+            }
+
+        return buildList {
+            if (itemType != BookmarkNodeType.SEPARATOR) {
+                add(editMenuOption)
+            }
+            if (itemType == BookmarkNodeType.ITEM) {
+                add(copyMenuOption)
+                add(shareMenuOption)
+                add(openInNewTabMenuOption)
+                add(openInPrivateTabMenuOption)
+            }
+
+            if (itemType == BookmarkNodeType.FOLDER && checkAtLeastOneChild(itemId)) {
+                add(openAllInNewTabsMenuOption)
+                add(openAllInPrivateTabsMenuOption)
+            }
+
+            add(deleteMenuOption)
+        }
     }
 
     /**
      * Update the menu items for the type of bookmark.
      */
     suspend fun updateMenu(itemType: BookmarkNodeType, itemId: String) {
         menuController.submitList(menuItems(itemType, itemId))
     }
+
+    /**
+     * Checks if a bookmark item has at least one child.
+     *
+     * @param itemId The ID of the bookmark item to check.
+     * @return `true` if the item has at least one child, `false` otherwise.
+     */
+    @VisibleForTesting
+    internal suspend fun checkAtLeastOneChild(itemId: String): Boolean =
+        bookmarkStorage.getTree(
+            itemId,
+            false,
+        )?.children?.isNotEmpty() == true
 }
--- a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt
+++ b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/viewholders/BookmarkNodeViewHolder.kt
@@ -7,16 +7,17 @@ package org.mozilla.fenix.library.bookma
 import androidx.core.view.isVisible
 import androidx.recyclerview.widget.RecyclerView
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
 import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
 import org.mozilla.fenix.R
+import org.mozilla.fenix.ext.bookmarkStorage
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.hideAndDisable
 import org.mozilla.fenix.ext.loadIntoView
 import org.mozilla.fenix.ext.removeAndDisable
 import org.mozilla.fenix.ext.showAndEnable
 import org.mozilla.fenix.library.LibrarySiteItemView
 import org.mozilla.fenix.library.bookmarks.BookmarkFragmentState
 import org.mozilla.fenix.library.bookmarks.BookmarkItemMenu
@@ -27,17 +28,20 @@ import org.mozilla.fenix.library.bookmar
 /**
  * Base class for bookmark node view holders.
  */
 class BookmarkNodeViewHolder(
     private val containerView: LibrarySiteItemView,
     private val interactor: BookmarkViewInteractor,
 ) : RecyclerView.ViewHolder(containerView) {
 
-    private val menu = BookmarkItemMenu(containerView.context)
+    private val menu = BookmarkItemMenu(
+        containerView.context,
+        containerView.context.bookmarkStorage,
+    )
 
     /**
      * Binds the view holder to the item
      */
     fun bind(
         item: BookmarkNode,
         mode: BookmarkFragmentState.Mode,
         payload: BookmarkPayload,
--- a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt
+++ b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/library/bookmarks/BookmarkItemMenuTest.kt
@@ -2,93 +2,90 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.fenix.library.bookmarks
 
 import android.content.Context
 import androidx.appcompat.view.ContextThemeWrapper
 import io.mockk.coEvery
-import io.mockk.every
+import io.mockk.coVerify
 import io.mockk.mockk
-import io.mockk.mockkStatic
-import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.test.runTest
 import mozilla.components.concept.menu.candidate.TextMenuCandidate
 import mozilla.components.concept.menu.candidate.TextStyle
+import mozilla.components.concept.storage.BookmarkNode
 import mozilla.components.concept.storage.BookmarkNodeType
+import mozilla.components.concept.storage.BookmarksStorage
 import mozilla.components.support.ktx.android.content.getColorFromAttr
 import mozilla.components.support.test.robolectric.testContext
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNotNull
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.mozilla.fenix.R
-import org.mozilla.fenix.ext.bookmarkStorage
 import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
 import org.mozilla.fenix.library.bookmarks.BookmarkItemMenu.Item
 
 @RunWith(FenixRobolectricTestRunner::class)
 class BookmarkItemMenuTest {
 
     private lateinit var context: Context
     private lateinit var menu: BookmarkItemMenu
+    private lateinit var bookmarksStorage: BookmarksStorage
+
     private var lastItemTapped: Item? = null
+    private val item = BookmarkNode(BookmarkNodeType.ITEM, "456", "123", 0u, "Mozilla", "http://mozilla.org", 0, 0, null)
+    private val emptyFolder = BookmarkNode(BookmarkNodeType.FOLDER, "987", "123", 0u, "Subfolder", null, 0, 0, emptyList())
+    private val notEmptyFolder = BookmarkNode(BookmarkNodeType.FOLDER, "987", "123", 0u, "Subfolder", null, 0, 0, listOf(item))
 
     @Before
     fun setup() {
         context = ContextThemeWrapper(testContext, R.style.NormalTheme)
-        menu = BookmarkItemMenu(context) {
-            lastItemTapped = it
-        }
+        bookmarksStorage = mockk()
+        menu = BookmarkItemMenu(context, bookmarksStorage) { lastItemTapped = it }
     }
 
     @Test
-    fun `delete item has special styling`() = runBlocking {
-        var deleteItem: TextMenuCandidate? = null
-        mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") {
-            every { any<Context>().bookmarkStorage } returns mockk(relaxed = true)
+    fun `delete item has special styling`() = runTest {
+        val deleteItem = menu.menuItems(BookmarkNodeType.SEPARATOR, "").last()
 
-            deleteItem = menu.menuItems(BookmarkNodeType.SEPARATOR, "").last()
-        }
         assertNotNull(deleteItem)
-        assertEquals("Delete", deleteItem!!.text)
+        assertEquals("Delete", deleteItem.text)
         assertEquals(
             TextStyle(color = context.getColorFromAttr(R.attr.textCritical)),
-            deleteItem!!.textStyle,
+            deleteItem.textStyle,
         )
 
-        deleteItem!!.onClick()
+        deleteItem.onClick()
 
         assertEquals(Item.Delete, lastItemTapped)
     }
 
     @Test
-    fun `edit item appears for folders`() = runBlocking {
+    fun `edit item appears for folders`() = runTest {
         // empty folder
-        var emptyFolderItems: List<TextMenuCandidate>? = null
-        mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") {
-            every { any<Context>().bookmarkStorage } returns mockk(relaxed = true)
+        coEvery { bookmarksStorage.getTree(any()) } returns emptyFolder
 
-            emptyFolderItems = menu.menuItems(BookmarkNodeType.FOLDER, "")
-        }
+        var emptyFolderItems = menu.menuItems(BookmarkNodeType.FOLDER, "")
+
         assertNotNull(emptyFolderItems)
-        assertEquals(2, emptyFolderItems!!.size)
+        assertEquals(2, emptyFolderItems.size)
 
-        // not empty
+        // not empty folder
+        coEvery { bookmarksStorage.getTree(any()) } returns notEmptyFolder
         var folderItems: List<TextMenuCandidate>? = null
-        mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") {
-            coEvery { any<Context>().bookmarkStorage.getTree("")?.children } returns listOf(mockk(relaxed = true))
 
-            folderItems = menu.menuItems(BookmarkNodeType.FOLDER, "")
-        }
+        folderItems = menu.menuItems(BookmarkNodeType.FOLDER, "")
+
         assertNotNull(folderItems)
-        assertEquals(4, folderItems!!.size)
+        assertEquals(4, folderItems.size)
 
-        val (edit, openAll, openAllPrivate, delete) = folderItems!!
+        val (edit, openAll, openAllPrivate, delete) = folderItems
 
         assertEquals("Edit", edit.text)
         assertEquals("Open all in new tabs", openAll.text)
         assertEquals("Open all in private tabs", openAllPrivate.text)
         assertEquals("Delete", delete.text)
 
         edit.onClick()
         assertEquals(Item.Edit, lastItemTapped)
@@ -99,26 +96,22 @@ class BookmarkItemMenuTest {
         openAllPrivate.onClick()
         assertEquals(Item.OpenAllInPrivateTabs, lastItemTapped)
 
         delete.onClick()
         assertEquals(Item.Delete, lastItemTapped)
     }
 
     @Test
-    fun `all item appears for sites`() = runBlocking {
-        var siteItems: List<TextMenuCandidate>? = null
-        mockkStatic("org.mozilla.fenix.ext.BookmarkNodeKt") {
-            every { any<Context>().bookmarkStorage } returns mockk(relaxed = true)
+    fun `all item appears for sites`() = runTest {
+        val siteItems = menu.menuItems(BookmarkNodeType.ITEM, "")
 
-            siteItems = menu.menuItems(BookmarkNodeType.ITEM, "")
-        }
         assertNotNull(siteItems)
-        assertEquals(6, siteItems!!.size)
-        val (edit, copy, share, openInNewTab, openInPrivateTab, delete) = siteItems!!
+        assertEquals(6, siteItems.size)
+        val (edit, copy, share, openInNewTab, openInPrivateTab, delete) = siteItems
 
         assertEquals("Edit", edit.text)
         assertEquals("Copy", copy.text)
         assertEquals("Share", share.text)
         assertEquals("Open in new tab", openInNewTab.text)
         assertEquals("Open in private tab", openInPrivateTab.text)
         assertEquals("Delete", delete.text)
 
@@ -136,12 +129,26 @@ class BookmarkItemMenuTest {
 
         openInPrivateTab.onClick()
         assertEquals(Item.OpenInPrivateTab, lastItemTapped)
 
         delete.onClick()
         assertEquals(Item.Delete, lastItemTapped)
     }
 
+    @Test
+    fun `checkAtLeastOneChild is called only for FOLDER type`() = runTest {
+        coEvery { bookmarksStorage.getTree(any()) } returns emptyFolder
+
+        menu.menuItems(BookmarkNodeType.SEPARATOR, "")
+        coVerify(exactly = 0) { bookmarksStorage.getTree(any()) }
+
+        menu.menuItems(BookmarkNodeType.ITEM, "")
+        coVerify(exactly = 0) { bookmarksStorage.getTree(any()) }
+
+        menu.menuItems(BookmarkNodeType.FOLDER, "")
+        coVerify { bookmarksStorage.getTree(any()) }
+    }
+
     private operator fun <T> List<T>.component6(): T {
         return get(5)
     }
 }