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

Include the table name in the with_x_translation #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

louim
Copy link

@louim louim commented Jul 31, 2019

When generating with_#{attr_name}_translation methods, include the table name in the query. This is to prevent a possible ambiguous column error when the model is joined with another model having the same translated column name.

For example:

class Post < ActiveRecord::Base
  has_many :tags
  translates :title, :body_1

  scope :tagged, -> (tag_title) { joins(:tags).merge(Tag.with_title_translation(tag_title)) }
end

class Tag < ActiveRecord::Base
  belongs_to :post
  translates :title
end

Calling Post.tagged("abc") would produce ActiveRecord::StatementInvalid: PG::AmbiguousColumn: ERROR: column reference "title_translations" is ambiguous because the table name was not included in the query. I added a test covering that case.

@frantisekrokusek
Copy link
Collaborator

I like it!

@frantisekrokusek frantisekrokusek self-assigned this Jan 5, 2021
@frantisekrokusek frantisekrokusek self-requested a review January 5, 2021 10:41
Comment on lines +210 to +245
p = Post.create!(:title_translations => { "en" => "Alice in Wonderland", "fr" => "Alice au pays des merveilles" })
Tag.create!(:title_translations => { "en" => "A Tag", "fr" => "Un tag" }, post: p)
I18n.with_locale(:en) do
assert_equal p.title_en, Post.tagged("A Tag").first.try(:title)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
p = Post.create!(:title_translations => { "en" => "Alice in Wonderland", "fr" => "Alice au pays des merveilles" })
Tag.create!(:title_translations => { "en" => "A Tag", "fr" => "Un tag" }, post: p)
I18n.with_locale(:en) do
assert_equal p.title_en, Post.tagged("A Tag").first.try(:title)
post = Post.create!(:title_translations => { "en" => "Alice in Wonderland", "fr" => "Alice au pays des merveilles" })
Tag.create!(:title_translations => { "en" => "A Tag", "fr" => "Un tag" }, post: post)
I18n.with_locale(:en) do
assert_equal post.title_en, Post.tagged("A Tag").first.title

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to make that change, but I originally went with p because that's what was used in all the tests. I think a change like that should be a separate PR and fix update all existing tests in one pass. What do you think?

This is to prevent a possible ambiguous column error when the model is joined
with another model having the same translated column name.
@louim louim force-pushed the improvement/include-table-name-in-query branch from 3675cee to 823019c Compare February 23, 2021 21:34
@louim
Copy link
Author

louim commented Feb 23, 2021

Hey @frantisekrokusek, I rebased this PR against the main branch. It seems like travis is not setup to run the tests anymore. Are you planning on switching to another CI tool setup?

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

Successfully merging this pull request may close these issues.

2 participants