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.

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

Conversation

ccen-stripe
Copy link
Contributor

@ccen-stripe ccen-stripe commented May 23, 2023

Summary

Correctly calculate the phone number based on the pattern for known regions.

Motivation

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

Before After
phone phoneAfter

Changelog

[Fixed] Fixed the length of phone number field.

@ccen-stripe
Copy link
Contributor Author

@stripe/stripe-identity-observers

@ccen-stripe ccen-stripe enabled auto-merge (squash) May 23, 2023 20:56
@@ -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?

@@ -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

@@ -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

Copy link
Contributor

@jameswoo-stripe jameswoo-stripe left a comment

Choose a reason for hiding this comment

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

Oh, can we add an entry to the changelog?

@jameswoo-stripe jameswoo-stripe self-requested a review May 23, 2023 21:33
@ccen-stripe
Copy link
Contributor Author

Oh, can we add an entry to the changelog?

done

@ccen-stripe ccen-stripe disabled auto-merge May 23, 2023 23:25
@ccen-stripe ccen-stripe enabled auto-merge (squash) May 23, 2023 23:25
@ccen-stripe ccen-stripe merged commit 80f83da into master May 24, 2023
@ccen-stripe ccen-stripe deleted the ccen/phoneLength branch May 24, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants