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

Add prefix to form field value in PhoneNumberController #9571

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

samer-stripe
Copy link
Collaborator

@samer-stripe samer-stripe commented Nov 7, 2024

Summary

Adds prefix of the selected to the phone number formFieldValue in PhoneNumberController

Motivation

formFieldValue indicates the final value we want to send for a customer's phone number when creating a payment method, signing up for Link, etc. Unfortunately on Android, we've been sending just the user's filtered text input and did not include the prefix of the selected country. This PR adds the prefix to the form field value in order to ensure the phone is transmitted properly.

Testing

  • Added tests
  • Modified tests
  • Manually verified

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Diffuse output:

OLD: paymentsheet-example-release-master.apk (signature: V1, V2)
NEW: paymentsheet-example-release-pr.apk (signature: V1, V2)

          │            compressed            │           uncompressed           
          ├───────────┬───────────┬──────────┼───────────┬───────────┬──────────
 APK      │ old       │ new       │ diff     │ old       │ new       │ diff     
──────────┼───────────┼───────────┼──────────┼───────────┼───────────┼──────────
      dex │   3.8 MiB │   3.8 MiB │   -626 B │   8.4 MiB │   8.4 MiB │    -44 B 
     arsc │   2.3 MiB │   2.3 MiB │      0 B │   2.3 MiB │   2.3 MiB │      0 B 
 manifest │     5 KiB │     5 KiB │      0 B │  24.9 KiB │  24.9 KiB │      0 B 
      res │ 902.2 KiB │ 902.2 KiB │      0 B │   1.4 MiB │   1.4 MiB │      0 B 
   native │   2.6 MiB │   2.6 MiB │      0 B │     6 MiB │     6 MiB │      0 B 
    asset │   1.6 MiB │   1.6 MiB │ -1.4 KiB │   1.6 MiB │   1.6 MiB │ -1.4 KiB 
    other │ 199.8 KiB │ 199.8 KiB │     -7 B │ 440.1 KiB │ 440.1 KiB │      0 B 
──────────┼───────────┼───────────┼──────────┼───────────┼───────────┼──────────
    total │  11.4 MiB │  11.4 MiB │   -2 KiB │  20.2 MiB │  20.2 MiB │ -1.4 KiB 

 DEX     │ old   │ new   │ diff       
─────────┼───────┼───────┼────────────
   files │     1 │     1 │  0         
 strings │ 39758 │ 39759 │ +1 (+2 -1) 
   types │ 13711 │ 13711 │  0 (+0 -0) 
 classes │ 11403 │ 11403 │  0 (+0 -0) 
 methods │ 58601 │ 58600 │ -1 (+1 -2) 
  fields │ 38901 │ 38901 │  0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  242 │  242 │  0   
 entries │ 6189 │ 6189 │  0
APK
     compressed      │     uncompressed     │                               
──────────┬──────────┼───────────┬──────────┤                               
 size     │ diff     │ size      │ diff     │ path                          
──────────┼──────────┼───────────┼──────────┼───────────────────────────────
  6.4 KiB │ -1.4 KiB │   6.3 KiB │ -1.4 KiB │ ∆ assets/dexopt/baseline.prof 
  3.8 MiB │   -626 B │   8.4 MiB │    -44 B │ ∆ classes.dex                 
 49.7 KiB │     -7 B │ 117.5 KiB │      0 B │ ∆ META-INF/MANIFEST.MF        
   53 KiB │     +1 B │ 117.6 KiB │      0 B │ ∆ META-INF/CERT.SF            
  1.2 KiB │     -1 B │   1.2 KiB │      0 B │ ∆ META-INF/CERT.RSA           
──────────┼──────────┼───────────┼──────────┼───────────────────────────────
    4 MiB │   -2 KiB │   8.6 MiB │ -1.4 KiB │ (total)
DEX
STRINGS:

   old   │ new   │ diff       
  ───────┼───────┼────────────
   39758 │ 39759 │ +1 (+2 -1) 
  
  + rawFieldValue
  + ~~R8{"backend":"dex","compilation-mode":"release","has-checksums":false,"min-api":21,"pg-map-id":"fd771d9","r8-mode":"full","version":"8.7.14"}
  
  - ~~R8{"backend":"dex","compilation-mode":"release","has-checksums":false,"min-api":21,"pg-map-id":"7f4827a","r8-mode":"full","version":"8.7.14"}
  

METHODS:

   old   │ new   │ diff       
  ───────┼───────┼────────────
   58601 │ 58600 │ -1 (+1 -2) 
  
  + F8.w0 k(String)
  
  - F8.w0 k(String) → String
  - F8.w0 m(String)

@samer-stripe samer-stripe force-pushed the samer/fix-phone-number-controller-form-value branch 3 times, most recently from 7310d31 to d02364c Compare November 8, 2024 15:46
@samer-stripe samer-stripe force-pushed the samer/fix-phone-number-controller-form-value branch from d02364c to 8a65403 Compare November 8, 2024 16:45
@samer-stripe samer-stripe marked this pull request as ready for review November 8, 2024 16:59
@samer-stripe samer-stripe requested review from a team as code owners November 8, 2024 16:59
@@ -95,7 +95,7 @@ internal class LinkSignupHandlerForNetworking @Inject constructor(
saveAccountToLink.new(
country = phoneController.getCountryCode(),
email = state.validEmail!!,
phoneNumber = phoneController.getE164PhoneNumber(state.validPhone!!),
phoneNumber = state.validPhone!!,
Copy link
Collaborator Author

@samer-stripe samer-stripe Nov 8, 2024

Choose a reason for hiding this comment

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

Not that formFieldValue transmits this, we don't need to convert it manually. This is covered by Maestro tests.

@@ -51,7 +51,7 @@ internal fun billingDetailsParams(): RequestMatcher {
internal fun fullBillingDetailsParams(): RequestMatcher {
return RequestMatchers.composite(
bodyPart(urlEncode("billing_details[name]"), urlEncode(CustomerSheetPage.NAME)),
bodyPart(urlEncode("billing_details[phone]"), CustomerSheetPage.PHONE_NUMBER),
bodyPart(urlEncode("billing_details[phone]"), urlEncode("+1${CustomerSheetPage.PHONE_NUMBER}")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were we sending this to the backend incorrectly!??!?

Copy link
Collaborator Author

@samer-stripe samer-stripe Nov 8, 2024

Choose a reason for hiding this comment

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

Unfortunately yeah :(. This has been a fairly long standing issue (since 2022 at least)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this broken in PaymentSheet as well?

Copy link
Collaborator Author

@samer-stripe samer-stripe Nov 8, 2024

Choose a reason for hiding this comment

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

Yeah since we weren't manually converting the text input. InlineLinkSignup did it properly but not any phone number field outside of it (through billing config, required by LPM)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. Nice fix!

@samer-stripe samer-stripe merged commit d235174 into master Nov 11, 2024
16 checks passed
@samer-stripe samer-stripe deleted the samer/fix-phone-number-controller-form-value branch November 11, 2024 15:39
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.

3 participants