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

changedAttributes doesn't return changed relationships #3045

Closed
jamesarosen opened this issue May 5, 2015 · 11 comments
Closed

changedAttributes doesn't return changed relationships #3045

jamesarosen opened this issue May 5, 2015 · 11 comments

Comments

@jamesarosen
Copy link

Currently, changedAttributes relies on comparing _data and _attributes. _attributes doesn't include any information about (belongs-to) relationships, so changing one won't show up in changedAttributes.

It's totally reasonable that changedAttributes should not return relationship information because of its name. In that case, there should be a separate changedRelationships to match it.

@jamesarosen
Copy link
Author

Related to #1367. Possibly should be part of #1308 or #2545

@arenoir
Copy link
Contributor

arenoir commented May 5, 2015

IMO changing a belongs to association should show up in changedAttributes because the foreign_key would have changed.

@igorT
Copy link
Member

igorT commented May 5, 2015

It's specifically called changed attributes, there will be a specific one for relationship pretty soon, but it is very not trivial. Maybe we can get away with partial solutions for a while.

@igorT igorT closed this as completed May 5, 2015
@arenoir
Copy link
Contributor

arenoir commented May 5, 2015

@igorT given the following example.

//models/job.js
import Ember from 'ember';
export default DS.Model.extend({
  address: DS.belongsTo('address')
});

//routes/job.js
export default Ember.Route.extend( {
  model: function() {
    return this.store.find('job', params.id);
  },

  actions: {
    changeAddress: function(address) {
      var model = this.get('currentModel');

      model.set('address', address);
    },

    save: function() {
      var model = this.get('currentModel);

      if (model.get('isDirty')) {
        return model.save();
      }
    }
  }
}

Doesn't it make sense that calling the changeAddress action would update the addressId attribute thus marking the model dirty?

Currently the save action would not fire because changing the address doesn't mark the model dirty even though the foreign key changed.

Am I missing something? Is belongs to not relational with a foreign key attribute?

@arenoir
Copy link
Contributor

arenoir commented Aug 23, 2015

@igorT, @jamesarosen, it keeps getting harder and harder to track if a belongs_to relationship has changed. I am not talking about testing for changes on the attributes of related models just testing if say a books.author has been changed.

In rails terms belongsTo is feeling more like hasOne.

Is there a current work around as the data hash no longer includes the foreign_key.

@leifdejong
Copy link

I don't know if this helps but I extended changedAttributes to also return changed relationships. I also included a snippet to commit without using the .save() method.

Model.reopen({

  /**
   * commit() saves changes locally without pushing to the server
   */
  commit() {
    this.get('_internalModel').send('willCommit');
    this.set('_internalModel._attributes', {});
    this.get('_internalModel').send('didCommit');
  },

  /**
   * changedAttributes() extends changedAttributes() to also return changed relationships
   */
  changedAttributes() {
    const attributes = this._super(...arguments);
    const relationships = {};

    // check relationships
    this.eachRelationship((name, meta) => {
      if (meta.kind === 'belongsTo') {
        if (this.get(`_data.${name}.id`) !== this.get(`${name}.id`)) {
          relationships[name] = [
            this.get(`_data.${name}.id`),
            this.get(`${name}.id`),
          ];
        }
      }
    });

    return Ember.merge(attributes, relationships);
  },
});

@BennyAlex
Copy link

@leifdejong thank you, this helped me alot

@BennyAlex
Copy link

@leifdejong why you check if meta.kind === 'belongsTo', what is with 'hasMany'?

@uhrohraggy
Copy link

@igorT Any reason this got closed? this is still a legitimate issue and something like changedRelationships is not available....

@atomkirk
Copy link

@leifdejong solution didn't work for me, so I modified it to get all up in those private apis bidness:

import DS from 'ember-data'

export default DS.Model.reopen({

  // this could break at the drop of a hat, so we'll want to test it thoroughly
  changedAttributes() {
    const attributes = this._super(...arguments)
    const relationships = {}

    // check relationships
    this.eachRelationship((name, meta) => {
      if (meta.kind === 'belongsTo') {

        let before = this.get(`_internalModel._relationships.initializedRelationships.${name}.canonicalState.id`)
        let now = this.get(`${name}.id`)

        if (before !== now) {
          relationships[name] = [before, now]
        }
      }
    })

    return Ember.merge(attributes, relationships)
  },

})

@Techn1x
Copy link

Techn1x commented Oct 25, 2017

Can confirm @leifdejong solution is broken for me too (was previously working though).

Really wish I didn't have to resort to using private api's for this sort of stuff...

@atomkirk's solution above works in most cases, although I did have an issue where on calling it the first time on a model that has been retreived through findRecord, with no changes made, it would return something like;
{company: [undefined, "a9bd3ba7-9c34-428b-a9a0-59751a319940"]}
for my company belongsTo relation.

But then if I called model.changedRelationships() again, it would then correctly show
{}

So not sure what's up there. Stepping through the code it seems that the line that sets now modifies the result the next time you go to set before

This solution also exists (but I haven't tried it)
https://github.com/isleofcode/ember-changed-relationships/blob/master/addon/mixins/changed-relationships.js

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

8 participants