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

Fix broken logic in isLpmLevelSetupFutureUsageSet #6434

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

tillh-stripe
Copy link
Collaborator

@tillh-stripe tillh-stripe commented Mar 28, 2023

Summary

This pull request fixes an issue where PaymentSheet would incorrectly assume that a merchant requested SFU for payment intents where payment_method_options[code] was set, even if payment_method_options[code]["setup_future_usage"] didn’t exist.

This is a short-term patch. Thinking longer-term, we should align this logic with iOS (which doesn’t use payment_method_options at all).

Motivation

RUN_MOBILESDK-2223

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

Before After
before screenshot after screenshot

Changelog

[FIXED] Fixed an issue where the `Save this card for future payments` checkbox wasn’t displayed even though it should have been

Comment on lines -226 to -230
JSONObject(paymentMethodOptionsJsonString)
.optJSONObject(code)
?.optString("setup_future_usage")?.let {
true
} ?: false
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 issue here is that jsonObject.optString(key) returns an empty string if key isn’t present, meaning that this code snippet would return true if payment_method_options[code] exists, even if payment_method_options[code]["setup_future_usage"] doesn’t.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2023

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.3 MiB │   3.3 MiB │ +25 B │   7.2 MiB │   7.2 MiB │ -4 B 
     arsc │   2.2 MiB │   2.2 MiB │   0 B │   2.2 MiB │   2.2 MiB │  0 B 
 manifest │   4.5 KiB │   4.5 KiB │   0 B │  21.4 KiB │  21.4 KiB │  0 B 
      res │     1 MiB │     1 MiB │   0 B │   1.8 MiB │   1.8 MiB │  0 B 
   native │   2.6 MiB │   2.6 MiB │   0 B │     6 MiB │     6 MiB │  0 B 
    asset │     3 MiB │     3 MiB │   0 B │     3 MiB │     3 MiB │  0 B 
    other │ 200.2 KiB │ 200.2 KiB │   0 B │ 454.1 KiB │ 454.1 KiB │  0 B 
──────────┼───────────┼───────────┼───────┼───────────┼───────────┼──────
    total │  12.2 MiB │  12.2 MiB │ +25 B │  20.6 MiB │  20.6 MiB │ -4 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 35354 │ 35354 │ 0 (+1 -1) 
   types │ 11622 │ 11622 │ 0 (+0 -0) 
 classes │  9761 │  9761 │ 0 (+0 -0) 
 methods │ 52040 │ 52040 │ 0 (+0 -0) 
  fields │ 32972 │ 32972 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  333 │  333 │  0   
 entries │ 6915 │ 6915 │  0
APK
    compressed    │   uncompressed   │                        
──────────┬───────┼───────────┬──────┤                        
 size     │ diff  │ size      │ diff │ path                   
──────────┼───────┼───────────┼──────┼────────────────────────
  3.3 MiB │ +25 B │   7.2 MiB │ -4 B │ ∆ classes.dex          
 65.4 KiB │  -6 B │ 146.7 KiB │  0 B │ ∆ META-INF/CERT.SF     
 50.5 KiB │  +5 B │ 146.6 KiB │  0 B │ ∆ META-INF/MANIFEST.MF 
  1.2 KiB │  +1 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA    
──────────┼───────┼───────────┼──────┼────────────────────────
  3.4 MiB │ +25 B │   7.5 MiB │ -4 B │ (total)
DEX
STRINGS:

   old   │ new   │ diff      
  ───────┼───────┼───────────
   35354 │ 35354 │ 0 (+1 -1) 
  + ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:2637eca,r8-mode:full,version:4.0.52}
  
  - ~~R8{backend:dex,compilation-mode:release,has-checksums:false,min-api:21,pg-map-id:79ba533,r8-mode:full,version:4.0.52}

@tillh-stripe tillh-stripe force-pushed the tillh/pi-incorrect-sfu-logic branch from 1c7bde1 to c1b22a9 Compare March 28, 2023 15:40
@tillh-stripe tillh-stripe marked this pull request as ready for review March 28, 2023 15:40
@tillh-stripe tillh-stripe requested review from a team as code owners March 28, 2023 15:40
@jaynewstrom-stripe
Copy link
Collaborator

@tillh-stripe do you have any idea if this fixes this issue too? https://jira.corp.stripe.com/browse/RUN_MOBILESDK-1820

@tillh-stripe tillh-stripe marked this pull request as draft March 31, 2023 20:28
@tillh-stripe tillh-stripe marked this pull request as ready for review March 31, 2023 21:10
@tillh-stripe tillh-stripe force-pushed the tillh/pi-incorrect-sfu-logic branch from 5eb5261 to f2878d8 Compare March 31, 2023 21:29
@tillh-stripe tillh-stripe force-pushed the tillh/pi-incorrect-sfu-logic branch from f2878d8 to 3c2472e Compare March 31, 2023 21:29
@jaynewstrom-stripe
Copy link
Collaborator

@tillh-stripe were you able to figure anything out with this? #6434 (comment)

Copy link
Collaborator

@jaynewstrom-stripe jaynewstrom-stripe left a comment

Choose a reason for hiding this comment

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

LGTM, would still like to know if it fixes the other issue as well.

@tillh-stripe
Copy link
Collaborator Author

@jaynewstrom-stripe Will follow up on the other issue offline.

@tillh-stripe tillh-stripe merged commit e67ff14 into master Apr 3, 2023
@tillh-stripe tillh-stripe deleted the tillh/pi-incorrect-sfu-logic branch April 3, 2023 14:02
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