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

Globalize migration guide: eager loading and language switching? #390

Closed
sedubois opened this issue Jun 9, 2020 · 6 comments
Closed

Globalize migration guide: eager loading and language switching? #390

sedubois opened this issue Jun 9, 2020 · 6 comments

Comments

@sedubois
Copy link
Contributor

sedubois commented Jun 9, 2020

I am migrating from Globalize following the migration guide, however it appears to be missing instructions on eager loading as well as checking for the existence of a translation.

This is how I understand I should rewrite eager loading to avoid N+1 queries:

-Post.with_translations(I18n.locale)
+Post.i18n.eager_load(:translations)

But how about replacing this, which I had been using in a language switcher?

Post.with_translations(I18n.locale).exists?(id: @post.id) ? do_this : do_that

NB (a bit off-topic): maybe to check for the presence of the translation, I just need to check if I18n.with_locale(:other_locale) { @post.title } is truthy? Is there any method to check for the presence of the translation in a general way that works for all models (without assuming the presence of a title)?

Context

Migrating from Globalize 5.3.0 to Mobility 0.0.13.

Expected Behavior

More detailed instructions in the wiki guide about:

  • eager loading
  • translation existence checks / implementing a language switcher

Actual Behavior

Missing instructions.

Possible Fix

Add info to Globalize migration guide.

@shioyama
Copy link
Owner

Howdy! 👋

Yeah documentation is sparse on this topic. Globalize has with_translations but this is very hard to implement in a way that would be supported by all backends.

I just looked up what Globalize does and here's what it looks like:

preload(:translations).joins(:translations).readonly(false).with_locales(locales).tap do |query|
  query.distinct! unless locales.flatten.one?
end

You can get the same effect with Mobility but there is no with_locales method. Here's what with_locales does:

def with_locales(*locales)
  # Avoid using "IN" with SQL queries when only using one locale.
  locales = locales.flatten.map(&:to_s)
  locales = locales.first if locales.one?
  where :locale => locales
end

So basically it's adding a where(locale: locales) to the query to narrow the scope of the joins.

Honestly, this is too much indirection for me and one of the reasons I think Globalize ends up with lots of issues. Mobility already has too much magic (trying to improve that) so definitely don't want to add anything.

Also, as mentioned, this is very specific to the strategy, so wouldn't generalize to other backends.

That said if there was a rough equivalent, it would make sense to document it in the wiki. I just made the wiki editable to anyone, would you like to add something about this to the migration guide?

We can discuss here first if you like, and once a rough mapping is working, add that to the wiki.

@sedubois
Copy link
Contributor Author

sedubois commented Jun 11, 2020

At this point I was not looking for a general solution for all backends but only making my Globalize => Mobility + :table migration as frictionless as possible.

After this will indeed come the question of how to refactor the backend from table to key_value.

I will check if anything can be added to the wiki once I'm done with my PR.

@shioyama
Copy link
Owner

See also #105

@sedubois
Copy link
Contributor Author

Although it is not recommended, you can make this query scope enabled by default using a default_scope, like this, in your model class:

default_scope { i18n }

@shioyama would you mind expanding on why this is not recommended?

@sedubois
Copy link
Contributor Author

sedubois commented Jun 11, 2020

So basically it's adding a where(locale: locales) to the query to narrow the scope of the joins.

Honestly, this is too much indirection for me and one of the reasons I think Globalize ends up with lots of issues. Mobility already has too much magic (trying to improve that) so definitely don't want to add anything.

@shioyama so for my specific model (which has validates :title, presence: true), the following works:

@posts = Post
-          .with_translations(I18n.locale)
+          .eager_load(:translations)
+          .where.not(title: nil)

Is this the "official"/"recommended" approach to load all posts available in a specific language?

When reading that code, I just feel that the intent is not as clear, and I feel inclined to add this to my model:

scope :with_translations, ->{ eager_load(:translations).where.not(title: nil) }

Then I can just call @posts = Post.with_translations.

@sedubois
Copy link
Contributor Author

sedubois commented Jun 13, 2020

So I just started using Mobility in production, here is what I did:

First to make thing easier and avoid having a huge PR (e.g with extra .i18n. everywhere) I created this concern:

# app/models/concerns/translatable.rb
module Translatable
  extend ActiveSupport::Concern

  included do
    extend Mobility

    default_scope { i18n }

    scope :with_locales, -> do
      where.not(self.translation_detection_column => nil)
    end

    scope :with_translations, -> do
      eager_load(:translations).with_locales
    end

    def self.translation_detection_column
      in?([Testimonial, Thought]) ? :quote : :title
    end
  end
end

(NB: the code impact could be made even smaller by allowing with_translations and with_locales to receive a locale parameter, to match the Globalize signatures. In my case there were not so many so I did not mind.)

Second, I made these various changes:

  • replace gem globalize with mobility, add config/initializers/mobility.rb as instructed in wiki
  • add include Translatable in the models that need it
  • replace globalized_model with translated_model
  • replace with_translations(I18n.locale) with with_translations
  • replace with_locales(I18n.locale) with with_locales

Third, I had various models like this:

class Post < ApplicationRecord
  ...
  translates :title, :slug
  has_paper_trail

  class Translation
    validates :title, presence: true
    has_paper_trail
    
    def slug=(slug)
      self[:slug] = slug || title&.parameterize
    end
  end
end

So for this example I did the following changes:

  • Mobility is incompatible with Papertrail, so remove has_paper_trail
  • move validates :title, presence: true into the parent model
  • move def slug=(slug) in the parent model but replace implementation with super(slug || title&.parameterize)
  • delete the nested class Translation

Here is the result:

class Post < ApplicationRecord
  include Translatable
  ...
  translates :title
  validates :title, presence: true
    
  def slug=(slug)
    super(slug || title&.parameterize)
  end
end

More generally I tried to remove the various references to <model>::Translation and only work with the parent model instead, as <model>::Translation is specific to the :table backend and I wanted to prepare for migrating to the :key-value backend.

Now it all works! 🎉 (except for Papertrail)

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

No branches or pull requests

2 participants