Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stripe-ui-core] Fix phone number length #6771

Merged
merged 6 commits into from
May 24, 2023
Merged

[stripe-ui-core] Fix phone number length #6771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1023e1a
Fix phone number length
ccen-stripe May 23, 2023
2935a20
resolve comments
ccen-stripe May 23, 2023
bedfa87
add test
ccen-stripe May 23, 2023
42a997d
update test name
ccen-stripe May 23, 2023
93f9ebb
changelog
ccen-stripe May 23, 2023
0e8bec8
update changelog
ccen-stripe May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# CHANGELOG

## XX.XX.XX - 2023-XX-XX
### All SDKs
* [FIXED][6771](https://github.com/stripe/stripe-android/pull/6771) Fixed the length of phone number field.

## 20.25.3 - 2023-05-23

Expand Down
10 changes: 9 additions & 1 deletion stripe-ui-core/src/main/java/com/stripe/android/uicore/elements/PhoneNumberElementUI.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import androidx.compose.ui.focus.focusRequester
import androidx.compose.ui.focus.onFocusChanged
import androidx.compose.ui.focus.onFocusEvent
import androidx.compose.ui.platform.LocalFocusManager
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.text.input.ImeAction
import androidx.compose.ui.text.input.KeyboardType
Expand All @@ -39,6 +40,9 @@ import kotlinx.coroutines.job
import kotlinx.coroutines.launch
import com.stripe.android.core.R as CoreR

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
const val PHONE_NUMBER_TEXT_FIELD_TAG = "PhoneNumberTextField"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this being used? If it is not being used externally, can we keep this internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allows us to write test to programatically input a valid/invalid phone number and assert other states, such as this


@Preview
@Composable
private fun PhoneNumberCollectionPreview() {
Expand Down Expand Up @@ -114,7 +118,8 @@ fun PhoneNumberElementUI(
controller.onFocusChange(it.isFocused)
}
hasFocus = it.isFocused
},
}
.testTag(PHONE_NUMBER_TEXT_FIELD_TAG),
enabled = enabled,
label = {
FormLabel(
Expand Down Expand Up @@ -163,3 +168,6 @@ fun PhoneNumberElementUI(
}
}
}

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
const val PHONE_NUMBER_FIELD_TAG = "phone_number"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used anywhere?

4 changes: 1 addition & 3 deletions stripe-ui-core/src/main/java/com/stripe/android/uicore/elements/PhoneNumberFormatter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,7 @@ internal sealed class PhoneNumberFormatter {
override val placeholder = metadata.pattern.replace('#', '5')
override val countryCode = metadata.regionCode

// Maximum number of digits for the subscriber number for this region.
private val maxSubscriberDigits = E164_MAX_DIGITS -
(prefix.length - 1) // prefix minus the '+'
private val maxSubscriberDigits = metadata.pattern.count { it == '#' }

override fun userInputFilter(input: String) =
input.filter { VALID_INPUT_RANGE.contains(it) }.run {
Expand Down
27 changes: 22 additions & 5 deletions stripe-ui-core/src/test/java/com/stripe/android/uicore/elements/PhoneNumberFormatterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ internal class PhoneNumberFormatterTest {

@Test
fun `Phone number is correctly formatted for US locale`() {
val formatter = PhoneNumberFormatter.forCountry("US")
val formatter = PhoneNumberFormatter.forCountry("US") // "(###) ###-####"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we test every phone number that we support? If not, can we add tests for them? Do you think it is valuable to add tests for each country?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm what we changed is just limiting the length of converted phone number, which is covered by the test, not sure adding a single test for each region adds too much value here, instead I added another test for a customized pattern


assertThat(formatter.format("123")).isEqualTo("(123")
assertThat(formatter.format("1234")).isEqualTo("(123) 4")
Expand All @@ -17,21 +17,21 @@ internal class PhoneNumberFormatterTest {
assertThat(formatter.format("1234567890")).isEqualTo("(123) 456-7890")
// prefix has 1 digit so full number must be at most 14 digits
assertThat(formatter.format("12345asdfg678901234567890"))
.isEqualTo("(123) 456-7890 1234")
.isEqualTo("(123) 456-7890")
}

@Test
fun `Phone number is correctly formatted for FI locale`() {
val formatter = PhoneNumberFormatter.forCountry("FI")
val formatter = PhoneNumberFormatter.forCountry("FI") // "## ### ## ##"

assertThat(formatter.format("123")).isEqualTo("12 3")
assertThat(formatter.format("1234")).isEqualTo("12 34")
assertThat(formatter.format("123456")).isEqualTo("12 345 6")
assertThat(formatter.format("1234567")).isEqualTo("12 345 67")
assertThat(formatter.format("1234567890")).isEqualTo("12 345 67 89 0")
assertThat(formatter.format("1234567890")).isEqualTo("12 345 67 89")
// prefix has 3 digits so full number must be at most 12 digits
assertThat(formatter.format("12345asdfg678901234567890"))
.isEqualTo("12 345 67 89 012")
.isEqualTo("12 345 67 89")
}

@Test
Expand All @@ -44,6 +44,23 @@ internal class PhoneNumberFormatterTest {
.isEqualTo("+123456789012345")
}

@Test
fun `WithRegion correctly formats with pattern`() {
val pattern = "(###)-###+#####!#"
val formatter = PhoneNumberFormatter.WithRegion(
PhoneNumberFormatter.Metadata(
"prefix",
"regionCode",
pattern
)
)

assertThat(formatter.format("123")).isEqualTo("(123")
assertThat(formatter.format("1234567")).isEqualTo("(123)-456+7")
assertThat(formatter.format("123456789012")).isEqualTo("(123)-456+78901!2")
assertThat(formatter.format("123456789012456")).isEqualTo("(123)-456+78901!2")
}

private fun PhoneNumberFormatter.format(input: String) =
visualTransformation.filter(AnnotatedString(userInputFilter(input))).text.text
}