openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 723 forks source link

Error when no shipping info has been selected in split checkout #10402

Closed dacook closed 1 year ago

dacook commented 1 year ago

Description

When using the split checkout feature toggle, On the first step, if you don't select any shipping options and click next, an unhandled error occurs.

The error is: https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/63e444615e34f60008a82b2b?event_id=63e4446100ad28aa1f5f0000&i=sk&m=nw

Expected Behavior

I assume this is required, so I think there should be a validation error to say so.

Actual Behaviour

šŸŒ

Steps to Reproduce

Animated Gif/Screenshot

Screen Shot 2023-02-09 at 12 12 49 pm Screen Shot 2023-02-09 at 12 13 03 pm

Workaround

Select a shipping method before clicking next.

Severity

Not sure how to classify for a feature toggle.

bug-s1: a critical feature is broken: checkout, payments, signup, login bug-s2: a non-critical feature is broken, no workaround bug-s3: a feature is broken but there is a workaround bug-s4: it's annoying, but you can use it bug-s5: we can live with it, only a few users impacted

https://github.com/openfoodfoundation/openfoodnetwork/wiki/Bug-severity -->

Your Environment

dacook commented 1 year ago

FYI @jibees

RachL commented 1 year ago

That's weird because I can't reproduce it in staging FR... what's the difference between the two?

image

filipefurtad0 commented 1 year ago

So @dacook has reproduced this in staging-UK and staging-AU (above).

I've checked a random account in staging-UK and cannot observe it -> I get the expected behavior (same as you've posted above @RachL ).

So there must be a data combination which triggers this. I'll look into the bugsnag, see if I can reproduce it consistently.

filipefurtad0 commented 1 year ago

Ok, I've reproduced it in staging-UK now: the error is triggered for first time customers only.

I guess if the shipping address (or any required information in Details) is missing, the app should not 'try' to update the customer:

app/services/default_address_updater.rb:18 customer.save

Right?

My steps to reproduce were:

Thanks so much for catching and reporting this one @dacook :pray: Now, to check why the spec does not nail this - I'm guessing, because we're using a guest account, but will address this elsewhere.

RachL commented 1 year ago

Nice catch !!! this makes it an s2 for the roll-out I think! Upgrading the severity

jibees commented 1 year ago

Nice one, nice catch! @filipefurtad0 I agree with you, I think it's important to catch that one with spec!

jibees commented 1 year ago

Actually pretty easy to text:

spec/system/consumer/split_checkout_spec.rb#287:

    context "when no selecting a shipping method" do
      before do
        fill_out_details
        fill_out_billing_address
      end

      it "does not throw an error" do
        click_button "Next - Payment method"
        expect(page).to have_content "Please select a shipping method"
      end
    end