Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide support to fix locale selection in admin login page #4493

Conversation

gsmendoza
Copy link
Contributor

@gsmendoza gsmendoza commented Aug 8, 2022

Expected behavior

Given I am running a Solidus app with Solidus I18n (see https://github.com/gsmendoza/rails_7_store/tree/solidus_30253731078c_3_2_0_alpha_solidus_i18n)

And I am in the admin login page

When I change the locale in the locale selector (e.g. to Espanol MX)

Then I should see the locale change on the login page

Actual behavior

The "Loading" popup window appears but nothing happens.

In the backend, a PUT request is submitted to "/admin/locale/set" but the request is redirected to the login page, e.g.

Started PUT "/admin/locale/set" for ::1 at 2022-08-04 17:24:49 +0800
Processing by Spree::Admin::LocaleController#set as JSON
Parameters: {"switch_to_locale"=>"es-MX"}
Redirected to http://localhost:3000/admin/login
Completed 200 OK in 11ms (ActiveRecord: 0.0ms | Allocations: 4703)

If you try to log in afterwards, you are redirected to http://localhost:3000/admin/locale/set with the following error:

Routing Error: No route matches [GET] "/admin/locale/set"

Causes

  1. Non-logged in users are not authorized to change the admin locale.
  2. When the language is changed on the admin, SolidusAuthDevise's admin controllers do not react to this, because their set_user_language_locale_key is not set to :admin_locale.

Background

solidusio/solidus_auth_devise#224 was submitted to fix the second cause of the bug: set_user_language_locale_key not being set to :admin_locale. Based on the discussions in the PR, we decided to update Solidus in two ways:

  1. Fix the first cause of the bug: Non-logged in users are not authorized to change the admin locale.
  2. Extract SetsUserLanguageLocaleKey so that Fix locale inconsistence and remove redundant template solidus_auth_devise#224 won't have to refer to :admin_locale key directly.

Fix Demo

Tested manually with this Rails app: https://github.com/gsmendoza/rails_7_store/tree/gsmendoza/eng-436-changing-the-locale-in-the-admin-login.

The Rails app includes this corresponding fix for SolidusAuthDevise: https://github.com/gsmendoza/solidus_auth_devise/tree/gsmendoza/eng-436-changing-the-locale-in-the-admin-login.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

  • [ ] I have added automated tests to cover my changes.
  • [ ] I have attached screenshots to demo visual changes.
  • [ ] I have opened a PR to update the guides.
  • [ ] I have updated the readme to account for my changes.

Devise::SessionsController classes in SolidusAuthDevise will include
this module in order to set the user language locale keys to the admin
key.

See
solidusio/solidus_auth_devise#224 (comment).
The admin authorization prevents a guest user from changing the
locale while in the admin login page.

See
solidusio/solidus_auth_devise#224 (comment).
@gsmendoza gsmendoza marked this pull request as ready for review August 8, 2022 09:18
Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @gsmendoza!

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gsmendoza!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants