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.

Sign up for GitHub

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

Jump to bottom

Support falling back to 2 letter language code locales and fix locale leaks across requests #1711

Merged
merged 3 commits into from
Aug 22, 2023
+24 −14
Merged

Support falling back to 2 letter language code locales and fix locale leaks across requests #1711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59e3abc
Support falling back to language codes
nickpith Aug 11, 2023
6b0b8cc
Update changelog
nickpith Aug 11, 2023
93a96a7
Switch to `around_action` and `I18n.with_locale`
nickpith Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ Unreleased
* Fix registration of event_bridge and pub_sub webhooks [#1635](https://github.com/Shopify/shopify_app/pull/1635)
* Adds support for adding any number of trial days within `EnsureBilling` by adding the `trial_days` field to `BillingConfiguration`
* Updated AppBridge to 3.7.8 [#1680](https://github.com/Shopify/shopify_app/pull/1680)
* Support falling back to 2 letter language code locales [#1711](https://github.com/Shopify/shopify_app/pull/1711)
* Fix locale leaks across requests [#1711](https://github.com/Shopify/shopify_app/pull/1711)

21.6.0 (July 11, 2023)
----------
Expand Down
19 changes: 11 additions & 8 deletions lib/shopify_app/controller_concerns/localization.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,23 @@ module Localization
extend ActiveSupport::Concern

included do
before_action :set_locale
around_action :set_locale
end

private

def set_locale
if params[:locale]
session[:locale] = params[:locale]
else
session[:locale] ||= I18n.default_locale
Copy link
Contributor

Choose a reason for hiding this comment

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

@nickpith

Noticed this being removed means the locale is not stored in the session.

Single page apps like (Hotwire-turbo) do not include the locale param on in-app page navigation. The locale is only included when the shopify admin page is reloaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's been too long since this change but the session[:locale] is still being set down on line 21?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - it is being set there.

Think the issue is more with single page apps like react / Hotwire-turbo that don't reload the Shopify iframe for embedded apps. Meaning the locale is not included after the initial page load. The session is overridden on the next page request - set to default.

Previous version was only setting when provided via params, and only setting the session[:locale] if it was not already set via ||=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I see that now. Thanks for pointing that out.

That definitely was not intended. If you want to fix that, go ahead and put up a PR and I'll gladly review. Otherwise, I'll get a fix out tomorrow if that is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI - I've fixed the issue and opening a PR shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix is up at #1878

def set_locale(&action)
locale = params[:locale] || I18n.default_locale

# Fallback to the 2 letter language code if the requested locale unavailable
unless I18n.available_locales.include?(locale.to_sym)
locale = locale.split("-").first
end
I18n.locale = session[:locale]

session[:locale] = locale
I18n.with_locale(session[:locale], &action)
rescue I18n::InvalidLocale
I18n.locale = I18n.default_locale
I18n.with_locale(I18n.default_locale, &action)
end
end
end
17 changes: 11 additions & 6 deletions test/shopify_app/controller_concerns/localization_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
class LocalizationController < ActionController::Base
include ShopifyApp::Localization

before_action :set_locale

def index
head(:ok)
render(plain: I18n.locale)
end
end

Expand All @@ -26,14 +24,21 @@ class LocalizationTest < ActionController::TestCase

with_test_routes do
get :index
assert_equal :ja, I18n.locale
assert_equal :ja, response.body.to_sym
end
end

test "set I18n.locale to passed locale param" do
with_test_routes do
get :index, params: { locale: "de" }
assert_equal :de, I18n.locale
assert_equal :de, response.body.to_sym
end
end

test "set I18n.locale to the 2 letter language code if fully-qualified locale param requested is not supported" do
with_test_routes do
get :index, params: { locale: "es-ES" }
assert_equal :es, response.body.to_sym
end
end

Expand All @@ -42,7 +47,7 @@ class LocalizationTest < ActionController::TestCase

with_test_routes do
get :index, params: { locale: "invalid_locale" }
assert_equal :en, I18n.locale
assert_equal :en, response.body.to_sym
end
end

Expand Down