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 setup_future_usage for saved Link payment methods from PM and passthrough mode. #8117

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

samer-stripe
Copy link
Collaborator

@samer-stripe samer-stripe commented Mar 18, 2024

Summary

Add setup_future_usage for saved payment methods from PM and passthrough mode.

Currently, we assume that paying with saved payment methods (using PaymentMethod objects) meant that the payment method was already attached to the customer. We recently found out that Link was not saving payment methods to the customer account due to a limitation from the Mobile SDK's continued usage of ephemeral keys over customer sessions.

This PR adds behavior for allowing payment methods saved on Link to also be saved on the customer's account so that they can be reused by the customer through Elements UI interfaces as well as expected when the customer asks to save a payment method through PaymentSheet.

When we move to CustomerSession, Link will automatically save the payment method on both Link and their Stripe customer's account rather than need to implement a solution on the client side.

Motivation

Fixes an issue in passthrough mode where a payment method saved through Link signup is not saved in as a customer payment method.

Testing

  • Added tests
  • Modified tests
  • Manually verified

Copy link
Contributor

github-actions bot commented Mar 18, 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.9 MiB │   3.9 MiB │ +283 B │   8.6 MiB │   8.6 MiB │ +568 B 
     arsc │   2.2 MiB │   2.2 MiB │    0 B │   2.2 MiB │   2.2 MiB │    0 B 
 manifest │   5.1 KiB │   5.1 KiB │    0 B │  25.4 KiB │  25.4 KiB │    0 B 
      res │ 911.1 KiB │ 911.1 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 │     3 MiB │     3 MiB │  +13 B │     3 MiB │     3 MiB │  +13 B 
    other │ 204.2 KiB │ 204.3 KiB │   +8 B │ 445.3 KiB │ 445.3 KiB │    0 B 
──────────┼───────────┼───────────┼────────┼───────────┼───────────┼────────
    total │  12.8 MiB │  12.8 MiB │ +304 B │  21.7 MiB │  21.7 MiB │ +581 B 

 DEX     │ old   │ new   │ diff       
─────────┼───────┼───────┼────────────
   files │     1 │     1 │  0         
 strings │ 42698 │ 42699 │ +1 (+2 -1) 
   types │ 14592 │ 14592 │  0 (+0 -0) 
 classes │ 12341 │ 12341 │  0 (+0 -0) 
 methods │ 60861 │ 60862 │ +1 (+7 -6) 
  fields │ 40030 │ 40033 │ +3 (+8 -5) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  242 │  242 │  0   
 entries │ 6029 │ 6029 │  0
APK
    compressed     │    uncompressed    │                                           
──────────┬────────┼───────────┬────────┤                                           
 size     │ diff   │ size      │ diff   │ path                                      
──────────┼────────┼───────────┼────────┼───────────────────────────────────────────
  3.9 MiB │ +283 B │   8.6 MiB │ +568 B │ ∆ classes.dex                             
  7.3 KiB │  +13 B │   7.2 KiB │  +13 B │ ∆ assets/dexopt/baseline.prof             
 62.1 KiB │   +9 B │   140 KiB │    0 B │ ∆ META-INF/CERT.SF                        
  1.2 KiB │   -3 B │   1.2 KiB │    0 B │ ∆ META-INF/CERT.RSA                       
 53.2 KiB │   +3 B │ 139.9 KiB │    0 B │ ∆ META-INF/MANIFEST.MF                    
    271 B │   -1 B │     120 B │    0 B │ ∆ META-INF/version-control-info.textproto 
──────────┼────────┼───────────┼────────┼───────────────────────────────────────────
    4 MiB │ +304 B │   8.8 MiB │ +581 B │ (total)
DEX
STRINGS:

   old   │ new   │ diff       
  ───────┼───────┼────────────
   42698 │ 42699 │ +1 (+2 -1) 
  
  + , requiresSaveOnConfirmation=
  + ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:7b7a6e8,r8-mode:full,version:8.3.37}
  
  - ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:e983f41,r8-mode:full,version:8.3.37}
  

METHODS:

   old   │ new   │ diff       
  ───────┼───────┼────────────
   60861 │ 60862 │ +1 (+7 -6) 
  
  + B4.b z(W0, boolean) → p
  + B4.c z(W0, boolean) → p
  + F6.M a(j, l1, m, boolean, e) → Object
  + K7.f z(W0, boolean) → p
  + e7.s <init>(n, m)
  + e7.y <init>(W0, w, int)
  + e7.y <init>(W0, w, boolean)
  
  - B4.b z(W0) → p
  - B4.c z(W0) → p
  - F6.M a(j, l1, boolean, e) → Object
  - K7.f z(W0) → p
  - e7.s <init>(n)
  - e7.y <init>(W0, w)
  

FIELDS:

   old   │ new   │ diff       
  ───────┼───────┼────────────
   40030 │ 40033 │ +3 (+8 -5) 
  
  + F6.H W: m
  + F6.H X: Object
  + F6.H Y: M
  + F6.H Z: int
  + e7.s V: p1
  + e7.s W: int
  + e7.s X: String
  + e7.y U: boolean
  
  - F6.H W: Object
  - F6.H X: M
  - F6.H Y: int
  - e7.s V: int
  - e7.s W: String

davidme-stripe
davidme-stripe previously approved these changes Mar 18, 2024
abstract fun create(paymentMethod: PaymentMethod): T
abstract fun create(
paymentMethod: PaymentMethod,
requiresSaveOnConfirmation: Boolean = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have tests for passing the right value into this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added some network tests for ensuring the value is properly passed in PM and passthrough mode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to have unit tests as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added unit tests in PaymentSheetViewModel.

Copy link
Collaborator

@amk-stripe amk-stripe left a comment

Choose a reason for hiding this comment

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

can you add something to the description about what the new behavior is? I think we discussed this a bit in a call last week and one of the possible fixes leads to duplicate payment methods showing up in some cases. It'd be nice to know if that's this fix and how this fix works

@samer-stripe samer-stripe changed the title Add setup_future_usage for saved payment methods from passthrough mode. Add setup_future_usage for saved payment methods from PM and passthrough mode. Mar 18, 2024
abstract fun create(paymentMethod: PaymentMethod): T
abstract fun create(
paymentMethod: PaymentMethod,
requiresSaveOnConfirmation: Boolean = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to have unit tests as well?

@samer-stripe samer-stripe changed the title Add setup_future_usage for saved payment methods from PM and passthrough mode. Add setup_future_usage for saved Link payment methods from PM and passthrough mode. Mar 18, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

are there existing tests for a different PaymentSelection.CustomerRequestedSave option for FlowController?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic for this actual is handled inside its PaymentSheet variant view model PaymentOptionsViewModel. Added some tests there.

@samer-stripe samer-stripe enabled auto-merge (squash) March 22, 2024 22:59
@samer-stripe samer-stripe merged commit 1d8ddfd into master Mar 22, 2024
15 checks passed
@samer-stripe samer-stripe deleted the samer/fix-link-bug branch March 22, 2024 23:11
samer-stripe added a commit that referenced this pull request Mar 25, 2024
samer-stripe added a commit that referenced this pull request Mar 25, 2024
* Update release notes for 20.41.0

* Update `CHANGELOG` with new `Link` items.

* Remove log item for PR #8148

* Update log items for PR #8117 & #8147
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.

4 participants