Record property changes

packages/ember-data/lib/system/model/internal-model.js

@@ -190,7 +190,8 @@ InternalModel.prototype = {
   },
 
   setupData: function(data) {
-    var changedKeys = mergeAndReturnChangedKeys(this._data, data);
+    var changedKeys = this._changedKeys(data);
+    merge(this._data, data);
     this.pushedData();
     if (this.record) {
       this.record._notifyProperties(changedKeys);
@@ -584,13 +585,12 @@ InternalModel.prototype = {
     @method adapterDidCommit
   */
   adapterDidCommit: function(data) {
-    var changedKeys;
     this.didCleanError();
+    var changedKeys = this._changedKeys(data);
 
+    merge(this._data, this._inFlightAttributes);
     if (data) {
-      changedKeys = mergeAndReturnChangedKeys(this._data, data);
-    } else {
-      merge(this._data, this._inFlightAttributes);
+      merge(this._data, data);
     }
 
     this._inFlightAttributes = Ember.create(null);
@@ -663,6 +663,80 @@ InternalModel.prototype = {
     this._inFlightAttributes = Ember.create(null);
   },
 
+  /**
+    @method _changedKeys
+
+    Ember Data has 3 buckets for storing the value of an attribute on an internalModel.
+
+    `_data` holds all of the attributes that have been acknowledged by
+    a backend via the adapter. When rollback is called on a model all
+    attributes will revert to the record's state in `_data`.
+
+    `_attributes` holds any change the user has made to an attribute
+    that has not been acknowledged by the adapter. Any values in
+    `_attributes` are have priority over values in `_data`.
+
+    `_inFlightAttributes`. When a record is being synced with the
+    backend the values in `_attributes` are copied to
+    `_inFlightAttributes`. This way if the backend acknowledges the
+    save but does not return the new state Ember Data can copy the
+    values from `_inFlightAttributes` to `_data`. Without having to
+    worry about changes made to `_attributes` while the save was
+    happenign.
+
+
+    Changed keys builds a list of all of the values that may have been
+    changed by the backend after a successful save.
+
+    It does this by iterating over each key, value pair in the payload
+    returned from the server after a save. If the `key` is found in
+    `_attributes` then the user has a local changed to the attribute
+    that has not been synced with the server and the key is not
+    included in the list of changed keys.
+
+
+
+    If the value, for a key differs from the value in what Ember Data
+    believes to be the truth about the backend state (A merger of the
+    `_data` and `_inFlightAttributes` objects where
+    `_inFlightAttributes` has priority) then that means the backend
+    has updated the value and the key is added to the list of changed
+    keys.
+
+    @private
+  */
+  _changedKeys: function(updates) {
+    var changedKeys = [];
+
+    if (updates) {
+      var original, i, value, key;
+      var keys = Ember.keys(updates);
+      var length = keys.length;
+
+      original = merge(Ember.create(null), this._data);
+      original = merge(original, this._inFlightAttributes);
+
+      for (i = 0; i < length; i++) {
+        key = keys[i];
+        value = updates[key];
+
+        // A value in _attributes means the user has a local change to
+        // this attributes. We never override this value when merging
+        // updates from the backend so we should not sent a change
+        // notification if the server value differs from the original.
+        if (this._attributes[key] !== undefined) {
+          continue;
+        }
+
+        if (!Ember.isEqual(original[key], value)) {
+          changedKeys.push(key);
+        }
+      }
+    }
+
+    return changedKeys;
+  },
+
   toString: function() {
     if (this.record) {
       return this.record.toString();
@@ -672,31 +746,5 @@ InternalModel.prototype = {
   }
 };
 
-// Like Ember.merge, but instead returns a list of keys
-// for values that fail a strict equality check
-// instead of the original object.
-function mergeAndReturnChangedKeys(original, updates) {
-  var changedKeys = [];
-
-  if (!updates || typeof updates !== 'object') {
-    return changedKeys;
-  }
-
-  var keys   = Ember.keys(updates);
-  var length = keys.length;
-  var i, val, key;
-
-  for (i = 0; i < length; i++) {
-    key = keys[i];
-    val = updates[key];
-
-    if (original[key] !== val) {
-      changedKeys.push(key);
-    }
-
-    original[key] = val;
-  }
-  return changedKeys;
-}
 
 export default InternalModel;

packages/ember-data/tests/integration/records/property-changes-test.js

@@ -0,0 +1,146 @@
+var env, store, Person;
+var attr = DS.attr;
+var run = Ember.run;
+
+module('integration/records/property-changes - Property changes', {
+  setup: function() {
+    Person = DS.Model.extend({
+      firstName: attr('string'),
+      lastName: attr('string')
+    });
+    Person.toString = function() { return 'Person'; };
+
+    env = setupStore({
+      person: Person
+    });
+    store = env.store;
+  },
+
+  teardown: function() {
+    Ember.run(function() {
+      env.container.destroy();
+    });
+  }
+});
+
+test('Calling push with partial records trigger observers for just those attributes that changed', function() {
+  expect(1);
+  var person;
+
+  run(function() {
+    person = store.push('person', {
+      id: 'wat',
+      firstName: 'Yehuda',
+      lastName: 'Katz'
+    });
+  });
+
+  person.addObserver('firstName', function() {
+    ok(false, 'firstName observer should not be triggered');
+  });
+
+  person.addObserver('lastName', function() {
+    ok(true, 'lastName observer should be triggered');
+  });
+
+  run(function() {
+    store.push('person', {
+      id: 'wat',
+      firstName: 'Yehuda',
+      lastName: 'Katz!'
+    });
+  });
+});
+
+test('Calling push does not trigger observers for locally changed attributes with the same value', function() {
+  expect(0);
+  var person;
+
+  run(function() {
+    person = store.push('person', {
+      id: 'wat',
+      firstName: 'Yehuda',
+      lastName: 'Katz'
+    });
+
+    person.set('lastName', 'Katz!');
+  });
+
+  person.addObserver('firstName', function() {
+    ok(false, 'firstName observer should not be triggered');
+  });
+
+  person.addObserver('lastName', function() {
+    ok(false, 'lastName observer should not be triggered');
+  });
+
+  run(function() {
+    store.push('person', {
+      id: 'wat',
+      firstName: 'Yehuda',
+      lastName: 'Katz!'
+    });
+  });
+});
+
+test('Saving a record trigger observers for locally changed attributes with the same canonical value', function() {
+  expect(1);
+  var person;
+
+  env.adapter.updateRecord = function(store, type, snapshot) {
+    return Ember.RSVP.resolve({ id: 'wat', lastName: 'Katz' });
+  };
+
+  run(function() {
+    person = store.push('person', {
+      id: 'wat',
+      firstName: 'Yehuda',
+      lastName: 'Katz'
+    });
+
+    person.set('lastName', 'Katz!');
+  });
+
+  person.addObserver('firstName', function() {
+    ok(false, 'firstName observer should not be triggered');
+  });
+
+  person.addObserver('lastName', function() {
+    ok(true, 'lastName observer should be triggered');
+  });
+
+  run(function() {
+    person.save();
+  });
+});
+
+test('store.push should not override a modified attribute', function() {
+  expect(1);
+  var person;
+
+  run(function() {
+    person = store.push('person', {
+      id: 'wat',
+      firstName: 'Yehuda',
+      lastName: 'Katz'
+    });
+
+    person.set('lastName', 'Katz!');
+  });
+
+  person.addObserver('firstName', function() {
+    ok(true, 'firstName observer should be triggered');
+  });
+
+  person.addObserver('lastName', function() {
+    ok(false, 'lastName observer should not be triggered');
+  });
+
+  run(function() {
+    person = store.push('person', {
+      id: 'wat',
+      firstName: 'Tom',
+      lastName: 'Dale'
+    });
+  });
+});

packages/ember-data/tests/unit/store/push-test.js

@@ -164,35 +164,6 @@ test("Calling push on normalize allows partial updates with raw JSON", function
   equal(person.get('lastName'), "Jackson", "existing fields are untouched");
 });
 
-test("Calling push with partial records triggers observers for just those attributes that changed", function() {
-  expect(1);
-  var person;
-
-  run(function() {
-    person = store.push('person', {
-      id: 'wat',
-      firstName: "Yehuda",
-      lastName: "Katz"
-    });
-  });
-
-  person.addObserver('firstName', function() {
-    ok(false, 'firstName observer should not be triggered');
-  });
-
-  person.addObserver('lastName', function() {
-    ok(true, 'lastName observer should be triggered');
-  });
-
-  run(function() {
-    store.push('person', {
-      id: 'wat',
-      firstName: 'Yehuda',
-      lastName: 'Katz!'
-    });
-  });
-});
-
 test("Calling push with a normalized hash containing related records returns a record", function() {
   var number1, number2, person;
   run(function() {