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 1 commit
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
5 changes: 3 additions & 2 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 @@ -56,8 +56,9 @@ internal sealed class PhoneNumberFormatter {
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 == '#' }
// private val maxSubscriberDigits = E164_MAX_DIGITS -
// (prefix.length - 1) // prefix minus the '+'
ccen-stripe marked this conversation as resolved.
Show resolved Hide resolved

override fun userInputFilter(input: String) =
input.filter { VALID_INPUT_RANGE.contains(it) }.run {
Expand Down
10 changes: 5 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 Down