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

Fixing locked orders due to invalid card details for a payment #2616

Closed
wants to merge 4 commits into
base: 1-3-stable
from

Conversation

Projects
None yet
4 participants
@robertoles
Contributor

robertoles commented Feb 26, 2013

This fixes the locked order issue described in previous issue #1767

A brief review of the issue:
If a user tries to place an order, but their credit card details are invalid the payment will change its state to failed, however due to database transaction being owned by the order for that payment, the transaction is rolled back because the code raises an exception during the state transition for the order.

To reproduce:

  1. Configure a payment method which uses a BogusSimple Gatway.
  2. Populate a Basket in the store
  3. Checkout the basket, during the payment step, enter an incorrect credit card number e.g. 1234123412341234
    If you follow the process_payments! method through using the ruby debugger, you will see the state of the payment will move from checkout -> processing -> failed, however, when process_payments! returns false due to the payment being failed, the transition (Spree::Order::Checkout before_transition :to => :complete) will rollback the Order transaction, causing the child payments state to be rolled back to checkout as well.
  4. The payment view will re render, with a blank credit card form, this time enter a valid credit card number e.g.
    4111111111111111
    At this point you would expect the order to complete, however the order attempts to process the initial payment, that is in the 'checkout' state due to the rollback, and once again this initial payment fails, therefore causing the whole Order transition to fail again.

To fix this I added an after_rollback hook to payment class which forces a save on the object if its state is failed.

Along side this fix, I have included some code to add a new 'invalid' state to the payment object. Payment#process! will invoke the invalidate! event if the payment_method does not support the payment source. I have implemented Gateway#supports? to check if the card brand is supported by the chosen gateway. I have done this because using an incorrect card with some gateways (for my case Worldpay) causes an exception to be thrown due to the card having no brand (due to an incorrectly formatted card number), I added the new state as I wasnt sure if setting the state to failed would imply a greater error / enables additional functionality elsewhere in the codebase.

Please note that although I have added some specs, and fixed any which I have broken, there were already some failing which I haven't been able to fix for you...

rspec ./spec/controllers/spree/orders_controller_extension_spec.rb:27 # Spree::OrdersController extension testing update specify symbol for handler instead of Proc POST has value success
rspec ./spec/controllers/spree/orders_controller_extension_spec.rb:44 # Spree::OrdersController extension testing update render POST has value success
rspec ./spec/controllers/spree/orders_controller_extension_spec.rb:61 # Spree::OrdersController extension testing update redirect POST has value success
rspec ./spec/controllers/spree/orders_controller_extension_spec.rb:78 # Spree::OrdersController extension testing update validation error POST has value success
rspec ./spec/controllers/spree/orders_controller_extension_spec.rb:94 # Spree::OrdersController extension testing update A different controllers respond_override. Regression test for #1301 POST should not effect the wrong controller
rspec ./spec/requests/admin/products/edit/taxons_spec.rb:11 # Product Taxons managing taxons should allow an admin to manage taxons

@cyu

This comment has been minimized.

Show comment
Hide comment
@cyu

cyu Mar 2, 2013

Contributor

This is the same problem as #2585.

Contributor

cyu commented Mar 2, 2013

This is the same problem as #2585.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 9, 2013

Member

Looks like this would fix #2652 as well. AWESOME. 🎉

Member

radar commented Mar 9, 2013

Looks like this would fix #2652 as well. AWESOME. 🎉

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 9, 2013

Member

I can validate that this problem is fixed with this pull request. I've also got another commit in to ensure that only valid payment records will be displayed on the order form. This means that if subsequent payments are added to the order, they'll also be displayed on the frontend. The look of this could probably do with some work, but it is functional now.

Member

radar commented Mar 9, 2013

I can validate that this problem is fixed with this pull request. I've also got another commit in to ensure that only valid payment records will be displayed on the order form. This means that if subsequent payments are added to the order, they'll also be displayed on the frontend. The look of this could probably do with some work, but it is functional now.

@rterbush

This comment has been minimized.

Show comment
Hide comment
@rterbush

rterbush Mar 9, 2013

Contributor

Nice work. I assume this is getting committed to 1-3-stable as well?

Randy

On Sat, Mar 9, 2013 at 12:32 PM, Ryan Bigg notifications@github.com wrote:

I can validate that this problem is fixed with this pull request. I've
also got another commithttps://github.com/radar/spree/commit/14a060833b4447cc7ef272870291f69f5f611813in to ensure that only valid payment records will be displayed on the order
form. This means that if subsequent payments are added to the order,
they'll also be displayed on the frontend. The look of this could probably
do with some work, but it is functional now.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2616#issuecomment-14669011
.

Contributor

rterbush commented Mar 9, 2013

Nice work. I assume this is getting committed to 1-3-stable as well?

Randy

On Sat, Mar 9, 2013 at 12:32 PM, Ryan Bigg notifications@github.com wrote:

I can validate that this problem is fixed with this pull request. I've
also got another commithttps://github.com/radar/spree/commit/14a060833b4447cc7ef272870291f69f5f611813in to ensure that only valid payment records will be displayed on the order
form. This means that if subsequent payments are added to the order,
they'll also be displayed on the frontend. The look of this could probably
do with some work, but it is functional now.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2616#issuecomment-14669011
.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 11, 2013

Member

Yes, the fix for this will be going into 1-3-stable as well.

Member

radar commented Mar 11, 2013

Yes, the fix for this will be going into 1-3-stable as well.

@radar radar closed this in 7772043 Mar 11, 2013

radar added a commit that referenced this pull request Mar 11, 2013

radar added a commit that referenced this pull request Mar 11, 2013

radar added a commit that referenced this pull request Mar 11, 2013

@rterbush

This comment has been minimized.

Show comment
Hide comment
@rterbush

rterbush Mar 26, 2013

Contributor

@radar can you confirm that these pull requests were applied to 1-3-stable? I seem to be seeing the same exact behavior in a clean spree store.

Contributor

rterbush commented Mar 26, 2013

@radar can you confirm that these pull requests were applied to 1-3-stable? I seem to be seeing the same exact behavior in a clean spree store.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Mar 28, 2013

Member

@rterbush: Yes. It's in b3e1245.

core (master)★git branch --contains b3e12451c19812b061d289857d2d6aad514a940b
  1-3-stable
Member

radar commented Mar 28, 2013

@rterbush: Yes. It's in b3e1245.

core (master)★git branch --contains b3e12451c19812b061d289857d2d6aad514a940b
  1-3-stable

jetsgit pushed a commit to jetsgit/spree that referenced this pull request Mar 31, 2013

Jerrold Thompson
Maunual patching for issues spree#2616 spree#2570 spree#2678 spree#2585
spree#2652

	modified:   core/app/models/spree/gateway.rb
	modified:   core/app/models/spree/payment.rb
	modified:   core/app/models/spree/payment/processing.rb
	modified:   core/app/models/spree/payment_method.rb
	modified:   core/spec/models/payment_spec.rb

jetsgit pushed a commit to jetsgit/spree that referenced this pull request Apr 1, 2013

Jerrold Thompson
Revert "Maunual patching for issues spree#2616 spree#2570 spree#2678 s…
…pree#2585 spree#2652"

This reverts commit 03750ac.

	modified:   core/app/models/spree/gateway.rb
	modified:   core/app/models/spree/payment.rb
	modified:   core/app/models/spree/payment/processing.rb
	modified:   core/app/models/spree/payment_method.rb
	modified:   core/spec/models/payment_spec.rb

alce added a commit to alce/spree that referenced this pull request Jun 3, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment