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

Fix: use klass.table_name instead of guessing from associated models #847

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

ocarta-l
Copy link
Contributor

Hi,

In a middle of a refactoring task, we came across a issue with globalize + annotate. Annotate is adding a none-existant field to the model:

  # Table name: countries
  #
+ #  country_id  :integer
  #  created_at  :datetime         not null
  ....

The issue come when you overide table_name. For exemple, if we have a model Country located at models/country.rb and we decide to move the file in a folder without changing the table_name, we will have:
models/folder/country.rb

module Folder
 class Country < ApplicationRecord
    self.table_name = "countries"
    ...
 end
end

# == Schema Information
#
# Table name: countries
#
#  country_id  :integer <- field added
# ....

With the orginal code, we have:

pry(main)> klass = Folder::Country
pry(main)> foreign_column_name = [klass.translation_class.to_s.gsub('::Translation', '').gsub('::', '_').downcase,'_id'].join.to_sym
=> :folder_country_id

Using table_name is a safer way to do it, in my opinion

pry(main)> klass = Folder::Country
pry(main)> foreign_column_name = [klass.table_name.singularize,'_id'].join.to_sym
 => :country_id

@ctran
Copy link
Owner

ctran commented Mar 24, 2021

Could you add a test to show this does fix the problem you mentioned?

@ctran ctran added the reviewed label Mar 24, 2021
@ocarta-l ocarta-l force-pushed the develop branch 2 times, most recently from fe3c50a to 1d6943d Compare April 2, 2021 16:33
@ocarta-l
Copy link
Contributor Author

ocarta-l commented Apr 2, 2021

Hi @ctran,
Sorry for the late answer, I messed up my notification config.

Test on this PR:
image

Same test with master version:
image

I just updated the current test for this part, adding a fake folder in the namespace Folder::Post::Translation

EDIT: It seems the ci has some issues, feel free to ping me :)

@ocarta-l
Copy link
Contributor Author

@ctran when you have time, can you review again this PR :)

@ocarta-l ocarta-l reopened this Jun 18, 2021
@drewlustro
Copy link

➕ this is more stable! Also might unblock shaky module support of annotate

@yao0204
Copy link

yao0204 commented Sep 8, 2021

@ctran could you take a look if this PR is ok? this could solve my issue too :)

@ctran ctran self-requested a review January 31, 2022 16:50
@ctran ctran merged commit 69ab184 into ctran:develop Jan 31, 2022
@ctran
Copy link
Owner

ctran commented Jan 31, 2022

Thanks.

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

Successfully merging this pull request may close these issues.

4 participants