Refactor association methods + fix issue with #find_by_id #158
Open
tiagopog wants to merge 4 commits intoactive-hash:masterfrom
Open
Refactor association methods + fix issue with #find_by_id #158tiagopog wants to merge 4 commits intoactive-hash:masterfrom
#find_by_id #158tiagopog wants to merge 4 commits intoactive-hash:masterfrom
Conversation
serradura
reviewed
Jan 16, 2018
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.
@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
serradura
reviewed
Jan 16, 2018
lib/associations/associations.rb
Outdated
| name.foreign_key | ||
| end | ||
| end | ||
| foreign_key.to_sym |
There was a problem hiding this comment.
Author
|
@zilkey uh, I was wondering here whether it's really necessary to keep the support for the old/deprecated Rails' |
Collaborator
|
For reference, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's this PR do?
It changes the use of
#find_by_idin 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_byrather 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
ActiveHashwas not retrieving abelongs_toassociation for calling the underlying#find_by_idon the related model and returningnil.I did a little debug and figured out that, unlike the
#find_by_*, calling the#find_bymethod 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