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

Rails ActionText backend to translate rich text #385

Closed
sedubois opened this issue May 30, 2020 · 12 comments
Closed

Rails ActionText backend to translate rich text #385

sedubois opened this issue May 30, 2020 · 12 comments

Comments

@sedubois
Copy link
Contributor

sedubois commented May 30, 2020

I would like the :key_value backend to be configurable in order to support translating ActionText rich text.

Context

It appears Mobility is becoming the new "Rails I18n de-facto standard library for ActiveRecord model/data translation", to re-use Globalize's description 😉 I also guess that many apps, including my own bilingual content-centric app, use rich text rather than plain text, where ActionText is supposed to be the ad-hoc Rails solution. Additionally, Mobility describes itself as adapting to different storage needs. I would thus like Mobility and ActionText to work elegantly together.

Expected Behavior

Being able to store and query translated ActionText rich text using a single table join using the existing Mobility API.

Actual Behavior

I am not aware of a way to make ActionText and Mobility work together without requiring two levels of joining. This example shows that translating ActionText can be done, however it requires two nested joins and fragile patches of code.

Possible Fix

Similar to the :key_value backend, ActionText also works by establishing a polymorphic relation on the model. This raises an opportunity to translate ActionText rich text "for free", i.e. at no extra performance cost compared to untranslated rich text, by directly using the action_text_rich_texts table as a :key_value translation backend instead of the mobility_text_translations table.

Consider the Mobility and ActionText schemas, they already have a 1:1 mapping, except for the locale column: key <=> name, value <=> body, translatable_id <=> record_id, translatable_type <=> record_type:

create_table "mobility_text_translations", force: :cascade do |t|
  t.string   "locale"
  t.string   "key"
  t.text     "value"
  t.integer  "translatable_id"
  t.string   "translatable_type"
  ...
end
create_table "action_text_rich_texts", force: :cascade do |t|
  t.string "name", null: false
  t.text "body"
  t.bigint "record_id", null: false
  t.string "record_type", null: false
  ...
end

Whereas for the example above, ActionText would normally store:

{ record_type: 'Post', record_id: 1, name: 'content', body: '<h1>hello world!</h1>' }

... it would instead store:

{ record_type: 'Post', record_id: 1, name: 'content', locale: 'en', body: '<h1>hello world!</h1>' }
{ record_type: 'Post', record_id: 1, name: 'content', locale: 'fr', body: '<h1>bonjour le monde !</h1>' }

So if Mobility offered a way to configure the table name and columns for the :key_value backend, in theory everything should "just work" 🤞😄

Mobility could be instructed to achieve it with something like this:

Mobility.configure do |config|
  config.default_backend = :key_value
  config.backends.key_value.text.table = :action_text_rich_texts
  config.backends.key_value.text.translatable = :record
  config.backends.key_value.text.key = :name
  config.backends.key_value.text.value = :text
end

Or with a shorthand:

Mobility.configure do |config|
  config.default_backend = :key_value
  config.key_value_text_backend = :action_text
end

The migration should be adapted to first check if the table exists, and if it exists, to insert the required columns/indices.

Then at the model level the following (existing) API should suffice:

# app/models/post.rb
class Post < ApplicationRecord
  has_rich_text :content
  translates :content
end

@shioyama I understand that you do not have much bandwidth, but would you consider a PR? In that case I would be willing to give it a try with your guidance.

@sedubois
Copy link
Contributor Author

sedubois commented Jun 1, 2020

I made a demo with these changes: sedubois@7ec6bdf, and the migrations below (coming on top of previously run ActionText migrations):

class CreateTextTranslations < ActiveRecord::Migration[6.0]
  def change
    add_column :action_text_rich_texts, :locale, :string
    add_index :action_text_rich_texts, [:record_id, :record_type, :locale, :name], unique: true, name: :index_rich_texts_uniqueness
    remove_index :action_text_rich_texts, name: :index_action_text_rich_texts_uniqueness
    add_index :action_text_rich_texts, [:record_id, :record_type, :name], name: :index_action_text_rich_texts_on_name
  end
end

class CreateStringTranslations < ActiveRecord::Migration[6.0]
  def change
    create_table :mobility_string_translations do |t|
      t.string :locale, null: false
      t.string :name, null: false
      t.string :body
      t.references :record, polymorphic: true, index: false
      t.timestamps null: false
    end
    add_index :mobility_string_translations, [:record_id, :record_type, :locale, :name], unique: true, name: :index_mobility_string_translations_on_names
    add_index :mobility_string_translations, [:record_id, :record_type, :name], name: :index_mobility_string_translations_on_record_attribute
    add_index :mobility_string_translations, [:record_type, :name, :body, :locale], name: :index_mobility_string_translations_on_query_names
  end
end

Seems to work! 👍

@shioyama instead of changing the names of the key/value attributes, is it possible with Arel to alias the columns, to make things simpler?

@shioyama
Copy link
Owner

shioyama commented Jun 4, 2020

@sedubois Thanks for the feature proposal and investigation! I actually don't know ActionText well at all so a bit hard to comment. However, on the surface it seems that within KeyValue, you need the flexiblity to specify a different translation class (which would also allow you to specify a different table to store translations). Then you could also specify the column on that table to store the translation (body in this case rather than value).

If that was possible within Mobility, then this might work without any special treatment for ActionText.

@shioyama
Copy link
Owner

shioyama commented Jun 4, 2020

Oh I see you've more or less proposed that above! Sorry now I completely understand.

So if Mobility offered a way to configure the table name and columns for the :key_value backend, in theory everything should "just work"

Exactly right. I think there should be a way to do this. Generally we'd want to avoid adding any configuration that is specific to one backend, but options are always accessible to the backend so you can pass custom ones which only KeyValue would use.

Actually backend options is something I'm thinking about possibly changing a bit in v1.0. Right now they are just mixed in with plugin options, and that works, but ideally they should be separated into their own hash. Maybe this is a chance to think this through. 🤔

Anyway though, thanks very much for POC and certainly this seems like it should be doable. 👍

@shioyama
Copy link
Owner

shioyama commented Jun 4, 2020

Actually come to think of it, another way to do this would be to define a custom action_text backend specifically for this case, which could subclass Mobility::Backends::ActiveRecord::KeyValue. Could be a separate gem or something. Then you could just override config as necessary.

@sedubois
Copy link
Contributor Author

sedubois commented Jun 8, 2020

Thanks @shioyama, I think I have a preference for configuring this through some options, as overriding the backend class sounds like a bit too coarse and might increase the API surface too much? As it would require overriding methods and might thus need to copy their logic.

I'm working on migrating to Mobility, if I have a chance to poke around with this I will report back.

@shioyama
Copy link
Owner

if I have a chance to poke around with this I will report back.

Great, sounds good 👍

@olimart
Copy link

olimart commented Jun 18, 2020

@sedubois how can I contact you?
I'm running into the same "issue" and would like to exchange if possible. Thanks

@sedubois
Copy link
Contributor Author

@olimart I've just joined the gitter, hopefully I should get a notification if you mention me there (I guess): https://gitter.im/mobility-ruby/mobility

@sedubois
Copy link
Contributor Author

This will be fixed in version 1.1 (includes #488), to be used in combination with mobility-actiontext.

@shioyama
Copy link
Owner

Thanks for working on that! I'll aim to release 1.1 sometime next week. I was going to wait to make some improvements to fallbacks but that can wait until 1.2.

@Dragonicity
Copy link

Did this get released?

@sedubois
Copy link
Contributor Author

Did this get released?

Yes

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

Successfully merging a pull request may close this issue.

4 participants