-
Notifications
You must be signed in to change notification settings - Fork 325
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
belongs_to association should respect association's collection_path #514
Conversation
@@ -112,24 +112,39 @@ | |||
data_key: :organization, | |||
default: nil, | |||
class_name: "Organization", | |||
foreign_key: "organization_id", | |||
path: "/organizations/:id" |
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.
We need each association in the list of <HerModel>.associations
to include a path. The documentation states this will default to /{class_name}.pluralize/{id}
for belongs_to
though I agree it should be resource_path
or collection_path
depending on the type of association.
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.
ok I guess that means extracting and generalizing the path building logic from fetch. I'll give it a try when I get a chance
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.
@keegangroth after further thought I'm more inclined to accept a default path
of nil
on belongs_to
👍 Long term we might want to refactor .associations
to resemble something like ActiveRecord::Reflection 😅
Until then, defaulting to nil
would have the following, expected behavior on belongs_to associations: use path
if provided, otherwise build_request_path
would default to resource_path
. We'd also need to update the docs for belongs_to (maybe remove the defaults to... language)
@edtjones see any issues here?
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.
sounds good, I was not seeing a good way to get at the relevant information where the option defaults were being set. Will have a go at updating the docs.
thanks @keegangroth 🎩 🎉 |
fixes #513