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

belongsTo changes not set isDirty to true #1188

Closed
OpakAlex opened this issue Sep 3, 2013 · 14 comments
Closed

belongsTo changes not set isDirty to true #1188

OpakAlex opened this issue Sep 3, 2013 · 14 comments

Comments

@OpakAlex
Copy link

OpakAlex commented Sep 3, 2013

I have next case:

App.User = DS.Model.extend
  name: DS.attr('string')
  address: DS.belongsTo('address')

When I have user model with some address, then change this address to another or null, and check model.get('isDirty') #=> false
Why?

@wycats
Copy link
Member

wycats commented Sep 3, 2013

We could make this work, but keep in mind that .save() doesn't check whether something is dirty before sending it on, so this isn't a major issue at the moment.

I plan to do some refactoring of relationships after beta2 that should get this kind of stuff in shape.

@wycats wycats closed this as completed Sep 3, 2013
@nragaz
Copy link
Contributor

nragaz commented Sep 4, 2013

Could I suggest keeping this issue open, to make sure that it doesn't get resolved in a later beta? It's actually a bit of an issue if, e.g. you're checking isDirty to enable a Save button on a form.

@ralfschimmel
Copy link

Any progress on this?

This is still an issue and in my opinion it is a bug preventing correct implementation of the use case described by @nragaz.

Working with ember 1.2.0 and data beta.3.

@markprzepiora
Copy link
Contributor

As an imperfect workaround, I've been using this pattern in my models:

App.Post = DS.Model.extend({
  link:  DS.belongsTo('link'),
  photo: DS.belongsTo('photo'),

  isDirtyWithAssociations: Ember.computed.or('isDirty', 'hasDirtyAssociations'),

  dirtyAssociations: (function() {
    this.set('hasDirtyAssociations', true);
  }).observes('link', 'photo'),

  clearDirtyAssociations: (function() {
    this.set('hasDirtyAssociations', false);
  }).on('didUpdate', 'didCreate')
});

Unfortunately, this will "dirty" the record whenever the relation is set, whether the relation has actually changed or not. So it's not perfect, but it works well enough for my use case.

@chopper
Copy link

chopper commented Jan 2, 2014

@markprzepiora: That is how I've been handling this issue, too. However the observer also fires when the data is loaded the first time, which is annoying. Somehow isLoaded is true and isLoading is false, so I'm not sure how to handle this case. Have you had this issue and if so have you found a solution?

@simonexmachina
Copy link
Contributor

Just hit this issue myself. Definitely think this is unexpected behaviour. Here's a JSBin that I just created to isolate the issue, and IMHO this isn't how it should work:

http://emberjs.jsbin.com/jaxoriki/1/edit

@GetContented
Copy link

I just hit this issue again, too... have to write an observer against all the BT assocs in my model. (I'm only saving when an object is dirty because I don't want to hit the server for every single object save I do when saivng a tree of objects). FWIW

@nojacko
Copy link

nojacko commented May 9, 2014

I'm with you guys on this. I'm using the isDirty to decide wether to rollback changes when a user cancels. If you change where a relationship points to the model has changed and should be dirty.

@duereg
Copy link
Contributor

duereg commented Jun 3, 2014

Still a bug in beta.7. Would love to see this fixed.

@mehulkar
Copy link

mehulkar commented Jun 4, 2014

dirtiness based on changes to relationships is a pretty complicated issue as far as I can tell. I think there's been a lot of discussions around this in the past year and I'm no longer sure what correct/expected behavior is/should be.

I've been out of the loop for a while, so I'm not sure what the current conversation is, but I assume it's related to the conversation around embedded records as well.

I'm pretty happy to leave isDirty to be concerned only with changes to DS.attr properties, and nothing else.

@duereg
Copy link
Contributor

duereg commented Jun 4, 2014

The issue is complicated. There are many different ways to interpret what "isDirty" could mean.

But if you have models like so:

var Tag = DS.Model.extend({
    name: DS.attr('string'),
    person: DS.belongsTo('person')
  });

  var Person = DS.Model.extend({
    name: DS.attr('string'),
    tags: DS.hasMany('tag')
  });

And you then did something like this:

tag.get('person'); //returns null
tag1.set('person', thatGuy); //set person on tag

I think most people would expect tag1 to be dirty at that point.

I think the complication to this issue comes from whether or not isDirty checks whether the dependent objects are also dirty. IMO, that's unnecessary, but I see the point for debate.

@simonexmachina
Copy link
Contributor

I agree with @duereg here: I think that we should deliver the expected behaviour, and that isDirty shouldn't consider other dependant objects.

@ryanjm
Copy link

ryanjm commented Aug 23, 2014

@markprzepiora have you found a better solution to this yet? Anyone else?

My objects are being marked hasDirtyAssociations when getting reloaded from the server.

@timrwood
Copy link

I've opened an rfc for this issue here. Please weigh in.

emberjs/rfcs#21

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