From cec90403c0c0927d95855d28b478f25527d8dda3 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 12 Mar 2018 17:30:00 -0700 Subject: [PATCH 1/5] Don't set user_return_to in LocaleController This no longer has any effect (we now use spree_user_return_to) and we should not be using it. --- frontend/app/controllers/spree/locale_controller.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/frontend/app/controllers/spree/locale_controller.rb b/frontend/app/controllers/spree/locale_controller.rb index aa7227f2427..c1d76763f76 100644 --- a/frontend/app/controllers/spree/locale_controller.rb +++ b/frontend/app/controllers/spree/locale_controller.rb @@ -3,9 +3,6 @@ module Spree class LocaleController < Spree::StoreController def set - if request.referer && request.referer.starts_with?('http://' + request.host) - session['user_return_to'] = request.referer - end if params[:locale] && I18n.available_locales.map(&:to_s).include?(params[:locale]) session[:locale] = I18n.locale = params[:locale] flash.notice = t('spree.locale_changed') From 0db484dd75e0dc3d455b965f298e2778ff216dcf Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 13 Mar 2018 10:47:28 -0700 Subject: [PATCH 2/5] Accept switch_to_locale param in LocaleController Previously we accepted only the :locale param in LocaleController#set. solidus_i18n, however, only accepts the :switch_to_locale param. This is because solidus_i18n uses the :locale param as part of it's path (ex. /en/products). For better compatiblity with solidus_i18n, and so that we can merge most of it's functionality, we should accept either param here (and probably should prefer :switch_to_locale). --- frontend/app/controllers/spree/locale_controller.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/frontend/app/controllers/spree/locale_controller.rb b/frontend/app/controllers/spree/locale_controller.rb index c1d76763f76..b3f877ce5d2 100644 --- a/frontend/app/controllers/spree/locale_controller.rb +++ b/frontend/app/controllers/spree/locale_controller.rb @@ -3,8 +3,12 @@ module Spree class LocaleController < Spree::StoreController def set - if params[:locale] && I18n.available_locales.map(&:to_s).include?(params[:locale]) - session[:locale] = I18n.locale = params[:locale] + available_locales = Spree.i18n_available_locales + requested_locale = params[:switch_to_locale] || params[:locale] + + if requested_locale && available_locales.map(&:to_s).include?(requested_locale) + session[:locale] = requested_locale + I18n.locale = requested_locale flash.notice = t('spree.locale_changed') else flash[:error] = t('spree.locale_not_changed') From 820c3b669e199b4255d7045ccfc9d0ba1176dc25 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 13 Mar 2018 14:00:16 -0700 Subject: [PATCH 3/5] Add specs against LocaleController --- .../controllers/locale_controller_spec.rb | 57 +++++++++++++++++++ frontend/spec/features/locale_spec.rb | 12 +--- .../spec/support/shared_contexts/locales.rb | 15 +++++ 3 files changed, 73 insertions(+), 11 deletions(-) create mode 100644 frontend/spec/controllers/locale_controller_spec.rb create mode 100644 frontend/spec/support/shared_contexts/locales.rb diff --git a/frontend/spec/controllers/locale_controller_spec.rb b/frontend/spec/controllers/locale_controller_spec.rb new file mode 100644 index 00000000000..d9741be492e --- /dev/null +++ b/frontend/spec/controllers/locale_controller_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Spree::LocaleController, type: :controller do + include_context "fr locale" + + context 'switch_to_locale specified' do + context "available locale" do + it 'sets locale and redirects' do + get :set, params: { switch_to_locale: 'fr' } + expect(I18n.locale).to eq :fr + expect(response).to redirect_to('/') + expect(session[:locale]).to eq('fr') + expect(flash[:notice]).to eq(I18n.t("spree.locale_changed")) + end + end + + context "unavailable locale" do + it 'does not change locale and redirects' do + get :set, params: { switch_to_locale: 'klingon' } + expect(I18n.locale).to eq :en + expect(response).to redirect_to('/') + expect(flash[:error]).to eq(I18n.t("spree.locale_not_changed")) + end + end + end + + context 'locale specified' do + context "available locale" do + it 'sets locale and redirects' do + get :set, params: { locale: 'fr' } + expect(I18n.locale).to eq :fr + expect(response).to redirect_to('/') + expect(flash[:notice]).to eq(I18n.t("spree.locale_changed")) + end + end + + context "unavailable locale" do + it 'does not change locale and redirects' do + get :set, params: { locale: 'klingon' } + expect(I18n.locale).to eq :en + expect(response).to redirect_to('/') + expect(flash[:error]).to eq(I18n.t("spree.locale_not_changed")) + end + end + end + + context 'both locale and switch_to_locale specified' do + it 'uses switch_to_locale value' do + get :set, params: { locale: 'en', switch_to_locale: 'fr' } + expect(I18n.locale).to eq :fr + expect(response).to redirect_to('/') + expect(flash[:notice]).to eq(I18n.t("spree.locale_changed")) + end + end +end diff --git a/frontend/spec/features/locale_spec.rb b/frontend/spec/features/locale_spec.rb index c1b9b9accfe..d5fc70b522a 100644 --- a/frontend/spec/features/locale_spec.rb +++ b/frontend/spec/features/locale_spec.rb @@ -14,17 +14,7 @@ def with_locale(locale) end context 'shopping cart link and page' do - before do - I18n.backend.store_translations(:fr, spree: { - i18n: { this_file_language: "Français" }, - cart: 'Panier', - shopping_cart: 'Panier' - }) - end - - after do - I18n.reload! - end + include_context "fr locale" it 'should be in french' do with_locale('fr') do diff --git a/frontend/spec/support/shared_contexts/locales.rb b/frontend/spec/support/shared_contexts/locales.rb new file mode 100644 index 00000000000..925850c9cb4 --- /dev/null +++ b/frontend/spec/support/shared_contexts/locales.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +shared_context 'fr locale' do + before do + I18n.backend.store_translations(:fr, spree: { + i18n: { this_file_language: "Français" }, + cart: 'Panier', + shopping_cart: 'Panier' + }) + end + + after do + I18n.reload! + end +end From 559538c5b9012532ea41f35c8e98ac07efaea22e Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 13 Mar 2018 14:02:21 -0700 Subject: [PATCH 4/5] Don't attempt to redirect back in LocaleController Previously, we attempted to redirect back to the previous URL, and failing that, the store's root URL. Redirecting back sounds like a nice feature, but if we are using the :locale filter from the routing-filter gem (as solidus_i18n does), this isn't what we want, because the previous URL won't be prefixed by the current locale. The existing behaviour of solidus_i18n is already to always redirect to the root, so we should adopt the same behaviour in solidus itself so that that override can be removed from solidus_i18n. --- frontend/app/controllers/spree/locale_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/app/controllers/spree/locale_controller.rb b/frontend/app/controllers/spree/locale_controller.rb index b3f877ce5d2..861bc293806 100644 --- a/frontend/app/controllers/spree/locale_controller.rb +++ b/frontend/app/controllers/spree/locale_controller.rb @@ -13,7 +13,8 @@ def set else flash[:error] = t('spree.locale_not_changed') end - redirect_back_or_default(spree.root_path) + + redirect_to spree.root_path end end end From 7d5e3830722a60474c66c47a025622b3343081c1 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 13 Mar 2018 14:22:54 -0700 Subject: [PATCH 5/5] Copy set_locale route from solidus_i18n Our existing route to locale#set was a GET (which it shouldn't be), this adds it again as a POST. The existing get route is left for compatibility. The route is named "select_locale" instead of "set_locale" to avoid a conflict with the existing solidus_i18n route. --- frontend/config/routes.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/config/routes.rb b/frontend/config/routes.rb index e3e76f0dd6f..9ab0a2b7447 100644 --- a/frontend/config/routes.rb +++ b/frontend/config/routes.rb @@ -6,6 +6,7 @@ resources :products, only: [:index, :show] get '/locale/set', to: 'locale#set' + post '/locale/set', to: 'locale#set', as: :select_locale # non-restful checkout stuff patch '/checkout/update/:state', to: 'checkout#update', as: :update_checkout