Remove transaction around Spree::Payment state transitions.

[shioyama]

Similar to #4542 and #4499.

Payment state transitions are wrapped in transactions. In the case we are working with, we have a payment observer that runs an after_transition callback which triggers an electronic shipment, which can occasionally fail. When it fails it rolls back the entire transaction, including the payment.

This means that although the payment has been successfully processed, we see it as pending.

Stripping the transaction does not seem to cause any problems, and allows for this type of case (i.e. where a payment has succeeded, but an after_callback has failed) to persist the payment to the db.

I'm submitting this to 1-3-stable since that's what we're working with, but it should apply to master as well. I can add a test too, just let me know.

I think in general that no state transitions on spree models really need to be wrapped in transactions, they are just set that way by default by state_machine.

[peterberkenbosch]

Thanks @shioyama, could you add a regression spec for this as well please? 👍

[shioyama]

@peterberkenbosch thanks will do.

[huoxito]

I think in general that no state transitions on spree models really need to be wrapped in transactions, they are just set that way by default by state_machine.

I'm not so sure about that @shioyama guess we need to review it more carefully. Take the Order#finalize method for example that runs after transition to complete. Maybe stores could get a order with complete state but without completed_at attribute touched. I think there might be good reasons that those transitions run on a transaction to ensure data is valid after it runs.

On the use case you described above, couldn't you take care of that failure on your method, like rescue the exception for example, so that the transaction is not rolled back? Assuming it should not really stop the state flow anyway.

[shioyama]

@huoxito I suppose in a way I wrote that to be provocative, to get a discussion going on this. 😉 It seems to be a really important aspect of how the spree checkout/payment flow works, but one that hasn't been analyzed carefully enough (IMHO).

Take the Order#finalize method for example that runs after transition to complete. Maybe stores could get a order with complete state but without completed_at attribute touched.

If that's a concern, you should look at #4542 and #4499 again, because they strip the transactions around checkout flow state machine callbacks. This affects callbacks for all transitions, including finalize on completed orders, but also create_tax_charge on delivered orders, after_resume on resumed orders, and after_cancel on cancelled orders. An order can conceivably complete, persist its completed state to the db, and not run any of those callbacks if the process is interrupted before it finishes.

The thing is, though, transactions are a double-edged sword here. As I mentioned in those PRs, transactions on the order state machine also wrap payment transitions, so the payment processing state is never actually persisted and you get double submission bugs like the one I mentioned.

In the case here, payment callbacks, I think the case can be made that the transaction is a bad thing. If a payment is successful, and something fails in a callback, then what do you want the state to be? If you rollback the transaction, the payment becomes pending, which is wrong in the case where it was processed by the gateway (purchase).

What we want in our case is for the payment_state to be completed, so that we know the payment is complete, but for the shipment_state to be ready (not shipped). This tells us exactly where things were left off.

This particular one we can solve with an after_rollback callback, but the other PRs I mention above are not so simple because you actually need persistence.

[huoxito]

I'm still ok with dropping the transactions on order state machine. Just
wanted to point that all callbacks are probably set around transactions for
a good reason.

And here I cant see an issue really because users can work around the
scenario by making the method not rollback, return false or something.
Anyway if none of payment depends on transactions it's ok lets drop it. I
just haven't looked through it yet myself

Washington L Braga Jr
Developer
Spree Commerce Inc
Em 28/04/2014 01:11, "Chris Salzberg" notifications@github.com escreveu:

@huoxito https://github.com/huoxito I suppose in a way I wrote that to
be provocative, to get a discussion going on this. [image: 😉] It
seems to be a really important aspect of how the spree checkout/payment
flow works, but one that hasn't been analyzed carefully enough (IMHO).

Take the Order#finalize method for example that runs after transition to
complete. Maybe stores could get a order with complete state but without
completed_at attribute touched.

If that's a concern, you should look at #4542https://github.com/spree/spree/pull/4542and
#4499 #4499 again, because they
strip the transactions around checkout flow state machine callbacks. This
affects callbacks for all transitions, including finalize on completed
orders, but also create_tax_charge on delivered orders, after_resume on
resumed orders, and after_cancel on cancelled orders. An order can
conceivably complete, persist its completed state to the db, and not run
any of those callbacks if the process is interrupted before it finishes.

The thing is, though, transactions are a double-edged sword here. As I
mentioned in those PRs, transactions on the order state machine also wrap
payment transitions, so the payment processing state is never actually
persisted and you get double submission bugs like the one I mentioned.

In the case here, payment callbacks, I think the case can be made that the
transaction is a bad thing. If a payment is successful, and something
fails in a callback, then what do you want the state to be? If you rollback
the transaction, the payment becomes pending, which is wrong because it
was processed by the gateway.

What we want in our case is for the payment_state to be completed, so
that we know the payment is complete, but for the shipment_state to be
ready (not shipped). This tells us exactly where things were left off.

This particular one we can solve with an after_rollback callback, but the
other PRs I mention above are not so simple because you actually need
persistence.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4603#issuecomment-41521610
.

[shioyama]

@huoxito Fair enough, I'll just save in an after rollback callback or something, and close this for now. Thanks.