Openfoodnetwork: User should not see a snail, when accessing OFN via direct link to a shop that does not exist or is inactive

[luisramos0]

Description

We have over 8k in AUS and 2.5 in UK events of this type:
https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/5dd433e559edc60018caf959?filters[event.since][0]=30d&filters[error.status][0]=open

Users type the correct shop URL but the shop name refers to a non existing shop or an inactive shop. The user sees a snail (and a bugsnag event is created).
This damages reputation and seo.

Expected Behavior

The user can be redirected to the list of shops. Ideally an flash message is displayed "The shop you are looking for doesn't exist or is inactive on OFN. Please check other shops."

Actual Behaviour

Snail.

Steps to Reproduce

Inexistent enterprise:
https://openfoodnetwork.org.uk/theres-no-place-like-home/shop

Inactive/not visible shop:
https://openfoodnetwork.org.uk/pomeroys-rare-breeds-robyns-famhouse/shop
Create a shop and set it so that it's not visible in the frontoffice then visit it's shopfront using it's permalink.

Workaround

None.

Severity

We are talking about hundreds of (potential) customers seeing the OFN snail page.
bug-s3: a feature is broken but there is a workaround

Your Environment

• Version used: UK and AU live
• Browser name and version: firefox
• Operating System and version (desktop or mobile): osx catalina

Possible Fix

We just need a redirect to the shop list page and flash message. A few lines of code on the controller.

[Matt-Yorkley]

Here's the other related issue: https://github.com/openfoodfoundation/openfoodnetwork/issues/3765

[jeduardo824]

Can I work on this one or leave it for new developers?

[sigmundpetersen]

Hey @jeduardo824 you are very welcome to pick this one up 🙂

[jeduardo824]

Great. I should send a PR by the weekend.

[jeduardo824]

Hey guys, I was working on a solution, but I don't know if the path I'm following is the best.

Basically, since .find throws an exception when the record is not found, I'm doing this:

rescue_from ActiveRecord::RecordNotFound, with: :redirect_to_shops

[...]

private

  def redirect_to_shops
    flash[:error] = #I18n with message
    redirect_to shops_path
  end
[...]

What do you guys think about that?

[jeduardo824]

I'm also can use a begin rescue block. The real doubt is about the exception rescue. 😄

[luisramos0]

yeah, I think that's it :+1: But I'd always go for a local rescue.
in this case I'd remove the distributor assignment form reset_order (remove the first line of the method) and add a method like this:

  def distributor
    @distributor ||= Enterprise.is_distributor.find_by_permalink(params[:id]) ||
                     Enterprise.is_distributor.find(params[:id])
  rescue ActiveRecord::RecordNotFound
    flash[:error] = #I18n with message
    redirect_to shops_path
  end

[jeduardo824]

The problem with this approach is when the method reset_distributor(order, distributor) is called. I'm getting a type mismatch error.

[luisramos0]

I'd suggest you create a draft PR and paste the error. Someone will be able to help you.

btw, looking at the code again, I realized most of this order reset logic here should be moved to the reset_order_service.