Skip to content
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

Handle additional confirmation in checkout complete flow #5179

Merged
merged 19 commits into from
Feb 4, 2020

Conversation

salwator
Copy link
Contributor

@salwator salwator commented Jan 20, 2020

This PR enables our checkout process to handle additional confirmation step needed for multi-factor authentication.

checkoutComplete mutation now returns additional flag indicating that action is required by payment gateway, next call will confirm payment instead of capturing again.

Fixes #4773

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. Database migration files are up to date.
  6. The changes are tested.
  7. GraphQL schema and type definitions are up to date.
  8. Changes are mentioned in the changelog.

@salwator salwator changed the title Handling additional confirmation in checkout complete flow Handle additional confirmation in checkout complete flow Jan 20, 2020
@salwator salwator force-pushed the add-confirmation-to-checkout-complete branch from ea229f3 to 70025f1 Compare January 22, 2020 11:23
@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #5179 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5179      +/-   ##
==========================================
+ Coverage    91.8%   91.81%   +<.01%     
==========================================
  Files         270      272       +2     
  Lines       17554    17708     +154     
  Branches     1526     1533       +7     
==========================================
+ Hits        16116    16258     +142     
- Misses       1040     1050      +10     
- Partials      398      400       +2
Impacted Files Coverage Δ
saleor/graphql/product/types/products.py 89.47% <ø> (ø) ⬆️
saleor/graphql/warehouse/schema.py 96.15% <80%> (-3.85%) ⬇️
saleor/graphql/warehouse/sorters.py 85.71% <85.71%> (ø)
saleor/webhook/payloads.py 98.61% <0%> (-1.39%) ⬇️
saleor/account/emails.py 84.21% <0%> (-1.13%) ⬇️
saleor/graphql/middleware.py 79.31% <0%> (-0.69%) ⬇️
saleor/graphql/views.py 84.5% <0%> (-0.5%) ⬇️
saleor/graphql/warehouse/mutations.py 100% <0%> (ø) ⬆️
saleor/graphql/warehouse/types.py 100% <0%> (ø) ⬆️
... and 5 more

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 581edeb...8ab156a. Read the comment docs.

Copy link

django-queries commented Jan 22, 2020

Here is the report for 8ab156a (mirumee/saleor @ add-confirmation-to-checkout-complete)
Base comparison is 997dbb5.

No differences were found. (click me)

# api.benchmark checkout
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  add billing address to checkout            	         34	         34	             20
  add shipping to checkout                   	          7	          7	              0
  checkout payment charge                    	         10	         10	              0
  complete checkout                          	          8	          8	              0
  create checkout                            	          5	          5	              1

# api.benchmark homepage
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  retrieve main menu                         	          5	          5	              0
  retrieve product list                      	          4	          4	              0
  retrieve secondary menu                    	          5	          5	              0
  retrieve shop                              	          2	          2	              0

# api.benchmark product
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  product details                            	         18	         18	              4
  retrieve product attributes                	          9	          9	              0

# api.benchmark variant
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  product variant bulk create                	         51	         51	              3
  retrieve variant list                      	         23	         23	              9

# api product sorting attributes
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  sort product not having attribute data     	         21	         21	              0

@salwator salwator marked this pull request as ready for review January 22, 2020 16:16
Copy link
Member

@korycins korycins left a comment

Choose a reason for hiding this comment

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

Have you tested these changes with any payment gateway?

saleor/payment/migrations/0016_payment_to_confirm.py Outdated Show resolved Hide resolved
saleor/payment/gateway.py Show resolved Hide resolved
saleor/graphql/checkout/mutations.py Show resolved Hide resolved

# return the success response with the newly created order data
return CheckoutComplete(order=order)
return CheckoutComplete(order=None, confirmation_needed=True)
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need a separate mutation for payment that needs confirmation?
Now we have CheckoutComplete but we don't complete the checkout as we need to do additional steps.

Copy link
Member

Choose a reason for hiding this comment

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

What should be the next step in the checkout flow when we get CheckoutComplete(order=None, confirmation_needed=True)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You confirm payment client side (depends on gateway) and call again.

Copy link
Member

Choose a reason for hiding this comment

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

So to finish checkout I need to execute mutation CheckoutComplete twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, in case there is additional confirmation required, you need to call mutation again

Copy link
Contributor Author

@salwator salwator left a comment

Choose a reason for hiding this comment

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

Have you tested these changes with any payment gateway?

That's a tricky part, little hard without a working client. I'll try to do that, but the real test will be integrating the stripe backend with a real client.

@salwator
Copy link
Contributor Author

@korycins
After brief attempt to integrate this branch with stripe client, I discovered additional issues to be addressed before full two-factor payment is possible.
Since those are loosly connected issues and this checkout change has far more implications, I suggest merging this and solving remaining issues separately @maarcingebala .

@maarcingebala
Copy link
Member

We officially support Braintree and Stripe. Is this PR tested with these gateways @salwator?

@maarcingebala
Copy link
Member

@salwator OK so if we know what needs to be done to fully support two-factor payments let's gather these requirements in one GH issue.

@maarcingebala maarcingebala merged commit 2f72dfa into master Feb 4, 2020
@maarcingebala maarcingebala deleted the add-confirmation-to-checkout-complete branch February 4, 2020 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add additional confirmation state to checkout
4 participants