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

Add StripeConnectAccount update endpoint for assigning an external account #583

Merged
merged 1 commit into from
Dec 21, 2016

Conversation

begedin
Copy link
Contributor

@begedin begedin commented Dec 19, 2016

What's in this PR?

Adds an update endpoint for StripeConnectAccount, which only allows assigning an external account. Everything else is reported as an unauthorized request.

References

Fixes #572

@joshsmith
Copy link
Contributor

@begedin LGTM

@joshsmith
Copy link
Contributor

@begedin to be clear, though, this does not fix #572 in its entirety.

@joshsmith
Copy link
Contributor

We still need to allow for updates to the StripeConnectAccount for all the other changes that can be made from the Ember app, like the personal/business information, identity document, etc.

@begedin
Copy link
Contributor Author

begedin commented Dec 19, 2016

I would either rewrite the issue description in #572 then, or change it's title and open a new issue. The description makes it seem like only the external_account field is supposed to be updatable by it.

I would actually lean against too specific update actions (this one, and the one proposed for attaching a verification document), but the issue description made it seem to me like this is what's required.

@joshsmith
Copy link
Contributor

Agree wholeheartedly on not having separate update actions, and think we should avoid that here. I'm definitely onboard with that. The only case where they are advisable, I think, is when they kick off complex separate actions that result in hitting Stripe's API at multiple endpoints. That's obviously avoidable here and instead we're listening for the external account solely in a webhook. Single update here, single update to Stripe.

StripeConnectAccountService.add_external_account(account, "ba_test123")
assert updated_account.external_account == "ba_test123"
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Really I think this should just be update now, no?

@joshsmith
Copy link
Contributor

I think the issue for #572 was written incorrectly by me and instead needs to focus on adapting to changes brought into stripity_stripe by @SViccari.

@begedin
Copy link
Contributor Author

begedin commented Dec 19, 2016

This needs more work. The process should be

  • update the Stripe API connect account record with external_account.
  • this will trigger a account.external_account.created webhook, handled in Add a webhook for account.external_account.created #573. Handling the webhook will insert an external account record into our database.
  • however, we should update the stripe_connect_account record as part of this request immediately, using the response we get from the stripe API. Otherwise, the account creation status fields will not update in time.

@joshsmith
Copy link
Contributor

@begedin stripity_stripe account update is merged now.

@begedin begedin force-pushed the 572-add-connect-account-update branch from 4959684 to 013d7ab Compare December 20, 2016 11:46
@begedin
Copy link
Contributor Author

begedin commented Dec 20, 2016

@joshsmith I'm not seeing any code related to Stripe.Account.update in stripity_stripe, merged or otherwise.

What did get merged was the rewrite of the conversion process.

I'll start a PR of my own.

@begedin
Copy link
Contributor Author

begedin commented Dec 20, 2016

@joshsmith What this does now is:

  • controller receives an update request with the parameter "external_account"
  • handle_update matches against this parameter only. There is a catch-all match that just returns a 403 (we can add in matchers for document/recipient in other PRs).
  • StripeConnectAccountService.add_external_account puts this change to the stripe api (new PR for Account.update).
  • Api replies with a Stripe.Account record, which we use to update the local record.

NOTES:

  • Right now, a Stripe list object is not handled properly with stripity stripe. Once that happens, we need to do a minor change here.
  • I had to modify StripeTesting.Account a good amount to allow for updates. Sadly, the Account object is complex, so I had to add helpers to build a fixture with nested data. I don't like it, but I couldn't think of a better way.
  • Tests are failing because we need the stripity_stripe PR merged. I used a local package. Add Account.update/2 beam-community/stripity-stripe#166

@joshsmith
Copy link
Contributor

@begedin can you bring in the new stripity_stripe or rerun the build so it does so?

@begedin
Copy link
Contributor Author

begedin commented Dec 21, 2016

@joshsmith stripity_stripe is up to date now.

Regardless of merge order, this is bound to have some conflicts with the other PRs. I recommending merging #588 first, because using that as a baseline will make resolving conflicts easier.

@joshsmith joshsmith force-pushed the 572-add-connect-account-update branch from a6250c1 to 914a4ae Compare December 21, 2016 22:25
@joshsmith joshsmith merged commit 9e49f92 into develop Dec 21, 2016
@joshsmith joshsmith deleted the 572-add-connect-account-update branch December 21, 2016 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants