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

accepts_nested_attributes_for with active-model-adapter #74

Closed
GCorbel opened this issue Dec 18, 2015 · 8 comments
Closed

accepts_nested_attributes_for with active-model-adapter #74

GCorbel opened this issue Dec 18, 2015 · 8 comments

Comments

@GCorbel
Copy link

GCorbel commented Dec 18, 2015

Hi,

When I save a record with association, I have this kind of params in my Rails app : {"course_id"=>"1", "user"=>{"email"=>"rst", "contacts"=>[{"fullname"=>"rst", "phone"=>"rst", "email"=>"rst"}]}} which is corrected.

When I do something like Subscription.new params, it gives ActiveRecord::AssociationTypeMismatch: User(#70224777299060) expected, got Hash(#70224747397500). Which is correct too but it can be useful to have an option to have this json {"course_id"=>"1", "user_attributes"=>{"email"=>"rst", "contacts_attributes"=>[{"fullname"=>"rst", "phone"=>"rst", "email"=>"rst"}]}} in order to use it with accepts_nested_attributes_for.

What you think ? I'm ready to contribute if you want.

@GCorbel
Copy link
Author

GCorbel commented Dec 18, 2015

It seems to work as I want with this serializer :

import { ActiveModelSerializer } from 'active-model-adapter';

export default ActiveModelSerializer.extend(DS.EmbeddedRecordsMixin, {
  attrs: {
    user: { embedded: 'always' },
    contacts: { embedded: 'always' },
  },
  _serializeEmbeddedBelongsTo: function (snapshot, json, relationship) {
    var embeddedSnapshot = snapshot.belongsTo(relationship.key);
    var serializedKey = this.keyForAttribute(relationship.key, 'serialize') + '_attributes';
    if (!embeddedSnapshot) {
      json[serializedKey] = null;
    } else {
      json[serializedKey] = embeddedSnapshot.record.serialize({ includeId: true });
      this.removeEmbeddedForeignKey(snapshot, embeddedSnapshot, relationship, json[serializedKey]);

      if (relationship.options.polymorphic) {
        this.serializePolymorphicType(snapshot, json, relationship);
      }
    }
  },
  _serializeEmbeddedHasMany: function (snapshot, json, relationship) {
    var serializedKey = this.keyForAttribute(relationship.key, 'serialize') + '_attributes';

    Ember.warn('The embedded relationship \'' + serializedKey + '\' is undefined for \'' + snapshot.modelName + '\' with id \'' + snapshot.id + '\'. Please include it in your original payload.', Ember.typeOf(snapshot.hasMany(relationship.key)) !== 'undefined', { id: 'ds.serializer.embedded-relationship-undefined' });

    json[serializedKey] = this._generateSerializedHasMany(snapshot, relationship);
  }
});

It's uggly. For a clean solution, I think we have contribute to ember data to change those functions in DS.EmbeddedRecordsMixin.

I still don't know if it's a good idea or if there is a better way. What you think?

@bmac
Copy link
Contributor

bmac commented Dec 21, 2015

Its strange that _serializeEmbeddedBelongsTo uses keyForAttribute and not keyForRelationship. Any idea why it does that @wecc or @fivetanley?

@GCorbel If Ember Data was changed to use keyForRelationship would overriding that method to return the key + _attributes solve your problem?

@fivetanley
Copy link
Contributor

probably scumbag stanley didn't think through the code all the way. keyForRelationship seems more correct.

@GCorbel
Copy link
Author

GCorbel commented Dec 21, 2015

@bmac, yes, I think it will solve my problem. Thanks!

@bmac
Copy link
Contributor

bmac commented Dec 21, 2015

Great @GCorbel. If you are still interested in contributing changing _serializeEmbeddedHasMany and _serializeEmbeddedBelongsTo to use keyForRelationship in https://github.com/emberjs/data/blob/master/addon/-private/serializers/embedded-records-mixin.js would be a great fix.

@GCorbel
Copy link
Author

GCorbel commented Dec 21, 2015

@bmac it makes sense. I'll try to do it tonight.

@GCorbel
Copy link
Author

GCorbel commented Dec 22, 2015

Done!

@GCorbel GCorbel closed this as completed Dec 22, 2015
@mmahalwy
Copy link

Hey, forgive my ignorance, how can I make this work with accepts_nested_attributes_for ? Just copy-pasta the code snippet above?

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

No branches or pull requests

4 participants