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

Uk/stripe connect #1353

Closed
wants to merge 59 commits into from
Closed

Conversation

stveep
Copy link
Contributor

@stveep stveep commented Dec 28, 2016

Hoping this gets automatically built for testing - no action needed.

…ises controller (now not needed as moved to admin/enterprises)
@stveep stveep force-pushed the uk/stripe-connect branch from f4d5f70 to f2d9a45 Compare January 19, 2017 23:18
@stveep
Copy link
Contributor Author

stveep commented Jan 21, 2017

This is ready for review. A Stripe account can be connected to the instance (giving permission to charge customers on its behalf) and associated with an enterprise in the Payment Methods tab on the edit enterprise page. Accounts can be disconnected from the same place, and there is webhook handling to ensure the stripe-enterprise association is removed from the db if users disconnect from the Stripe dashboard.

One problem that I sometimes had locally, though not on staging, is OFN requiring me to log in again after being redirected back from Stripe. I couldn't work out why. After login, it doesn't redirect correctly and the query params are lost, so the Stripe accounts end up being connected but there is no entry in the OFN database. If I go through the process again it works, and the fact that the Stripe accts are already connected doesn't affect it. This could be improved by passing the callback params through when redirecting to the OFN authentication page, or by making an entry in the database before redirecting to stripe which then gets updated (if the stripe callback is received) or deleted/warning if the details are not received.

Send me a slack if you need API keys for testing, or it's quite easy and quick to set up your own account. You can set the callback URL to localhost in the Stripe dashboard for testing the connection flow, and disconnection via OFN, but the webhook for disconnection via Stripe is probably best tested on an actual server.

@oeoeaio oeoeaio changed the base branch from uk/stripe-connect to master February 1, 2017 05:03
@oeoeaio
Copy link
Contributor

oeoeaio commented Feb 1, 2017

Hey mate, bit of a problem here - looks like you might have merged in a copy of a stripe-connect branch that had already been rebased. See how there are two copies of most of your commits? Commits e97baca and/or ad934f1 look to be the offenders. If you're going to rebase - you need to remember that your local copy is the single source of truth. Ideally you would keep public copies of feature branches that you are going to be rebasing to a minimum. Any public branches that you do push to will need to be written over whenever you rebase your local branch (rather than merged back into the local copy). This is because git can't tell that the commits in the un-rebased public branch are the same as those in the rebased local branch, so it just includes both copies in the commit history.

@oeoeaio
Copy link
Contributor

oeoeaio commented Feb 1, 2017

I've had a quick go at fixing the commit history - it's in my repository stripe-connect.

From what I can gather, you were merging the public branches back in because you wanted to pick up af67f56, right? Best way to do this on a branch that you're going to be rebasing is to use git cherry-pick af67f56. That way you get the code that you actually want, without all of the baggage of the out-of-date (un-rebased) commit history as you do in a git merge.

@stveep
Copy link
Contributor Author

stveep commented Feb 1, 2017

Hi sorry about this - must have been something to do with switching between computers to work on and pushing to my repo in the process and/or having to merge master to deploy to staging to test the webhooks. Your repo branch looks correct as far as I can see - thanks for sorting it. Let me know if you still need me to do anything?

@oeoeaio
Copy link
Contributor

oeoeaio commented Feb 1, 2017

No worries! I have done far far worse, trust me. :)

I'll keep playing around today and try to work out what our next steps are...

@oeoeaio
Copy link
Contributor

oeoeaio commented Feb 23, 2017

Superseded by #1468

@oeoeaio oeoeaio closed this Feb 23, 2017
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.

2 participants