NoMethodError in admin/order_cycles#index

[sauloperez]

Description

Katuma's Bugsnag reported the following error a week ago.

Expected Behavior

At the point the OrderCycle model is used user should always be defined.

Actual Behavior

The app fails with the mentioned error when issuing the following request:

curl --request GET \
  --header 'Accept: application/json, text/javascript, */*' \
  --header 'X-Csrf-Token: 7xGaO9wLnV3Ey6PjKvrt9KY/4u+VXVpFEYZFAiERzaI=' \
  --header 'User-Agent: Mozilla/5.0 (Linux; Android 7.0; SM-J710F Build/NRD90M) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.126 Mobile Safari/537.36' \
  --header 'Referer: https://alpha.katuma.org/admin/order_cycles' \
  --header 'Accept-Encoding: gzip, deflate, br' \
  --header 'Accept-Language: es-ES,es;q=0.9' \
  --header 'Cookie: [FILTERED]' \
  'https://alpha.katuma.org/admin/order_cycles.json?ams_prefix=index&q%5Borders_close_at_gt%5D=Sun+Apr+01+2018+00:00:00+GMT%2B0200+(CEST)'

Steps to Reproduce

1. Log in as admin
2. Navigate to Order Cycles
3. Clear session in browser manually
4. Click "Show 30 more days"
5. 💥

Severity

The reach of this bug is still unknown but doesn't seem to affect too many users, so severity 3.

Your Environment

• Version used: v1.14
• Browser name and version: Possibly Firefox
• Operating System and version (desktop or mobile): Android

[frank-west-iii]

Seems like this might be related to a session timeout.

The problem lies in the order of execution for the before_filters:

openfoodnetwork/app/controllers/admin/order_cycles_controller.rb

Line 9 in d218a51

prepend_before_filter :load_data_for_index, :only => :index

Due to the prepend we are executing the query before any auth happens. If we remove the prepend we get no error on the server, however, we don't get a great user experience either. They receive a console error and an infinite spinner. Not entirely sure why prepend was needed here so I am not sure removing it is completely safe. Seems to work though.

JSON requests that are unauthorized are returning html (the home page) instead of JSON with a 401 status.

There might be a way to override this behavior from Spree?

[sauloperez]

great investigation @frank-west-iii ! I'm not sure what's best then here. Perhaps it's an error we can live with.

There might be a way to override this behavior from Spree?

These two words together (override and Spree) are terrifying

[frank-west-iii]

Agree re: overriding Spree. 👍

[myriamboure]

So what is the status for this bug @sauloperez @frank-west-iii do we move it forward? Leave with it and close? Or do something?

[frank-west-iii]

We could at least fix the error so it does not occur again.

The user will still not see any updates to their page when they click an action like "Show 30 more days". It will just fail silently for them. The user might then refresh their browser or take an action that will require them to log in again and the page will work again for them until they time out their session again 😄

The UX for it is not great, but that may be a more widespread issue across the application rather than this specific issue.

I can remove the error from our logs, but a much bigger conversation may need to be had about handling 401 errors across the application on AJAX requests.

[sauloperez]

The user will still not see any updates to their page when they click an action like "Show 30 more days". It will just fail silently for them. The user might then refresh their browser or take an action that will require them to log in again and the page will work again for them until they time out their session again

That sounds like a good trade-off to me. Can you work on it?

I can remove the error from our logs, but a much bigger conversation may need to be had about handling 401 errors across the application on AJAX requests.

Agree we need to review it but if we could live with it a bit longer... we have too many open 🔥 🔥

Proposals are welcome though @frank-west-iii