Add support for Stripe Connect #336

lukeburden · 2017-05-11T19:48:40Z

I've implemented managed accounts. This code is stable and in-use, now I'd like to get it back into this project.

It adds the basic ability to create managed accounts, transact on their behalf and transfer funds to external bank accounts.

@paltman I've obviously got some work to do on tests, but before finishing those up I'd like the greenlight from you on:

general approach
addition of stripe_account to various models
improvements to the robustness of the webhook handling code
the initial and dynamic forms for collecting Bank Account data
changes to sync_customers (I think this really needs to be built upon as it's still only customers, sources, charges and invoices)

Let me know what you think!

Thanks,

Luke

paltman

A few small changes requested, but nothing major.

VERY IMPRESSIVE work! Thanks for taking the time to make a pull request.

paltman · 2017-05-19T03:37:12Z

pinax/stripe/models.py


 def __str__(self):
- return str(self.user)
+ return unicode(self.user)


shouldn't need unicode here because of the @python_2_unicode_compatible class decorator.

paltman · 2017-05-19T03:40:21Z

.pep8

@@ -0,0 +1,5 @@
+[flake8]


why is this file added? there is already a config for flake8 in tox.ini. I'm also not so quick to ignore all those other things.

Just a config file for pep8/flake8. I'll remove it.

paltman · 2017-05-19T03:40:53Z

pinax/stripe/actions/accounts.py

+ Returns:
+ a pinax.stripe.models.Account object
+ """
+ kwargs['country'] = country


I'd like to stay consistent with rest of code base and not use single-quotes, but instead use double-quotes.

lukeburden · 2017-06-16T00:49:52Z

Hola @paltman - I've updated as requested. Glad it's mostly good! Let me know if there are any other adjustments you'd like me to make.

Is your preference for everything to be rebased into a single commit for merge?

iMerica · 2017-06-21T20:26:29Z

I'm glad I found this. I was about to embark on the same work!

I've implemented managed accounts

Per Stripe's documentation, there are three Connect Account Types:

Standard
Express
Custom

Which of these types are supported by this PR?

iMerica · 2017-06-21T20:48:08Z

pinax/stripe/models.py

@@ -24,8 +24,16 @@ class Meta:
 abstract = True


+class AccountRelatedStripeObject(StripeObject):


Would it be better to have two abstract classes (mixin pattern) instead of the hierarchy? This way only specific models that need the field can use it. Example:

class Customer(StripeObject, AccountRelatedStripeObject): ...

If you think Stripe Connect's business logic touches every model, then it makes sense, but I'm not sure it does.

@iMerica - "specific models that need the field can use it" - this is the intent. Models that can be account specific (which isn't all of them - it can be redundant to make them all partitioned by account) inherit from the AccountRelatedStripeObject abstract model.

Let me know if I've missed your point.

lukeburden · 2017-06-21T22:20:13Z

@iMerica My use case required full customisation of the Connect experience - so the pieces are added with the ability to add, update, transfer to and payout to connected accounts via the API.

The other account types won't allow you to manipulate connected account data via the API. But I suppose some subset of the Account info is available and sync'd via webhooks?

You'll need to experiment!

iMerica · 2017-06-21T22:32:42Z

Okay cool thanks for the update. I'm not an expert or maintainer in this codebase, but your changes LGTM. Maybe just add a new "Connect" section to the docs describing a hypothetical use case that can be supported by these changes?

blueyed · 2017-08-30T11:45:03Z

This needs to be rebased / updated.
What is the status of this?
I'd be glad to help out getting this finished.

lukeburden · 2017-08-31T20:05:19Z

Hello - I'll rebase my changes in the next couple of days. Keen to get this pushed through, too.

@blueyed - one way you could help would be playing around a bit with the DynamicManagedAccountForm and InitialManagedAccountForm classes. I wrote them and have not had a chance to give them much of a try, as the project I'm working on is lower-volume for account owners and we won't be hitting the later stages of account verification in the near-term.

blueyed · 2017-09-01T14:12:08Z

@lukeburden

Keen to get this pushed through, too.

Awesome.

I think there are things that could be moved out of this PR into separate ones, starting with "Improving speed of syncing large data sets from stripe" already (and the followups to it).
I could move them out into separate PRs, if you don't have the time yourself at the moment.
We're not making use of the forms ourselves, so they are likely the last for me to look at anyway.
Just for reference: some discussion on Stripe Connect integration for dj-stripe: Including Stripe Connect functionality dj-stripe/dj-stripe#97 (comment)

blueyed

"Managed accounts" has been renamed to "Custom accounts": https://stripe.com/docs/connect/custom-accounts#name-change

paltman · 2017-09-04T11:00:54Z

I'm very excited for this feature. Thanks for hammering on it.

…

On Sep 4, 2017, at 5:27 AM, Daniel Hahler ***@***.***> wrote: @blueyed commented on this pull request. "Managed accounts" has been renamed to "Custom accounts": https://stripe.com/docs/connect/custom-accounts#name-change — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

blueyed

Left some comments.
I have some fixup commits, which I've pushed to https://github.com/blueyed/pinax-stripe/tree/connect-fixes.

@lukeburden
You should be able to squash the fixup commits, and otherwise I've taken some "minor:" things out of the previous commit(s) - so you could squash them all into a single one.

blueyed · 2017-09-04T11:47:15Z

pinax/stripe/actions/subscriptions.py

- raise
+ subscription = stripe.Subscription.retrieve(sub_id)
+ # if subscription and not subscription.customer == customer.stripe_id:
+ # return


Commented code. Should likely throw in case it does not match the customer instead?!

I'll just uncomment this for now, leaving the behaviour unchanged.

blueyed · 2017-09-04T12:09:10Z

pinax/stripe/models.py

+ return stripe.Invoice.retrieve(
+ self.stripe_id,
+ stripe_account=(
+ self.customer.stripe_account if self.customer_id else None


Better to use hasattr(self, 'customer') here / or a try/except self._meta.model.customer.RelatedObjectDoesNotExist?!

Care to explain why those more verbose approaches would be better? :)

blueyed · 2017-09-04T12:13:13Z

pinax/stripe/models.py

+
+ @property
+ def stripe_account(self):
+ return stripe.Account.retrieve(self.stripe_id)


This should be a cached property, similar to 79424ed probably?!

@blueyed - that hash you reference doesn't seem to link to a relevant change.

I'm going to leave these as properties for now, as this is what is used in general for accessing the Stripe object related to local instances. If we suddenly cache this, we could break people's code in subtle ways. They may, for example, call it multiple times on an instance and expect to see changes reflected in the returned value.

blueyed · 2017-09-04T12:13:55Z

pinax/stripe/models.py

+ def stripe_bankaccount(self):
+ return self.account.stripe_account.external_accounts.retrieve(
+ self.stripe_id
+ )


cached property?!

blueyed · 2017-09-04T12:19:47Z

tox.ini

@@ -30,4 +30,4 @@ setenv =
 LC_ALL=en_US.UTF-8
 commands =
 flake8 pinax
- coverage run setup.py test
+ coverage run setup.py test {posargs}


Added to f717fbe.

blueyed · 2017-09-04T12:21:58Z

setup.py

@@ -88,6 +88,7 @@
 "django>=1.7",
 "pytz",
 "six",
+ "django-ipware==1.1.6"


django-ipware is only used for the InitialManagedAccountForm - might be better to make this an optional dependency then only (in case it cannot be avoided altogether).

@blueyed I've had a think about this - it strikes me that having an optional dependency creates complexity during deployment.

If you have a project that uses pinax-stripe and you are using InitialManagedAccountForm and installing pinax-stripe doesn't include django-ipware, doesn't that put the deployer in an odd situation where they have to remember to install django-ipware as part of their projects specific dependencies?

Let me know if you have a good reference implementation of an optional dependency in Python that I can take a peek at.

You can use extras_require for this, see e.g. https://github.com/celery/celery/blob/de2d075fb07850eee247766ebeb11d5530a43734/setup.py#L217 for something more involved.

There could then be e.g. pinax-stripe[forms] (to have all dependencies for forms) and/or pinax-stripe[full] (to really have all optional deps).

For now pinax-stripe[ipware] could be used - but I am not sure how much hassle it is for when you expect that everything works out of the box?!

Neat that there is a mechanism for this.

If django-ipware was a heavyweight dependency with serious storage, memory or cascading dependency consequences it would seem worthwhile to me to go that route. As it stands, django-ipware is a lightweight, pure-python lib with no dependencies. My gut instinct is that it is not a strong candidate for use of extras_require.

is a lightweight

But still it adds up in your requirements - and I assume that lot of users are not using forms (and even this specific) one.

@blueyed once we're merged, feel free to knock up a PR and we can discuss with @paltman. My sense is that adding complexity to avoid a tiny dependency is not the way to go so I'm not keen on spending further time on this.

paltman · 2017-09-04T15:01:18Z

I am starting to catch up on all the details (and trying to do so in the abstract as I've never dealt with connected accounts before). At one point in this thread, it's mentioned that this is for custom type accounts. Would that mean that express is inherently supported as a subset of what custom would require?

blueyed · 2017-09-04T17:48:00Z

pinax/stripe/actions/charges.py

+ if stripe_account:
+ kwargs['destination'] = {
+ 'account': stripe_account
+ }


Should there be support for direct charges, too?
https://stripe.com/docs/connect/charges#choosing-approach
This seems to be the recommended method for Standard accounts: https://stripe.com/docs/connect/charges#direct-charges.

@blueyed I can adjust this to allow for both direct and destination charges.

I've now done this - take a peek!

lukeburden · 2017-10-03T22:31:17Z

@paltman the docs are on my mind, just a bit slammed with my work so struggling to get to it. How would you recommend I add to them? What editor, or workflow do you use? How can I test what they'll look like on RTD, etc? A little info would be great to speed me up when I get to it.

paltman · 2017-10-03T22:44:26Z

Just markdown, don't stress about formatting. Maybe just add a new markdown file in the docs/ folder. I can tidy up formatting, linking, etc. I'll test the docs myself doing a trial run setup and polish off the rough corners as well. Just need the broad strokes. I appreciate the patience on the back and forth of this really long PR.

paltman · 2017-10-10T17:52:16Z

I’ll merge this when I some free time this evening.

Thanks for all this great work.

paltman requested changes May 19, 2017

View reviewed changes

ticosax mentioned this pull request Jun 16, 2017

Add support for Connect #174

Closed

iMerica reviewed Jun 21, 2017

View reviewed changes

blueyed reviewed Sep 4, 2017

View reviewed changes

lukeburden added 15 commits September 10, 2017 15:01

Improving speed of syncing large data sets from stripe

69dd619

Further updates to sync_customers and tests, WIP

dbe0567

Do not freak when plan does not exist

4f619c9

BankAccount, Account and Transfer changes WIP

b960ba4

Ongoing WIP connect

0fecf73

Fix issues with account syncing

d7e5a1e

Add sync logic for BankAccount and update admin

2afda63

Refining creation methods for account and bank account.

c7d6b40

Allowing a single user to have multiple Stripe accounts

fccdcdf

Remove personal ID number

8dae8c2

Lazy WIP commit; tons of stuff going on!

6b3773d

WIP for improvement of transfers, accounts, external accounts

aede03e

Clean up connect migrations

291dc9c

Add availability info to Charge

cdd8564

Add expansion to capture method to get availability data

c8e3edb

Add on_delete=models.CASCADE to FKs for compat with 2.0

d46b2c4

lukeburden added 2 commits October 3, 2017 15:56

Updates from Master + Fixing Django 2.0 issues

f777615

Merge remote-tracking branch 'upstream/master' into connect

a6e1aee

paltman mentioned this pull request Oct 6, 2017

Install codecov integration #388

Closed

lukeburden and others added 21 commits October 7, 2017 23:00

Initial pass on Connect documentation. Very much a WIP!

a3f1f0d

Update Connect Documentation

146fe05

Merge remote-tracking branch 'upstream/master' into connect

a4133cd

Add import checking with isort

d8a0f3c

The quote checking config is in [flake8] section already

dd8befa

Bust cache

5310153

Fix imports

fb93501

Lint errors left by isort

c558b48

Show diff from isort if there is something to show

e127634

Improve the isort syntax and add tox checksum to cache key

d6100d8

More imports compliance

7774c8a

Explicitly run isort checks with tox.ini

6777156

Lint with 3.6.1

bef42ed

Fix some isort lints

b0fe6a7

Use tox.ini as source of truth DRYing up CI config

46844af

Add some tests and don't worry about old branching for coverage

61a66fc

Fix lint

99e8239

Subscription: factor out STATUS_CURRENT

6aec5ac

Remove dupe import

07cd007

isort the imports

340ed22

Resolve Conflicts

1e7f81f

paltman merged commit 1e7f81f into pinax:master Oct 11, 2017

paltman added this to the Samwise milestone Oct 20, 2017

		@@ -24,8 +24,16 @@ class Meta:
		abstract = True


		class AccountRelatedStripeObject(StripeObject):

Add support for Stripe Connect #336

Add support for Stripe Connect #336

Conversation

lukeburden commented May 11, 2017 • edited Loading

paltman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeburden commented Jun 16, 2017

iMerica commented Jun 21, 2017 • edited Loading

iMerica Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeburden commented Jun 21, 2017

iMerica commented Jun 21, 2017

blueyed commented Aug 30, 2017

lukeburden commented Aug 31, 2017

blueyed commented Sep 1, 2017

blueyed left a comment

Choose a reason for hiding this comment

paltman commented Sep 4, 2017 via email

blueyed left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

lukeburden Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

paltman commented Sep 4, 2017 • edited Loading

blueyed Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeburden commented Oct 3, 2017

paltman commented Oct 3, 2017

paltman commented Oct 10, 2017

lukeburden commented May 11, 2017 •

edited

Loading

iMerica commented Jun 21, 2017 •

edited

Loading

iMerica Jun 21, 2017 •

edited

Loading

blueyed left a comment •

edited

Loading

blueyed Sep 15, 2017 •

edited

Loading

lukeburden Sep 15, 2017 •

edited

Loading

paltman commented Sep 4, 2017 •

edited

Loading

blueyed Sep 4, 2017 •

edited

Loading