Switch from Globalize to Mobility #3360

bricesanchez · 2018-03-24T04:57:43Z

In order to translate our content we have been using a library called
Globalize. This has worked really well for us up until now, however a
new library has been released called Mobility which seems like it would
work better for our case.

This commit switches us from Globalize to Mobility in conjunction with
the release of refinerycms-i18n version 5.0.0.

This PR is stable, it needs code review but we have a green build:

Big thanks to @shioyama !

I will merge this PR after :

code review
the release of mobility 0.6.0: https://github.com/shioyama/mobility/releases
decide if mobility should be a dependency of refinerycms-i18n
release a new version of refinerycms-i18n, we will probably bump to 5.0

shioyama · 2018-03-29T12:22:10Z

pages/app/models/refinery/page.rb

@@ -36,7 +26,7 @@ def self.options
          friendly_id_options[:use] << :scoped
          friendly_id_options.merge!(scope: :parent)
        end
-        friendly_id_options[:use] << :globalize
+        friendly_id_options[:use] << :mobility


Can you try moving this line up so it comes before :scoped in the array to friendly_id_options[:use] ?

I've tried but it sadly does not change anything 😢

shioyama · 2018-04-05T12:47:04Z

pages/app/models/refinery/page.rb

@@ -61,7 +49,7 @@ def should_generate_new_friendly_id?

    has_many :parts, -> {
      scope = order('position ASC')
-      scope = scope.includes(:translations) if ::Refinery::PagePart.respond_to?(:translation_class)
+      scope = scope.includes(:translations) if ::Refinery::PagePart.respond_to?(:mobility)


I think rather than use a default_scope on everything, you could add it here:

scope = ::Refinery::PagePart.respond_to?(:mobility) ? i18n.includes(:translations) : all scope = scope.order('position ASC') scope

shioyama · 2018-04-05T12:47:55Z

pages/app/models/refinery/page.rb

-    attribute :slug
-
-    after_save { translations.collect(&:save) }
+    default_scope { i18n }


This might fix some issues, but ideally it would be better not to use it if possible.

shioyama · 2018-04-05T12:49:07Z

pages/lib/refinery/pages/finder.rb


        # A join implies readonly which we don't really want.
-        Page.where(globalized_conditions).
+        Page.where(mobility_conditions).


Page.i18n.where(mobility_conditions)

shioyama · 2018-04-05T13:57:40Z

pages/app/models/refinery/page.rb

@@ -376,9 +364,9 @@ def puts_destroy_help
    end

    def slug_locale
-      return Globalize.locale if translation_for(Globalize.locale, false).try(:slug).present?
+      return Mobility.locale if slug_backend.read(Mobility.locale).present?


I'm not quite sure about the context, but slug_backend.read(Mobility.locale) is basically the same as slug here. This will build the translation for that locale if none exists yet.

With Globalize you have translation_for(Globalize.locale, false) which will avoid building the translation for the current locale if none yet exists. This is a bit different.

Mobility does expose a method translation_for on the backend, but it has no option to override building an empty translation. You shouldn't really need that option though because Mobility anyway removes any empty translations before saving, so I think here you should be able to safely replace slug_backend.read(Mobility.locale) with just slug, and slug_backend.read(Refinery::I18n.default_frontend_locale) with just slug(locale: Refinery::I18n.default_frontend_locale).

You don't really need present? either since the Presence plugin converts any blank strings to nil for you already.

shioyama · 2018-04-05T13:58:33Z

I haven't tried running this PR locally, but just looking at the code I noticed a few places where perhaps the i18n scope was missing. Maybe that will get some more tests passing... not really sure.

bricesanchez · 2018-04-05T21:11:59Z

@shioyama Thank you for your review! I will take a look on this today :)

shioyama · 2018-04-05T23:39:08Z

pages/lib/refinery/pages/finder.rb

@@ -51,7 +51,7 @@ def translations_conditions(original_conditions)
        translations_conditions = {}
        original_conditions.keys.each do |key|
          if translated_attributes.include? key.to_s
-            translations_conditions["#{Page.translation_class.table_name}.#{key}"] = original_conditions.delete(key)
+            translations_conditions["#{Page::Translation.table_name}.#{key}"] = original_conditions.delete(key)


Do you even need to do this? Both Globalize and Mobility (with i18n scope) should seemlessly handle translated attributes as if they were normal attributes. What Refinery is doing here is basically what Mobility is doing here, which it automatically does when you call Post.i18n.where(title: "foo") etc.

;tldr; I don't think you should really need the translations_conditions method with either Globalize or Mobility.

You're probably right, it's the first time i dig into the i18n logic of Refinery, there is probably legacy things we don't ever need now.

@bricesanchez do you want to try removing it? I believe it was just a performance optimisation to get everything loaded in the single query.

@parndt i had a lot of failling specs (108 fails) when i tried to removed this method:

108) Crudify xhr_paging performs ajax paging of index Failure/Error: stmt = SQLite3::Statement.new( self, sql ) ActiveRecord::StatementInvalid: SQLite3::SQLException: no such column: refinery_pages.locale: SELECT "refinery_pages".* FROM "refinery_pages" INNER JOIN "refinery_page_translations" "translations_refinery_pages" ON "translations_refinery_pages"."refinery_page_id" = "refinery_pages"."id" INNER JOIN "refinery_page_translations" ON "refinery_page_translations"."refinery_page_id" = "refinery_pages"."id" AND "refinery_page_translations"."locale" = 'en' WHERE "refinery_pages"."locale" = 'en' AND "refinery_pages"."parent_id" IS NULL AND "refinery_page_translations"."slug" = ? ORDER BY "refinery_pages"."id" ASC LIMIT ? # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/sqlite3-1.3.13/lib/sqlite3/database.rb:91:in `initialize' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/sqlite3-1.3.13/lib/sqlite3/database.rb:91:in `new' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/sqlite3-1.3.13/lib/sqlite3/database.rb:91:in `prepare' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/2.5.0/monitor.rb:226:in `mon_synchronize' # ./pages/lib/refinery/pages/finder.rb:122:in `parent_page' # ./pages/lib/refinery/pages/finder.rb:106:in `find' # ./pages/lib/refinery/pages/finder.rb:83:in `find' # ./pages/lib/refinery/pages/finder.rb:149:in `find' # ./pages/lib/refinery/pages/finder.rb:9:in `by_path_or_id' # ./pages/app/models/refinery/page.rb:80:in `find_by_path_or_id' # ./pages/app/controllers/refinery/pages_controller.rb:110:in `call' # ./pages/app/controllers/refinery/pages_controller.rb:75:in `find_page' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/request_store-1.4.1/lib/request_store/middleware.rb:19:in `call' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/2.5.0/webrick/httpserver.rb:140:in `service' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/2.5.0/webrick/httpserver.rb:96:in `run' # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/2.5.0/webrick/server.rb:307:in `block in start_thread' # ------------------ # --- Caused by: --- # SQLite3::SQLException: # no such column: refinery_pages.locale # /Users/bricesanchez/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/sqlite3-1.3.13/lib/sqlite3/database.rb:91:in `initialize'

I had seen that i need this method so i kept it.

Note that you don't need joins(:translations) here, Mobility will do that for you in the i18n scope.

Actually wait, that doesn't quite work...

Ok, long story short, this requires shioyama/mobility#51 to really work. The problem is that Mobility joins ON the current locale, and this clobbers this refinery query so you can't query on more than one locale. So for now, I'd say just leave this, and once shioyama/mobility#51 is done it should be refactorable.

btw, good test coverage! Only 5 tests ended up failing, but they highlighted the problem here.

I'm glad! 🥇

shioyama · 2018-04-05T23:43:12Z

images/db/migrate/20150430171341_translate_refinery_images.rb

    end
+
+    add_index :refinery_image_translations, :refinery_image_id, name: :index_refinery_image_translations_on_refinery_image_id


This index has been removed in the generators in the latest version of Mobility (on master), see shioyama/mobility#198. I think you don't need this one (since it's covered by the last index below on the combo of refinery_image_id and locale).

See: * #3360 (review) * shioyama/mobility#198

See: #3360 (review)

See: #3360 (comment)

bricesanchez · 2018-04-06T12:37:54Z

Thanks @shioyama for your help! Now there is just 11 fail specs.

shioyama · 2018-04-06T13:14:53Z

Removing translations_conditions didn't work? That's strange...

bricesanchez · 2018-04-06T14:16:02Z

Removing translations_conditions didn't work? That's strange...

I've revert it for now because it causes more fail specs.
Most of my problems was caused by should_generate_new_friendly_id?

I have to clean up my PR too.

bricesanchez · 2018-04-08T04:38:18Z

Hi @parndt and @shioyama ! The last 2 failing specs of this PR are caused by seo_meta and mobility, do you have any idea to fix them?

shioyama · 2018-04-08T06:39:37Z

pages/spec/features/refinery/pages_spec.rb

@@ -425,12 +425,12 @@ def standard_page_menu_items_exist?
        let(:nested_page_slug) { 'nested_page' }

        let!(:nested_page) do
-          Globalize.fallbacks = [:ru]
-          _page = Globalize.with_locale(:en) {
+          Mobility.new_fallbacks[:ru]


Do you need this? I'm planning to deprecate new_fallbacks. I don't think this does anything here.

If you want to just set the global fallbacks for this one spec, couldn't you set I18n.fallbacks instead?

shioyama · 2018-04-08T06:47:24Z

pages/app/models/refinery/page.rb

@@ -164,20 +150,20 @@ def nullify_duplicate_slugs_under_the_same_parent!
    end

    def translated_to_default_locale?
-      persisted? && translations.any?{ |t| t.locale == Refinery::I18n.default_frontend_locale}
+      persisted? && translations.any?{ |t| t.locale.to_sym == Refinery::I18n.default_frontend_locale}


Nice catch! Mobility doesn't convert translation locales to symbols like Globalize does.

Added a note about this to the Mobility wiki page on migrating from Globalize.

shioyama · 2018-04-10T00:43:58Z

images/refinerycms-images.gemspec

@@ -20,7 +20,7 @@ Gem::Specification.new do |s|
  s.test_files        = `git ls-files -- spec/*`.split("\n")

  s.add_dependency 'refinerycms-dragonfly',   '~> 1.0'
-  s.add_dependency 'globalize',               ['>= 5.1.0.beta1', '< 5.2']
+  s.add_dependency 'mobility',               '~> 0.5'


Isn't it a bit strange to have the mobility dependency in refinerycms-images ? I had trouble tracking down where it was. I would expect it to be in refinerycms-i18n.

You're right, it's a bit strange, i think it's because refinerycms-i18n was not a dependency of refinerycms-core a long time ago.

We could add a dependency of mobility gem in refinerycms-i18n.

What do you think @parndt?

Unless I'm mistaken it's because refinerycms-images doesn't depend on refinerycms-i18n so in order for images to be translated it has to depend on the translation library?

Unless I'm mistaken it's because refinerycms-images doesn't depend on refinerycms-i18n

You're right

so in order for images to be translated it has to depend on the translation library?

You're right but refinerycms-core depends to refinerycms-i18n:

refinerycms/core/refinerycms-core.gemspec

Line 25 in 90a44fc

s.add_dependency 'refinerycms-i18n', ['~> 4.0', '>= 4.0.0']

And refinerycms-images depends to refinerycms-core.

So what should we do @parndt?

ah well in that case this library shouldn't duplicate dependencies 😄

I'd go with @shioyama's suggestion

ah well in that case this library shouldn't duplicate dependencies

It does not duplicate dependencies because refinerycms-i18n has no dependencies to mobility nor globalize: https://github.com/refinery/refinerycms-i18n/blob/master/refinerycms-i18n.gemspec#L19-L20

even if we call Mobility in this gem: https://github.com/refinery/refinerycms-i18n/pull/76/files

So i will create a dependency to mobility in refinerycms-i18n like it should be.

shioyama · 2018-04-10T00:53:15Z

@bricesanchez The problem with seo_meta has to do with how Mobility handles dirty attributes, which is slightly different from Globalize.

Refinery has seo_meta on the model's translations, but when the seo_meta changes, Mobility picks it up as a change on the model, not the translation. So it doesn't save the translation, and loses the association to the seo meta, which is why it comes out nil.

I'm trying to figure out if/how Mobility should handle this.

shioyama · 2018-04-10T02:14:23Z

So after a lot of digging through Mobility, Globalize, SeoMeta, and Rails, I understand the issue here and have a quick fix, although not terribly elegant.

Just add an after_save callback to the Refinery::Pages::Core::BaseModel class like this:

after_save { translations.find { |t| t.locale == Mobility.locale.to_s }.seo_meta.save! }

This will force the model to save seo meta for the translation in the current locale. I find this fixes specs.

In shioyama/mobility#202, I'm adding an extension to the translations association which will make this a bit cleaner, so you will be able to do:

after_save { translations.in(Mobility.locale).seo_meta.save! }

if you don't mind waiting until I release 0.6.0 (should be sometime this week), this would be nicer.

The problem, in a nutshell, is that Mobility detects that there is a change in the translation and marks it as a change to a virtual attribute meta_description_en. For a normal column on the translations table, this works fine, but in this case SeoMeta is delegating meta_description setter/getter to its association, so this fails to trigger an update to the translation, and thus the SeoMeta also does not get updated.

By using after_save on the parent model, we just skip the translation entirely and force the seo meta to get updated.

I don't think I want to do anything special in Mobility to handle this case since it's actually quite tricky; I don't think most people will have delegated methods on their model translation class.

…urrent locale See #3360 (comment)

shioyama · 2018-04-10T03:59:48Z

I've merged shioyama/mobility#202, so you can point to master for now. I'll release a new version soon.

See: https://github.com/refinery/refinerycms/pull/3360/files#r179629155

This reverts commit fa12fe7.

and tidy up model for mobility and friendly usage

It will fix site_bar edit link with switch_locale param presence

See: shioyama/mobility#180 (comment)

This will fix: ActiveModel::UnknownAttributeError: unknown attribute 'locale' for Refinery::PagePart.

…urrent locale See #3360 (comment)

See: #3360

At this time, only refinerycms-pages needs this dependency.

This dependency will be added to refinerycms-i18n which is a core dependency

because 0.6.0 is released

The project switched from Globalize in refinery/refinerycms#3360

bricesanchez added the Feature Request label Mar 24, 2018

bricesanchez force-pushed the feature/mobility branch 4 times, most recently from 8e6c04f to 682232f Compare March 28, 2018 03:33

bricesanchez mentioned this pull request Mar 29, 2018

Scoped support? shioyama/friendly_id-mobility#7

Closed

shioyama reviewed Mar 29, 2018

View reviewed changes

shioyama reviewed Apr 5, 2018

View reviewed changes

bricesanchez added a commit that referenced this pull request Apr 6, 2018

Remove unnecessary mobility index

da69e7a

See: * #3360 (review) * shioyama/mobility#198

bricesanchez added a commit that referenced this pull request Apr 6, 2018

Remove default_scope i18n

aeea485

See: #3360 (review)

bricesanchez added a commit that referenced this pull request Apr 6, 2018

Improve Refiner::Page#slug_locale for mobility

0f831f6

See: #3360 (comment)

shioyama reviewed Apr 8, 2018

View reviewed changes

shioyama reviewed Apr 10, 2018

View reviewed changes

shioyama mentioned this pull request Apr 10, 2018

Add extension to find translations in a locale for Table backend shioyama/mobility#202

Merged

bricesanchez added a commit that referenced this pull request Apr 10, 2018

Add after_save callback to save seo meta for the translation in the c…

2260300

…urrent locale See #3360 (comment)

bricesanchez and others added 20 commits May 25, 2018 11:10

Remove unnecessary translations_conditions method

5eecdae

See: https://github.com/refinery/refinerycms/pull/3360/files#r179629155

Revert "Remove unnecessary translations_conditions method"

b7a644b

This reverts commit fa12fe7.

Fix should_generate_new_friendly_id? method

332774a

and tidy up model for mobility and friendly usage

Fix translated_to_default_locale?: compare symbols

550cdc4

It will fix site_bar edit link with switch_locale param presence

Remove hacks for Globalize 2

b1312d0

Fix Mobility fallbacks

5dc3072

See: shioyama/mobility#180 (comment)

Remove :locale from permitted_new_page_part_params

658c652

This will fix: ActiveModel::UnknownAttributeError: unknown attribute 'locale' for Refinery::PagePart.

Use ::I18n.fallbacks instead of Mobility.new_fallbacks

544af04

Clean friendly_id_options in Page model

4f1d9fc

Add after_save callback to save seo meta for the translation in the c…

3bb95a0

…urrent locale See #3360 (comment)

Temp add mobility gem from table_finder_extension branch

398dbd4

FIX localized create_table in engine generator

36a3b7b

Switch mobility to master branch

23c9cb2

Page Mobility: replace in method for in_locale method

54d4fbf

See: #3360

Remove awesome_nested_set dependency from core

bef22e5

At this time, only refinerycms-pages needs this dependency.

Remove mobility dependency from images and

cfb024f

This dependency will be added to refinerycms-i18n which is a core dependency

Clean images, pages and resources dependencies

2c40d60

Remove moibility from Gemfile

0d260cf

because 0.6.0 is released

Update refinerycms-i18n to 5.0.0

f1f7fbc

update refinerycms-i18n branch

f48af77

parndt force-pushed the feature/mobility branch from 8f4b3f7 to f48af77 Compare May 25, 2018 00:58

Remove refinerycms-i18n from Gemfile

e214351

parndt changed the title ~~WIP: Feature/mobility~~ Switch from Globalize to Mobility May 25, 2018

parndt merged commit d181fbb into master May 25, 2018

parndt deleted the feature/mobility branch May 25, 2018 01:50

parndt mentioned this pull request Oct 29, 2018

Planned release compatible with Rails 5.2? globalize/globalize#696

Closed

parndt mentioned this pull request Sep 23, 2019

Switch test files to use Mobility parndt/refinerycms-wymeditor#56

Merged

parndt added a commit to parndt/refinerycms-wymeditor that referenced this pull request Sep 23, 2019

Switch test files to use Mobility (#56)

84af0d0

The project switched from Globalize in refinery/refinerycms#3360

parndt mentioned this pull request Apr 14, 2020

State of the gem / stale bot too aggressive? globalize/globalize#753

Closed

Matho mentioned this pull request Jul 21, 2022

SQLite3::SQLException: no such column: refinery_pages.custom_slug #3519

Open

		end

		add_index :refinery_image_translations, :refinery_image_id, name: :index_refinery_image_translations_on_refinery_image_id

Switch from Globalize to Mobility #3360

Switch from Globalize to Mobility #3360

Conversation

bricesanchez commented Mar 24, 2018 • edited by parndt 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

shioyama Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shioyama commented Apr 5, 2018

bricesanchez commented Apr 5, 2018

Choose a reason for hiding this comment

shioyama Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

bricesanchez Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricesanchez Apr 11, 2018 • 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

shioyama Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

bricesanchez commented Apr 6, 2018

shioyama commented Apr 6, 2018

bricesanchez commented Apr 6, 2018 • edited Loading

bricesanchez commented Apr 8, 2018

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

shioyama commented Apr 10, 2018

shioyama commented Apr 10, 2018 • edited Loading

shioyama commented Apr 10, 2018

bricesanchez commented Mar 24, 2018 •

edited by parndt

Loading

shioyama Apr 5, 2018 •

edited

Loading

shioyama Apr 5, 2018 •

edited

Loading

bricesanchez Apr 5, 2018 •

edited

Loading

bricesanchez Apr 11, 2018 •

edited

Loading

shioyama Apr 5, 2018 •

edited

Loading

bricesanchez commented Apr 6, 2018 •

edited

Loading

shioyama commented Apr 10, 2018 •

edited

Loading