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

Fixes to the has_many finder builder #179

Closed
wants to merge 2 commits into from
Closed

Fixes to the has_many finder builder #179

wants to merge 2 commits into from

Conversation

JKring
Copy link

@JKring JKring commented Jul 21, 2015

This is my first PR to any Rails project, so let me know if I'm overstepping my bounds or if I missed the memo about how all this works.

I am attempting to resolve two bugs in the has_many association (one documented, one not).

The undocumented bug is that if you call a has_many association on a new record, it returns the collection parser for that record's class. It should probably return the collection parser for the associated class, no?

The other bug is documented here and here. In short, the documentation says:

# If the response body does not contain an attribute matching the association name
# a request sent to the index action under the current resource.
# For the example above, if the comments are not present the requested path would be:
# GET /posts/123/comments.xml

But that does not seem to be the case. In actuality, the behavior generates a URL like:

GET comments.xml?post_id=123

I believe this PR fixes both those bugs. But I will do further testing to be sure.

This addresses an undocumented bug: if you call a has_many
association on a new record, it returns the collection parser
for that record's class. It should return the collection parser
for the associated class. This commit also includes tests for
previously untested, related code.
This resolves an issue in the has_many association builder.
The documentations says:

  if the comments are not present the requested path would be:
  GET /posts/123/comments.xml

The actual behavior sends a request to GET comments.xml?post_id=123
This commit fixes that, sending the request to the expected endpoint.
Also includes tests to indicate and assert desired behavior.
@rafaelfranca
Copy link
Member

Thank you for the pull request. I still unsure what if it is the doc that is wrong or the code. I'll investigate.

@rails-bot rails-bot bot added the stale label Aug 31, 2017
@rails-bot
Copy link

rails-bot bot commented Aug 31, 2017

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.

If it is an issue and you can still reproduce this error on the master branch,
please reply with all of the information you have about it in order to keep the issue open.

If it is a pull request and you are still interested on having it merged please make sure it can be merged clearly.

Thank you for all your contributions.

@rails-bot rails-bot bot closed this Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants