Rebase and test cleanup for @drogus work on keeping records after del…

…eteRecord - Removed disconnectRelationship method calls from model/states.js - Removed reconnectRelationships from modle/internal-model.js - Remove one-sided disconnect and reconnect relationship methods - Removed method definitions in internal-model for: - disconnectRelationships - reconnectRelationships - Removed method definitions in relationship for: - disconnect - reconnect - addRecordToInverse - Fixed incorrect tests
emberjs · Jun 7, 2015 · 7b44c3e · 7b44c3e
1 parent ca89980
commit 7b44c3e
Show file tree

Hide file tree

Showing 10 changed files with 42 additions and 102 deletions.
diff --git a/packages/ember-data/lib/system/model/internal-model.js b/packages/ember-data/lib/system/model/internal-model.js
@@ -335,7 +335,6 @@ InternalModel.prototype = {
     if (this.isDeleted()) {
       //TODO: Should probably move this to the state machine somehow
       this.becameReady();
-      this.reconnectRelationships();
     }
 
     if (this.isNew()) {
@@ -445,7 +444,6 @@ InternalModel.prototype = {
     this.eachRelationship(function(name, relationship) {
       if (this._relationships.has(name)) {
         var rel = this._relationships.get(name);
-        //TODO(Igor) figure out whether we want to clear or disconnect
         rel.clear();
         rel.destroy();
       }
@@ -457,26 +455,6 @@ InternalModel.prototype = {
     });
   },
 
-  disconnectRelationships: function() {
-    this.eachRelationship(function(name, relationship) {
-      this._relationships.get(name).disconnect();
-    }, this);
-    var model = this;
-    forEach.call(Ember.keys(this._implicitRelationships), function(key) {
-      model._implicitRelationships[key].disconnect();
-    });
-  },
-
-  reconnectRelationships: function() {
-    this.eachRelationship(function(name, relationship) {
-      this._relationships.get(name).reconnect();
-    }, this);
-    var model = this;
-    forEach.call(Ember.keys(this._implicitRelationships), function(key) {
-      model._implicitRelationships[key].reconnect();
-    });
-  },
-
 
   /**
     When a find request is triggered on the store, the user can optionally pass in

diff --git a/packages/ember-data/lib/system/model/states.js b/packages/ember-data/lib/system/model/states.js
@@ -322,7 +322,6 @@ var DirtyState = {
     // EVENTS
     deleteRecord: function(internalModel) {
       internalModel.transitionTo('deleted.uncommitted');
-      internalModel.disconnectRelationships();
     },
 
     didSetProperty: function(internalModel, context) {
@@ -410,7 +409,6 @@ var updatedState = dirtyState({
 });
 
 createdState.uncommitted.deleteRecord = function(internalModel) {
-  internalModel.disconnectRelationships();
   internalModel.transitionTo('deleted.saved');
   internalModel.send('invokeLifecycleCallbacks');
 };
@@ -435,7 +433,6 @@ updatedState.inFlight.unloadRecord = assertAgainstUnloadRecord;
 
 updatedState.uncommitted.deleteRecord = function(internalModel) {
   internalModel.transitionTo('deleted.uncommitted');
-  internalModel.disconnectRelationships();
 };
 
 var RootState = {
@@ -572,7 +569,6 @@ var RootState = {
 
       deleteRecord: function(internalModel) {
         internalModel.transitionTo('deleted.uncommitted');
-        internalModel.disconnectRelationships();
       },
 
       unloadRecord: function(internalModel) {

diff --git a/packages/ember-data/lib/system/record-array-manager.js b/packages/ember-data/lib/system/record-array-manager.js
@@ -61,8 +61,7 @@ export default Ember.Object.extend({
   updateRecordArrays: function() {
     forEach(this.changedRecords, function(record) {
       // TODO: it can be refactored after #2862 && #2859 are closed
-      if (get(record, 'isDestroyed') || get(record, 'isDestroying') ||
-          (get(record, 'isDeleted') && !get(record, 'isDirty'))) {
+      if (get(record, 'record.isDestroyed') || get(record, 'record.isDestroying') || (record.isDeleted() && !record.isDirty())) {
         this._recordWasDeleted(record);
       } else {
         this._recordWasChanged(record);

diff --git a/packages/ember-data/lib/system/relationships/state/relationship.js b/packages/ember-data/lib/system/relationships/state/relationship.js
@@ -33,18 +33,6 @@ Relationship.prototype = {
     }
   },
 
-  disconnect: function() {
-    this.members.forEach(function(member) {
-      this.removeRecordFromInverse(member);
-    }, this);
-  },
-
-  reconnect: function() {
-    this.members.forEach(function(member) {
-      this.addRecordToInverse(member);
-    }, this);
-  },
-
   removeRecords: function(records) {
     var self = this;
     forEach(records, function(record) {
@@ -142,12 +130,6 @@ Relationship.prototype = {
     }
   },
 
-  addRecordToInverse: function(record) {
-    if (this.inverseKey) {
-      record._relationships.get(this.inverseKey).addRecord(this.record);
-    }
-  },
-
   removeRecordFromInverse: function(record) {
     var inverseRelationship = record._relationships.get(this.inverseKey);
     //Need to check for existence, as the record might unloading at the moment

diff --git a/packages/ember-data/tests/integration/records/delete-record-test.js b/packages/ember-data/tests/integration/records/delete-record-test.js
@@ -36,15 +36,15 @@ test("records should not be removed from record arrays just after deleting, but
   var all  = env.store.all('person');
 
   // pre-condition
-  equal(all.get('length'), 2, 'expected 2 records');
+  equal(all.get('length'), 2, 'pre-condition: 2 records in array');
 
   Ember.run(adam, 'deleteRecord');
 
-  equal(all.get('length'), 2, 'expected 2 record');
+  equal(all.get('length'), 2, '2 records in array after deleteRecord');
 
   Ember.run(adam, 'save');
 
-  equal(all.get('length'), 1, 'expected 1 record');
+  equal(all.get('length'), 1, '1 record in array after deleteRecord and save');
 });
 
 test("records can be deleted during record array enumeration", function () {

diff --git a/packages/ember-data/tests/integration/relationships/has-many-test.js b/packages/ember-data/tests/integration/relationships/has-many-test.js
@@ -1322,7 +1322,7 @@ test("adding and removing records from hasMany relationship #2666", function() {
         return comments.get('lastObject').destroyRecord();
       }).then(function() {
         var comments = post.get('comments');
-        equal(comments.get('length'), 3, "Comments count after delete");
+        equal(comments.get('length'), 3, "Comments count after destroy");
 
         // Add another comment #4
         var comment = env.store.createRecord('comment');

diff --git a/packages/ember-data/tests/integration/relationships/many-to-many-test.js b/packages/ember-data/tests/integration/relationships/many-to-many-test.js
@@ -171,10 +171,10 @@ test("Removing a record from a hasMany reflects on the other hasMany side - sync
 });
 
 /*
-Deleting tests
+Deleting
 */
 
-test("Deleting a record that has a hasMany relationship removes it from the otherMany array but does not remove the other record from itself - async", function () {
+test("Deleting a record that has a hasMany relationship does not remove it from the otherMany array and does not remove the other record from itself - async", function () {
   var user, topic;
   run(function() {
     user = store.push('user', { id: 1, name: 'Stanley', topics: [2] });
@@ -183,24 +183,23 @@ test("Deleting a record that has a hasMany relationship removes it from the othe
   run(topic, 'deleteRecord');
   run(function() {
     topic.get('users').then(async(function(fetchedUsers) {
-      equal(fetchedUsers.get('length'), 1, 'Users are still there');
+      equal(fetchedUsers.objectAt(0), user, 'Topic still has the user');
     }));
     user.get('topics').then(async(function(fetchedTopics) {
-      equal(fetchedTopics.get('length'), 0, 'Topic got removed from the user');
-      equal(fetchedTopics.objectAt(0), null, "Topic can't be fetched");
+      equal(fetchedTopics.objectAt(0), topic, 'User still has the topic');
     }));
   });
 });
 
-test("Deleting a record that has a hasMany relationship removes it from the otherMany array but does not remove the other record from itself - sync", function () {
+test("Deleting a record that has a hasMany relationship does not remove it from the otherMany array and does not remove the other record from itself - sync", function () {
   var account, user;
   run(function() {
     account = store.push('account', { id: 2 , state: 'lonely' });
     user = store.push('user', { id: 1, name: 'Stanley', accounts: [2] });
   });
   run(account, 'deleteRecord');
-  equal(account.get('users.length'), 1, 'Users are still there');
-  equal(user.get('accounts.length'), 0, 'Acocount got removed from the user');
+  equal(account.get('users').objectAt(0), user, 'Account still has the user');
+  equal(user.get('accounts').objectAt(0), account, 'User still has the account');
 });
 
 /*

diff --git a/packages/ember-data/tests/integration/relationships/one-to-many-test.js b/packages/ember-data/tests/integration/relationships/one-to-many-test.js
@@ -386,7 +386,7 @@ test("Setting the belongsTo side to null removes the record from the hasMany sid
 Deleting
 */
 
-test("When deleting a record that has a belongsTo it is removed from the hasMany side but not the belongsTo side- async", function () {
+test("Deleting a record that has a belongsTo does not remove it from the hasMany side and does not remove the other record from itself - async", function () {
   var user, message;
   run(function() {
     user = store.push('user', { id: 1, name: 'Stanley', messages: [2] });
@@ -398,24 +398,23 @@ test("When deleting a record that has a belongsTo it is removed from the hasMany
       equal(fetchedUser, user, 'Message still has the user');
     });
     user.get('messages').then(function(fetchedMessages) {
-      equal(fetchedMessages.get('length'), 0, 'User was removed from the messages');
-      equal(fetchedMessages.get('firstObject'), null, "Message can't be accessed");
+      equal(fetchedMessages.objectAt(0), message, 'User still has the message');
     });
   });
 });
 
-test("When deleting a record that has a belongsTo it is removed from the hasMany side but not the belongsTo side- sync", function () {
+test("Deleting a record that has a belongsTo does not remove it from the hasMany side and does not remove the other record from itself - sync", function () {
   var account, user;
   run(function() {
     account = store.push('account', { id: 2 , state: 'lonely' });
     user = store.push('user', { id: 1, name: 'Stanley', accounts: [2] });
     account.deleteRecord();
   });
-  equal(user.get('accounts.length'), 0, "User was removed from the accounts");
+  equal(user.get('accounts').objectAt(0), account, 'User still has the account');
   equal(account.get('user'), user, 'Account still has the user');
 });
 
-test("When deleting a record that has a hasMany it is removed from the belongsTo side but not the hasMany side- async", function () {
+test("Deleting a record that has a hasMany does not remove from the belongsTo side and does not remove the other record from itself - async", function () {
   var user, message;
   run(function() {
     user = store.push('user', { id: 1, name: 'Stanley', messages: [2] });
@@ -424,15 +423,15 @@ test("When deleting a record that has a hasMany it is removed from the belongsTo
   run(user, 'deleteRecord');
   run(function() {
     message.get('user').then(function(fetchedUser) {
-      equal(fetchedUser, null, 'Message does not have the user anymore');
+      equal(fetchedUser, user, 'Message still has the user');
     });
     user.get('messages').then(function(fetchedMessages) {
-      equal(fetchedMessages.get('length'), 1, 'User still has the messages');
+      equal(fetchedMessages.objectAt(0), message, 'User still has the message');
     });
   });
 });
 
-test("When deleting a record that has a hasMany it is removed from the belongsTo side but not the hasMany side - sync", function () {
+test("Deleting a record that has a hasMany does not remove from the belongsTo side and does not remove the other record from itself - sync", function () {
   var account, user;
   run(function() {
     account = store.push('account', { id: 2 , state: 'lonely' });
@@ -441,8 +440,8 @@ test("When deleting a record that has a hasMany it is removed from the belongsTo
   run(function() {
     user.deleteRecord();
   });
-  equal(user.get('accounts.length'), 1, "User still has the accounts");
-  equal(account.get('user'), null, 'Account no longer has the user');
+  equal(user.get('accounts').objectAt(0), account, 'User still has the account');
+  equal(account.get('user'), user, 'Account still has the user');
 });
 
 /*

diff --git a/packages/ember-data/tests/integration/relationships/one-to-one-test.js b/packages/ember-data/tests/integration/relationships/one-to-one-test.js
@@ -303,37 +303,27 @@ test("Setting a belongsTo to a different record, sets the old relationship to nu
 });
 
 /*
-Deleting tests
+Deleting
 */
 
-test("When deleting a record that has a belongsTo relationship, the record is removed from the inverse but still has access to its own relationship - async", function () {
-  // This observer is here to make sure that inverseRecord gets cleared, when
-  // the record is deleted, before notifyRecordRelationshipRemoved() and in turn
-  // notifyPropertyChange() gets called. If not properly cleared observers will
-  // trigger with the old value of the relationship.
-  User.reopen({
-    bestFriendObserver: Ember.observer('bestFriend', function() {
-      this.get('bestFriend');
-    })
-  });
+test("Deleting a record that has a belongsTo relationship does not remove the record from the inverse and still has access to its own relationship - async", function () {
   var stanleysFriend, stanley;
-
   run(function() {
     stanleysFriend = store.push('user', { id: 2, name: "Stanley's friend" });
     stanley = store.push('user', { id: 1, name: 'Stanley', bestFriend: 2 });
   });
   run(function() {
     stanley.deleteRecord();
     stanleysFriend.get('bestFriend').then(function(fetchedUser) {
-      equal(fetchedUser, null, 'Stanley got removed');
+      equal(fetchedUser, stanley, 'Stanleys friend still has Stanley');
     });
     stanley.get('bestFriend').then(function(fetchedUser) {
-      equal(fetchedUser, stanleysFriend, 'Stanleys friend did not get removed');
+      equal(fetchedUser, stanleysFriend, 'Stanley still has Stanleys friend');
     });
   });
 });
 
-test("When deleting a record that has a belongsTo relationship, the record is removed from the inverse but still has access to its own relationship - sync", function () {
+test("Deleting a record that has a belongsTo relationship does not remove the record from the inverse and still has access to its own relationship - sync", function () {
   var job, user;
   run(function() {
     job = store.push('job', { id: 2 , isGood: true });
@@ -342,7 +332,7 @@ test("When deleting a record that has a belongsTo relationship, the record is re
   run(function() {
     job.deleteRecord();
   });
-  equal(user.get('job'), null, 'Job got removed from the user');
+  equal(user.get('job'), job, 'User still has the job');
   equal(job.get('user'), user, 'Job still has the user');
 });
 

diff --git a/packages/ember-data/tests/unit/record-array-test.js b/packages/ember-data/tests/unit/record-array-test.js
@@ -79,8 +79,8 @@ test("stops updating when destroyed", function() {
 });
 
 
-test("a loaded record is removed from a record array when it is deleted", function() {
-  expect(4);
+test("a loaded record is not removed from a record array when it is deleted", function() {
+  expect(5);
 
   var Tag = DS.Model.extend({
     people: DS.hasMany('person')
@@ -123,12 +123,13 @@ test("a loaded record is removed from a record array when it is deleted", functi
 
       scumbag.deleteRecord();
 
-      equal(get(recordArray, 'length'), 0, "record is removed from the record array");
+      equal(get(recordArray, 'length'), 1, "record array still has one item");
+      equal(get(recordArray.objectAt(0), 'name'), "Scumbag Dale", "item at index 0 is record with id 1");
     });
   });
 });
 
-test("a loaded record is removed from a record array when it is deleted even if the belongsTo side isn't defined", function() {
+test("a loaded record is not removed from a record array when it is deleted even if the belongsTo side isn't defined", function() {
   var Tag = DS.Model.extend({
     people: DS.hasMany('person')
   });
@@ -146,10 +147,11 @@ test("a loaded record is removed from a record array when it is deleted even if
     scumbag.deleteRecord();
   });
 
-  equal(tag.get('people.length'), 0, "record is removed from the record array");
+  equal(tag.get('people.length'), 1, 'record is not removed from the record array');
+  equal(tag.get('people').objectAt(0), scumbag, 'tag still has the scumbag');
 });
 
-test("a loaded record is removed both from the record array and from the belongs to, even if the belongsTo side isn't defined", function() {
+test("a loaded record is not removed from both the record array and from the belongs to, even if the belongsTo side isn't defined", function() {
   var Tag = DS.Model.extend({
     people: DS.hasMany('person')
   });
@@ -179,12 +181,12 @@ test("a loaded record is removed both from the record array and from the belongs
     scumbag.deleteRecord();
   });
 
-  equal(tag.get('people.length'), 0, "record is removed from the record array");
-  equal(tool.get('person'), null, "the tool is now orphan");
+  equal(tag.get('people.length'), 1, "record is stil in the record array");
+  equal(tool.get('person'), scumbag, "the tool still belongs to the record");
 });
 
 // GitHub Issue #168
-test("a newly created record is removed from a record array when it is deleted", function() {
+test("a newly created record is not removed from a record array when it is deleted", function() {
   var store = createStore({
     person: Person
   });
@@ -207,19 +209,14 @@ test("a newly created record is removed from a record array when it is deleted",
   });
 
   equal(get(recordArray, 'length'), 4, "precond - record array has the created item");
-  equal(get(recordArray.objectAt(0), 'name'), "Scumbag Dale", "item at index 0 is record with id 1");
+  equal(recordArray.objectAt(0), scumbag, "item at index 0 is record with id 1");
 
   run(function() {
     scumbag.deleteRecord();
   });
 
-  equal(get(recordArray, 'length'), 3, "record is removed from the record array");
-
-  run(function() {
-    recordArray.objectAt(0).set('name', 'toto');
-  });
-
-  equal(get(recordArray, 'length'), 3, "record is still removed from the record array");
+  equal(get(recordArray, 'length'), 4, "record array still has the created item");
+  equal(recordArray.objectAt(0), scumbag, "item at index 0 is still record with id 1");
 });
 
 test("a record array returns undefined when asking for a member outside of its content Array's range", function() {