pinax/pinax-stripe-light

Upcoming Sprint

paltman opened this issue · 33 comments

paltman commented

I have been very swamped over the past number months since the last release and there are a lot of things we need to get back to to keep this project alive and healthy. I'm planning an upcoming sprint (still don't have a date yet, but will likely occur over a (long) weekend in the next 4-6 weeks.

I wanted to open this issue so that I could get feedback from folks of what their favorite issues would be to see knocked out, preferably with associated PRs to consider/review. Please link to them in the comments with any supporting details as to why it should be considered priority knowing that I'll have only so much that I can do in a fixed window of time.

Obviously, the more help I can get with this in terms of good PRs versus just Issues, the more that can get done.

@blueyed @lukeburden @ticosax as some of the top-most recent contributors to the project in way of commits, I'd love your help in triaging items on this issue, if you have any time to spare.

lukeburden commented

Excellent call @paltman.

I'd like to see added support for storing Dispute objects. I notice that @blueyed has #591 up which fixes the charge updated webhook, but I'd be curious to go a bit further and store some of the Dispute's info to allow for more complex tracking in-app.

tyndyll commented

I'd like to see the Connect Custom accounts extended to more countries than US/CA. Happy to help by submitting a PR

blueyed commented

Connect Custom accounts extended to more countries than US/CA

Why/where is that limited?

lukeburden commented

There are some dynamic choice fields for countries and states - ideally, we could remove that, and derive everything from Stripe's CountrySpec objects.

Are you thinking of a major release, allowing us to break backward compat? If so, two things we should include in this sprint:

  • add a Payout model separate to Transfer to properly reflect Stripe's changes.
  • review all the API updates since the default API version in this lib, adjust to reflect the latest API version, update the default API version in this lib to the latest
paltman commented

I'd also like to see updates for what is needed for changes in Subscription model, naming the move Stripe made a while back to add Product objects that each have a Plan that you subscribe too. I think @blueyed's #588 cracks the door a bit to address this.

paltman commented

@lukeburden it might be easiest to have the freedom of a major release and then we can consider how to back port / maintain certain things but I'm wary of getting into any multiple release management. I can barely keep up with just a single release. :)

paltman commented

Does anyone see any problem with the next release being a major release that just pins to latest API version and updates everything to match the changes since last API version?

Trying to maintain BC is going to be a pain and not sure it's worth it, especially given that the models in this app are just storing a cache of what's in Stripe. It should always be safe to rehydrate your local cache from the API in what are essentially read only models. If that's not working this there is a bug that we should fix.

Thoughts?

lukeburden commented
paltman commented

I've started the Next milestone. I'd like to keep it relatively focused though I don't mind loading it up with outstanding PRs that are non-controversial and easily forward ready.

My biggest goal for this next release is to get it all updated for latest API version.

paltman commented

I've gone through the change log and I think these are the items that affect us:

  • #596 - Bank Account name is renamed to account_holder_name
  • #597 - currencies_supported is removed from Account object. Use CountrySpec instead.
  • #598 - listing subscriptions will now include canceled. Can pass status=canceled to get only canceled. Probably no impact.
  • #599 - 403 now returned instead of 401 for bad permissions
  • #600 - sourced_transfers removed from Balance Transaction object. No impact, I think
  • #601 - Charge.dispute is now only the id of the Dispute object. Previously it was the entire object. You can now pass an option to expand the object to preserve BC.
  • #602 - Charge.outcome[rule] now only contains the ID of the Rule. Previously was object. Can expand when fetching the charge to preserve BC.
  • #595 - Transfer object is split out into Payout and Transfer. Update modeling to support this. See https://stripe.com/docs/transfer-payout-split
  • #603 - Account.managed moves from just a boolean to being a choice field of standard, express, or custom. One of which is required when creating an account.
  • #604 - Event.request now is the id AND impotency key. Previously was only the id.
  • #605 - on Connect related Event objects, the user_id property is renamed to account
  • #606 - invoice line items will always have a description set
  • #607 - Plan objects have a Product with a type=service. statement_descriptor and name have moved from Plan to Product.
  • #608 - Subscriptions now have a billing_cycle_anchor with an optional pro-ration up that that that allows specifying first invoice on a subscription to be a future date.
  • #609 - a Source, Card, three_d_secure can now have a recommended value instead of only not supported, optional and required.
  • #610 - To clear a cancellation status on a subscription, set cancel_at_period_end=False when updating subscription.
  • #611 - Subscriptions no longer inherit the plan’s trial_period_days automatically. They must be created with trial_from_plan set to True.
  • #612 - Invoice line item ids for subscriptions have their own ids and are no longer subscription Ids.
  • #613 - percent_off on Coupon was changed from Integer to Float with 2 decimal place precision.
  • #614 - subscription endpoints no longer accept a source parameter. Changing the default source for a customer we should create a new source using the source creation API and then update the customer to set it as the default through customer update API.
  • #615 - amount field in tiers for plans was renamed to unit_amount
  • #616 - business_vat_id is no longer a string but rather an object called tax_info with tax_id and type keys.
  • #617 - can’t set at_period_end on the subscription DELETE endpoint. This endpoint is for only immediately cancellations. Set cancel_at_period_end on the subscription update endpoint for a future cancellation.
paltman commented

crap, that's a lot.

  • 😄1
lukeburden commented

Divide and conquer!

paltman commented

@lukeburden there. they are now divided. :)

paltman commented

I've had a chance to play around with py.test a bit too since @blueyed first suggested we move to using it and I'm finally on board with that. Doesn't have to be apart of this milestone but if someone wants to update CI bits to run on py.text, it has my full support.

  • 👍1
paltman commented

I wonder if we should add library identification.

Maybe it's as simple as adding:

stripe.set_app_info(
    "Pinax Stripe",
    version=PULLED_FROM_PKG_INFO,
    url="https://github.com/pinax/pinax-stripe"
)
  • 👍1
blueyed commented

Migration to pytest is ready in #618.

I am not sure about API breakage / having to sync things manually really, but if it helps / is required to get the project back on track, sure.
Long-standing issues like #158 however still indicate that there are more general issues, where lacking manpower is missing.

blueyed commented

btw: what happened to the last sprint? https://github.com/pinax/pinax-stripe/milestone/12

paltman commented
paltman commented

@blueyed what do you mean by:

I am not sure about API breakage / having to sync things manually really, but if it helps / is required to get the project back on track, sure.

Do you think we can handle everything through data migrations?

paltman commented

where lacking manpower is missing.

@blueyed would you be willing/able to help me maintain this project?

blueyed commented

Do you think we can handle everything through data migrations?

I would expect so, yes.
But cannot really say if it is worth the effort.
For my work we're using a fork though anyway already (the "next" branch), and I would assume that it is OK for our work to stay with an older API version as long as Stripe supports it.

would you be willing/able to help me maintain this project?

Not really, sorry.

paltman commented

I would expect so, yes.
But cannot really say if it is worth the effort.

I think we can probably handle 90% of it fairly easily. But there are some parts that would require too much predicting what/how the person was doing with their stripe account. Did they create products? If so, we need to sync them down to get their IDs before we can create FKs, etc.

Not really, sorry.

No problem. It's a LOT of work and I understand everyone has priorities.

Stripe moves so quickly, I do feel like this project is moving beyond a single maintainer in order to keep up.

blueyed commented

Yeah - but it's also about not merging trivial thinks like #593 right away. I'd be happy to help out with this (i.e. not just approve them, but merge them when coming across those)

Anyway, good to see some activity here again.
Please also take a look at #558.

paltman commented

@blueyed happy to give you access to merge if you want to help with the low-hanging fruit ones like that. yes,. there are trivial ones like that, but it would be a HUGE help to me.

paltman commented

I honestly just miss a lot of them.

screen shot 2018-10-09 at 4 11 33 pm

blueyed commented

@paltman
Sure, happy to help.
tip: you can start with "Participating".. ;)

  • 👍1
paltman commented

Thanks. I think I mostly just need to carve out time each week to go through my Github messages/review/merge, separate and apart from my desire to code on things.

paltman commented

The more I have thought about this and the more I talk with some of my team members, the more I'm convinced the right strategy going forward for this app is to make it more of a "toolkit" than a complete cache/wrapper.

The sheer number of API methods and the rate at which new ones get added and the new versions of the API that are released, make it nearly impossible to maintain pace. Unless the Stripe team itself were to take over maintenance of this app, I don't see there being enough resources and man power, even with now two maintainers, to keep up.

In addition, Stripe's API is well documented and easy to understand. We have lacked solid documentation for a while in part because it's just a lot to do and to maintain so we never seem to get to it. It seems awkward to force the developer to have to know what they want to do on Stripe, see the Stripe docs and then have to figure out what that means in this package by picking the right action function.

I do think there is a lot of value in making it easy to capture and emit signals for web hooks out of the box. I also think there is value in shipping with some mixing and/or abstract models for the site building to construct whatever caching models are appropriate for their application.

Therefore, in a broad stroke, I think the next major version of this app should kill most models (I think we still need model(s) for the webhook events, at least), and all the actions. We can probably kill the views as well, as they are mostly CBV against the models we are getting rid of (and personally, I almost always override them).

With this in mind, I can't even begin to imagine what the upgrade path looks like, but maybe we sort that out once this new approach starts taking shape.

While this will make this app a lot leaner, I think we will be able to keep things up to date a lot better while still providing a lot of utility. We can then provide some "cookbook" or other type documentation for how to setup the caching models for certain common situations and perhaps open the door for secondary packages that depend on pinax-stripe to provide these caching models that can be maintained and released independently.

@blueyed what are you thoughts on this?

lukeburden commented

@paltman that seems like a pragmatic and considered course of action, to me.

I'll have to think through how this would affect my projects that use this library, but I'm not using the cache models beyond the benefit of having something to point a foreign key at. Storing the stripe-id for the related object in a non-null field would probably be just about as effective.

The webhooks processing is an important utility in all my uses of this project.

There's a fair bit of complexity in their Connect product that the forms I contributed were intended to offset, but in practice, people could make these themselves or we could package them separately.

The upgrade path is probably a lot of work for most people, so I'm a bit worried we'll lose a lot of people on this move - but it has to be this way if you're certain it's infeasible to keep up with Stripe's pace of change.

paltman commented
lukeburden commented

@paltman I thought I'd check in with some further thoughts on this project.

I've recently been making some changes to a platform's payments system, and am increasingly using this library as almost solely a webhook processing utility. I typically store the Stripe ID in a unique field on a local model, and perform whatever behaviour is required on receipt of a webhook.

Often, if I need a bit more contextual data, I'll have a few fields on a local model. Often though, this model is not specific to Stripe, so this usually takes the form of a JSON field.

Basically, I still think your gut instincts are right for this project, and that as a stripped down set of utilities sans most models would have succinct and real value.

It's also worth noting that djstripe has continued to progress down the path of local model storage. They have a pretty strong group of project maintainers now, so they can seemingly keep up with the changes required. It would be the right thing to do after stripping back this project to link to them as an alternative that provides a full local DB cache.

paltman commented

@lukeburden great feedback. thanks! you comment also bumps this issue back into my view. I had completely forgotten about it.

  • 🎉1
paltman commented

@lukeburden @blueyed @tyndyll it's been some years...

I've gotten way away from using the cache-model and service layer methods in my various projects and have just been using the webhooks on the 5.x branch so my plan is to cut a new v5 release after merging that open PR #620 (after a bit more work) and make this project just the lean bits that I'm finding useful. If someone is interesting in maintaining a fuller version based on the 4.x line we can split the project and I can make a pinax-stripe-light or something, but given that djstripe now appears to have robust commercial support and a team of active maintainers, it probably makes sense for people wanting/needing more to consider that project. Personally, I've found it overkill on projects that I'm active on that are doing significant volume in complex stripe connect based settings and therefore can't justify putting a lot time into maintenance.