shioyama / mobility

Pluggable Ruby translation framework
MIT License
1.01k stars 82 forks source link

1.3.0.rc1 Can't assign an translated attribute with a column fallback #635

Open ScotterC opened 8 months ago

ScotterC commented 8 months ago

I'm just getting started with Mobility. I found that to use the container strategy on postgres and preserve my existing columns it was best to use 1.3.0.rc1. Rails 7.1.3

Context

  plugins do
    backend :jsonb
    active_record
    column_fallback true
  end

My Document model has an existing title column with a default locale of english.

# Migration
add_column :documents, :translations, :jsonb, default: {}

class Document < ApplicationRecord
  extend Mobility
  translates :title
end

Expected Behavior

Document.last.title #=> "Document Title in fallback column"
 title_es = "hablas espagnol"
  Mobility.with_locale(:es) {
    document.title = title_es
    puts document.title
  } #=>  "hablas espagnol"

and to have title_es stored in the translations column of the Document record.

Actual Behavior

Here's my test script to reproduce

  document = Document.last
  puts document.title
  title_es = "hablas espagnol"
  Mobility.with_locale(:es) {
    document.title = title_es
    puts document.title
  }
# => mobility-1.3.0.rc1/lib/mobility/backends/active_record/pg_hash.rb:26:in `[]=': string not matched (IndexError)

Possible Fix

Spelunking in pg_hash, here's what I find

# ...
        def write(locale, value, _options = nil)
          if value.nil?
            translations.delete(locale.to_s)
          else
            translations[locale.to_s] = value
          end
        end
# ...
        def translations
          model[column_name]
        end
# ...

translations[locale.to_s] = value has an IndexError because translations is returning the value of the column fallback :title attribute.

translations #=> "Document Title in fallback column"
column_name #=> "title"
model.translations #=> {}

A very naive fix would be to change translations but I know there's a lot more going on here.

ScotterC commented 8 months ago

The good news for me is I should be using :container backend and not :jsonb backend which resolves my issue. I'm not 100% sure but it seems that the above still holds true for a :jsonb backend with a column_fallback true plugin.

This failing test reveals the issue which could be added if this is intended behavior.

# spec/mobility/plugins/active_record/column_fallback_spec.rb
  context "column_fallback: true with jsonb backend" do
    plugin_setup :slug, column_fallback: true

    before do
      translates model_class, :slug, backend: :jsonb, column_fallback: true
    end

    it "reads/writes from/to backend if locale is not I18n.default_locale" do
      instance.send(:write_attribute, :slug, "foo")

      Mobility.with_locale(:fr) do
        expect(listener).to receive(:read).with(:fr, any_args).and_return("bar")
        expect(instance.slug).to eq("bar")

        expect(listener).to receive(:write).with(:fr, "baz", any_args)
        instance.slug = "baz"
      end
    end
  end
JacobAlexander commented 4 months ago

Correct me but in documentation jsonb column for for your translates :title should be added as add_column(:documents, :title, :jsonb, default: {}) not the add_column :documents, :translations, :jsonb, default: {}