-
Notifications
You must be signed in to change notification settings - Fork 324
Scopes on associations #268
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
base: master
Are you sure you want to change the base?
Changes from all commits
4391a24
0def4c8
eb55314
d3846ac
1c36fab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,7 +89,13 @@ | |
builder.adapter :test do |stub| | ||
stub.get("/users/1") { |env| [200, {}, { :id => 1, :name => "Tobias Fünke", :comments => [{ :comment => { :id => 2, :body => "Tobias, you blow hard!", :user_id => 1 } }, { :comment => { :id => 3, :body => "I wouldn't mind kissing that man between the cheeks, so to speak", :user_id => 1 } }], :role => { :id => 1, :body => "Admin" }, :organization => { :id => 1, :name => "Bluth Company" }, :organization_id => 1 }.to_json] } | ||
stub.get("/users/2") { |env| [200, {}, { :id => 2, :name => "Lindsay Fünke", :organization_id => 2 }.to_json] } | ||
stub.get("/users/1/comments") { |env| [200, {}, [{ :comment => { :id => 4, :body => "They're having a FIRESALE?" } }].to_json] } | ||
stub.get("/users/1/comments") do |env| | ||
if env[:params]["locked"] == "true" | ||
[200, {}, [{ :comment => { :id => 5, :body => "Is this the tiny town from Footloose?", :locked => true } }].to_json] | ||
else | ||
[200, {}, [{ :comment => { :id => 4, :body => "They're having a FIRESALE?" } }].to_json] | ||
end | ||
end | ||
stub.get("/users/2/comments") { |env| [200, {}, [{ :comment => { :id => 4, :body => "They're having a FIRESALE?" } }, { :comment => { :id => 5, :body => "Is this the tiny town from Footloose?" } }].to_json] } | ||
stub.get("/users/2/comments/5") { |env| [200, {}, { :comment => { :id => 5, :body => "Is this the tiny town from Footloose?" } }.to_json] } | ||
stub.get("/users/2/role") { |env| [200, {}, { :id => 2, :body => "User" }.to_json] } | ||
|
@@ -119,6 +125,8 @@ | |
spawn_model "Foo::Comment" do | ||
belongs_to :user | ||
parse_root_in_json true | ||
collection_path 'users/:user_id/comments' | ||
scope :locked_scope, lambda { where(locked: true) } | ||
end | ||
spawn_model "Foo::Post" do | ||
belongs_to :admin, :class_name => 'Foo::User' | ||
|
@@ -228,6 +236,11 @@ | |
params[:comments].length.should eq(2) | ||
end | ||
|
||
it 'supports scopes on assoications' do | ||
locked_comments = @user_with_included_data.comments.locked_scope | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i found this call will append another
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I'm not sure there is currently a good way to prevent that from happening. Any suggestions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fixed now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, confirmed the behavior 👍 |
||
locked_comments.first.id.should eq(5) | ||
end | ||
|
||
[:create, :save_existing, :destroy].each do |type| | ||
context "after #{type}" do | ||
let(:subject) { self.send("user_with_included_data_after_#{type}")} | ||
|
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.
does it mean we need to specify
collection_path
on the association's model if we want to call the scopes on it? is there any way to get around it?This is the official document example,
wonder if we could just call it without adding that
collection_path
: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.
I'm not sure exactly what your asking about.
You don't have to explicitly set a collection path (https://github.com/remiprev/her/blob/master/lib/her/model/paths.rb#L48). If you don't set the collection_path it basically defaults to
/class_name
.However, if a user has many comments, and you need to get the comments through
/user/:user_id/comments
Her requires a collection path to be set. There are some open issues discussing ways to deal with that and its far out side of the scope of this PR.If you're suggesting that the right behavior is just to ignore whats in the collection path, I'd disagree since that would limit functionality.
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.
sorry I didn't make it clear. I'll switch the example from "comment" to "post", please bear with me for a moment.
In web app we normally don't show a list of comment, they tend to attach to a specific user so specifying the
collection_path
totally makes sense. But things like Post we may also want to show a full list of them(/posts
), and also posts that belong to a user(/users/:user_id/posts
). I was seeking a way to make the "scopes on associations" thing work on this kind of usage.If remove that
collection_path
from Post modelThere 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.
Got, it. This is the same issue raised in #253 . The reason that is has not been resolved is that there is no clear answer on what to do when a class has multiple
belongs_to
associations.I strongly believe that this issue is outside of the scope of this pr.
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.
thanks for the reply, seems there're a lot unsolved discussions on the nested resource path topic. Sorry for bothering you all along 🙇 , hope this pull request will be merged..! i'm sure it'll solve lots of real world problems