Merge pull request #5718 from Matt-Yorkley/available-locales-bug

Available locales bug

app/controllers/spree/user_sessions_controller.rb

@@ -11,6 +11,7 @@ module Spree
     ssl_allowed :login_bar
 
     before_action :set_checkout_redirect, only: :create
+    after_action :ensure_valid_locale_persisted, only: :create
 
     def create
       authenticate_spree_user!
@@ -48,5 +49,13 @@ module Spree
       redirect_to(session["spree_user_return_to"] || default)
       session["spree_user_return_to"] = nil
     end
+
+    def ensure_valid_locale_persisted
+      # When creating a new user session we have to wait until after a successful
+      # login to be able to persist a selected locale on the current user
+
+      UserLocaleSetter.new(spree_current_user, params[:locale], cookies).
+        ensure_valid_locale_persisted
+    end
   end
 end

app/helpers/i18n_helper.rb

@@ -1,32 +1,9 @@
 module I18nHelper
   def set_locale
-    # Save a given locale
-    if params[:locale] && available_locale?(params[:locale])
-      spree_current_user&.update!(locale: params[:locale])
-      cookies[:locale] = params[:locale]
-    end
-
-    # After logging in, check if the user chose a locale before
-    if spree_current_user && spree_current_user.locale.nil? && cookies[:locale]
-      spree_current_user.update!(locale: params[:locale])
-    end
-
-    I18n.locale = spree_current_user.andand.locale || cookies[:locale] || I18n.default_locale
+    UserLocaleSetter.new(spree_current_user, params[:locale], cookies).set_locale
   end
 
   def valid_locale(user)
-    if user.present? &&
-       user.locale.present? &&
-       available_locale?(user.locale)
-      user.locale
-    else
-      I18n.default_locale
-    end
-  end
-
-  private
-
-  def available_locale?(locale)
-    Rails.application.config.i18n.available_locales.include?(locale)
+    UserLocaleSetter.new(user).valid_current_locale
   end
 end

app/services/user_locale_setter.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+class UserLocaleSetter
+  def initialize(current_user, params_locale = nil, cookies = {})
+    @current_user = current_user
+    @params_locale = params_locale
+    @cookies = cookies
+  end
+
+  def set_locale
+    save_locale_from_params
+
+    I18n.locale = valid_current_locale
+  end
+
+  def ensure_valid_locale_persisted
+    return unless current_user && !available_locale?(current_user.locale)
+
+    current_user.update!(locale: valid_current_locale)
+  end
+
+  def valid_current_locale
+    if current_user_locale && available_locale?(current_user_locale)
+      current_user_locale
+    elsif cookies[:locale] && available_locale?(cookies[:locale])
+      cookies[:locale]
+    else
+      I18n.default_locale
+    end
+  end
+
+  private
+
+  attr_reader :current_user, :params_locale, :cookies
+
+  def save_locale_from_params
+    return unless params_locale && available_locale?(params_locale)
+
+    current_user&.update!(locale: params_locale)
+    cookies[:locale] = params_locale
+  end
+
+  def available_locale?(locale)
+    Rails.application.config.i18n.available_locales.include?(locale)
+  end
+
+  def current_user_locale
+    current_user&.locale
+  end
+end

spec/features/consumer/authentication_spec.rb

@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 feature "Authentication", js: true do
+  include AuthenticationWorkflow
   include UIComponentHelper
   include OpenFoodNetwork::EmailHelper
 
@@ -135,5 +136,57 @@ feature "Authentication", js: true do
       uri = URI.parse(current_url)
       expect(uri.path + "#" + uri.fragment).to eq('/#/login')
     end
+
+    describe "with user locales" do
+      before do
+        visit root_path
+        open_login_modal
+      end
+
+      context "when the user has a valid locale saved" do
+        before do
+          user.update!(locale: "es")
+        end
+
+        it "logs in successfully, applying the saved locale" do
+          fill_in_and_submit_login_form(user)
+          expect_logged_in
+
+          expect(page).to have_content I18n.t(:home_shop, locale: :es).upcase
+        end
+      end
+
+      context "when the user has an unavailable locale saved" do
+        before do
+          user.update!(locale: "xx")
+        end
+
+        it "logs in successfully and resets the user's locale to the default" do
+          fill_in_and_submit_login_form(user)
+          expect_logged_in
+
+          expect(page).to have_content I18n.t(:home_shop, locale: :en).upcase
+          expect(user.reload.locale).to eq "en"
+        end
+      end
+
+      context "when the user has never selected a locale, but one has been selected before login" do
+        before do
+          user.update!(locale: nil)
+        end
+
+        it "logs in successfully and uses the locale from cookies" do
+          page.driver.browser.manage.add_cookie(name: 'locale', value: 'es')
+
+          fill_in_and_submit_login_form(user)
+          expect_logged_in
+
+          expect(page).to have_content I18n.t(:home_shop, locale: :es).upcase
+          expect(user.reload.locale).to eq "es"
+
+          page.driver.browser.manage.delete_cookie('locale')
+        end
+      end
+    end
   end
 end

spec/services/user_locale_setter_spec.rb

@@ -0,0 +1,147 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe UserLocaleSetter do
+  let(:user) { create(:user) }
+  let(:default_locale) { I18n.default_locale }
+  let(:locale_params) { {} }
+  let(:cookies) { {} }
+  let(:service) { UserLocaleSetter.new(user, locale_params, cookies) }
+
+  describe "#set_locale" do
+    describe "persists selected locale from params" do
+      let(:locale_params) { "es" }
+
+      context "when the user is logged in" do
+        it "saves selected locale to user.locale and cookies[:locale]" do
+          service.set_locale
+
+          expect(user.reload.locale).to eq "es"
+          expect(cookies).to eq({ locale: "es" })
+        end
+      end
+
+      context "when the user is not logged in" do
+        it "saves selected locale to cookies[:locale]" do
+          service.set_locale
+
+          expect(cookies).to eq({ locale: "es" })
+        end
+      end
+    end
+
+    describe "sets the current locale" do
+      context "when the user is logged in" do
+        context "and has a valid locale saved" do
+          before { user.update(locale: "es") }
+
+          it "applies the user's locale" do
+            service.set_locale
+
+            expect(I18n.locale).to eq :es
+          end
+        end
+
+        context "and has an invalid locale saved" do
+          before { user.update(locale: "xx") }
+
+          it "applies the default locale" do
+            service.set_locale
+
+            expect(I18n.locale).to eq I18n.default_locale
+          end
+        end
+
+        context "and has no locale saved" do
+          before { user.update(locale: nil) }
+
+          it "applies the locale from cookies if present" do
+            cookies[:locale] = "es"
+            service.set_locale
+
+            expect(I18n.locale).to eq :es
+          end
+
+          it "applies the default locale otherwise " do
+            service.set_locale
+
+            expect(I18n.locale).to eq I18n.default_locale
+          end
+        end
+      end
+
+      context "when the user is not logged in" do
+        let(:user) { nil }
+
+        context "with a locale set in cookies" do
+          it "applies the value from cookies" do
+            cookies[:locale] = "es"
+            service.set_locale
+
+            expect(I18n.locale).to eq :es
+          end
+        end
+
+        context "with no locale set in cookies" do
+          it "applies the default locale" do
+            service.set_locale
+
+            expect(I18n.locale).to eq I18n.default_locale
+          end
+        end
+      end
+    end
+  end
+
+  describe "#ensure_valid_locale_persisted" do
+    context "when a user is present" do
+      context "and has an unavailable locale saved" do
+        before { user.update(locale: "xx") }
+
+        context "with no locale set in cookies" do
+          it "set the user's locale to the default" do
+            service.ensure_valid_locale_persisted
+
+            expect(user.reload.locale).to eq default_locale.to_s
+          end
+        end
+
+        context "with a locale set in cookies" do
+          let(:cookies) { {locale: "es"} }
+
+          it "set the user's locale to the cookie value" do
+            service.ensure_valid_locale_persisted
+
+            expect(user.reload.locale).to eq "es"
+          end
+        end
+      end
+    end
+  end
+
+  describe "#valid_current_locale" do
+    let(:service) { UserLocaleSetter.new(user) }
+
+    context "when the user has a locale set" do
+      it "returns the user's locale" do
+        user.update(locale: "es")
+        expect(service.valid_current_locale).to eq "es"
+      end
+    end
+
+    context "when the user has no locale set" do
+      it "returns the default locale" do
+        expect(service.valid_current_locale).to eq default_locale
+      end
+    end
+
+    context "when the given user argument is nil" do
+      let(:user) { nil }
+
+      it "returns the default locale" do
+        expect(service.valid_current_locale).to eq default_locale
+      end
+    end
+  end
+end

spec/support/request/authentication_workflow.rb

@@ -68,6 +68,11 @@ module AuthenticationWorkflow
     fill_in "password", with: user.password
     click_button "Login"
   end
+
+  def expect_logged_in
+    # Ensure page has been reloaded after submitting login form
+    expect(page).to_not have_selector ".menu #login-link"
+  end
 end
 
 RSpec.configure do |config|