BIT-1703: Card brand UI error (#997)

2025-03-15 18:58:59 +03:00 · 2024-02-13 11:04:40 -07:00 · 2024-02-13 11:04:40 -07:00 · 2739b9e001
commit 2739b9e001
parent 5ca514b1e2
11 changed files with 332 additions and 29 deletions
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditCardItems.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditCardItems.kt
@ -30,6 +30,7 @@ import com.x8bit.bitwarden.ui.vault.feature.addedit.handlers.VaultAddEditCommonH
 import com.x8bit.bitwarden.ui.vault.model.VaultCardBrand
 import com.x8bit.bitwarden.ui.vault.model.VaultCardExpirationMonth
 import com.x8bit.bitwarden.ui.vault.model.VaultLinkedFieldType
+import com.x8bit.bitwarden.ui.vault.util.longName
 import kotlinx.collections.immutable.persistentListOf
 import kotlinx.collections.immutable.toImmutableList

@ -84,14 +85,14 @@ fun LazyListScope.vaultAddEditCardItems(
            label = stringResource(id = R.string.brand),
            options = VaultCardBrand
                .entries
-                .map { it.value() }
+                .map { it.longName() }
                .toImmutableList(),
-            selectedOption = cardState.brand.value(),
+            selectedOption = cardState.brand.longName(),
            onOptionSelected = { selectedString ->
                cardHandlers.onBrandSelected(
                    VaultCardBrand
                        .entries
-                        .first { it.value.toString(resources) == selectedString },
+                        .first { it.longName.toString(resources) == selectedString },
                )
            },
            modifier = Modifier.padding(horizontal = 16.dp),
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemCardContent.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemCardContent.kt
@ -23,6 +23,7 @@ import com.x8bit.bitwarden.ui.platform.components.model.IconResource
 import com.x8bit.bitwarden.ui.vault.feature.item.handlers.VaultCardItemTypeHandlers
 import com.x8bit.bitwarden.ui.vault.feature.item.handlers.VaultCommonItemTypeHandlers
 import com.x8bit.bitwarden.ui.vault.model.VaultCardBrand
+import com.x8bit.bitwarden.ui.vault.util.shortName

 /**
 * The top level content UI state for the [VaultItemScreen] when viewing a Card cipher.
@ -104,7 +105,7 @@ fun VaultItemCardContent(
                Spacer(modifier = Modifier.height(8.dp))
                BitwardenTextField(
                    label = stringResource(id = R.string.brand),
-                    value = cardState.brand.value(),
+                    value = cardState.brand.shortName(),
                    onValueChange = {},
                    readOnly = true,
                    singleLine = false,
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/vault/VaultViewModel.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/vault/VaultViewModel.kt
@ -34,7 +34,9 @@ import com.x8bit.bitwarden.ui.vault.feature.vault.util.toActiveAccountSummary
 import com.x8bit.bitwarden.ui.vault.feature.vault.util.toAppBarTitle
 import com.x8bit.bitwarden.ui.vault.feature.vault.util.toVaultFilterData
 import com.x8bit.bitwarden.ui.vault.feature.vault.util.toViewState
+import com.x8bit.bitwarden.ui.vault.model.VaultCardBrand
 import com.x8bit.bitwarden.ui.vault.model.VaultItemListingType
+import com.x8bit.bitwarden.ui.vault.util.shortName
 import dagger.hilt.android.lifecycle.HiltViewModel
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.launchIn
@ -814,15 +816,15 @@ data class VaultState(
                override val extraIconList: List<IconRes> = emptyList(),
                override val overflowOptions: List<ListingItemOverflowAction.VaultAction>,
                override val shouldShowMasterPasswordReprompt: Boolean,
-                val brand: Text? = null,
+                private val brand: VaultCardBrand? = null,
                val lastFourDigits: Text? = null,
            ) : VaultItem() {
                override val supportingLabel: Text?
                    get() = when {
-                        brand != null && lastFourDigits != null -> brand
+                        brand != null && lastFourDigits != null -> brand.shortName
                            .concat(", *".asText(), lastFourDigits)

-                        brand != null -> brand
+                        brand != null -> brand.shortName
                        lastFourDigits != null -> "*".asText().concat(lastFourDigits)
                        else -> null
                    }
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/vault/util/VaultAddItemStateExtensions.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/vault/util/VaultAddItemStateExtensions.kt
@ -18,6 +18,7 @@ import com.x8bit.bitwarden.ui.vault.feature.addedit.model.UriItem
 import com.x8bit.bitwarden.ui.vault.model.VaultCardBrand
 import com.x8bit.bitwarden.ui.vault.model.VaultCardExpirationMonth
 import com.x8bit.bitwarden.ui.vault.model.VaultIdentityTitle
+import com.x8bit.bitwarden.ui.vault.util.stringLongNameOrNull
 import java.time.Instant

 /**
@ -81,7 +82,7 @@ private fun VaultAddEditState.ViewState.Content.ItemType.toCardView(): CardView?
                .takeUnless { brand ->
                    brand == VaultCardBrand.SELECT
                }
-                ?.name,
+                ?.stringLongNameOrNull,
            number = it.number.orNullIfBlank(),
        )
    }
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/vault/util/VaultDataExtensions.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/vault/util/VaultDataExtensions.kt
@ -15,6 +15,7 @@ import com.x8bit.bitwarden.ui.vault.feature.util.toLabelIcons
 import com.x8bit.bitwarden.ui.vault.feature.util.toOverflowActions
 import com.x8bit.bitwarden.ui.vault.feature.vault.VaultState
 import com.x8bit.bitwarden.ui.vault.feature.vault.model.VaultFilterType
+import com.x8bit.bitwarden.ui.vault.model.findVaultCardBrandWithNameOrNull

 private const val ANDROID_URI = "androidapp://"
 private const val IOS_URI = "iosapp://"
@ -175,7 +176,7 @@ private fun CipherView.toVaultItemOrNull(
        CipherType.CARD -> VaultState.ViewState.VaultItem.Card(
            id = id,
            name = name.asText(),
-            brand = card?.brand?.asText(),
+            brand = card?.brand?.findVaultCardBrandWithNameOrNull(),
            lastFourDigits = card?.number
                ?.takeLast(4)
                ?.asText(),
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/model/VaultCardBrand.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/model/VaultCardBrand.kt
@ -1,25 +1,20 @@
 package com.x8bit.bitwarden.ui.vault.model

-import com.x8bit.bitwarden.R
-import com.x8bit.bitwarden.ui.platform.base.util.Text
-import com.x8bit.bitwarden.ui.platform.base.util.asText
-import com.x8bit.bitwarden.ui.vault.feature.addedit.util.SELECT_TEXT
-
 /**
 * Defines all available brand options for cards.
 */
-enum class VaultCardBrand(val value: Text) {
-    SELECT(value = SELECT_TEXT),
-    VISA(value = "Visa".asText()),
-    MASTERCARD(value = "Mastercard".asText()),
-    AMERICAN_EXPRESS(value = "American Express".asText()),
-    DISCOVER(value = "Discover".asText()),
-    DINERS_CLUB(value = "Diners Club".asText()),
-    JCB(value = "JCB".asText()),
-    MAESTRO(value = "Maestro".asText()),
-    UNIONPAY(value = "UnionPay".asText()),
-    RUPAY(value = "RuPay".asText()),
-    OTHER(value = R.string.other.asText()),
+enum class VaultCardBrand {
+    SELECT,
+    VISA,
+    MASTERCARD,
+    AMEX,
+    DISCOVER,
+    DINERS_CLUB,
+    JCB,
+    MAESTRO,
+    UNIONPAY,
+    RUPAY,
+    OTHER,
 }

 /**
--- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/util/VaultCardBrandExtensions.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/util/VaultCardBrandExtensions.kt
@ -0,0 +1,61 @@
+package com.x8bit.bitwarden.ui.vault.util
+
+import com.x8bit.bitwarden.R
+import com.x8bit.bitwarden.ui.platform.base.util.Text
+import com.x8bit.bitwarden.ui.platform.base.util.asText
+import com.x8bit.bitwarden.ui.vault.feature.addedit.util.SELECT_TEXT
+import com.x8bit.bitwarden.ui.vault.model.VaultCardBrand
+
+/**
+ * Helper that exposes the long name Text for a [VaultCardBrand].
+ */
+val VaultCardBrand.longName: Text
+    get() = when (this) {
+        VaultCardBrand.SELECT -> SELECT_TEXT
+        VaultCardBrand.VISA -> "Visa".asText()
+        VaultCardBrand.MASTERCARD -> "Mastercard".asText()
+        VaultCardBrand.AMEX -> "American Express".asText()
+        VaultCardBrand.DISCOVER -> "Discover".asText()
+        VaultCardBrand.DINERS_CLUB -> "Diners Club".asText()
+        VaultCardBrand.JCB -> "JCB".asText()
+        VaultCardBrand.MAESTRO -> "Maestro".asText()
+        VaultCardBrand.UNIONPAY -> "UnionPay".asText()
+        VaultCardBrand.RUPAY -> "RuPay".asText()
+        VaultCardBrand.OTHER -> R.string.other.asText()
+    }
+
+/**
+ * Helper that exposes the short name Text for a [VaultCardBrand].
+ */
+val VaultCardBrand.shortName: Text
+    get() = when (this) {
+        VaultCardBrand.SELECT -> SELECT_TEXT
+        VaultCardBrand.VISA -> "Visa".asText()
+        VaultCardBrand.MASTERCARD -> "Mastercard".asText()
+        VaultCardBrand.AMEX -> "Amex".asText()
+        VaultCardBrand.DISCOVER -> "Discover".asText()
+        VaultCardBrand.DINERS_CLUB -> "Diners Club".asText()
+        VaultCardBrand.JCB -> "JCB".asText()
+        VaultCardBrand.MAESTRO -> "Maestro".asText()
+        VaultCardBrand.UNIONPAY -> "UnionPay".asText()
+        VaultCardBrand.RUPAY -> "RuPay".asText()
+        VaultCardBrand.OTHER -> R.string.other.asText()
+    }
+
+/**
+ * Helper that exposes the long name string or null for a [VaultCardBrand].
+ */
+val VaultCardBrand.stringLongNameOrNull: String?
+    get() = when (this) {
+        VaultCardBrand.SELECT -> null
+        VaultCardBrand.VISA -> "Visa"
+        VaultCardBrand.MASTERCARD -> "Mastercard"
+        VaultCardBrand.AMEX -> "Amex"
+        VaultCardBrand.DISCOVER -> "Discover"
+        VaultCardBrand.DINERS_CLUB -> "Diners Club"
+        VaultCardBrand.JCB -> "JCB"
+        VaultCardBrand.MAESTRO -> "Maestro"
+        VaultCardBrand.UNIONPAY -> "UnionPay"
+        VaultCardBrand.RUPAY -> "RuPay"
+        VaultCardBrand.OTHER -> "Other"
+    }
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreenTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreenTest.kt
@ -1717,7 +1717,7 @@ class VaultAddEditScreenTest : BaseComposeTest() {
        mutableStateFlow.update { currentState ->
            updateCardType(currentState) {
                copy(
-                    brand = VaultCardBrand.AMERICAN_EXPRESS,
+                    brand = VaultCardBrand.AMEX,
                )
            }
        }
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/vault/util/VaultAddItemStateExtensionsTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/vault/util/VaultAddItemStateExtensionsTest.kt
@ -1,5 +1,6 @@
 package com.x8bit.bitwarden.ui.vault.feature.vault.util

+import com.bitwarden.core.CardView
 import com.bitwarden.core.CipherRepromptType
 import com.bitwarden.core.CipherType
 import com.bitwarden.core.CipherView
@ -14,6 +15,8 @@ import com.bitwarden.core.SecureNoteView
 import com.bitwarden.core.UriMatchType
 import com.x8bit.bitwarden.ui.vault.feature.addedit.VaultAddEditState
 import com.x8bit.bitwarden.ui.vault.feature.addedit.model.UriItem
+import com.x8bit.bitwarden.ui.vault.model.VaultCardBrand
+import com.x8bit.bitwarden.ui.vault.model.VaultCardExpirationMonth
 import com.x8bit.bitwarden.ui.vault.model.VaultIdentityTitle
 import com.x8bit.bitwarden.ui.vault.model.VaultLinkedFieldType
 import io.mockk.every
@ -24,6 +27,7 @@ import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Test
 import java.time.Instant

+@Suppress("LargeClass")
 class VaultAddItemStateExtensionsTest {

    @AfterEach
@ -513,6 +517,151 @@ class VaultAddItemStateExtensionsTest {
            result,
        )
    }
+
+    @Test
+    fun `toCipherView should transform Card ItemType to CipherView`() {
+        mockkStatic(Instant::class)
+        every { Instant.now() } returns Instant.MIN
+        val viewState = VaultAddEditState.ViewState.Content(
+            common = VaultAddEditState.ViewState.Content.Common(
+                name = "mockName-1",
+                selectedFolderId = "mockId-1",
+                favorite = false,
+                masterPasswordReprompt = false,
+                notes = "mockNotes-1",
+                selectedOwnerId = "mockOwnerId-1",
+            ),
+            isIndividualVaultDisabled = false,
+            type = VaultAddEditState.ViewState.Content.ItemType.Card(
+                cardHolderName = "mockName-1",
+                number = "1234567",
+                brand = VaultCardBrand.VISA,
+                expirationMonth = VaultCardExpirationMonth.MARCH,
+                expirationYear = "2028",
+                securityCode = "987",
+            ),
+        )
+
+        val result = viewState.toCipherView()
+
+        assertEquals(
+            CipherView(
+                id = null,
+                organizationId = "mockOwnerId-1",
+                folderId = "mockId-1",
+                collectionIds = emptyList(),
+                key = null,
+                name = "mockName-1",
+                notes = "mockNotes-1",
+                type = CipherType.CARD,
+                login = null,
+                identity = null,
+                card = CardView(
+                    cardholderName = "mockName-1",
+                    expMonth = "3",
+                    expYear = "2028",
+                    code = "987",
+                    brand = "Visa",
+                    number = "1234567",
+                ),
+                secureNote = null,
+                favorite = false,
+                reprompt = CipherRepromptType.NONE,
+                organizationUseTotp = false,
+                edit = true,
+                viewPassword = true,
+                localData = null,
+                attachments = null,
+                fields = emptyList(),
+                passwordHistory = null,
+                creationDate = Instant.MIN,
+                deletedDate = null,
+                revisionDate = Instant.MIN,
+            ),
+            result,
+        )
+    }
+
+    @Test
+    fun `toCipherView should transform Card ItemType to CipherView with original cipher`() {
+        val cipherView = DEFAULT_CARD_CIPHER_VIEW
+        val viewState = VaultAddEditState.ViewState.Content(
+            common = VaultAddEditState.ViewState.Content.Common(
+                originalCipher = cipherView,
+                name = "mockName-1",
+                selectedFolderId = "mockId-1",
+                favorite = true,
+                masterPasswordReprompt = false,
+                customFieldData = listOf(
+                    VaultAddEditState.Custom.BooleanField("testId", "TestBoolean", false),
+                    VaultAddEditState.Custom.TextField("testId", "TestText", "TestText"),
+                    VaultAddEditState.Custom.HiddenField("testId", "TestHidden", "TestHidden"),
+                    VaultAddEditState.Custom.LinkedField(
+                        "testId",
+                        "TestLinked",
+                        VaultLinkedFieldType.USERNAME,
+                    ),
+                ),
+                notes = "mockNotes-1",
+                selectedOwnerId = "mockOwnerId-1",
+            ),
+            isIndividualVaultDisabled = false,
+            type = VaultAddEditState.ViewState.Content.ItemType.Card(
+                cardHolderName = "mockName-1",
+                number = "1234567",
+                brand = VaultCardBrand.VISA,
+                expirationMonth = VaultCardExpirationMonth.MARCH,
+                expirationYear = "2028",
+                securityCode = "987",
+            ),
+        )
+
+        val result = viewState.toCipherView()
+
+        assertEquals(
+            cipherView.copy(
+                name = "mockName-1",
+                notes = "mockNotes-1",
+                organizationId = "mockOwnerId-1",
+                folderId = "mockId-1",
+                favorite = true,
+                reprompt = CipherRepromptType.NONE,
+                fields = listOf(
+                    FieldView(
+                        name = "TestBoolean",
+                        value = "false",
+                        type = FieldType.BOOLEAN,
+                        linkedId = null,
+                    ),
+                    FieldView(
+                        name = "TestText",
+                        value = "TestText",
+                        type = FieldType.TEXT,
+                        linkedId = null,
+                    ),
+                    FieldView(
+                        name = "TestHidden",
+                        value = "TestHidden",
+                        type = FieldType.HIDDEN,
+                        linkedId = null,
+                    ),
+                    FieldView(
+                        name = "TestLinked",
+                        value = null,
+                        type = FieldType.LINKED,
+                        linkedId = VaultLinkedFieldType.USERNAME.id,
+                    ),
+                ),
+                passwordHistory = listOf(
+                    PasswordHistoryView(
+                        password = "old_password",
+                        lastUsedDate = Instant.MIN,
+                    ),
+                ),
+            ),
+            result,
+        )
+    }
 }

 private val DEFAULT_BASE_CIPHER_VIEW: CipherView = CipherView(
@ -643,3 +792,15 @@ private val DEFAULT_IDENTITY_CIPHER_VIEW: CipherView = DEFAULT_BASE_CIPHER_VIEW.
        licenseNumber = "mockLicenseNumber",
    ),
 )
+
+private val DEFAULT_CARD_CIPHER_VIEW: CipherView = DEFAULT_BASE_CIPHER_VIEW.copy(
+    type = CipherType.CARD,
+    card = CardView(
+        cardholderName = "mockName-1",
+        expMonth = "3",
+        expYear = "2028",
+        code = "987",
+        brand = "Visa",
+        number = "1234567",
+    ),
+)
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/model/VaultCardBrandTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/model/VaultCardBrandTest.kt
@ -9,7 +9,7 @@ class VaultCardBrandTest {
    fun `findVaultCardBrandWithNameOrNull should return matching brand, regardless of format`() {
        val names = listOf(
            "UNIONpay",
-            "AMERICAN_EXPRESS",
+            "AMEx",
            "diNERs  cLub",
            "rupay",
            "nothing card",
@ -20,7 +20,7 @@ class VaultCardBrandTest {
        assertEquals(
            listOf(
                VaultCardBrand.UNIONPAY,
-                VaultCardBrand.AMERICAN_EXPRESS,
+                VaultCardBrand.AMEX,
                VaultCardBrand.DINERS_CLUB,
                VaultCardBrand.RUPAY,
                null,
--- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/util/VaultCardBrandExtensionsTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/util/VaultCardBrandExtensionsTest.kt
@ -0,0 +1,80 @@
+package com.x8bit.bitwarden.ui.vault.util
+
+import com.x8bit.bitwarden.R
+import com.x8bit.bitwarden.ui.platform.base.util.asText
+import com.x8bit.bitwarden.ui.vault.feature.addedit.util.SELECT_TEXT
+import com.x8bit.bitwarden.ui.vault.model.VaultCardBrand
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
+
+class VaultCardBrandExtensionsTest {
+
+    @Test
+    fun `longName should return the correct value for each VaultCardBrand`() {
+        mapOf(
+            VaultCardBrand.SELECT to SELECT_TEXT,
+            VaultCardBrand.VISA to "Visa".asText(),
+            VaultCardBrand.MASTERCARD to "Mastercard".asText(),
+            VaultCardBrand.AMEX to "American Express".asText(),
+            VaultCardBrand.DISCOVER to "Discover".asText(),
+            VaultCardBrand.DINERS_CLUB to "Diners Club".asText(),
+            VaultCardBrand.JCB to "JCB".asText(),
+            VaultCardBrand.MAESTRO to "Maestro".asText(),
+            VaultCardBrand.UNIONPAY to "UnionPay".asText(),
+            VaultCardBrand.RUPAY to "RuPay".asText(),
+            VaultCardBrand.OTHER to R.string.other.asText(),
+        )
+            .forEach { (type, label) ->
+                assertEquals(
+                    label,
+                    type.longName,
+                )
+            }
+    }
+
+    @Test
+    fun `shortName should return the correct value for each VaultCardBrand`() {
+        mapOf(
+            VaultCardBrand.SELECT to SELECT_TEXT,
+            VaultCardBrand.VISA to "Visa".asText(),
+            VaultCardBrand.MASTERCARD to "Mastercard".asText(),
+            VaultCardBrand.AMEX to "Amex".asText(),
+            VaultCardBrand.DISCOVER to "Discover".asText(),
+            VaultCardBrand.DINERS_CLUB to "Diners Club".asText(),
+            VaultCardBrand.JCB to "JCB".asText(),
+            VaultCardBrand.MAESTRO to "Maestro".asText(),
+            VaultCardBrand.UNIONPAY to "UnionPay".asText(),
+            VaultCardBrand.RUPAY to "RuPay".asText(),
+            VaultCardBrand.OTHER to R.string.other.asText(),
+        )
+            .forEach { (type, label) ->
+                assertEquals(
+                    label,
+                    type.shortName,
+                )
+            }
+    }
+
+    @Test
+    fun `stringLongNameOrNull should return the correct value for each VaultCardBrand`() {
+        mapOf(
+            VaultCardBrand.SELECT to null,
+            VaultCardBrand.VISA to "Visa",
+            VaultCardBrand.MASTERCARD to "Mastercard",
+            VaultCardBrand.AMEX to "Amex",
+            VaultCardBrand.DISCOVER to "Discover",
+            VaultCardBrand.DINERS_CLUB to "Diners Club",
+            VaultCardBrand.JCB to "JCB",
+            VaultCardBrand.MAESTRO to "Maestro",
+            VaultCardBrand.UNIONPAY to "UnionPay",
+            VaultCardBrand.RUPAY to "RuPay",
+            VaultCardBrand.OTHER to "Other",
+        )
+            .forEach { (type, label) ->
+                assertEquals(
+                    label,
+                    type.stringLongNameOrNull,
+                )
+            }
+    }
+}