Skip to content
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

Merged
merged 5 commits into from
Feb 13, 2019

Conversation

keegangroth
Copy link
Contributor

fixes #513

@zacharywelch zacharywelch self-requested a review February 11, 2019 20:12
@@ -112,24 +112,39 @@
data_key: :organization,
default: nil,
class_name: "Organization",
foreign_key: "organization_id",
path: "/organizations/:id"
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

@zacharywelch zacharywelch merged commit 983b528 into remi:master Feb 13, 2019
@zacharywelch
Copy link
Collaborator

thanks @keegangroth 🎩 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

belongs_to associations no longer respect collection_paths
2 participants