[PM-18873] Refactor ItemHeader.kt to improve location display (#4814)

2026-03-21 13:52:07 -05:00 · 2025-03-07 12:02:15 -05:00
parent d19ab498ff
commit d03c6c243d
3 changed files with 162 additions and 40 deletions
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/component/ItemHeader.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/component/ItemHeader.kt
@@ -13,7 +13,6 @@ import androidx.compose.foundation.layout.size
 import androidx.compose.foundation.lazy.LazyColumn
 import androidx.compose.foundation.lazy.LazyItemScope
 import androidx.compose.foundation.lazy.LazyListScope
-import androidx.compose.foundation.lazy.items
 import androidx.compose.foundation.lazy.itemsIndexed
 import androidx.compose.material3.Icon
 import androidx.compose.material3.Text
@@ -47,11 +46,6 @@ import com.x8bit.bitwarden.ui.vault.feature.item.model.VaultItemLocation
 import kotlinx.collections.immutable.ImmutableList
 import kotlinx.collections.immutable.persistentListOf

-/**
- * The max number of items that can be displayed before the "show more" text is visible.
- */
-private const val EXPANDABLE_THRESHOLD = 2
-
 /**
 * Reusable composable for displaying the cipher name, favorite status, and related locations.
 *
@@ -65,7 +59,7 @@ private const val EXPANDABLE_THRESHOLD = 2
 * @param textFieldTestTag The test tag for the name field.
 * @param onExpandClick The action to be performed when the expandable text row is clicked.
 */
-@Suppress("LongMethod", "LongParameterList")
+@Suppress("CyclomaticComplexMethod", "LongMethod", "LongParameterList")
 fun LazyListScope.itemHeader(
    value: String,
    isFavorite: Boolean,
@@ -77,6 +71,15 @@ fun LazyListScope.itemHeader(
    textFieldTestTag: String? = null,
    onExpandClick: () -> Unit,
 ) {
+    val organizationLocation = relatedLocations
+        .firstOrNull { it is VaultItemLocation.Organization }
+
+    val collectionLocations = relatedLocations
+        .filterIsInstance<VaultItemLocation.Collection>()
+
+    val folderLocations = relatedLocations
+        .filterIsInstance<VaultItemLocation.Folder>()
+
    item {
        Row(
            verticalAlignment = Alignment.CenterVertically,
@@ -124,6 +127,8 @@ fun LazyListScope.itemHeader(
        }
    }

+    // When the item does not belong to an Org and is not assigned to a collection or folder we
+    // display the "No Folder" indicator.
    if (relatedLocations.isEmpty()) {
        item(key = "noFolder") {
            ItemLocationListItem(
@@ -144,38 +149,97 @@ fun LazyListScope.itemHeader(
        return
    }

-    itemsIndexed(
-        key = { index, _ -> "locations_$index" },
-        items = relatedLocations.take(EXPANDABLE_THRESHOLD),
-    ) { index, location ->
-        ItemLocationListItem(
-            vectorPainter = rememberVectorPainter(location.icon),
-            iconTestTag = "ItemLocationIcon",
-            text = location.name,
-            modifier = Modifier
-                .fillMaxWidth()
-                .standardHorizontalMargin()
-                .animateItem()
-                .cardStyle(
-                    cardStyle = if (index == relatedLocations.size - 1) {
-                        CardStyle.Bottom
-                    } else {
-                        CardStyle.Middle(hasDivider = false)
-                    },
-                    paddingVertical = 0.dp,
-                    paddingHorizontal = 16.dp,
-                ),
-        )
+    // When the item is owned by an Org we display the organization name.
+    if (organizationLocation != null) {
+        item(key = "organization") {
+            ItemLocationListItem(
+                vectorPainter = rememberVectorPainter(organizationLocation.icon),
+                iconTestTag = "ItemLocationIcon",
+                text = organizationLocation.name,
+                modifier = Modifier
+                    .fillMaxWidth()
+                    .standardHorizontalMargin()
+                    .animateItem()
+                    .cardStyle(
+                        cardStyle = if (relatedLocations.size == 1) {
+                            CardStyle.Bottom
+                        } else {
+                            CardStyle.Middle(hasDivider = false)
+                        },
+                        paddingVertical = 0.dp,
+                        paddingHorizontal = 16.dp,
+                    ),
+            )
+        }
    }

-    if (isExpanded) {
-        items(
-            key = { "expandableLocations_$it" },
-            items = relatedLocations.drop(EXPANDABLE_THRESHOLD),
-        ) {
+    // When the item is assigned to a single collection and a single folder we display both the
+    // collection and folder names.
+    if (collectionLocations.size == 1 && folderLocations.size == 1) {
+        itemsIndexed(
+            items = collectionLocations + folderLocations,
+            key = { index, location -> "locations_$index" },
+        ) { index, location ->
            ItemLocationListItem(
-                vectorPainter = rememberVectorPainter(it.icon),
-                text = it.name,
+                vectorPainter = rememberVectorPainter(location.icon),
+                iconTestTag = "ItemLocationIcon",
+                text = location.name,
+                modifier = Modifier
+                    .fillMaxWidth()
+                    .standardHorizontalMargin()
+                    .animateItem()
+                    .cardStyle(
+                        cardStyle = if (index == 1) {
+                            CardStyle.Bottom
+                        } else {
+                            CardStyle.Middle(hasDivider = false)
+                        },
+                        paddingVertical = 0.dp,
+                        paddingHorizontal = 16.dp,
+                    ),
+            )
+        }
+        return
+    }
+
+    // When the item is assigned to multiple collections we only display the first collection by
+    // default and collapse the remaining locations.
+    collectionLocations.firstOrNull()
+        ?.let {
+            item(key = "visibleCollection") {
+                ItemLocationListItem(
+                    vectorPainter = rememberVectorPainter(it.icon),
+                    iconTestTag = "ItemLocationIcon",
+                    text = if (collectionLocations.size > 1 && !isExpanded) {
+                        stringResource(R.string.x_ellipses, it.name)
+                    } else {
+                        it.name
+                    },
+                    modifier = Modifier
+                        .fillMaxWidth()
+                        .standardHorizontalMargin()
+                        .animateItem()
+                        .cardStyle(
+                            cardStyle = if (collectionLocations.size > 1) {
+                                CardStyle.Middle(hasDivider = false)
+                            } else {
+                                CardStyle.Bottom
+                            },
+                            paddingVertical = 0.dp,
+                            paddingHorizontal = 16.dp,
+                        ),
+                )
+            }
+        }
+
+    if (isExpanded) {
+        itemsIndexed(
+            key = { index, _ -> "expandableLocations_$index" },
+            items = collectionLocations.drop(1) + folderLocations,
+        ) { index, location ->
+            ItemLocationListItem(
+                vectorPainter = rememberVectorPainter(location.icon),
+                text = location.name,
                iconTestTag = "ItemLocationIcon",
                modifier = Modifier
                    .fillMaxWidth()
@@ -190,8 +254,8 @@ fun LazyListScope.itemHeader(
        }
    }

-    if (relatedLocations.size > EXPANDABLE_THRESHOLD) {
-        item(key = "expandableLocationsShowMore") {
+    if (collectionLocations.size > 1) {
+        item(key = "expandableLocationsExpansionIndicator") {
            BitwardenExpandingHeader(
                collapsedText = stringResource(R.string.show_more),
                expandedText = stringResource(R.string.show_less),
--- a/app/src/main/res/values/strings.xml
+++ b/app/src/main/res/values/strings.xml
@@ -1226,4 +1226,5 @@ Do you want to switch to this account?</string>
  <string name="show_more">Show more</string>
  <string name="no_folder">No folder</string>
  <string name="show_less">Show less</string>
+  <string name="x_ellipses">%s...</string>
 </resources>
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemScreenTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemScreenTest.kt
@@ -431,6 +431,7 @@ class VaultItemScreenTest : BaseComposeTest() {
                            relatedLocations = persistentListOf(
                                VaultItemLocation.Organization("My organization"),
                                VaultItemLocation.Collection("My collection"),
+                                VaultItemLocation.Collection("My other collection"),
                                VaultItemLocation.Folder("My folder"),
                            ),
                        ),
@@ -458,6 +459,7 @@ class VaultItemScreenTest : BaseComposeTest() {
                            relatedLocations = persistentListOf(
                                VaultItemLocation.Organization("My organization"),
                                VaultItemLocation.Collection("My collection"),
+                                VaultItemLocation.Collection("My other collection"),
                                VaultItemLocation.Folder("My folder"),
                            ),
                        ),
@@ -465,21 +467,76 @@ class VaultItemScreenTest : BaseComposeTest() {
                )
            }

-            // Verify only two locations are shown when content is collapsed.
+            // Verify only the first collection name is shown
+            composeTestRule
+                .onNodeWithText("My collection...")
+                .assertIsDisplayed()
+
+            // Verify other collection names are not shown by default.
+            composeTestRule
+                .onNodeWithText("My other collection")
+                .assertIsNotDisplayed()
+
+            // Verify folder name is not shown by default.
            composeTestRule
                .onNodeWithText("My folder")
                .assertIsNotDisplayed()

-            // Verify all locations are show when content is expanded.
+            // Verify all locations are show when content is expanded and ellipses is removed from
+            // the first collection name.
            composeTestRule
                .onNodeWithText("Show more")
                .performClick()
+            composeTestRule
+                .onNodeWithText("My collection")
+                .assertIsDisplayed()
+            composeTestRule
+                .onNodeWithText("My other collection")
+                .assertIsDisplayed()
            composeTestRule
                .onNodeWithText("My folder")
                .assertIsDisplayed()
        }
    }

+    @Test
+    fun `ExpandingHeader should show all locations when assigned to org, collection, and folder`() {
+        DEFAULT_VIEW_STATES.forEach { viewState ->
+            mutableStateFlow.update {
+                DEFAULT_STATE.copy(
+                    viewState = viewState.copy(
+                        common = DEFAULT_COMMON.copy(
+                            relatedLocations = persistentListOf(
+                                VaultItemLocation.Organization("My organization"),
+                                VaultItemLocation.Collection("My collection"),
+                                VaultItemLocation.Folder("My folder"),
+                            ),
+                        ),
+                    ),
+                )
+            }
+
+            // Verify all location names are not shown by default.
+            composeTestRule
+                .onNodeWithText("My organization")
+                .assertIsDisplayed()
+            composeTestRule
+                .onNodeWithText("My collection")
+                .assertIsDisplayed()
+            composeTestRule
+                .onNodeWithText("My folder")
+                .assertIsDisplayed()
+
+            // Verify expander button is not displayed when all locations are shown.
+            composeTestRule
+                .onNodeWithText("Show more")
+                .assertIsNotDisplayed()
+            composeTestRule
+                .onNodeWithText("Show less")
+                .assertIsNotDisplayed()
+        }
+    }
+
    @Test
    fun `lastUpdated should be displayed according to state`() {
        EMPTY_VIEW_STATES