Fix double store credits creation when performing refunds#3989
Conversation
Success and failure messages are hardcoded in multiple methods, they can be easily DRYed up by using constants, with the extra benefit that these can be referenced also in subclasses and tests.
Stubs can make tests britte, especially ones that, although presented in a unit test file, are actually integrating the behavior of many classes. Specifically, being this stub defined at the top level, it prevents writing specs that actually want to hit the real implementation of the stubbed method. These seem good enough reasons for removing it.
| action: Spree::StoreCredit::CAPTURE_ACTION, | ||
| amount_remaining: credit.amount - payment.amount, | ||
| authorization_code: payment.transaction_id | ||
| ) |
There was a problem hiding this comment.
Layout/MultilineMethodCallBraceLayout: Closing method call brace must be on the same line as the last argument when opening brace is on the same line as the first argument.
There was a problem hiding this comment.
This is actually fixed, it seems hound didn't catch up with the changes 🤷
|
|
||
| let!(:credit_event) do | ||
| create(:store_credit_event, | ||
| store_credit: credit, |
There was a problem hiding this comment.
Layout/AlignArguments: Align the arguments of a method call if they span more than one line.
There was a problem hiding this comment.
This is actually fixed, it seems hound didn't catch up with the changes 🤷
spaghetticode
force-pushed
the
spaghetticode/double-store-credit-refund
branch
from
0761f86 to
f4aa847
Compare
Under certain specific circumstances, store credit refunds were creating 2 new store credits instead of one, in fact doubling the amount of credit returned to the customer.
spaghetticode
force-pushed
the
spaghetticode/double-store-credit-refund
branch
from
f4aa847 to
3dc064f
Compare
|
Hey @spaghetticode, thanks for fixing this regression. I'm concerned about one thing though. If we merge the first two commits in 2.11 only, the code in 3.0 will be quite different and I'd prefer this not to happen. What about moving them into a PR for |
|
@kennyadsl sure, that makes totally sense 👍 |
5 participants
Description
This PR aims at solving a very specific bug that happens when refunding a store credit payment by creating a new store credit record (i.e. when the preference
credit_to_new_allocationistrue).Under these circumstances, performing the refund will generate 2 identical store credits records instead of one, so the customer ends up getting store credits for twice the refund amount. This happens because the method
process!ends up being called twice.The issue does not exist in master as the code that enables the bug has already been removed.
Checklist: