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 missing app info to some Stripe API requests #4557

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

lng-stripe
Copy link
Contributor

@lng-stripe lng-stripe commented Feb 8, 2022

Summary

Add missing app info to the headers of some Stripe API requests.

Motivation

RUN_MOBILESDK-728

Testing

  • Added tests
  • Modified tests
  • Manually verified

Steps taken:

  1. Ensure Stripe.appInfo is being set in the app
  2. Run Android Studio and begin observing network requests
  3. Make a payment using FlowController API
  4. Verify user-agent and x-stripe-client-user-agent request headers include the AppInfo part

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

Diffuse output:

OLD: paymentsheet-example-release-master.apk (signature: none)
NEW: paymentsheet-example-release-pr.apk (signature: none)

          │           compressed           │         uncompressed          
          ├───────────┬───────────┬────────┼───────────┬───────────┬───────
 APK      │ old       │ new       │ diff   │ old       │ new       │ diff  
──────────┼───────────┼───────────┼────────┼───────────┼───────────┼───────
      dex │  11.8 MiB │  11.8 MiB │ -102 B │  40.1 MiB │  40.1 MiB │ +12 B 
     arsc │   1.6 MiB │   1.6 MiB │    0 B │   1.6 MiB │   1.6 MiB │   0 B 
 manifest │   2.7 KiB │   2.7 KiB │    0 B │  11.1 KiB │  11.1 KiB │   0 B 
      res │ 697.1 KiB │ 697.1 KiB │    0 B │   1.1 MiB │   1.1 MiB │   0 B 
    asset │  77.8 KiB │  77.8 KiB │   -3 B │ 109.3 KiB │ 109.3 KiB │  -3 B 
    other │  78.3 KiB │  78.3 KiB │   +2 B │   154 KiB │   154 KiB │   0 B 
──────────┼───────────┼───────────┼────────┼───────────┼───────────┼───────
    total │  14.2 MiB │  14.2 MiB │ -103 B │  43.1 MiB │  43.1 MiB │  +9 B 

         │          raw           │           unique            
         ├────────┬────────┬──────┼────────┬────────┬───────────
 DEX     │ old    │ new    │ diff │ old    │ new    │ diff      
─────────┼────────┼────────┼──────┼────────┼────────┼───────────
   files │      3 │      3 │    0 │        │        │           
 strings │ 189052 │ 189052 │    0 │ 170340 │ 170340 │ 0 (+0 -0) 
   types │  32886 │  32886 │    0 │  30879 │  30879 │ 0 (+0 -0) 
 classes │  28488 │  28488 │    0 │  28488 │  28488 │ 0 (+0 -0) 
 methods │ 165500 │ 165500 │    0 │ 160603 │ 160603 │ 0 (+0 -0) 
  fields │ 122103 │ 122103 │    0 │ 121430 │ 121430 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  297 │  297 │  0   
 entries │ 5389 │ 5389 │  0
APK
    compressed    │  uncompressed   │                                                       
─────────┬────────┼─────────┬───────┤                                                       
 size    │ diff   │ size    │ diff  │ path                                                  
─────────┼────────┼─────────┼───────┼───────────────────────────────────────────────────────
 2.7 MiB │ -102 B │ 7.7 MiB │ +12 B │ ∆ classes2.dex                                        
 5.6 KiB │   -3 B │ 5.4 KiB │  -3 B │ ∆ assets/dexopt/baseline.prof                         
   188 B │   +2 B │     6 B │   0 B │ ∆ META-INF/androidx.activity_activity-compose.version 
─────────┼────────┼─────────┼───────┼───────────────────────────────────────────────────────
 2.7 MiB │ -103 B │ 7.7 MiB │  +9 B │ (total)

@lng-stripe lng-stripe marked this pull request as ready for review February 8, 2022 21:30
skyler-stripe
skyler-stripe previously approved these changes Feb 9, 2022
Copy link
Contributor

@skyler-stripe skyler-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. Could you add some more detail in the testing?

@lng-stripe
Copy link
Contributor Author

@skyler-stripe done.

@lng-stripe
Copy link
Contributor Author

r-codeowners?

Copy link
Contributor

@brnunes-stripe brnunes-stripe left a comment

Choose a reason for hiding this comment

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

I think a better way is to provide the AppInfo through Dagger, like all other parameters in this constructor. Or, to make things easier, keep this constructor as is and make the default Stripe.appInfo instead of null in line 95

@lng-stripe
Copy link
Contributor Author

@brnunes-stripe Can you confirm that Stripe.appInfo should be the default value? There are several other cases (e.g. 1, 2) where we're not providing app info, so your proposed change would alter behavior in parts of the library that I'm unfamiliar with.

@lng-stripe
Copy link
Contributor Author

Maybe a better question is: are there any requests where we should not be providing app info?

@brnunes-stripe
Copy link
Contributor

brnunes-stripe commented Feb 9, 2022

I think those cases are also bugs, if the user sets the AppInfo I don't see why we wouldn't send it in all requests.

@lng-stripe
Copy link
Contributor Author

@brnunes-stripe @skyler-stripe Updated primary constructor and CHANGELOG.

skyler-stripe
skyler-stripe previously approved these changes Feb 9, 2022
brnunes-stripe
brnunes-stripe previously approved these changes Feb 9, 2022
@brnunes-stripe
Copy link
Contributor

Thanks!

@lng-stripe
Copy link
Contributor Author

Rebased master to resolve CHANGELOG merge conflict.

@lng-stripe lng-stripe merged commit 8d3476e into master Feb 9, 2022
@lng-stripe lng-stripe deleted the lng/fix-missing-app-info branch February 9, 2022 22:09
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.

None yet

3 participants