Support for reifying belongs_to relationships #730
Conversation
|
Need help. RuboCop tests are failing because of Cyclomatic and Perceived complexity for reify is too high. Don't know how to fix it. |
| belongs_to: false)) | ||
| end | ||
|
|
||
| model.send("#{assoc.name}=".to_sym, collection) |
There was a problem hiding this comment.
to_sym isn't necessary here -- send automatically does that for you
There was a problem hiding this comment.
I think the "send" method in ruby v1.9.3 accepts only a symbol.
I'm sure we can find a way to extract a private method. (http://refactoring.com/catalog/extractMethod.html) |
This should help: #734 Oh!, I see you're also working on that? |
Haha, we came up with the same thing :D |
theRealNG
force-pushed
the
issue_503
branch
2 times, most recently
from
7cf94a3
to
11c23f0
Compare
|
@jaredbeck Refactored the "reify_associations" method to return the model, so that in future we can use it in any other place if required. |
jaredbeck
changed the title
| collection = if version.nil? | ||
| assoc.klass.where(assoc.klass.primary_key => collection_key).first | ||
| elsif version.event == "create" | ||
| options[:mark_for_destruction] ? collection.mark_for_destruction : nil |
There was a problem hiding this comment.
Is there a test that covers this line? It seems like the reference to collection here would raise a NameError. What am I missing?
There was a problem hiding this comment.
@jaredbeck
Nice catch. But I don't think this condition is ever met as the base model is always created before the associated model. Ex:- Widget instance is created before the wotsit instance.
There is a case when this condition is met, i.e when a new widget is created and the association is changed to point to the new widget.
In this case we should not destroy the widget instance as widget instance is not dependent on the wotsit instance.
I have written tests which covers the above case.
|
Please rebase on master, squash commits, and add a changelog entry. Thanks. |
|
@jaredbeck Done |
| @@ -55,20 +56,10 @@ def reify(version, options) | |||
| reify_attributes(model, version, attrs) | |||
| model.send "#{model.class.version_association_name}=", version | |||
| reify_associations(model, options, version) | |||
| model | |||
There was a problem hiding this comment.
I think I'd rather keep the explicit return value here, instead of moving it to reify_associations.
|
I've read through your tests and they look very thorough, thanks! Other than my minor point about the return value of |
|
@jaredbeck Changes the the return value of |
|
Merged. Thanks NG! Please submit a changelog entry when you get a chance. |
Support for reifying belongs_to relationships.
#503
So you can call @project.versions.last.reify( belongs_to: true) and get the client model at the time of last change of the project model.