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

Use Stripe Express #373

Merged
merged 3 commits into from Nov 10, 2018
Merged

Use Stripe Express #373

merged 3 commits into from Nov 10, 2018

Conversation

@sholladay
Copy link
Contributor

sholladay commented Jul 8, 2018

"Express", as Stripe calls it, is a new flow for setting up Stripe Connect accounts. It is more friendly to users and looks better. Stripe is recommending using this going forward, though the older "Standard" account setup is still available.

I'm not sure what the semver policy is here for changes to providers, but this should probably be considered a breaking change for Stripe users, to be safe, since there are some slight differences when interacting with Stripe Express accounts via their API. There are minimal changes for bell users, though.

@sholladay sholladay force-pushed the sholladay:master branch from 69d2c9d to 6ff713c Jul 9, 2018
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Jul 10, 2018

I do love me some shiny new flows
Do you have some docs or a changelog about this change?

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Jul 10, 2018

Users will want to review these:

The extent to which the above affects an app depends on how Stripe is being used. Most apps will simply need to provide a link to the Express dashboard for their users.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Jul 12, 2018

Hmm this kinda bothers me Users based in the United States :/
Also this would be a breaking change.
Would it be an option to add this as a separate provider?

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Jul 12, 2018

Companies outside the US are able to contact support to have their account set up manually. Removing that step is also on the roadmap.

this would be a breaking change

Agreed. Our app kept working as-is after this change, though. The dashboard is an optional thing, as are the params for prefilling forms. I would say the API version is the most notable here, but even that merely requires an API version that is well over a year old. I doubt any bell users are still on an even older one, but they could be.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Jul 16, 2018

I'm guessing your app is US based then? :P
I know for a fact that there are companies using 1 year old Stripe api versions but maybe those are outliers.
I guess we can go with a major version and then document and point to those links.

@AdriVanHoudt AdriVanHoudt self-assigned this Jul 18, 2018
@AdriVanHoudt AdriVanHoudt added this to the 10.0.0 milestone Jul 18, 2018
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Oct 30, 2018

I would add this as a new provider for now, not replacing the existing one.

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Oct 30, 2018

IMO, if the breaking change is too unpalatable, then an express boolean option is the way to go. Stripe support has made it clear to me that they are advising all apps to move away from "Standard" accounts. I'm not sure it's fair to call the old flow officially deprecated yet, but it's days are numbered. All new accounts should be using Express and thus I think bell should move to this, otherwise new users are setting themselves up for more pain later, possibly without realizing it.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Oct 30, 2018

On its own, doesn't seem like this is worth a breaking change release. How you choose to do this is up to you.

@sholladay sholladay force-pushed the sholladay:master branch from 6ff713c to 60b355a Oct 30, 2018
@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Oct 30, 2018

Okay, this is ready for review again. I rebased and added an express option that maintains compatibility. This is no longer a breaking change. Hoping the option can go away in a future release, though. :)

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Oct 31, 2018

CI failed only for Node 11 and I don't think it's related to this PR. The output is:

The following leaks were detected:queueMicrotask

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 3, 2018

Need lab 17.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Nov 7, 2018

I like the option but don't make it required to pass an object into Bell.providers.stripe() cause that would be breaking and doesn't have to be I think.

It is weird that Stripe is advocating to move to Express if it is only available for US entities :D /shrug

Rest LGTM

@Nargonath

This comment has been minimized.

Copy link
Member

Nargonath commented Nov 7, 2018

Perhaps it would be nice to display a deprecation warning when not using the express option so people are aware of this change to come if not already.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Nov 7, 2018

As long as this is on their website

While platforms located anywhere Stripe is available can use Express, the Express account itself can only be created for individuals, businesses, and non-profits based in the United States.

I don't think I want that. If Stripe is so persistent on switching they will contact their customers anyway no? I don't think that is Bell's responsibility.

@Nargonath

This comment has been minimized.

Copy link
Member

Nargonath commented Nov 7, 2018

Yeah I think they'd contact their customers directly for sure but perhaps that'd be a good thing to already allow such behavior without replacing the old one. That way we don't break anything for the existing users and those that want to use express can do so. express will eventually replace the old way it is just a matter of time as I understood. Let's just get ahead of it.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Nov 7, 2018

I'm totally on board with adding this. If you don't require the options object this is good to go for me.

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Nov 8, 2018

I will make the object optional. Earlier, I noticed that all of the other providers that take an object require it. I just copied what the others were doing on the assumption that bell would pass an empty object if needed. But yeah, the provider can be used more directly, so I guess that makes sense despite the inconsistency.

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Nov 8, 2018

Okay, I pushed the requested changes. Ready for review.

For future reference, Stripe will manually set you up if you need an Express account or customers outside of the United States. Their documentation is overly cautious so as to set expectations. It was one of our first concerns when signing up. They are working on automating it but in the meantime, it's a manual process and you have to contact them and ask.

@sholladay

This comment has been minimized.

Copy link
Contributor Author

sholladay commented Nov 8, 2018

Woah, I'm getting completely different results locally (where everything is passing) vs CI.

Will do that Lab upgrade... Scratch that, probably best for #385 to land first.

@hueniverse hueniverse merged commit 03d326b into hapijs:master Nov 10, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse hueniverse modified the milestones: 10.0.0, 9.3.2 Nov 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.