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

WIP: Stripe SDK Bump, refactoring #172

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

ened
Copy link
Contributor

@ened ened commented Jun 12, 2020

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

@jonasbark
Copy link
Owner

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

@ened
Copy link
Contributor Author

ened commented Jun 26, 2020

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

@jonasbark
Copy link
Owner

@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.

@ened
Copy link
Contributor Author

ened commented Jul 22, 2020

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

@JeSuisAlrick
Copy link

Need any help with this?

@ened
Copy link
Contributor Author

ened commented Sep 8, 2020

@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?

@JeSuisAlrick
Copy link

JeSuisAlrick commented Sep 10, 2020

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?

@ened
Copy link
Contributor Author

ened commented Sep 10, 2020

@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.

@JeSuisAlrick
Copy link

@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.

@dariotrombello
Copy link

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

@ened
Copy link
Contributor Author

ened commented Dec 6, 2020

@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.

@jonasbark
Copy link
Owner

Thank you, @ened

@jonasbark
Copy link
Owner

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
Copy link
Owner

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.

@ened
Copy link
Contributor Author

ened commented Jan 24, 2021

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

@jonasbark
Copy link
Owner

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

@remonh87
Copy link

remonh87 commented Feb 16, 2021

@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).

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.

5 participants