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

Add _ids has_many association array #190

Closed
wants to merge 1 commit into from

Conversation

lleger
Copy link

@lleger lleger commented Oct 30, 2013

What
This commit adds a getter and setter to has many associations for an _ids array. It returns this array in to_params and removes the association parameter.

Why
If you have a user who has many posts and you send a posts parameter, AR will throw a ActiveRecord::AssociationTypeMismatch because it's expecting an association when you're sending a hash. You can either, instead, send along post_attributes (to create the object and relationship) or post_ids (to create the relationship if the object already exists). The former was already present in Her; this commit adds the latter. This commit also avoids the AR error.

How
A getter and setter is defined when attaching a has many association. The list of attributes when requesting to_params is filtered to remove the association array and add the _ids array.

This PR includes passing tests and has been integrated into a production app. This problem was discussed in #153 and credit to @calmyournerves for the code and tests to filter out the association array.

Example

class Post
  include Her::Model
  belongs_to :user

  attributes :title
end

class User
  include Her::Model
  has_many :posts

  attributes :name
end

@user = User.find(1)
@user.post_ids # => [0, 1, 5]
@user.post_ids = [0, 1, 5, 14]
@user.to_params # => { name: "Tobias Funke", post_ids: [0, 1, 5, 14] }

This commit adds an `_ids` array getter and setter on `has_many`
associations, which is returned in `to_params`. The association array is
also removed from `to_params`.

**Example**
```
class Post
  include Her::Model
  belongs_to :user

  attributes :title
end

class User
  include Her::Model
  has_many :posts

  attributes :name
end

@user = User.find(1)
@user.post_ids # => [0, 1, 5]
@user.post_ids = [0, 1, 5, 14]
@user.to_params # => { name: "Tobias Funke", post_ids: [0, 1, 5, 14] }
```

- Add `_ids` getter and setter to
  `Her::Model::Associations::HasManyAssociation` on attach
- Add `_ids` array to `to_params` output
- Remove association array from `to_params` output
- Add specs to test presence of `_ids` array and absence of association
  array
@yonahforst
Copy link

Any suggestions for saving objects along with nested associations? (that is, how to automagically append _attributes to associations)

@lleger
Copy link
Author

lleger commented Feb 18, 2014

Any thoughts on getting this merged in?

@hubert
Copy link
Contributor

hubert commented Aug 11, 2015

hi @lleger. first off, thanks for the contribution. i have a couple concerns tho.

  1. i worry about the association_ids method and the association being separate entities in the response. this opens up the possibility that the data could conflict with each other.
  2. the automagic addition of the _ids method. given the inherent separate nature of a remote service, i feel it prudent to be conservative about sending anything not explicitly specified by the caller. while it might work smoothly for rails-specific services, we have no assurances that the remote service is indeed that.

if you feel i'm erring in my judgment, i encourage you to let me know.

@hubert hubert closed this Aug 11, 2015
@lleger
Copy link
Author

lleger commented Aug 11, 2015

In all honesty, I had forgotten completely about this PR. 2013 was a much different time! In any case, I couldn't really comment on your decision; we haven't used Her in production in over a year, and I'm thoroughly unfamiliar with the project in its current state. Even if I did disagree with closing this, the branch is almost two years outdated, and it would hardly be worth the time to update it for reconsideration.

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.

None yet

3 participants