Skip to content

Code spike for switch to managed accounts #552

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.

Sign up for GitHub

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

Jump to bottom
Closed
begedin wants to merge 4 commits into from
Closed

Code spike for switch to managed accounts #552

begedin wants to merge 4 commits into from

Conversation

@begedin
Copy link
Contributor

@begedin begedin commented Dec 9, 2016 •
edited
Loading

What's in this PR?

This is a code spike to make the switch to managed stripe accounts

What I got

  1. connect account create endpoint recieves arguments from client
  2. calls StripeService.StripeManagedConnectAccountService.create
  3. Adapter parses arguments and creates a param hash to create account on stripe API
  4. Record is created using stripity-stripe (the latest commit from Add Account.create beam-community/stripity-stripe#153) and then inserted into the db locally.

NOTES:

Haven't added/changed any database fields yet. Not sure which ones will be needed. I use those we have, others are virtual for now.

References

@begedin begedin added this to the Launch Donations milestone Dec 9, 2016
This was referenced Dec 9, 2016
Merged
Closed
@joshsmith
Copy link
Contributor

joshsmith commented Dec 12, 2016

Hadn't had a chance to comment on this, but may want to consider reducing scope by pulling off some of the individual tasks in the milestone now that they're around.

joshsmith
joshsmith reviewed Dec 12, 2016
View reviewed changes
web/controllers/stripe_connect_account_controller.ex Outdated

alias CodeCorps.StripeConnectAccount
alias CodeCorps.StripeService.StripeConnectAccountService
alias CodeCorps.StripeService.StripeManagedConnectAccountService
Copy link
Contributor

@joshsmith joshsmith Dec 12, 2016

Choose a reason for hiding this comment

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

Would just keep this StripeConnectAccountService personally. managed is just a param.

Copy link
Contributor Author

@begedin begedin Dec 12, 2016

Choose a reason for hiding this comment

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

It was just a temporary thing so I can work on a new one while still having the old one for reference.

lib/code_corps/stripe_service/adapters/stripe_connect_account.ex Outdated
last_name: last_name,
ssn_last_4: ssn_last4,
type: recipient_type,
# TODO: Decide on setting legal_entity.address or legal_entity.personal_address
Copy link
Contributor

@joshsmith joshsmith Dec 12, 2016

Choose a reason for hiding this comment

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

I think this is mentioned in that Ember issue.

lib/code_corps/stripe_service/stripe_managed_connect_account.ex Outdated
|> CodeCorps.Repo.insert
end
end
end
Copy link
Contributor

@joshsmith joshsmith Dec 12, 2016

Choose a reason for hiding this comment

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

I rewrote some of this with pseudocode in #575. I would consider taking the pattern matching of that function. Would also just use create_changeset/2 rather than a special one for managed, per comment above.

web/models/stripe_connect_account.ex
:managed, :id_from_stripe, :charges_enabled, :transfers_enabled
]

def managed_create_changeset(struct, params) do
Copy link
Contributor

@joshsmith joshsmith Dec 12, 2016

Choose a reason for hiding this comment

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

Don't think we want distinction between create and managed_create. Also, create fields will be just country and managed, where the latter is always true.

@begedin begedin force-pushed the spike-for-managed-accounts branch from f04a125 to 1af50be Compare December 12, 2016 14:52
@joshsmith
Copy link
Contributor

joshsmith commented Dec 18, 2016

I'm going to close this since I think it's pretty out of date.

@joshsmith joshsmith closed this Dec 18, 2016
@joshsmith joshsmith deleted the spike-for-managed-accounts branch December 18, 2016 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshsmith joshsmith joshsmith left review comments

Assignees

No one assigned

Labels

needs code review

Projects

None yet

Milestone

Launch Donations

Development

Successfully merging this pull request may close these issues.

2 participants

@begedin @joshsmith