-
Notifications
You must be signed in to change notification settings - Fork 286
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 support for Stripe Connect #336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small changes requested, but nothing major.
VERY IMPRESSIVE work! Thanks for taking the time to make a pull request.
pinax/stripe/models.py
Outdated
|
||
def __str__(self): | ||
return str(self.user) | ||
return unicode(self.user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't need unicode
here because of the @python_2_unicode_compatible
class decorator.
.pep8
Outdated
@@ -0,0 +1,5 @@ | |||
[flake8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a config file for pep8/flake8. I'll remove it.
pinax/stripe/actions/accounts.py
Outdated
Returns: | ||
a pinax.stripe.models.Account object | ||
""" | ||
kwargs['country'] = country |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to stay consistent with rest of code base and not use single-quotes, but instead use double-quotes.
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? |
I'm glad I found this. I was about to embark on the same work!
Per Stripe's documentation, there are three Connect Account Types:
Which of these types are supported by this PR? |
@@ -24,8 +24,16 @@ class Meta: | |||
abstract = True | |||
|
|||
|
|||
class AccountRelatedStripeObject(StripeObject): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@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! |
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? |
This needs to be rebased / updated. |
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. |
Awesome.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Managed accounts" has been renamed to "Custom accounts": https://stripe.com/docs/connect/custom-accounts#name-change
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
raise | ||
subscription = stripe.Subscription.retrieve(sub_id) | ||
# if subscription and not subscription.customer == customer.stripe_id: | ||
# return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented code. Should likely throw in case it does not match the customer
instead?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just uncomment this for now, leaving the behaviour unchanged.
pinax/stripe/models.py
Outdated
return stripe.Invoice.retrieve( | ||
self.stripe_id, | ||
stripe_account=( | ||
self.customer.stripe_account if self.customer_id else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use hasattr(self, 'customer')
here / or a try/except self._meta.model.customer.RelatedObjectDoesNotExist
?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Care to explain why those more verbose approaches would be better? :)
|
||
@property | ||
def stripe_account(self): | ||
return stripe.Account.retrieve(self.stripe_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a cached property, similar to 79424ed probably?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
def stripe_bankaccount(self): | ||
return self.account.stripe_account.external_accounts.retrieve( | ||
self.stripe_id | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cached property?!
tox.ini
Outdated
@@ -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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to f717fbe.
setup.py
Outdated
@@ -88,6 +88,7 @@ | |||
"django>=1.7", | |||
"pytz", | |||
"six", | |||
"django-ipware==1.1.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
pinax/stripe/actions/charges.py
Outdated
if stripe_account: | ||
kwargs['destination'] = { | ||
'account': stripe_account | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blueyed I can adjust this to allow for both direct and destination charges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now done this - take a peek!
@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. |
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. |
I’ll merge this when I some free time this evening. Thanks for all this great work. |
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:
Let me know what you think!
Thanks,
Luke