Removes unused spec related to legacy checkout #12417
Conversation
filipefurtad0
added
the
technical changes only
| it "clears shipments and payments before rendering the checkout" do | ||
| put update_checkout_path, params:, as: :json | ||
|
|
||
| # Checking out a bogus Stripe Gateway without a source fails at :payment | ||
| # Shipments and payments should then be cleared before rendering checkout | ||
| expect(response.status).to be 400 | ||
| expect(flash[:error]) | ||
| .to eq 'Payment could not be processed, please check the details you entered' | ||
| order.reload | ||
| expect(order.shipments.count).to be 0 | ||
| expect(order.payments.count).to be 0 | ||
| expect(order.adjustment_total).to eq 0 |
There was a problem hiding this comment.
Do we need to cover this scenario for the new checkout ?
There was a problem hiding this comment.
Do we need to cover this scenario for the new checkout ?
Thanks, indeed this is the question I wanted to raise. I don't know if we reach this state on the current checkout - a (Stripe) payment entity without a source. If we do, then I think we should expect a status 400, and not 302. If we don't, then I guess there is no point having this new test.
For #10958 I'm trying to find a correspondence between old and new specs. I might run into such situations more often.
There was a problem hiding this comment.
I've added a commit to undo changes and remove the added test, as it is perhaps not particularly useful.
There was a problem hiding this comment.
Thanks, indeed this is the question I wanted to raise. I don't know if we reach this state on the current checkout - a (Stripe) payment entity without a source. If we do, then I think we should expect a status 400, and not 302. If we don't, then I guess there is no point having this new test.
I don't know enough about payment and source to know if it's plausible scenario, happy to have a look if needed. I am wondering if the new checkout does not return a 400 in this scenario but display an error to the user instead.
There was a problem hiding this comment.
It sounds like we should try to replicate it with a feature spec. If not replicable, then I guess we can move on.
There was a problem hiding this comment.
It's tough to replicate Stripe errors on feature specs - we'd need a proxy to intercept HTTP requests to the real Stripe... I've tried this with puffing billy but did not succeed.
This error just came in on bugsnag, it looks somewhat related (touches the checkout controller and the payment source). So maybe this scenario is relevant after all. I'll have a more thorough look tomorrow.
There was a problem hiding this comment.
Good pickup on the error!
I noticed that some data was blank in the request. Perhaps (hopefully) the credit card details were scrubbed before logging, but I wonder about gateway_payment_profile_id.
{
"_method": "put",
...
"order": {
"payments_attributes": [
{
"source_attributes": {
...
"month": "",
"year": "",
"cc_type": "",
"last_digits": "",
"gateway_payment_profile_id": ""
},
"payment_method_id": "201053"
}
]
},
"commit": "Next - Order summary"
}
It gets set a few different ways in the codebase, but perhaps it's getting lost somehow.
There was a problem hiding this comment.
Interesting. The request points to "payment_method_id": "201053" which - by looking at the production DB using Metabase - leads to a Spree::PaymentMethod::Check payment method. It seems we're getting this error across production servers (an error 500 on checkout 👀 ). Below, also on FR-prod:
One our codebase, we have the line:
# nil means the payment method doesn't require a source e.g. check
I wonder if this happens due to a sort of race condition in which initially a Stripe payment method is selected for checkout but, during this time, disabled but the admin... or so. Since it relates to checkout, I think it might deserve an issue.
There was a problem hiding this comment.
Ok, I've tried to reproduce the bugsnag error with a spec:
- with a cash payment method, we can trigger the error message, but somehow we get a status 200 (I was expecting 500')
- with Stripe payment method, we can see the data is incomplete, and just get the expected 422, with no error message
I'm not sure how to proceed on this one; do we expect payment_source_class to be required for payment methods with type: "Spree::PaymentMethod::Check"?
There was a problem hiding this comment.
Good find Filipe! I think the comment is right, we should just make it nil in this case. I've updated the code and the spec to show the error is now avoided. How does that look?
filipefurtad0
force-pushed
the
migrate_legacy_specs
branch
3 times, most recently
from
db4c4b0
to
f2eae55
Compare
| it "clears shipments and payments before rendering the checkout" do | ||
| put update_checkout_path, params:, as: :json | ||
|
|
||
| # Checking out a bogus Stripe Gateway without a source fails at :payment | ||
| # Shipments and payments should then be cleared before rendering checkout | ||
| expect(response.status).to be 400 | ||
| expect(flash[:error]) | ||
| .to eq 'Payment could not be processed, please check the details you entered' | ||
| order.reload | ||
| expect(order.shipments.count).to be 0 | ||
| expect(order.payments.count).to be 0 | ||
| expect(order.adjustment_total).to eq 0 |
There was a problem hiding this comment.
Good find Filipe! I think the comment is right, we should just make it nil in this case. I've updated the code and the spec to show the error is now avoided. How does that look?
|
Sorry I forgot to press submit on my comments 😀 |
filipefurtad0
added
user facing changes
Awesome, @dacook . I think this can have an impact on checkout, so:
We do have system specs for checkout with cash/check. Do you think it's worth having a round of manual testing? |
|
Great job Filipe 👍
I'm not sure, but given that we don't have a system spec for this case and don't know how to replicate it, I think it would be worth a manual test. Just to see if we can replicate or learn anything more about it. Would you be willing to do that? |
|
I just realised we only have one review for my code change. I'll request Maikel's review before proceeding. |
| @@ -308,6 +308,62 @@ | |||
| end | |||
| end | |||
|
|
|||
| context "with no payment source" do | |||
There was a problem hiding this comment.
FYI: you referenced a Bugsnag issue but Bugsnags retention is quite limited and it a month or so the link won't be valid anymore.
|
I think that we should check the checkout on staging though, with a cash payment method. |
drummer83
added
pr-staged-au
|
Hi @filipefurtad0, I have staged the PR and tested the following:
We do see some errors in the console. I am just now staging master to check if we have it there too. Probably it's unrelated. We shall see. Here is the console output: I have to stop here and check master later. Thanks again! |
|
Sorry it took so long, @filipefurtad0! I can confirm that the errors are in master as well. Unfortunately there is a conflict now. Could you have a look at that please? Let me know if I should re-test afterwards. Thanks and sorry again! 🙏 |
drummer83
added
feedback-needed
and removed
no-staging-AU
We can see on the respective controller spec, that having a Stripe SCA payment, with no source does not trigger the error 400, observed on the legacy checkout.
For details see: https://app.bugsnag.com/open-food-network-canada-1/open-food-network-canada/errors/66314b2e78673c00073d2de9?filters[event.since]=30d&filters[error.status]=open&filters[search]=payment_source_class&event_id=66314b2e00e6e45d746f0000 Adds test case for Cash and Stripe payment methods With no source
Most of the time this doesn't get called because source_required: false. But sometimes it [does happen](https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/66329690f4b6380007e8a4f8) I have a feeling that source_required? could be moved to the superclass as payment_source_class.present?. But I don't know enough about this area of the system to try it...
Removes unused error message
filipefurtad0
force-pushed
the
migrate_legacy_specs
branch
from
661a9cf
to
a1e2d3a
Compare
Hey @drummer83 , All good - thank you for testing. Everything else is left untouched. I would say it's ok to merge straight away - I've moved to ready to go and wait till someone confirms :-) |
|
Merged, thanks all! |
Closes #12439.
Removes a legacy spec (
./spec/requests/checkout/failed_checkout_spec.rb) and updates an existing spec (./spec/controllers/checkout_controller_spec.rb).What? Why?
Contributes to #10958 (first bullet point)
From previous specs, attempting to checkout with a payment with
source = nil, related a payment on legacy checkout, with a Stripe SCA payment method, triggered a error 400, preventing checkout completion.This seems not to be the case on the current checkout.
The legacy spec has been removed and the existing controller spec updated to reflect this.
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates