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

[MBL-864] Stripe Upgrade #1834

Merged
merged 6 commits into from Jul 19, 2023
Merged

[MBL-864] Stripe Upgrade #1834

merged 6 commits into from Jul 19, 2023

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Jul 12, 2023

📲 What

Part of maintenance to upgrade all libraries to latest versions. 2023 Technical Plan lists Stripe because their have been many updates on Stripes side.

  • They've added the address element, many fixes to the payment sheet (including more payment types), a customer sheet, etc.
  • They've also enabled Apple Pay Later, which we should not have enabled in our app.

Entire release logs go from 22.8.4 -> 23.10.0
https://github.com/stripe/stripe-ios/releases/tag/22.8.4

🤔 Why

Upkeep with iOS system improvements and features from Stripe.

🛠 How

  • Started with 22.8.4.

  • 23.0.0 was a breaking change because Stripe split their single framework into (Stripe, StripePaymentSheet, StripePaymentUI, StripeApplePay and a few others). Details.

    • Basically in app these means we just switch import Stripe to import StripePaymentSheet or leave it as is for non-payment sheet references.
    • One change to fix our tests was to go from using PaymentSheet.FlowController.PaymentOptionDisplayData to PaymentSheetPaymentOptionsDisplayData. The former is a Stripe data structure that crashes in tests because finding a card brand causes a module reference to fail being found.
    • PaymentSheet.FlowController.PaymentOptionDisplayData is just an UIImage and String so we can make our own version and use it in-app easily.
  • Updated Stripe in SPM to Up to Next Major (23.10.0 < 24.0.0).

  • Also did a bit of clean up removing the AddNewCardViewController from Settings storyboard, a card brand type extension and an unused PaymentCardTextField

✅ Acceptance criteria

  • Ensure no changes to Apple Pay
  • Ensure no changes to Payment Sheet (iPad and iPhone) UI
  • Ensure that AirTable script that presents the Pledge Page Stripe Payment Sheet is run through and all tests pass.
  • Ensure that AirTable script that presents the Stripe Payment Sheet in User Settings is run through and all tests pass.

need to do device testing.
try add tests back in of pre-existing payment sheet.
@msadoon msadoon added this to the release-5.9.0 milestone Jul 12, 2023
@msadoon msadoon self-assigned this Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #1834 (e4d0488) into main (4b48213) will increase coverage by 3.90%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main    #1834      +/-   ##
==========================================
+ Coverage   84.47%   88.37%   +3.90%     
==========================================
  Files        1278      887     -391     
  Lines      115258    79619   -35639     
  Branches    30688    21051    -9637     
==========================================
- Hits        97359    70363   -26996     
+ Misses      16831     8500    -8331     
+ Partials     1068      756     -312     
Impacted Files Coverage Δ
Library/Tracking/KSRAnalytics.swift 82.48% <ø> (ø)
...iewModels/PledgePaymentMethodsViewModelTests.swift 96.80% <ø> (ø)
Library/TestHelpers/Stripe+PaymentMethod.swift 87.50% <83.33%> (-12.50%) ⬇️
...ibrary/PaymentSheetPaymentOptionsDisplayData.swift 100.00% <100.00%> (ø)
Library/ViewModels/PaymentMethodsViewModel.swift 98.17% <100.00%> (ø)
...ary/ViewModels/PledgePaymentMethodsViewModel.swift 90.23% <100.00%> (ø)

... and 394 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msadoon msadoon marked this pull request as ready for review July 13, 2023 20:03
@msadoon msadoon requested a review from scottkicks July 13, 2023 21:47
@msadoon msadoon added needs review and removed WIP labels Jul 13, 2023
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

All AC passed

  • No changes to Apple Pay look to have changed
  • No changes to Payment Sheet (iPad and iPhone) UI have changed
  • Airtable scripts/tests pass

Just left a comment about the module imports you've updated. I think just a couple small changes could be made, but nothing major.

OOO the rest of the week after today, so use your best judgment to merge this if you make those changes and I'm already gone.

@@ -5,6 +5,7 @@ import Prelude
import ReactiveExtensions
import ReactiveExtensions_TestHelpers
@testable import Stripe
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this import now that we're using StripePaymentSheet module.

I think we can update in PaymentMethodsViewModelTests as well.

we could also update the import in STPPaymentHandlerActionStatus extension. importing Stripe does technically give us access to StripePaymentHandlerActionStatus, but StripePaymentSheet is more directly what we want I think.

Copy link
Contributor Author

@msadoon msadoon Jul 17, 2023

Choose a reason for hiding this comment

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

Good point - tested removing import Stripe in favor of import StripePaymentSheet and tests still pass.
In STPPaymentHandler+StripePaymentHandlerActionStatus and PledgePaymentMethodsViewModelTests
Corrections made, will update in next commit, just want to run the app again, surface the payment sheet as a sanity check. corrections

Copy link
Contributor Author

@msadoon msadoon Jul 19, 2023

Choose a reason for hiding this comment

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

@scottkicks I wanted to call out this mistake in your last pr that caused a build crash. I only noticed it when trying to build this branch to run on device.

It's easy to miss in the review because its' a change to the .project.pbxproj file, which is a massive directory so hard to track file membership (ie which file belongs to which framework).

Turns out GraphAPI.TriggerThirdPartyEventInput+TriggerThirdPartyEventInputTests.swift and TriggerThirdPartyEventInputTests.swift were added to KsApi when they should be added to KsApiTests

Fixed here.

I had to go back to my fix graph schema pr, run it on device, see that it was working then work backwards to find the pr above caused the on-launch crash.

Just be careful with adding the files to the right frameworks because it effects the run-time scenario in the app, even if the code compiles. Not sure why tests still passed in CI.

@msadoon msadoon merged commit d9734ff into main Jul 19, 2023
6 checks passed
@msadoon msadoon deleted the mbl-864/upgrade-stripe branch July 19, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants