Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

payment methods #470

Closed
wants to merge 34 commits into from
Closed

payment methods #470

wants to merge 34 commits into from

Conversation

galkahana
Copy link

@galkahana galkahana commented Apr 17, 2019

Proposed changes

  • added payment methods dialog support
  • upgraded android sdk

Types of changes

What types of changes does your code introduce to tipsi-stripe?
Put an x in the boxes that apply

  • Bugfix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! Next steps are a reminder of what we are going to look at before merging your code.

  • I have added tests that prove my fix is useful or that my feature works
  • I have added necessary documentation (if appropriate)
  • I know that my PR will be merged only if it has tests and they pass

Further comments

example:
To support client session manipulation, and ephemeral key must be sent. but you need to version of the sdk to send it. so - 2 stages to win:

// first step. you can send any option that gets sent to add card dialog. support is precisely the same.
// note that apple pay option is provided automatically if you add a merchant id in stripe options (google pay is underway...first i have to get the stripe android guy to pull my other PR ;) )
const apiVersion = await tipsiStripe.paymentRequestWithPaymentMethods({
    createCardSource: true,
})
// get the ephemeral key based on the response api version
const ephemeralKey = await this.requestEphemeralKey(apiVersion)
// complete the call which will load the dialog.
const result = await tipsiStripe.completePaymentRequestWithPaymentMethods(ephemeralKey)

returned result is an object with a resultType member that can be one of:

  • STPSource
  • STPCard
  • STPApplePayPaymentMethod
    (and hopefully soon also STPGooglePayPaymentMethod).
    For the first two you'll continue by charging the customer at backend. for Apple pay (and the other one, when added) you'll follow up launching the relevant native payment workflow, with the existing tipsi functionality, then pass the result token to server and perform a regular charge (this will automatically make that method the default, for subscriptions purpose).

Cheers, and thanks for this lovely library.

@cybergrind
Copy link
Member

Hi @galkahana ! that is quite a big feature, thank you for your work. There are some things to do before we can merge it:

  1. tests should pass
  2. documentation for the new API
  3. E2E tests for the new API

@galkahana
Copy link
Author

cool. i'll try to get to it soon.

@galkahana
Copy link
Author

Hi, I updated the branch to pass CI tests, as well as add a test for the payment method scene.
How boy that was loads of fun ;).

A couple of notes:

  1. Stripe Android SDK - I needed an updated of the Stripe Android SDK that Fixed a bug where the android add card dialog would just hang. Unfortunately,
    there's yet to be a release that is working with tipsi (there's this bug, and later there was android X not compatible with RN, and then it was removed
    but another bug came in), so currently i'm using my own copy of the current master, which is where things are corrected and which i tested and works. I think there's gonna be an official release soon. hopefully it will work. I can then update to use an official version.
  2. Added test backend - The Payment Methods dialog requires an ephemeral key providing server. This is tied to ones Stripe account private key. Obviously - I don't have your key, so I can only operate the tests either on my own travis account, or locally. At this point I made the test stop and pass if there's no available server. If you want to add a server, Please add an environment variable named BACKEND_URL with a relevant server. you can easily create one by using the ready sample server (This is what i did) available here. Have your stripe account private key ready.
  3. Building docs - While I did update the docs with the new methods, I didn't understand how to build them (and test what I built). So either lemme know how to do that, or complement yourself. please.
  4. Running tests locally - On an unrelated (but much related) note: Running the tests locally for iOS is a bit of a mess thanks to XCode 10 incompatibility with React Native. Two things I detected that are a problem for the tests:
    • The workspace needs to be set to run with Legacy build system, otherwise compilation fails. While I did find workspace for the example, for some reason it's not copied (?) to the example_tmp, and so whatever I do there don't matter. If, however, the example_tmp project workspace file is fixed, compiling works.
    • simctl, run with xcrun simctl help, changed it's output so now running react-native run-ios fails cause it can't find a simulator. The common solution is to tinker with the method that finds the simulators in post-install like proposed here.

Cheers,
Gal.

@cybergrind
Copy link
Member

Great job, Gal!
Yeah, CI fixed can be rough, we know that =)
Probably 2 will require some work on our side.
Also I prefer to merge it after we get 1 in official release, so we have to hang on with this PR little bit longer.
Anyway, you did awesome work, thank you!

@galkahana
Copy link
Author

Cheers man. thank you very much for the lib.

android/build.gradle Outdated Show resolved Hide resolved
@galkahana
Copy link
Author

updated the android lib. and now the pr doesn't pass on iOS. makes me think it doesn't have to do much but i cant experiment here by restrting the job. so gonna play on my travis copy and let you know.

@galkahana
Copy link
Author

updated to use the latest android sdk release, so no need to rely on my repo anymore. non pod ios fails test...but honestly didn't touch this. especially shouldn't trigger no workspace...as there's none...plus no relevant change to this since the test succeeded last time. was all about android.

@cybergrind
Copy link
Member

@galkahana everything is good. But Alipay tests are failing again, I'm not sure were they changing designs from time to time but it looks like we need to fix this anyway, probably with some better selector.

@galkahana will you be able to fix Alipay tests or I should ask someone else to do that?

@galkahana
Copy link
Author

i'll try to find some time but would be grateful if someone else could do it :)
b.t.w i reckon the firebase thing was failing the ios builds. as of march 2019 they no longer support xcode 9

@cybergrind cybergrind mentioned this pull request Jun 13, 2019
@@ -133,6 +166,19 @@ private static int androidPayModeToEnvironment(@NonNull String androidPayMode) {
return ANDROID_PAY_MODE_TEST.equals(androidPayMode.toLowerCase()) ? WalletConstants.ENVIRONMENT_TEST : WalletConstants.ENVIRONMENT_PRODUCTION;
}

public void delayEphermalKeyResolution(String apiVersion,

Choose a reason for hiding this comment

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

Typo: Ephermal 👉 Ephemeral

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public void delayEphermalKeyResolution(String apiVersion,
public void delayEphemeralKeyResolution(String apiVersion,

@@ -7,7 +7,7 @@
<uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/>

<uses-sdk
android:minSdkVersion="16"
android:minSdkVersion="19"
android:targetSdkVersion="22" />

Choose a reason for hiding this comment

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

android:targetSdkVersion probably should be bumped to match the 28 set elsewhere here too.

@@ -7,65 +7,66 @@ import errorCodes from './errorCodes'

const { StripeModule } = NativeModules

function toPlatformEphermalKey(keyAsObject) {

Choose a reason for hiding this comment

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

Same typo: Ephermal to Ephemeral

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants