Set fallback locale for Mobility based on current store #11906

rafalcymerys · 2023-07-12T14:21:51Z

viezly · 2023-07-12T14:22:21Z

Changes preview:

Legend:

kshalot · 2023-07-13T14:17:19Z

core/app/services/spree/locales/set_fallback_locale_for_store.rb

+        fallbacks = store.supported_locales_list.each_with_object({}) do |locale, object|
+          object[locale] = [store_default_locale]
+        end


You can also consider doing:

Suggested change

fallbacks = store.supported_locales_list.each_with_object({}) do |locale, object|

object[locale] = [store_default_locale]

end

fallbacks = store.supported_locales_list.to_h { |locale| [locale, [store_default_locale]] }

Hm, isn't it more explicit and clear when we do each_with_object?

I don't have a strong preference for either to be honest. each_with_object probably looks more familiar to most readers. In defense of #to_h - to me it's clear in the way it just deals with simple [key, value] objects instead of having this "state" object that's filled in a loop. I like the original as well though, hence my original comment mentioning the suggestion as something to consider.

kshalot · 2023-07-13T14:18:46Z

core/app/services/spree/locales/set_fallback_locale_for_store.rb

+        fallbacks_class = I18n::Locale::Fallbacks
+        fallbacks_instance = fallbacks_class.new(fallbacks)


Any reason why it's not just:

Suggested change

fallbacks_class = I18n::Locale::Fallbacks

fallbacks_instance = fallbacks_class.new(fallbacks)

fallbacks_instance = I18n::Locale::Fallbacks.new(fallbacks)

Or even doing the entire thing in one line:

Mobility.store_based_fallbacks.value = I18n::Locale::Fallbacks.new(fallbacks)

That were some leftovers from the original implementation in mobility. Good point :)

kshalot · 2023-07-13T14:21:02Z

core/lib/mobility/plugins/store_based_fallbacks.rb

+        def read(locale, fallback: true, **kwargs)
+          return super(locale, **kwargs) if !fallback || kwargs[:locale]
+
+          locales = fallback == true ? Mobility.store_based_fallbacks.value[locale] : [locale, *fallback]


There's already a !fallback check above so you know for sure that it's true here. locales will always be equal to:

Mobility.store_based_fallbacks.value[locale]

That's a leftover from the stock fallbacks implementation, good point :)

kshalot · 2023-07-13T14:26:22Z

core/lib/mobility/plugins/store_based_fallbacks.rb

+            return value if Util.present?(value)
+          end
+
+          super(locale, **kwargs)


I don't get this part. The method gets a locale positional argument, but also a locale keyword argument. Shouldn't the logic be something in the lines of:

Check whether a translation exists for locale

If it exists - return it.

If it doesn't exist then try to use fallback.

And here I see that the fallback is calculated first, and only then the original locale in the super(locale, **kwargs) line. The first line that short-circuits if the kwarg is set is also confusing. What's the difference between the keyword argument and the positional argument? I guess it's some Mobility internal so in our codebase it looks a bit muddy.

@kshalot that's what this implementation does.

The logic is as follows:

If a specific locale is requested or fallback is disabled - skip the fallbacks mechanism and read the value without the fallbacks in specified locale

Iterate through the fallbacks for requested locale. This list always starts with the locale itself, so e.g. fallbacks['en'] = ['en', (other locales that are configured)]. If a value is found for any of them, return it

If this didn't return any result, read the value without the fallbacks for specified locale

The signature of this function is not the most clear one, but I'd prefer to keep it, not to break other mobility plugins that need to work alongside it

Okay. The signature is totally confusing, but I see that the original fallbacks work in a similar way. By glancing at the code, I don't see where the locale kwarg is used. Are we sure it's not a bug?

To illustrate my point using the first step of your answer - "If a specific locale is requested or fallback is disabled...". Why is the bold part not checked with locale? What is the difference between that and kwargs[:locale]? If kwargs[:locale] is the specific locale that was requested then what is locale? I guess if it did not work then someone would have caught it by now, but it's very muddy. Do you have some examples of what the values of those variables are for some call?

I agree that mirroring the function signature of the original fallback plugin is a smart move, but I just want to make sure we don't follow it blindly given the fact that the original code contained some dubious statements.

I did some further investigation on that, it seems that Mobility's internals call it like read(locale, locale: true). Example here: https://github.com/shioyama/mobility/blob/master/lib/mobility/plugins/active_model/dirty.rb
So the kwargs[:locale] can be true/false and it has a different semantic.

This part of the implementation is a slightly modified implementation of the default fallbacks plugin, so I kept the behavior in this case the same.

kshalot · 2023-07-13T14:32:54Z

core/app/services/spree/locales/set_fallback_locale_for_store.rb

+        fallbacks_class = I18n::Locale::Fallbacks
+        fallbacks_instance = fallbacks_class.new(fallbacks)
+
+        Mobility.store_based_fallbacks.value = fallbacks_instance


Maybe we could abstract this into Mobility in some way? I mean not explicitly set the instance variable on the Mobility singleton class. My problem with this now is that it leaks the way we store the fallback in a way. Doing a

Mobility.set_store_fallback(fallbacks_instance)

would intuitively make more sense to me, since no implementation details are exposed.

Hm, how about I write custom store_fallback and store_fallback= methods that hide this behavior?

Yeah, a getter and setter would do the job.

Please take a look again

kshalot · 2023-07-21T13:38:58Z

core/lib/mobility/plugins/store_based_fallbacks.rb

+      # Applies fallbacks plugin to attributes. Completely disables fallbacks
+      # on model if option is +false+.
+      included_hook do |_, backend_class|
+        unless options[:fallbacks] == false


Maybe this is clearer? if x instead if not not x

Suggested change

unless options[:fallbacks] == false

if options[:fallbacks]

It would be, but I'm not sure about the interface here :) After this change it will behave differently for a falsey value (e.g. nil), and I think Mobility's default fallbacks plugin would work if that option is not set. Only setting it explicitly to false disabled it.

kshalot · 2023-07-21T13:56:13Z

core/lib/mobility/plugins/store_based_fallbacks.rb

+            return value if Util.present?(value)
+          end
+
+          super(locale, **kwargs)


Okay. The signature is totally confusing, but I see that the original fallbacks work in a similar way. By glancing at the code, I don't see where the locale kwarg is used. Are we sure it's not a bug?

To illustrate my point using the first step of your answer - "If a specific locale is requested or fallback is disabled...". Why is the bold part not checked with locale? What is the difference between that and kwargs[:locale]? If kwargs[:locale] is the specific locale that was requested then what is locale? I guess if it did not work then someone would have caught it by now, but it's very muddy. Do you have some examples of what the values of those variables are for some call?

I agree that mirroring the function signature of the original fallback plugin is a smart move, but I just want to make sure we don't follow it blindly given the fact that the original code contained some dubious statements.

kshalot · 2023-07-21T14:07:00Z

api/spec/requests/spree/api/v2/storefront/taxons_spec.rb

@@ -267,6 +267,7 @@
      let(:translated_slug) { 'test_slug_es' }

      before do
+        Spree::Locales::SetFallbackLocaleForStore.new.call(store: store2)


Now that I'm looking at the tests - it would probably be wise to cover this fallback functionality with tests in general. I guess what we are missing is:

Test whether the locale controller helper sets the fallback when current_store is present.

Test whether the fallback plugin itself works as intended.

This is covered e.g. by the requests tests for the API. I forgot to remove setting it manually here, it works without it. I moved away from generating the slugs with fallbacks manually, and it now fully relies on mobility, so the old assertions check this behavior end-to-end now.

I added two specs for that

kshalot · 2023-07-21T14:15:22Z

core/app/services/spree/locales/set_fallback_locale_for_store.rb

+      def call(store:)
+        store_default_locale = store.default_locale
+        fallbacks = store.supported_locales_list.each_with_object({}) do |locale, object|
+          object[locale] = [store_default_locale]


What if there's only one locale? Will it still try to fallback using itself? This would obviously be redundant.

It won't change anything then. I18n::Locale::Fallbacks consolidates duplicate entries in the list of fallbacks, and the first "fallback" on the list for each locale is the actual locale. Only the later entries denote actual fallbacks in case the requested one is not present.

kshalot

Just three remarks (one actual question in the spec).

Apart from that I'd say it looks good 👍 ✅

kshalot · 2023-07-24T11:56:56Z

core/spec/services/spree/locales/set_fallback_locale_for_store_spec.rb

+      let(:name_en) { 'name en' }
+      let(:name_pl) { 'name pl' }
+
+      let!(:translation_en) { product.translations.find_or_create_by(locale: 'en') { |e| e.name = name_en } }


Is this translation (and the en locale in general) relevant to any of the tests?

Or is it there to demonstrate that the fallback will be the default_locale and not some random locale?

We want to check whether it falls back to a specific locale (in this case pl), without taking the en value (which is by default set as I18n.default_locale and Mobility uses it as a default for fallbacks)

kshalot · 2023-07-24T12:01:48Z

core/app/models/spree/taxon.rb

@@ -66,10 +67,12 @@ class Taxon < Spree::Base

      def set_permalink
        parent = translated_model.parent
+        name_with_fallback = name || translated_model.name


Just a question - this refers to the name in the translation and when it's not set it falls back to the model's name which kicks in the whole fallback mechanism, right?

kshalot · 2023-07-24T12:19:09Z

core/app/models/concerns/spree/translatable_resource_slug.rb

        supported_locales = store.supported_locales_list

        supported_locales.each_with_object({}) do |locale, hash|
-          hash[locale] = localized_slugs[locale] || localized_slugs[default_locale]
+          hash[locale] = I18n.with_locale(locale) { slug }


Technically, we could loosen the contract between this module and the including module by introducing a dependency on FriendlyId. I think you can access the slug_column config value by doing model.friendly_id_config.slug_column. Not great either, but it popped in my mind after seeing that you had to add a slug alias to the Taxon model just so this works. Something to consider in the future maybe

We wanted to unify the naming of the permalink -> slug column anyway, as it's a bit confusing right now. Technically all of them should be called slug, as permalink refers to the whole URL, including domain name.

kshalot suggested changes Jul 13, 2023

View reviewed changes

rafalcymerys requested a review from kshalot July 21, 2023 13:15

rafalcymerys force-pushed the fix/set_fallback_locale_based_on_store branch from aca59bc to e73465f Compare July 21, 2023 13:20

rafalcymerys marked this pull request as ready for review July 21, 2023 13:20

rafalcymerys force-pushed the fix/set_fallback_locale_based_on_store branch from e73465f to e48b2e6 Compare July 21, 2023 13:46

kshalot suggested changes Jul 21, 2023

View reviewed changes

rafalcymerys force-pushed the fix/set_fallback_locale_based_on_store branch from e48b2e6 to 383ed45 Compare July 21, 2023 14:48

Set fallback locale for Mobility based on current store

8a25054

rafalcymerys force-pushed the fix/set_fallback_locale_based_on_store branch from 383ed45 to 8a25054 Compare July 21, 2023 14:49

kshalot approved these changes Jul 24, 2023

View reviewed changes

rafalcymerys merged commit 618ff48 into main Jul 24, 2023
13 checks passed

delete-merged-branch bot deleted the fix/set_fallback_locale_based_on_store branch July 24, 2023 13:10

rafalcymerys mentioned this pull request Jul 24, 2023

Set fallback locale for Mobility based on current store for [Spree 4.6] #11908

Merged

rafalcymerys mentioned this pull request Aug 11, 2023

set permalink when creating taxonomy. Fixes #11899 #11903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set fallback locale for Mobility based on current store #11906

Set fallback locale for Mobility based on current store #11906

rafalcymerys commented Jul 12, 2023

viezly bot commented Jul 12, 2023 •

edited

Loading

kshalot Jul 13, 2023

rafalcymerys Jul 17, 2023

kshalot Jul 17, 2023 •

edited

Loading

kshalot Jul 13, 2023

rafalcymerys Jul 17, 2023

kshalot Jul 13, 2023

rafalcymerys Jul 17, 2023

kshalot Jul 13, 2023

rafalcymerys Jul 21, 2023

kshalot Jul 21, 2023

rafalcymerys Jul 24, 2023

kshalot Jul 13, 2023

rafalcymerys Jul 17, 2023

kshalot Jul 17, 2023

rafalcymerys Jul 21, 2023

kshalot Jul 21, 2023

rafalcymerys Jul 22, 2023

kshalot Jul 21, 2023

kshalot Jul 21, 2023

rafalcymerys Jul 21, 2023

kshalot Jul 21, 2023

rafalcymerys Jul 21, 2023

kshalot left a comment

kshalot Jul 24, 2023

kshalot Jul 24, 2023

rafalcymerys Jul 24, 2023

kshalot Jul 24, 2023

rafalcymerys Jul 24, 2023

kshalot Jul 24, 2023

rafalcymerys Jul 24, 2023

		fallbacks_class = I18n::Locale::Fallbacks
		fallbacks_instance = fallbacks_class.new(fallbacks)

	fallbacks_class = I18n::Locale::Fallbacks
	fallbacks_instance = fallbacks_class.new(fallbacks)
	fallbacks_instance = I18n::Locale::Fallbacks.new(fallbacks)

Set fallback locale for Mobility based on current store #11906

Set fallback locale for Mobility based on current store #11906

Conversation

rafalcymerys commented Jul 12, 2023

viezly bot commented Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshalot Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshalot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viezly bot commented Jul 12, 2023 •

edited

Loading

kshalot Jul 17, 2023 •

edited

Loading