flutter_stripe_payment icon indicating copy to clipboard operation
flutter_stripe_payment copied to clipboard

WIP: Stripe SDK Bump, refactoring

Open ened opened this issue 5 years ago • 17 comments

Apologies for this way-too-large PR. I understand this may be to much to consider merging, hence I marked it WIP and will try to cut things out when possible.

We are developing a paid feature in our Flutter app and have run into various use cases where the current flutter stripe payment plugin was not working as expected.

This started as a SDK bump (hence the name) but quickly evolved in a full refactor to match the latest conventions and resolve some memory issues.

A rough summary of changes:

  • Bumps SDK versions on iOS & Android
  • Adds new constants to the iOS card networks
  • Implements createPaymentMethodFromGooglePay (our app supports CC, Pay, GPay)
  • Removes memory leaks and static context references (critical once you start enabling developer options on Android)
  • Some bug fixes regarding the handling of payment method vs. setup intent creation
  • Tiny cleanups along the way
  • Replace manually written Dart DTOs with generated versions. This results in less code to maintain and adds support for immutability.

There is more work to do, therefore please do not merge, however we are planning to ship our current app with this version of the plugin applied.

The upcoming change is about the list of supported card networks (we need to be able to limit those from app side during Apple/Google pay selection) and there is a new method to display the Google Pay overlay, the recommended method uses Androids JSON classes for configuring the display request.

If you have any feedback, please share it here. Thank you.

Help testing this PR

The PR is large & contains a few new features & fixes. It does require a bit of manual testing.

Add the following to your pubspec.yaml:

dependency_overrides:
  stripe_payment:
    git:
      url: https://github.com/ened/flutter_stripe_payment.git
      ref: stripe-sdk-bump

ened avatar Jun 12 '20 17:06 ened

Thank you for this! I hope to be able to give it a closer look, soon

jonasbark avatar Jun 25 '20 18:06 jonasbark

@jonasbark thank you. There will be a few more commits to clean things up..

ened avatar Jun 26 '20 13:06 ened

@ened I finally had some time. Looks pretty solid in my opinion. You mentioned additional commits - do you still plan to make them? I'll do a proper review + merge afterwards.

jonasbark avatar Jul 16 '20 13:07 jonasbark

Yes still planning to. Will need a few days, currently busy with other tasks

ened avatar Jul 22 '20 05:07 ened

Need any help with this?

JeSuisAlrick avatar Sep 08 '20 12:09 JeSuisAlrick

@JeSuisAlrick think it would be great to get the basic unit tests done - eg. check whether the native side is called with the correct parameters, in step 1. Is this something you'd like to help with?

ened avatar Sep 08 '20 12:09 ened

I'll see how best I can contribute.

The working code is stripe-sdk-bump-modernize-gpay-integration, correct?

Also, I'm not familiar with the codebase or the Stripe flow. When you say step 1, what are you referring to?

JeSuisAlrick avatar Sep 10 '20 03:09 JeSuisAlrick

@JeSuisAlrick the code for this PR is in my fork: https://github.com/ened/flutter_stripe_payment/tree/stripe-sdk-bump. I suggest you fork that and send a PR towards the stripe-sdk-bump branch there and I can merge it in manually. This will gradually expand this PR.

Step 1 referred to step 1 in the help needed, not related to Stripe at all. :)

Here's a example on the type of tests needed: https://github.com/nashfive/phone_number/blob/master/test/phone_number_test.dart - essentially test that all API methods call their correct counterparts in the plugin using invokeMethod.

ened avatar Sep 10 '20 05:09 ened

@JeSuisAlrick the code for this PR is in my fork: https://github.com/ened/flutter_stripe_payment/tree/stripe-sdk-bump. I suggest you fork that and send a PR towards the stripe-sdk-bump branch there and I can merge it in manually. This will gradually expand this PR.

Step 1 referred to step 1 in the help needed, not related to Stripe at all. :)

Here's a example on the type of tests needed: https://github.com/nashfive/phone_number/blob/master/test/phone_number_test.dart - essentially test that all API methods call their correct counterparts in the plugin using invokeMethod.

Simple enough. I'll get it sorted.

JeSuisAlrick avatar Sep 11 '20 01:09 JeSuisAlrick

Is anything happening here? I really need some fixes in this PR to be able to use this package in production.

dariotrombello avatar Dec 04 '20 18:12 dariotrombello

@dariotrombello The description has been updated with testing instructions. Appreciate if you could help out testing and report back issues / crashes etc.

@jonasbark Rebase is done, hope I find time next week to finish the open topics.

ened avatar Dec 06 '20 07:12 ened

Thank you, @ened

jonasbark avatar Dec 06 '20 07:12 jonasbark

Hi @ened I had a look at the current branch and it looks fairly well to me, although I didn't test everything. Are there any major things missing?

jonasbark avatar Jan 23 '21 15:01 jonasbark

I pushed a commit in the same branch in this repo which fixes two issues I encountered. createTokenWithCard still fails with a NPE - I'm not sure why.

jonasbark avatar Jan 23 '21 15:01 jonasbark

@jonasbark do you have a test case for this? I am looking at bumping the native SDK versions once more before we can merge.

ened avatar Jan 24 '21 14:01 ened

Just started the example app on Android (11), clicked on Create Token with Card and got the error

jonasbark avatar Jan 24 '21 15:01 jonasbark

@ened @jonasbark what is still needed to integrate this PR? I am willing to help out since I think it would be good to update the dependencies (btw I noticed some are already outdated).

remonh87 avatar Feb 16 '21 15:02 remonh87