tiwariayush commented 4 years ago

Add models and functionalities to support SCA regulations (https://stripe.com/docs/strong-customer-authentication/migration)

New Models:

Payment Method
Setup Intent
Payment Intent
Checkout

Also, extra changes to support model additions such as:

Update Subscriptions with new pending_setup_intent field
Rename property payment_methods for Customer object to solve conflict: Due to addition of new Payment Method model, there has been a conflict in the name of property payment_methods on Customer object, hence the required name change.
Update migrations and test fixtures.
Add Charge.payment_intent + test fixture

This resolves #803, #897

therefromhere commented 4 years ago

Thanks @tiwariayush , I've not tried using this yet do you have an idea what more work is needed?

From my point of view, it needs some tests, and a usage example like I've added in #913 would be great.

tiwariayush commented 4 years ago

Hi @therefromhere. Thanks for checking in.

Things which I need to fix to make it ready for review:

Add tests (I'll be adding it today).
Fix tests failing because of migration conflict (small issue)
Addition of function add_payment_intent
Cleaner documentation and usage example.

UPDATE: I'll be working on it this week sporadically to fix all the issue and get it ready asap.

therefromhere commented 4 years ago

FYI @lx-stripe if you're working on #913 as well?

therefromhere commented 4 years ago

Yes, a separate PR would be good for that. Also add a property with a deprecation warning on the old name.

therefromhere commented 4 years ago

Good stuff! Did you get anywhere with a working example like #913?

If not I might be able to take a look at that?

I've not used PaymentIntent yet, but it seems that duplicating the existing example with one that does PaymentIntent with manual confirmation would be a logical choice?

https://stripe.com/docs/payments/payment-intents/migration

tiwariayush commented 4 years ago

Did you get anywhere with a working example like #913?

@therefromhere I haven't tried yet. Today I'll try to focus on this and tests today. Bit confused on test fixtures for now, since I am using test account from France, trying to update/add test fixtures changes some unnecessary values (like country, id, etc)

I've not used PaymentIntent yet, but it seems that duplicating the existing example with one that does PaymentIntent with manual confirmation would be a logical choice?

Manual confirmation priority wise would be my target to use as an example.

therefromhere commented 4 years ago

Did you get anywhere with a working example like #913?

@therefromhere I haven't tried yet. Today I'll try to focus on this and tests today.

@tiwariayush Cool, let me know if you need a hand. Also if you haven't found it already we have a discord channel here: https://discordapp.com/invite/UJY8fcc

Bit confused on test fixtures for now, since I am using test account from France, trying to update/add test fixtures changes some unnecessary values (like country, id, etc)

Yeah, understandable I know the fixture thing is a bit confusing. I suggest creating a test account with country set to US (and naming it "dj-stripe scratch") to minimize changes - it looks like pending_setup_intent is the only one we care about - so maybe worth reverting the other changes to the fixtures to reduce churn.

Some fixtures with a euro-based test account would be a good idea, but that's probably a problem for another time.

tiwariayush commented 4 years ago

@kavdev : Thanks a lot for review :) I have added changes accordingly. Some questions still remaining.

I'm assuming the example app works as expected.

Currently, its not finished the example especially sever-side part. I try to finish it as not much is left so that we can merge asap.

There are also some commits and changes which can pass in another PR (one with field name changes and depricated methods and all). I will create new PRs for them so as to merge slowly by slowly and not hold on to this one (which starts to get large).

tiwariayush commented 4 years ago

@therefromhere @kavdev : Please have a look. The example is working :)

therefromhere commented 4 years ago

I noticed that Charge.payment_intent relation isn't implemented yet, I'll look at doing that in a branch and do a PR back to this branch - note that I think this will mean creating a fixture for the PaymentIntent model, which hopefully will be useful for your other tests.

codecov[bot] commented 4 years ago

Codecov Report

Merging #914 into master will decrease coverage by 0.3%. The diff coverage is 90.35%.

@@            Coverage Diff             @@
##           master     #914      +/-   ##
==========================================
- Coverage   97.34%   97.03%   -0.31%     
==========================================
  Files          24       25       +1     
  Lines        2182     2294     +112     
  Branches      212      212              
==========================================
+ Hits         2124     2226     +102     
- Misses         42       52      +10     
  Partials       16       16

Impacted Files	Coverage Δ
djstripe/signals.py	`100% <ø> (ø)`	:arrow_up:
djstripe/event_handlers.py	`98.85% <ø> (ø)`	:arrow_up:
djstripe/enums.py	`100% <100%> (ø)`	:arrow_up:
djstripe/models/checkout.py	`100% <100%> (ø)`
djstripe/models/__init__.py	`100% <100%> (ø)`	:arrow_up:
djstripe/models/billing.py	`98.83% <100%> (ø)`	:arrow_up:
djstripe/models/payment_methods.py	`95.48% <81.81%> (-1.05%)`	:arrow_down:
djstripe/models/core.py	`95.47% <83.63%> (-1.44%)`	:arrow_down:
djstripe/models/base.py	`97.3% <0%> (+0.43%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 482a9d2...313d8b6. Read the comment docs.

therefromhere commented 4 years ago

One other tidyup (once the above are resolved and you think it's ready to merge) will be to squash migrations - maybe manually (delete those this branch, makemigrations and + add back in the ALTER INDEX), since there's a few back and forth changes (eg the AlterFields in migration 0006) - I noticed that currently ./manage.py migrate zero doesn't work.

codecov[bot] commented 4 years ago

Codecov Report

Merging #914 into master will decrease coverage by 0.12%. The diff coverage is 94.78%.

@@            Coverage Diff             @@
##           master     #914      +/-   ##
==========================================
- Coverage   97.34%   97.21%   -0.13%     
==========================================
  Files          24       25       +1     
  Lines        2182     2296     +114     
  Branches      212      212              
==========================================
+ Hits         2124     2232     +108     
- Misses         42       48       +6     
  Partials       16       16

Impacted Files	Coverage Δ
djstripe/signals.py	`100% <ø> (ø)`	:arrow_up:
djstripe/event_handlers.py	`98.85% <ø> (ø)`	:arrow_up:
djstripe/enums.py	`100% <100%> (ø)`	:arrow_up:
djstripe/models/checkout.py	`100% <100%> (ø)`
djstripe/models/__init__.py	`100% <100%> (ø)`	:arrow_up:
djstripe/models/billing.py	`98.83% <100%> (ø)`	:arrow_up:
djstripe/models/payment_methods.py	`96.79% <100%> (+0.26%)`	:arrow_up:
djstripe/models/core.py	`96.05% <88.88%> (-0.86%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 482a9d2...ba608b4. Read the comment docs.

tiwariayush commented 4 years ago

@therefromhere @kavdev : Thanks for review. I have updated the changed suggested. Please have a look.

Things left from my perspective:

The commits are not clean (lot of fixup commits), but I have kept them as it is to not hamper review for now.
Use of payment method fixtures in test (adding coverage for method like add_payment_method https://github.com/dj-stripe/dj-stripe/pull/914#discussion_r308112949). I am a bit stuck on this, will appreciate help.

Other than that, example work for all the cards now. I have squashed the migrations. So, I think its ready to be merged after cleanup.

therefromhere commented 4 years ago

Great! It's definitely nearly there. I'll take a look at the test issue.

One last thing - do a code reformat with tan/isort as per docs.

As far as cleaning up the commit history, if we're happy for this to land as a single squashed commit it's maybe not necessary to squash commits on this branch - though if you could add the commit message you want to the top of the PR description I can edit that in when I squash the branch.

@kavdev I've recently been using squash and merge via the github UI to land most PRs - apart from anything else I like that this adds the PR id to the commit title by default, which is useful when researching the git history - OK with you?

tiwariayush commented 4 years ago

@therefromhere :

Great! It's definitely nearly there. I'll take a look at the test issue.

Thanks !

One last thing - do a code reformat with tan/isort as per docs.

Done (421f397826)

As far as cleaning up the commit history, if we're happy for this to land as a single squashed commit it's maybe not necessary to squash commits on this branch - though if you could add the commit message you want to the top of the PR description I can edit that in when I squash the branch.

ok. Makes sense. I have updated the description of PR.

therefromhere commented 4 years ago

OK, I think this is about ready to go, I'll have another read over tomorrow and there's a couple of TODOs remaining (eg set_default param for Customer.add_payment_method?), but I don't think they're blocker.

FYI @kavdev when you're happy I think we can squash this.

therefromhere commented 4 years ago

Great work @tiwariayush !

kavdev commented 4 years ago

@therefromhere yep, squashing is fine on prs, unless there are purposeful atomic commits, in which case I'd prefer a rebase. Either way, avoid merge commits where possible. In the squash message, remove or combine any commit messages that don't help future people understand what the commit was about.

If you say it's good to go, go ahead and merge 👍🏼

tasn commented 4 years ago

I think I noticed something that may not work correctly (just by reviewing, haven't tested): The can_charge() method of Customer that is used to check if a Customer can be charged relies on has_valid_source() that only checks if a Customer has a default_source set, which is not the case with the new payment_methods (on stripe default_source and default_payment_method are split), at least as far as I can see.

Relevant line: https://github.com/BureauxLocaux/dj-stripe/blob/payment_intent/djstripe/models/core.py#L921

Thanks for all your great work, can't wait to have it merged so I can start using it.

therefromhere commented 4 years ago

I think I noticed something that may not work correctly (just by reviewing, haven't tested): The can_charge() method of Customer that is used to check if a Customer can be charged relies on has_valid_source() that only checks if a Customer has a default_source set, which is not the case with the new payment_methods (on stripe default_source and default_payment_method are split), at least as far as I can see.

Relevant line: https://github.com/BureauxLocaux/dj-stripe/blob/payment_intent/djstripe/models/core.py#L921

Yeah I think you're right. Should be an easy fix.

edit @tasn actually it's not super easy to fix, due to the nested Customer.invoice_settings field. I've added a separate ticket for it since I don't think it's a blocker on this landing in master (though probably a blocker on 2.1 release). See #929

therefromhere commented 4 years ago

Merged. Thanks again @tiwariayush !

tasn commented 4 years ago

Should I open new tickets for issues with this change or comment here so all the relevant people are referenced? Anyhow, it looks like the code uses "paymentmethod" as the stripe object, rather than "payment_method" which is what I'm getting from stripe. It's an issue because this means objects are not created from the stripe events. I assume payment intent and setup intent suffer from the same issues.

tasn commented 4 years ago

Another issue: the "attach" function of the PaymentMethod model expects a stripe customer rather than a normal customer model, a customer id, or better yet either (like many other places in the API).

Edit: and there's no "detach" function to detach the payment method.

therefromhere commented 4 years ago

@tasn Thanks, can you create new tickets for each of those, with a comment here linking to them so relevant people get a heads up.

tasn commented 4 years ago

Done. Created #941, #942 and #943

tasn commented 4 years ago

And #944 :)

kavdev commented 4 years ago

@tasn thanks for taking the time to test out this feature

tasn commented 4 years ago

Sure thing, thanks to everyone involved in creating this. I have SCA working for me thanks to all of your good work (had to workaround the issues above, but otherwise all is good).

stygmate commented 4 years ago

Hi,

The SCA will be applied september 14. I see that the pull request is merged but when do you plan to make a new release of dj-stripe including this changes ?. (For us to have the time to integrate it a little before september 14)

therefromhere commented 4 years ago

Hi, there are still some outstanding issues I'd like to resolve before release (see the 2.1 milestone), PRs are welcome!

Alternatively, you should be OK using master, providing you review what's changing.

Note that there will be a migration squash immediately before release, so if you're running from master you'll need to run the squashed migration before upgrading.

matteing commented 4 years ago

Hi!

Is there an example implementation of PaymentIntent for subscription flows? Anything short would be fine.

Thanks!

therefromhere commented 4 years ago

@matteing Hi, not currently no.

I think you want something like this -

1) create a payment method 2) either a) attach to your customer, then create a subscription b) create a subscription with it as subscription.default_payment_method 3) get the payment intent from subscription.invoice.payment_intent (I think the first invoice of a subscription is finalized immediately). 4) confirm the payment intent server-side 5) handle actions as needed (as per manual confirmation quickstart)

I'm working on something similar for a project currently (though just involving an invoice, without a subscription), based on the manual confirmation quickstart, except I use the invoice to get the payment intent as above instead of creating it https://stripe.com/docs/payments/payment-intents/web-manual - note that you need to use handleCardPayment instead of handleCardAction in the JS.

See also SCA migration guide for billing, scenario 1: https://stripe.com/docs/billing/migration/strong-customer-authentication#scenario-1

therefromhere commented 4 years ago

Heads up to anyone using Customer.add_payment_method / PaymentMethod.attach that I'm changing the param names in #936 (payment_method_id -> payment_method, and stripe_customer->customer).

therefromhere commented 4 years ago

FYI see #978 for 2.1.0 release plan.

dj-stripe / dj-stripe

Payment Intent and Checkout Session (SCA, EU Regulations) #914

Codecov Report

Codecov Report