-
Notifications
You must be signed in to change notification settings - Fork 178
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
Refactor association methods + fix issue with #find_by_id
#158
base: master
Are you sure you want to change the base?
Conversation
lib/associations/associations.rb
Outdated
elsif klass.respond_to?(:scoped) | ||
klass.scoped(:conditions => {foreign_key => primary_key_value}) | ||
def normalize_primary_key(type, association_class) | ||
primary_key = type == :belongs_to ? association_class.primary_key : name.constantize.primary_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiagopog I'd like to recommend to invoke the primary_key method just once before of .to_sym
calling. e.g:
klass = type == :belongs_to ? association_class : name.constantize
klass.primary_key.to_sym
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, @serradura 👍
lib/associations/associations.rb
Outdated
end | ||
end | ||
foreign_key.to_sym |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zilkey uh, I was wondering here whether it's really necessary to keep the support for the old/deprecated Rails' |
What's this PR do?
It changes the use of
#find_by_id
in favor of#find_by
. It also refactors some portion of the code related to the associations macros (belongs_to
,has_one
,has_many
).TODOs
#find_by
rather than#find_by_*
when fetching single associations;Background context
Recently we went through a weird situation at Beauty Date while running our seeds file. It turned out that a model based on
ActiveHash
was not retrieving abelongs_to
association for calling the underlying#find_by_id
on the related model and returningnil
.I did a little debug and figured out that, unlike the
#find_by_*
, calling the#find_by
method was actually working as expected. I don't know yet the reason for this bug to be happening apparentlyonly when executing the seed task, but maybe it's something related to the seed context and the metaprogamming applied while defining those dynamic finders. I sure will need to make a deeper debug on it.
For now this fork has fixed the issue and I also think that avoiding unnecessary metaprogramming is a good practice (safer, faster etc).
Manual test