Fix ability to perform refunds after a first failed attempt #3831
Conversation
The first line of perform! avoid the method to be executed under certain circumstances (which are actually happening in the spec context). Due to this, we are not testing the current behavior since perform! should be skipped and in the spec it's raising an exception. To fix this, it's better to stub process!, which is where the simulated exception can happen. We now have a failing spec but it's reflecting what happens in the real world.
There was a problem hiding this comment.
What if we initialize but don't save the refund before calling perform! and have perform! save the record (if it doesn't already)?
This would avoid this weird code that recreates the refund.
Regardless, I agree that tracing the success/fail state would be a better solution in the long run. Would probably be worth having three states: pending (default state), complete and failed.
|
I would also prefer not to save the refund at all unless it's been successfully performed. Would that be possible? |
|
I think it's a great idea, working on that and will let you know, thanks! |
If we save unsuccessful refund on payment gateway errors we'll end up having a refund associated to the payment without the actual refund processed. Since at the moment refunds have no states to indicate whether is successful or not, the system is considering it as done and doesn't allow to perform a new one since the credit allowed amount for the payment becomes 0: payment.credit_allowed = payment total - refund amount and this formula is true also when the refund has not been succesful. This comes from our past behavior of creating and calling perform! on refunds at the same time. By just validating the refund we have the same result. The "perform_after_create: false" attribute setting has been moved into the perform! itself. When called on a record still unpersisted, it will set its value to false to avoid deprecation warnings and well.. to call perform! again! A more clean solution would be considering in that formula only refunds that are completed but at the moment there's no way to do that. This way we could also keep track of the failed attempts to perform refunds, saving all the information we get back from the gateway.
kennyadsl
force-pushed
the
kennyadsl/refund-perform-fix
branch
from
70dcce9 to
ea7cbe0
Compare
|
@aldesantis @jarednorman please review again when you have time, and thanks again for pointing in the right direction. The first attempt was so bad! |
|
@jarednorman will wait for your review before merging if you have some time. Please let me know! |
Description
With #1415 we introduced a bug in the Admin:
When attempting to create a refund for a payment that fails with a gateway error, it's no more possible to create refunds for that payment.
To reproduce:
At this point, we have the flash message correctly printed with the error:
This is due to the split of record creation and calling
perform!inSpree::Admin::RefundsController#create. In fact, when we create a refund in the first place, it is added to the database but when we callperform!right after the creation, and it fails, the record created is not marked as failed anyhow. The system assumes all refunds are successful and consider them done if when they failed, disabling us to create new ones for those payments.In fact, the refundable amount is calculated with
and this formula is true also when the refund has not been successful.
This comes from our past behavior of creating and calling perform! on refunds at the same time. Records weren't created if the refund failed to perform.
A more clean solution would be considering in that formula only refunds that are completed but at the moment there's no way to do that. This way we can also keep track of the failed attempts to perform refunds, saving all the information we get back from the gateway. But this is way more invasive and we can't do this in a patch level fix.
This change is supposed to be merged in master (for 3.0) and backported to v2.11, which is the version that introduces the bug.
Checklist:
I have updated Guides and README accordingly to this change (if needed)