openfoodfoundation / openfoodnetwork

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

[User->New] Language is displayed twice #11513

Open drummer83 opened 8 months ago

drummer83 commented 8 months ago

Description

Follow up of #11487.

Admin > Users > New

The default language (English in this case) is appearing twice in the list of languages. I assume this happens because we are listing the LOCALES + AVAILABLE LOCALES (see .env file). If the locale is listed as available locale as well (as it is probably in a well configured instance) we should only show it once.

Expected Behavior

Show every language once only.

Actual Behaviour

The default language is shown twice.

Steps to Reproduce

  1. In the .env file (or .env.development or .env.staging) make sure you have the LOCALE language listed in the AVAILABLE LOCALES as well. Restart Puma if you had to change this.
  2. Log in as super admin.
  3. Check the language dropdown menu on the /admin/users/new page

Animated Gif/Screenshot

image

Workaround

Severity

bug-s5: we can live with it, only a few users impacted

Your Environment

Possible Fix

Decoder76 commented 4 months ago

Assign this to me

drummer83 commented 4 months ago

Thanks @Decoder76!

Don't hesitate to get in touch here on in Slack if you get stuck. Response times may be a bit longer during Christmas time though...

RachL commented 4 months ago

Hello @Decoder76 just checking in: are you still working on this issue?

dacook commented 2 months ago

This form is located at app/views/spree/admin/users/_form.html.haml, which points to https://github.com/openfoodfoundation/openfoodnetwork/blob/master/lib/open_food_network/i18n_config.rb#L18-L21 Looks like the duplicates are not considered by uniq. Maybe we need to add a block with a custom comparison for uniq.

sergioosouzaa commented 2 months ago

Hey, I'm interested in solving this issue, but I've had a hard time reproducing it.

I listed the locale again in the available locales, but the language appears only once on the list.

Is there anything else I should try to reproduce the issue? Has this bug been solved?

My .env:

LOCALE="en"
AVAILABLE_LOCALES="en,fr,es"

Image

dacook commented 2 months ago

Thanks for looking into this. @drummer83 I can see now this is a configuration problem, not a bug. I just tried this:

LOCALE="en"
AVAILABLE_LOCALES="en,en_AU,en_GB,fr,es,en"

I can see three "English" options, which I believe is correct: en, en_AU and en_GB. There's no duplicate of en. They look like duplicates because they have the same label, because normally we don't have more than one English enabled on a system.

Screenshot 2024-03-19 at 9 23 02 am

I looked at the staging environment, and we have something a bit different:

LOCALE="en_AU"
AVAILABLE_LOCALES="en,fr,es"

I don't think it should be configured like that, so will update it.

dacook commented 2 months ago

Ok, I fixed the staging config, but there is still a bug. The default en language is always showing. For example with this config, en is appended to the end of the list. @sergioosouzaa would you be able to investigate why?

LOCALE="en_AU"
AVAILABLE_LOCALES="en_AU,fr,es"

Screenshot 2024-03-19 at 9 56 33 am

sergioosouzaa commented 2 months ago

@dacook I was able to identify the issue, however, I need some clarification about the business rules. English is always being appended at the end of the list, because the source_locale defined on the code (en) is always appended at the end, and in this case, since the default language is en_AU, this passes the .uniq method. Is this behaviour intended? Should English always be available even if it is not in the .env variables?

dacook commented 1 month ago

I think that here, when creating a user, only the configured "AVAILABLE_LOCALES" should be showing.

Looking again now, I think I can see this was an unintended effect in PR referred to at the top of this issue. I think that I18nHelper::locale_options, should be using the method OpenFoodNetwork::I18nConfig.selectable_locales instead of available_locale.

https://github.com/openfoodfoundation/openfoodnetwork/blob/master/lib/open_food_network/i18n_config.rb#L12-L21 (it's a bit confusing because the method names don't match the env var name)

mkllnk commented 2 weeks ago

Fix was reverted:

mkllnk commented 2 weeks ago

Instead of modifying available_locales we should make use of locale_options in the front-end.

dacook commented 1 week ago

☝️ Agreed!!