Openfoodnetwork: Available locales login bug

[filipefurtad0]

Description

Error 500 observed by @RachL when logging in to staging-FR - possibly with account: rachel.[email protected]

Bugsnag error:

https://app.bugsnag.com/open-food-france/open-food-france-prod/errors/5ef354f0dc9a3f001732340b?event_id=5ef354f0005b757586b80000&i=sk&m=nw

Unhandled error
I18n::InvalidLocale: "en_GB" is not a valid locale
Location
app/helpers/i18n_helper.rb:14 - set_locale

Possible Fix

Not much more information, but perhaps related to #5648?

[luisramos0]

I think this should block the roll out... I am tagging s2.

[luisramos0]

I went to the DB and changed the locale manually to the unsupported locale en_GB, I get the snail.
The error is the same as the one in the description above but not the one on bugsnag.

F, [2020-06-30 20:55:23#19750] FATAL -- :
I18n::InvalidLocale ("en_GB" is not a valid locale):
  app/helpers/i18n_helper.rb:14:in `set_locale'

Where did you see this error Filipe?

The bugsnag error comes from this method enforce_available_locales that is only called if config.enforce_available_locales is enabled.
But we have
https://github.com/openfoodfoundation/openfoodnetwork/blob/master/config/application.rb#L136

Anyway, I am not too worried about this, this must have been a mess up of configs and tests in staging. I dont think this should block the rollout so I am moving it to S3, ok?

[luisramos0]

This could be a regression but there's no indication that it is...

[filipefurtad0]

Hey @luisramos0 ,
Thanks for checking this.

I didn't trigger this error myself, I saw it on the slack channel, as @RachL commented it:
https://openfoodnetwork.slack.com/archives/CEF14NU3V/p1593005297365300

And decided to open an issue should it happen again and also, due to the unexpected logout during testing (issue #5648). I have no further information on this, so I agree with downgrading. @RachL do you feel there is any detail to add?

[Matt-Yorkley]

Did the available languages get changed? Maybe an old login with that locale set for the account?

[luisramos0]

@filipefurtad0 sorry, my mistake, now I see the errors are the same!

**gems/i18n-0.6.11/lib/i18n.rb:285:in `enforce_available_locales!':**
en_GB" is not a valid locale (I18n::InvalidLocale)
    from gems/i18n-0.6.11/lib/i18n/config.rb:11:in `locale='
    from gems/actionpack-4.0.13/lib/action_view/lookup_context.rb:226:in `locale='
    from gems/actionpack-4.0.13/lib/abstract_controller/rendering.rb:28:in `locale='
    from gems/i18n-0.6.11/lib/i18n.rb:35:in `locale='
    **from app/helpers/i18n_helper.rb:14:in `set_locale'**

@Matt-Yorkley yes, something like that. but I dont understand why enforce_available_locales is blowing up when the config is set to false...

[sauloperez]

I'm picking this up because it's blocking testing.

[sauloperez]

The site is back after https://github.com/openfoodfoundation/openfoodnetwork/pull/5561#issuecomment-652291640. There are some chances that someone may have this old locale stored in the cookie but we can fix it when and if it happens.

[luisramos0]

yeah, maybe we should leave this open as an S4?
I am saying this because I think that although we have config.enforce_available_locales set to false, the locales are being enforced.
Or we could just change config.enforce_available_locales to true in the config file?

[sauloperez]

we seem to be using i18n 0.6.11 and the fix at https://github.com/ruby-i18n/i18n/pull/249/commits/a65fffd6f0472bc413ba41809dafc6292e29d2ae is included in this version so we should be good to remove that configuration and rely on the default enforcement.

So yes, we can take that path so we better keep this open.

[Matt-Yorkley]

We noticed this in the CI build during the upgrade as well, no? Locales were being strictly enforced, we had to tweak some tests to get the build green.

Any chance config.enforce_available_locales is deprecated now, and not being used?

[luisramos0]

I think this will happen in prod, I think enforce_available_locales is activated.

For example, in FR, 4 users will see the snail:

10 users in uk:

875 users in AUS :warning:

I'd say this is a release blocker after all.

[luisramos0]

I'd say a simple migration will be enough to fix this: load available locales and update locale of all users with locales not on that list.

[Matt-Yorkley]

Either way, this error is thrown when a session is loaded when the user logs in, right? We should catch it and resolve it nicely. Maybe resetting the selected locale to the default one for the server if the locale saved in the user's session cookie doesn't exist?

Otherwise I can imagine other potential edge cases in the future, like: an instance updates their available locales list, and suddenly lots of users can't log in :boom:

[luisramos0]

yes, that solution will be more resilient than a migration :+1:

[sauloperez]

just for me to understand, you suggest we don't apply any migration for the invalid locales in DB and fix them manually as they happen @Matt-Yorkley ?

[luisramos0]

that's what I understood and that's better, that way we will support the removal of a locale from the available locales.

[sauloperez]

what if the locale is not set in the cookie but loaded from DB? that will blow up, right? or am I missing something? In any case, we should doc somewhere that updating the list of locales may require cleaning up invalid values from DB.

[luisramos0]

I dont think we need that.
We just need to make I18nHelper#set_locale resilient
something like this?

EDIT: and probably add
spree_current_user.update locale: I18n.default_locale

[sauloperez]

EDIT: and probably add spree_current_user.update locale: I18n.default_locale

ok, at least this. Otherwise, exchanging a data migration to update invalid data in DB for defensive coding doesn't end well IMO

[Matt-Yorkley]

We could do both, if you want? A one-off migration that fixes current broken values in the DB, and defensive code for any potential future problems (with specs).

[Matt-Yorkley]

Bumping this to s1, as it looks like various users might be unable to log in due to this. I think it also blocks the next release, and the rollout in general.

[Matt-Yorkley]

cookies[:locale] could equally contain an invalid locale, right? We shouldn't return it without checking it.

[luisramos0]

yes. defensive coding on cookies :+1:

Sent from my iPhone

On 02 Jul 2020, at 11:51, Matt-Yorkley [e-mail] wrote:

cookies[:locale] could equally contain an invalid locale, right?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[sauloperez]

Agree with this manageable bit of defensive coding but

We could do both, if you want? A one-off migration that fixes current broken values in the DB, and defensive code for any potential future problems (with specs).

IMO we go with one or the other otherwise, we increase costs with the same result. In this case, I think leaning the trade off towards the defensive coding it's fine in this case. It'd be different if we enforced this at the DB-schema level.