[PM-24938] Improve Autofill Card Expiration Month and Year Parsing (#5717)

2025-12-10 20:07:59 -06:00 · 2025-08-18 17:27:39 -04:00 · 2025-08-18 17:27:39 -04:00 · 44410efe56
commit 44410efe56
parent a999592fb6
11 changed files with 220 additions and 26 deletions
--- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/model/AutofillView.kt
+++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/model/AutofillView.kt
@ -48,10 +48,14 @@ sealed class AutofillView {
        ) : Card()

        /**
-         * The expiration year [AutofillView] for the [Card] data partition.
+         * The expiration year [AutofillView] for the [Card] data partition. This implementation
+         * also has its own [yearValue] because it can be present in lists, in which case there
+         * is specialized logic for determining its [yearValue]. The [Data.textValue] is very
+         * likely going to be a very different value.
         */
        data class ExpirationYear(
            override val data: Data,
+            val yearValue: String?,
        ) : Card()

        /**
--- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillPartitionExtensions.kt
+++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillPartitionExtensions.kt
@ -9,16 +9,19 @@ import com.x8bit.bitwarden.data.autofill.model.AutofillView
 val AutofillPartition.Card.expirationMonthSaveValue: String?
    get() = this
        .views
-        .firstOrNull { it is AutofillView.Card.ExpirationMonth && it.monthValue != null }
-        ?.data
-        ?.textValue
+        .filterIsInstance<AutofillView.Card.ExpirationMonth>()
+        .firstOrNull { it.monthValue != null }
+        ?.monthValue

 /**
 * The text value representation of the year from the [AutofillPartition.Card].
 */
 val AutofillPartition.Card.expirationYearSaveValue: String?
    get() = this
-        .extractNonNullTextValueOrNull { it is AutofillView.Card.ExpirationYear }
+        .views
+        .filterIsInstance<AutofillView.Card.ExpirationYear>()
+        .firstOrNull { it.yearValue != null }
+        ?.yearValue

 /**
 * The text value representation of the card number from the [AutofillPartition.Card].
@ -37,7 +40,7 @@ val AutofillPartition.Card.securityCodeSaveValue: String?
 /**
 * The text value representation of the cardholder name from the [AutofillPartition.Card].
 */
-val AutofillPartition.Card.cardholderName: String?
+val AutofillPartition.Card.cardholderNameSaveValue: String?
    get() = this
        .extractNonNullTextValueOrNull { it is AutofillView.Card.CardholderName }

--- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillRequestExtensions.kt
+++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillRequestExtensions.kt
@ -11,7 +11,7 @@ fun AutofillRequest.Fillable.toAutofillSaveItem(): AutofillSaveItem =
    when (this.partition) {
        is AutofillPartition.Card -> {
            AutofillSaveItem.Card(
-                cardholderName = partition.cardholderName,
+                cardholderName = partition.cardholderNameSaveValue,
                number = partition.numberSaveValue,
                expirationMonth = partition.expirationMonthSaveValue,
                expirationYear = partition.expirationYearSaveValue,
--- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillValueExtensions.kt
+++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillValueExtensions.kt
@ -35,3 +35,21 @@ fun AutofillValue.extractTextValue(): String? =
    } else {
        null
    }
+
+/**
+ * Extract a year value from this [AutofillValue].
+ */
+fun AutofillValue.extractYearValue(
+    autofillOptions: List<String>,
+): String? =
+    when {
+        this.isList && autofillOptions.isNotEmpty() -> {
+            autofillOptions.getOrNull(listValue)
+        }
+
+        this.isText -> {
+            this.textValue.toString()
+        }
+
+        else -> null
+    }
--- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillViewExtensions.kt
+++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillViewExtensions.kt
@ -42,22 +42,40 @@ fun AutofillView.buildFilledItemOrNull(
 private fun AutofillView.buildListAutofillValueOrNull(
    value: String,
 ): AutofillValue? =
-    if (this is AutofillView.Card.ExpirationMonth) {
-        val autofillOptionsSize = this.data.autofillOptions.size
-        // The idea here is that `value` is a numerical representation of a month.
-        val monthIndex = value.toIntOrNull()
-        when {
-            monthIndex == null -> null
-            // We expect there is some placeholder or empty space at the beginning of the list.
-            autofillOptionsSize == 13 -> AutofillValue.forList(monthIndex)
-            autofillOptionsSize >= monthIndex -> AutofillValue.forList(monthIndex - 1)
-            else -> null
+    when (this) {
+        is AutofillView.Card.ExpirationMonth -> {
+            val autofillOptionsSize = this.data.autofillOptions.size
+            // The idea here is that `value` is a numerical representation of a month.
+            val monthIndex = value.toIntOrNull()
+            when {
+                monthIndex == null -> null
+                // We expect there is some placeholder or empty space at the beginning of the list.
+                autofillOptionsSize == 13 -> AutofillValue.forList(monthIndex)
+                autofillOptionsSize >= monthIndex -> AutofillValue.forList(monthIndex - 1)
+                else -> null
+            }
+        }
+
+        is AutofillView.Card.ExpirationYear -> {
+            val autofillOptions = this.data.autofillOptions
+            autofillOptions
+                .firstOrNull { it == value || it.takeLast(2) == value.takeLast(2) }
+                ?.let { AutofillValue.forList(autofillOptions.indexOf(it)) }
+        }
+
+        is AutofillView.Card.CardholderName,
+        is AutofillView.Card.ExpirationDate,
+        is AutofillView.Card.Number,
+        is AutofillView.Card.SecurityCode,
+        is AutofillView.Login.Password,
+        is AutofillView.Login.Username,
+        is AutofillView.Unused,
+            -> {
+            this
+                .data
+                .autofillOptions
+                .indexOfFirst { it == value }
+                .takeIf { it != -1 }
+                ?.let { AutofillValue.forList(it) }
        }
-    } else {
-        this
-            .data
-            .autofillOptions
-            .indexOfFirst { it == value }
-            .takeIf { it != -1 }
-            ?.let { AutofillValue.forList(it) }
    }
--- a/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensions.kt
+++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensions.kt
@ -141,8 +141,15 @@ private fun AssistStructure.ViewNode.buildAutofillView(
    }

    AutofillHint.CARD_EXPIRATION_YEAR -> {
+        val yearValue = this
+            .autofillValue
+            ?.extractYearValue(
+                autofillOptions = autofillOptions,
+            )
+
        AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = yearValue,
        )
    }

--- a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillPartitionExtensionsTest.kt
+++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillPartitionExtensionsTest.kt
@ -26,6 +26,7 @@ class AutofillPartitionExtensionsTest {
        hasPasswordTerms = false,
    )

+    //region Card tests
    @Test
    fun `expirationMonthSaveValue should return null when no month views present`() {
        // Setup
@ -107,6 +108,7 @@ class AutofillPartitionExtensionsTest {
            views = listOf(
                AutofillView.Card.ExpirationYear(
                    data = autofillDataEmptyText,
+                    yearValue = null,
                ),
            ),
        )
@ -125,6 +127,7 @@ class AutofillPartitionExtensionsTest {
            views = listOf(
                AutofillView.Card.ExpirationYear(
                    data = autofillDataValidText,
+                    yearValue = TEXT_VALUE,
                ),
            ),
        )
@ -143,6 +146,7 @@ class AutofillPartitionExtensionsTest {
            views = listOf(
                AutofillView.Card.ExpirationYear(
                    data = autofillDataValidText,
+                    yearValue = TEXT_VALUE,
                ),
            ),
        )
@ -197,6 +201,7 @@ class AutofillPartitionExtensionsTest {
            views = listOf(
                AutofillView.Card.ExpirationYear(
                    data = autofillDataValidText,
+                    yearValue = TEXT_VALUE,
                ),
            ),
        )
@ -244,6 +249,60 @@ class AutofillPartitionExtensionsTest {
        assertEquals(TEXT_VALUE, actual)
    }

+    @Test
+    fun `cardholderNameSaveValue should return null when no name views present`() {
+        // Setup
+        val autofillPartition = AutofillPartition.Card(
+            views = listOf(
+                AutofillView.Card.ExpirationYear(
+                    data = autofillDataValidText,
+                    yearValue = TEXT_VALUE,
+                ),
+            ),
+        )
+
+        // Test
+        val actual = autofillPartition.cardholderNameSaveValue
+
+        // Verify
+        assertNull(actual)
+    }
+
+    @Test
+    fun `cardholderNameSaveValue should return null when has name view but no textValue`() {
+        // Setup
+        val autofillPartition = AutofillPartition.Card(
+            views = listOf(
+                AutofillView.Card.CardholderName(
+                    data = autofillDataEmptyText,
+                ),
+            ),
+        )
+
+        val actual = autofillPartition.cardholderNameSaveValue
+
+        assertNull(actual)
+    }
+
+    @Test
+    fun `cardholderNameSaveValue should return text value when has name view has textValue`() {
+        // Setup
+        val autofillPartition = AutofillPartition.Card(
+            views = listOf(
+                AutofillView.Card.CardholderName(
+                    data = autofillDataValidText,
+                ),
+            ),
+        )
+
+        val actual = autofillPartition.cardholderNameSaveValue
+
+        assertEquals(TEXT_VALUE, actual)
+    }
+
+    //endregion Card tests
+
+    // region Login tests
    @Test
    fun `passwordSaveValue should return null when no password views present`() {
        // Setup
@ -351,6 +410,7 @@ class AutofillPartitionExtensionsTest {
        // Verify
        assertEquals(TEXT_VALUE, actual)
    }
+    //endregion Login tests
 }

 private const val TEXT_VALUE: String = "TEXT_VALUE"
--- a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillRequestExtensionsTest.kt
+++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillRequestExtensionsTest.kt
@ -31,7 +31,7 @@ class AutofillRequestExtensionsTest {
            every { expirationYearSaveValue } returns SAVE_VALUE_YEAR
            every { numberSaveValue } returns SAVE_VALUE_NUMBER
            every { securityCodeSaveValue } returns SAVE_VALUE_CODE
-            every { cardholderName } returns SAVE_VALUE_CARDHOLDER_NAME
+            every { cardholderNameSaveValue } returns SAVE_VALUE_CARDHOLDER_NAME
        }
        val autofillRequest: AutofillRequest.Fillable = mockk {
            every { partition } returns autofillPartition
--- a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillValueExtensionsTest.kt
+++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillValueExtensionsTest.kt
@ -75,6 +75,72 @@ class AutofillValueExtensionsTest {
        assertNull(actual)
    }

+    @Test
+    fun `extractYearValue should return listValue when isList and options are not empty`() {
+        // Setup
+        val autofillOptions = List(8) { "$it" }
+        val autofillValue: AutofillValue = mockk {
+            every { isList } returns true
+            every { listValue } returns LIST_VALUE
+        }
+        val expected = LIST_VALUE.toString()
+
+        // Test
+        val actual = autofillValue.extractYearValue(autofillOptions)
+
+        // Verify
+        assertEquals(expected, actual)
+    }
+
+    @Test
+    fun `extractYearValue should return null when isList and options are empty`() {
+        // Setup
+        val autofillOptions = emptyList<String>()
+        val autofillValue: AutofillValue = mockk {
+            every { isList } returns true
+            every { isText } returns false
+        }
+
+        // Test
+        val actual = autofillValue.extractYearValue(autofillOptions)
+
+        // Verify
+        assertNull(actual)
+    }
+
+    @Test
+    fun `extractYearValue should return textValue when isText`() {
+        // Setup
+        val autofillOptions = emptyList<String>()
+        val autofillValue: AutofillValue = mockk {
+            every { isList } returns false
+            every { isText } returns true
+            every { textValue } returns TEXT_VALUE
+        }
+
+        // Test
+        val actual = autofillValue.extractYearValue(autofillOptions)
+
+        // Verify
+        assertEquals(TEXT_VALUE, actual)
+    }
+
+    @Test
+    fun `extractYearValue should return null not list or text`() {
+        // Setup
+        val autofillOptions = List(1) { "option-$it" }
+        val autofillValue: AutofillValue = mockk {
+            every { isList } returns false
+            every { isText } returns false
+        }
+
+        // Test
+        val actual = autofillValue.extractYearValue(autofillOptions)
+
+        // Verify
+        assertNull(actual)
+    }
+
    @Test
    fun `extractTextValue should return textValue when not blank`() {
        // Setup
--- a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillViewExtensionsTest.kt
+++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/AutofillViewExtensionsTest.kt
@ -16,6 +16,8 @@ import org.junit.jupiter.api.Assertions.assertNull
 import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test

+private const val TEXT_VALUE = "TEXT_VALUE"
+
 class AutofillViewExtensionsTest {
    private val autofillId: AutofillId = mockk()
    private val autofillValue: AutofillValue = mockk()
@ -48,6 +50,7 @@ class AutofillViewExtensionsTest {
        )
        val autofillView = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = TEXT_VALUE,
        )
        val expected = FilledItem(
            autofillId = autofillId,
@ -76,6 +79,7 @@ class AutofillViewExtensionsTest {
        )
        val autofillView = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = null,
        )

        // Test
@ -179,13 +183,14 @@ class AutofillViewExtensionsTest {
    fun `buildFilledItemOrNull should return index list value when list type, and value is found in options`() {
        // Setup
        val expectedValue = 1
-        val value = "2025"
+        val value = "1"
        val autofillViewData = autofillViewData.copy(
            autofillType = View.AUTOFILL_TYPE_LIST,
            autofillOptions = listOf("2024", value, "2026"),
        )
        val autofillView = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = null,
        )
        val expected = FilledItem(
            autofillId = autofillId,
@ -216,6 +221,7 @@ class AutofillViewExtensionsTest {
        )
        val autofillView = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = value,
        )

        // Test
--- a/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensionsTest.kt
+++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/autofill/util/ViewNodeExtensionsTest.kt
@ -54,12 +54,18 @@ class ViewNodeExtensionsTest {
        mockkStatic(Int::isPasswordInputType)
        mockkStatic(Int::isUsernameInputType)
        mockkStatic(AutofillValue::extractMonthValue)
+        mockkStatic(AutofillValue::extractYearValue)
        mockkStatic(AutofillValue::extractTextValue)
        every {
            testAutofillValue.extractMonthValue(
                autofillOptions = AUTOFILL_OPTIONS_LIST,
            )
        } returns MONTH_VALUE
+        every {
+            testAutofillValue.extractYearValue(
+                autofillOptions = AUTOFILL_OPTIONS_LIST,
+            )
+        } returns YEAR_VALUE
        every { testAutofillValue.extractTextValue() } returns TEXT_VALUE
    }

@ -70,6 +76,7 @@ class ViewNodeExtensionsTest {
        unmockkStatic(Int::isPasswordInputType)
        unmockkStatic(Int::isUsernameInputType)
        unmockkStatic(AutofillValue::extractMonthValue)
+        unmockkStatic(AutofillValue::extractYearValue)
        unmockkStatic(AutofillValue::extractTextValue)
    }

@ -170,6 +177,7 @@ class ViewNodeExtensionsTest {
        val autofillHint = View.AUTOFILL_HINT_CREDIT_CARD_EXPIRATION_YEAR
        val expected = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = YEAR_VALUE,
        )
        every { viewNode.autofillHints } returns arrayOf(autofillHint)

@ -183,6 +191,7 @@ class ViewNodeExtensionsTest {
        setupUnsupportedInputFieldViewNode()
        val expected = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = YEAR_VALUE,
        )
        SUPPORTED_RAW_CARD_EXP_YEAR_HINTS.forEach { hint ->
            every { viewNode.hint } returns hint
@ -199,6 +208,7 @@ class ViewNodeExtensionsTest {
        setupUnsupportedInputFieldViewNode()
        val expected = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = YEAR_VALUE,
        )
        every { viewNode.htmlInfo.hints() } returns SUPPORTED_RAW_CARD_EXP_YEAR_HINTS

@ -495,6 +505,7 @@ class ViewNodeExtensionsTest {
        val autofillHintTwo = View.AUTOFILL_HINT_CREDIT_CARD_EXPIRATION_YEAR
        val expected = AutofillView.Card.ExpirationYear(
            data = autofillViewData,
+            yearValue = YEAR_VALUE,
        )
        every { viewNode.autofillHints } returns arrayOf(autofillHintOne, autofillHintTwo)

@ -855,4 +866,5 @@ private val SUPPORTED_RAW_CARDHOLDER_NAME_HINTS: List<String> = listOf(
    "name_on_card",
 )
 private const val MONTH_VALUE: String = "MONTH_VALUE"
+private const val YEAR_VALUE: String = "YEAR_VALUE"
 private const val TEXT_VALUE: String = "TEXT_VALUE"