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

Include has_one attributes correctly when calling to_params #262

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dturnerTS
Copy link
Contributor

Follow on to remiprev#206 to add support for has_one in to_params.

Also fixes remiprev#258

balvig added a commit to cooksnaps/her that referenced this pull request Sep 12, 2014
…arams

Include has_one attributes correctly when calling to_params
@pisaruk
Copy link

pisaruk commented Nov 13, 2014

Great job. Would you mind merge this code into your own fork? I would like to point to your fork/master.
Thank you.

@kindrowboat
Copy link
Contributor

👍

@mcfilib
Copy link

mcfilib commented Apr 17, 2015

@remiprev Besides this now causing conflicts, what was the reason this was never merged? This is causing pain for us and I'd really like to see this on master. How can I make that happen?

@dturnerTS
Copy link
Contributor Author

@Filib I suspect it has to do with #324 . Once things get back on track I'll be happy to merge master back into this PR

@mcfilib
Copy link

mcfilib commented Apr 17, 2015

@dturnerTS Ah, didn't see that and didn't realise that @remiprev was looking for a new maintainer. Thanks for the heads-up!

@hubert
Copy link
Contributor

hubert commented Aug 18, 2015

@dturnerTS i'm working to get Her down to 0 PRs. given there is already has_many serialization, i think it makes sense to do the same for has_one associations.

few things before we merge:

  1. how does it handle the case where the has_one association key is present but the value is nil?
  2. please rebase with master so that we can make sure this works with the current travis checks.
  3. please remove the commit bumping the version.

…s_in_to_params

* upstream/master: (54 commits)
  Provide a more ActiveRecord-like interface by aliasing new_record? to new?
  Do not check if the record is persisted to get the default value.
  Add a failing test for a belongs_to association with nil foreign key in a non-persisted record.
  Fix belongs_to fetch
  Fix find through an association
  Fix parsing associations (Parse#extract_array)
  Add specs for testing associations with active_model_serializers
  use `request_path` in specs instead of internal `build_request_path`
  Fixes remi#357
  specify the code block as ruby
  Escape path variables
  Parse has_one association data before initializing objects
  acquire a mutex when defining attribute methods
  use ActiveModel convention for defining attribute methods
  define all attribute methods inside generated_attribute_methods
  add specs for when attribute methods predefined
  make possible to spawn_model from superclass
  Update release notes for info on security fix
  Bump version to 0.7.6
  Loosen activemodel, activesupport dependencies to < 4.3 to accommodate rails security fixes
  ...
…t the case where associations[:has_one] or associations[:has_many] is nil
@dturnerTS
Copy link
Contributor Author

@hubert Thanks for looking into this again.
(1) I've changed the code to be more defensive.
(2) Conflicts are resolved
(3) Version bump is removed

@hubert hubert self-assigned this Sep 4, 2015
@hubert
Copy link
Contributor

hubert commented Sep 18, 2015

@dturnerTS thanks for taking the time to go through this again. can you rebase this instead of merging it?

@dturnerTS
Copy link
Contributor Author

I'm not a big fan or rebasing. Can you explain why you think its preferable to the master merge here?

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.

Problem saving resource using form.fields_for
5 participants