From 91df6b5e25a0d6b1111a6a69919ca62d52843dfe Mon Sep 17 00:00:00 2001 From: Lucas Kivi <125697099+lucas-livefront@users.noreply.github.com> Date: Tue, 23 Jan 2024 13:33:30 -0600 Subject: [PATCH] Update CipherView subtitle generation (#730) --- .../platform/util/CipherViewExtensions.kt | 70 ++++ .../search/util/SearchTypeDataExtensions.kt | 21 +- .../util/VaultItemListingDataExtensions.kt | 21 +- .../data/util/CipherViewExtensionsTest.kt | 332 ++++++++++++++++++ .../VaultItemListingDataExtensionsTest.kt | 9 + .../util/VaultItemListingDataUtil.kt | 9 +- 6 files changed, 418 insertions(+), 44 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/util/CipherViewExtensions.kt create mode 100644 app/src/test/java/com/x8bit/bitwarden/data/util/CipherViewExtensionsTest.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/CipherViewExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/CipherViewExtensions.kt new file mode 100644 index 0000000000..f0c7c6e88c --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/CipherViewExtensions.kt @@ -0,0 +1,70 @@ +package com.x8bit.bitwarden.data.platform.util + +import com.bitwarden.core.CardView +import com.bitwarden.core.CipherType +import com.bitwarden.core.CipherView + +/** + * If someone has multiple AMEX cards, they tend to have the same last 4 digits. So we provide a + * fifth card number digit to allow users to differentiate between cards based on the subtitle. + */ +private const val AMEX_DIGITS_DISPLAYED: Int = 5 + +/** + * A normal number of card number digits to show in a subtitle for all non-AMEX cards. + */ +private const val CARD_DIGITS_DISPLAYED: Int = 4 + +/** + * The subtitle for a [CipherView] used to give extra information about a particular instance. + */ +val CipherView.subtitle: String? + get() = when (type) { + CipherType.LOGIN -> this.login?.username.orEmpty() + CipherType.SECURE_NOTE -> null + CipherType.CARD -> { + this + .card + ?.let { cardView -> + when { + cardView.brand.isNullOrBlank() -> cardView.subtitleCardNumber + cardView.subtitleCardNumber.isNullOrBlank() -> cardView.brand + else -> "${cardView.brand}, *${cardView.subtitleCardNumber}" + } + } + } + + CipherType.IDENTITY -> { + this + .identity + ?.let { identityView -> + when { + identityView.firstName.isNullOrBlank() -> identityView.lastName + identityView.lastName.isNullOrBlank() -> identityView.firstName + else -> "${identityView.firstName} ${identityView.lastName}" + } + } + } + } + +/** + * Get the card number as it should be shown in the subtitle. + */ +private val CardView.subtitleCardNumber: String? + get() { + val digitsDisplayedCount = if (this.number.isAmEx) { + AMEX_DIGITS_DISPLAYED + } else { + CARD_DIGITS_DISPLAYED + } + return this + .number + ?.takeLast(digitsDisplayedCount) + } + +/** + * American express cards start with "34" or "37". This function determine whether a string + * matches that amex standard. + */ +private val String?.isAmEx: Boolean + get() = this?.startsWith("34") == true || this?.startsWith("37") == true diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/util/SearchTypeDataExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/util/SearchTypeDataExtensions.kt index bbc8869ab2..b20cd9abf8 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/util/SearchTypeDataExtensions.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/util/SearchTypeDataExtensions.kt @@ -10,6 +10,7 @@ import com.bitwarden.core.FolderView import com.bitwarden.core.SendType import com.bitwarden.core.SendView import com.x8bit.bitwarden.R +import com.x8bit.bitwarden.data.platform.util.subtitle import com.x8bit.bitwarden.ui.platform.base.util.asText import com.x8bit.bitwarden.ui.platform.base.util.removeDiacritics import com.x8bit.bitwarden.ui.platform.components.model.IconData @@ -171,26 +172,6 @@ private fun CipherView.toDisplayItem( overflowOptions = emptyList(), ) -@Suppress("MagicNumber") -private val CipherView.subtitle: String? - get() = when (type) { - CipherType.LOGIN -> login?.username.orEmpty() - CipherType.SECURE_NOTE -> null - CipherType.CARD -> { - card - ?.number - ?.takeLast(4) - .orEmpty() - } - - CipherType.IDENTITY -> { - identity - ?.firstName - .orEmpty() - .plus(identity?.lastName.orEmpty()) - } - } - private fun CipherView.toIconData( baseIconUrl: String, isIconLoadingDisabled: Boolean, diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt index 2d0a97779c..6d23d59579 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt @@ -8,6 +8,7 @@ import com.bitwarden.core.FolderView import com.bitwarden.core.SendType import com.bitwarden.core.SendView import com.x8bit.bitwarden.R +import com.x8bit.bitwarden.data.platform.util.subtitle import com.x8bit.bitwarden.ui.platform.components.model.IconData import com.x8bit.bitwarden.ui.platform.util.toFormattedPattern import com.x8bit.bitwarden.ui.tools.feature.send.util.toLabelIcons @@ -211,26 +212,6 @@ private fun SendView.toDisplayItem( overflowOptions = toOverflowActions(baseWebSendUrl = baseWebSendUrl), ) -@Suppress("MagicNumber") -private val CipherView.subtitle: String? - get() = when (type) { - CipherType.LOGIN -> login?.username.orEmpty() - CipherType.SECURE_NOTE -> null - CipherType.CARD -> { - card - ?.number - ?.takeLast(4) - .orEmpty() - } - - CipherType.IDENTITY -> { - identity - ?.firstName - .orEmpty() - .plus(identity?.lastName.orEmpty()) - } - } - @get:DrawableRes private val CipherType.iconRes: Int get() = when (this) { diff --git a/app/src/test/java/com/x8bit/bitwarden/data/util/CipherViewExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/util/CipherViewExtensionsTest.kt new file mode 100644 index 0000000000..7709739ff1 --- /dev/null +++ b/app/src/test/java/com/x8bit/bitwarden/data/util/CipherViewExtensionsTest.kt @@ -0,0 +1,332 @@ +package com.x8bit.bitwarden.data.util + +import com.bitwarden.core.CardView +import com.bitwarden.core.CipherType +import com.bitwarden.core.CipherView +import com.bitwarden.core.IdentityView +import com.x8bit.bitwarden.data.platform.util.subtitle +import io.mockk.every +import io.mockk.mockk +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Test + +class CipherViewExtensionsTest { + + @Test + fun `subtitle should return empty string when type is LOGIN and login is null`() { + // Setup + val cipherView: CipherView = mockk { + every { login } returns null + every { type } returns CipherType.LOGIN + } + val expected = "" + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Test + fun `subtitle should return username when type is LOGIN and it is username is non-null`() { + // Setup + val expected = "Bitwarden" + val cipherView: CipherView = mockk { + every { login?.username } returns expected + every { type } returns CipherType.LOGIN + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Test + fun `subtitle should return empty string when type is LOGIN and username is null`() { + // Setup + val cipherView: CipherView = mockk { + every { login?.username } returns null + every { type } returns CipherType.LOGIN + } + val expected = "" + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Test + fun `subtitle should return null when type is SECURE_NOTE`() { + // Setup + val cipherView: CipherView = mockk { + every { type } returns CipherType.SECURE_NOTE + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertNull(actual) + } + + @Test + fun `subtitle should return null when type is CARD and card is null`() { + // Setup + val cipherView: CipherView = mockk { + every { card } returns null + every { type } returns CipherType.CARD + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertNull(actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return 4 digit display number when type is CARD, brand is blank, and number doesn't start with 34 or 37`() { + // Setup + val cardNumber = "4441233456789" + val cardView: CardView = mockk { + every { brand } returns " " + every { number } returns cardNumber + } + val cipherView: CipherView = mockk { + every { card } returns cardView + every { type } returns CipherType.CARD + } + val expected = cardNumber.takeLast(4) + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return 5 digit display number when type is CARD, brand is null, and number starts with 34`() { + // Setup + // Amex cards start with 34 or 37. + val cardNumber = "341233456789" + val cardView: CardView = mockk { + every { brand } returns null + every { number } returns cardNumber + } + val cipherView: CipherView = mockk { + every { card } returns cardView + every { type } returns CipherType.CARD + } + val expected = cardNumber.takeLast(5) + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return 5 digit display number when type is CARD, brand is null, and number starts with 37`() { + // Setup + // Amex cards start with 34 or 37. + val cardNumber = "371233456789" + val cardView: CardView = mockk { + every { brand } returns null + every { number } returns cardNumber + } + val cipherView: CipherView = mockk { + every { card } returns cardView + every { type } returns CipherType.CARD + } + val expected = cardNumber.takeLast(5) + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return 5 digit display number when type is CARD, brand is blank, and number starts with 37`() { + // Setup + // Amex cards start with 34 or 37. + val cardNumber = "371233456789" + val cardView: CardView = mockk { + every { brand } returns " " + every { number } returns cardNumber + } + val cipherView: CipherView = mockk { + every { card } returns cardView + every { type } returns CipherType.CARD + } + val expected = cardNumber.takeLast(5) + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Test + fun `subtitle should return brand when type is CARD and number is null`() { + // Setup + val expected = "American Express" + val cardView: CardView = mockk { + every { brand } returns expected + every { number } returns null + } + val cipherView: CipherView = mockk { + every { card } returns cardView + every { type } returns CipherType.CARD + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Test + fun `subtitle should return brand when type is CARD and number is blank`() { + // Setup + val expected = "American Express" + val cardView: CardView = mockk { + every { brand } returns expected + every { number } returns " " + } + val cipherView: CipherView = mockk { + every { card } returns cardView + every { type } returns CipherType.CARD + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return brand and display number when type is CARD and brand and subtitle are populated`() { + // Setup + val expectedBrand = "American Express" + val cardNumber = "4441233456789" + val cardView: CardView = mockk { + every { brand } returns expectedBrand + every { number } returns cardNumber + } + val cipherView: CipherView = mockk { + every { card } returns cardView + every { type } returns CipherType.CARD + } + val shownCardDigits = cardNumber.takeLast(4) + val expected = "$expectedBrand, *$shownCardDigits" + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return first name when type is IDENTITY, first name is populated, and last name is null`() { + // Setup + val expected = "John" + val identityView: IdentityView = mockk { + every { firstName } returns expected + every { lastName } returns null + } + val cipherView: CipherView = mockk { + every { identity } returns identityView + every { type } returns CipherType.IDENTITY + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return last name when type is IDENTITY, first name is null, and last name is populated`() { + // Setup + val expected = "Doe" + val identityView: IdentityView = mockk { + every { firstName } returns null + every { lastName } returns expected + } + val cipherView: CipherView = mockk { + every { identity } returns identityView + every { type } returns CipherType.IDENTITY + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return first name when type is IDENTITY and first and last name are populated`() { + // Setup + val expectedFirstName = "John" + val expectedLastName = "Doe" + val identityView: IdentityView = mockk { + every { firstName } returns expectedFirstName + every { lastName } returns expectedLastName + } + val cipherView: CipherView = mockk { + every { identity } returns identityView + every { type } returns CipherType.IDENTITY + } + val expected = "$expectedFirstName $expectedLastName" + + // Test + val actual = cipherView.subtitle + + // Verify + assertEquals(expected, actual) + } + + @Suppress("MaxLineLength") + @Test + fun `subtitle should return null when type is IDENTITY and first and last name are null`() { + // Setup + val identityView: IdentityView = mockk { + every { firstName } returns null + every { lastName } returns null + } + val cipherView: CipherView = mockk { + every { identity } returns identityView + every { type } returns CipherType.IDENTITY + } + + // Test + val actual = cipherView.subtitle + + // Verify + assertNull(actual) + } +} diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt index 2ee0dd0950..b357c15a70 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt @@ -2,10 +2,12 @@ package com.x8bit.bitwarden.ui.vault.feature.itemlisting.util import android.net.Uri import com.bitwarden.core.CipherType +import com.bitwarden.core.CipherView import com.bitwarden.core.SendType import com.x8bit.bitwarden.data.platform.repository.model.Environment import com.x8bit.bitwarden.data.platform.repository.util.baseIconUrl import com.x8bit.bitwarden.data.platform.repository.util.baseWebSendUrl +import com.x8bit.bitwarden.data.platform.util.subtitle import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCipherView import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCollectionView import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockFolderView @@ -300,8 +302,10 @@ class VaultItemListingDataExtensionsTest { @Test fun `toViewState should transform a list of CipherViews into a ViewState`() { + mockkStatic(CipherView::subtitle) mockkStatic(Uri::class) val uriMock = mockk() + every { any().subtitle } returns null every { Uri.parse(any()) } returns uriMock every { uriMock.host } returns "www.mockuri.com" @@ -339,24 +343,29 @@ class VaultItemListingDataExtensionsTest { createMockDisplayItemForCipher( number = 1, cipherType = CipherType.LOGIN, + subtitle = null, ), createMockDisplayItemForCipher( number = 2, cipherType = CipherType.CARD, + subtitle = null, ), createMockDisplayItemForCipher( number = 3, cipherType = CipherType.SECURE_NOTE, + subtitle = null, ), createMockDisplayItemForCipher( number = 4, cipherType = CipherType.IDENTITY, + subtitle = null, ), ), ), result, ) + unmockkStatic(CipherView::subtitle) unmockkStatic(Uri::class) } diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt index dfc1f75744..a16d70c3ea 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt @@ -15,13 +15,14 @@ import com.x8bit.bitwarden.ui.vault.feature.itemlisting.model.ListingItemOverflo fun createMockDisplayItemForCipher( number: Int, cipherType: CipherType = CipherType.LOGIN, + subtitle: String? = "mockUsername-$number", ): VaultItemListingState.DisplayItem = when (cipherType) { CipherType.LOGIN -> { VaultItemListingState.DisplayItem( id = "mockId-$number", title = "mockName-$number", - subtitle = "mockUsername-$number", + subtitle = subtitle, iconData = IconData.Network( "https://vault.bitwarden.com/icons/www.mockuri.com/icon.png", fallbackIconRes = R.drawable.ic_login_item, @@ -35,7 +36,7 @@ fun createMockDisplayItemForCipher( VaultItemListingState.DisplayItem( id = "mockId-$number", title = "mockName-$number", - subtitle = null, + subtitle = subtitle, iconData = IconData.Local(R.drawable.ic_secure_note_item), extraIconList = emptyList(), overflowOptions = emptyList(), @@ -46,7 +47,7 @@ fun createMockDisplayItemForCipher( VaultItemListingState.DisplayItem( id = "mockId-$number", title = "mockName-$number", - subtitle = "er-$number", + subtitle = subtitle, iconData = IconData.Local(R.drawable.ic_card_item), extraIconList = emptyList(), overflowOptions = emptyList(), @@ -57,7 +58,7 @@ fun createMockDisplayItemForCipher( VaultItemListingState.DisplayItem( id = "mockId-$number", title = "mockName-$number", - subtitle = "mockFirstName-${number}mockLastName-$number", + subtitle = subtitle, iconData = IconData.Local(R.drawable.ic_identity_item), extraIconList = emptyList(), overflowOptions = emptyList(),