Improve Bogus (test) Credit Card voiding #4861
Conversation
kennyadsl
added
type:enhancement
chrean
changed the title
kennyadsl
force-pushed
the
kennyadsl/better-bogus-try-void
branch
from
eeef264 to
a8d2f06
Compare
There was a problem hiding this comment.
Thanks, @kennyadsl. Leaving some comments. It looks like the tests don't reflect the current responsibilities. Feel free to merge if I'm missing something or if you see it as something that we can't address at this point (this PR is already an improvement on its own).
| end | ||
| end | ||
|
|
||
| it "should not change the payment state whe the payment is completed" do | ||
| expect(order).to be_allow_cancel |
There was a problem hiding this comment.
Shouldn't this expectation fail now?
There was a problem hiding this comment.
Mhh, no. The completed payment state doesn't change. It stays completed but a refund will be issued.
There was a problem hiding this comment.
Anyway, there's a typo in the spec description which I'm going to fix.
| @@ -113,9 +113,18 @@ | |||
| end | |||
| end | |||
|
|
|||
| it "should automatically refund all payments" do | |||
| context "with a pending payment" do | |||
There was a problem hiding this comment.
If I understand correctly, the tested behavior depends on the underlying payment method. Do these tests make sense here?
There was a problem hiding this comment.
You are right, this should probably go in the specific payment method specs. And we should just test that the outer interface method is called here. Let me see if I can do the change.
There was a problem hiding this comment.
Again, you were right. This behavior is already tested in order specs and payment cancellation specs. This change gives us the opportunity to remove the duplication.
In payment method specs instead, we now have the opportunity to just use the bogus credit card instead of the anonymous classes. The downside is that we'll connect these bogus implementations to the tests, but I think it also has an advantage: if we make some changes that make these specs fail, it most likely will be the same for other payment gateways with the same interface, and at least we will notice immediately.
Please re-review now, commit-by-commit should be easier.
| # @see Spree::PaymentMethod#try_void | ||
| def try_void(_payment) | ||
| ActiveMerchant::Billing::Response.new(true, SUCCESS_MESSAGE, {}, test: true, authorization: AUTHORIZATION_CODE) | ||
| def void(_response_code, _credit_card, options = {}) |
There was a problem hiding this comment.
Should we add/update tests for that in the bogus credit card specs?
There was a problem hiding this comment.
At the moment, there are no specs for this class: https://github.com/solidusio/solidus/blob/c6050d71707063d1e46693f7d388ebc4c07d1a60/core/spec/models/spree/payment_method/bogus_credit_card_spec.rb
There was a problem hiding this comment.
This is tested now, but in the payment method specs, see the second commit. We could move these tests into the bogus own spec files, but I tend to think it has more value if we leave them together in the payment_method specs. Still open to changing my mind, though.
|
@waiting-for-dev I think your suggestions make sense, I'll take a look. To be honest, I didn't expect any specs to fail after this change but apparently we have a lot of specs relying on the BogusCreditCard behavior, being that the parent class of the factories we are using in specs for credit card payments. |
It now simulates what really happens on most credit cards payment gateway. If the payment has been already captured, it's not possible to void it, and the operation will fail.
kennyadsl
force-pushed
the
kennyadsl/better-bogus-try-void
branch
3 times, most recently
from
014fc4f to
cb22d29
Compare
It will fall back to Spree::PaymentMethod#try_void via class inheritance. Now that the parent class' try_void has an implementation, we can rely on that for the Simple/BogusCreditCard. Before this change, all the attempts to voiding a payment in the admin panel would be succeding. Now we can simulate a failure (by trying to cancel an order with captured payment) and this will create a refund for that payment instead. Some specs have been updated as well: - order state machine: refactored and added some scenarios. Now the specs are only testing that cancel! on payment is actually called, leaving to Spree::Payment::Cancellation and its specs the responsibility to test what happens when it's called. - payment method: to simulate what happens with real payment methods, we used to create an anonymus class with the expected behavior. This behavior is now identical to the default implementation, used by simple bogus credit card and bogus credit card. We can just use the factories.
We had a lot of duplication between specs in order_spec and order/state_machine_spec. I tried to make some order, moving all the cancel! logic specs into order_spec and leaving to order/state_machine the only responsibily to test the value of can_cancel? because that's the only thing that is done by the state machine, other than calling Spree::Order#after_cancel when the transition happens.
kennyadsl
force-pushed
the
kennyadsl/better-bogus-try-void
branch
from
cb22d29 to
2eab6a4
Compare
Follow up for #4843 and #4859.
Summary
Before this change, all the attempts to void a payment in the admin panel would be succeeding. This is not realistic because, in real life, you can't void captured credit card payments.
With this change, we can simulate a failure (by trying to cancel an order with captured payment). This will leave the payment in the complete state and will create a refund for that payment.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):