From 75e5638c50f72df34f2c47262c3d9215d86962a9 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Mon, 30 Apr 2018 15:46:58 -0700 Subject: [PATCH] [BUGFIX] destroyRecord should also unload, to avoid unload use deleteRecord + save --- .../system/model/internal-model.js | 11 ++-- addon/-legacy-private/system/store.js | 20 +++--- .../system/model/model.js | 4 +- .../integration/records/delete-record-test.js | 62 +++++++------------ .../relationships/has-many-test.js | 21 +------ tests/unit/model-test.js | 6 +- 6 files changed, 44 insertions(+), 80 deletions(-) diff --git a/addon/-legacy-private/system/model/internal-model.js b/addon/-legacy-private/system/model/internal-model.js index 0f4f7968950..91bade1df90 100644 --- a/addon/-legacy-private/system/model/internal-model.js +++ b/addon/-legacy-private/system/model/internal-model.js @@ -1197,12 +1197,13 @@ export default class InternalModel { @method adapterDidCommit */ adapterDidCommit(data) { + let store = this.store; + if (data) { - this.store._internalModelDidReceiveRelationshipData( - this.modelName, - this.id, - data.relationships - ); + // normalize relationship IDs into records + store.updateId(this, data); + store._setupRelationshipsForModel(this, data); + store._internalModelDidReceiveRelationshipData(this.modelName, this.id, data.relationships); data = data.attributes; } diff --git a/addon/-legacy-private/system/store.js b/addon/-legacy-private/system/store.js index e9612191bdd..7766ec46389 100644 --- a/addon/-legacy-private/system/store.js +++ b/addon/-legacy-private/system/store.js @@ -1926,20 +1926,14 @@ Store = Service.extend({ if (dataArg) { data = dataArg.data; } - if (data) { - // normalize relationship IDs into records - this.updateId(internalModel, data); - this._setupRelationshipsForModel(internalModel, data); - } else { - assert( - `Your ${ - internalModel.modelName - } record was saved to the server, but the response does not have an id and no id has been set client side. Records must have ids. Please update the server response to provide an id in the response or generate the id on the client side either before saving the record or while normalizing the response.`, - internalModel.id - ); - } - //We first make sure the primary data has been updated + assert( + `Your ${ + internalModel.modelName + } record was saved to the server, but the response does not have an id and no id has been set client side. Records must have ids. Please update the server response to provide an id in the response or generate the id on the client side either before saving the record or while normalizing the response.`, + internalModel.id || (data && data.id) + ); + //TODO try to move notification to the user to the end of the runloop internalModel.adapterDidCommit(data); }, diff --git a/addon/-record-data-private/system/model/model.js b/addon/-record-data-private/system/model/model.js index 772b6efa7ff..1f539a168f2 100644 --- a/addon/-record-data-private/system/model/model.js +++ b/addon/-record-data-private/system/model/model.js @@ -615,7 +615,9 @@ const Model = EmberObject.extend(Evented, { */ destroyRecord(options) { this.deleteRecord(); - return this.save(options); + return this.save(options).then(() => { + this.unloadRecord(); + }); }, /** diff --git a/tests/integration/records/delete-record-test.js b/tests/integration/records/delete-record-test.js index 0c09a99f3de..cdb14ad7680 100644 --- a/tests/integration/records/delete-record-test.js +++ b/tests/integration/records/delete-record-test.js @@ -133,27 +133,32 @@ test('deleting a record that is part of a hasMany removes it from the hasMany re assert.equal(person.get('name'), 'Adam Sunderland', 'expected related records to be loaded'); run(function() { - person.destroyRecord(); + person.deleteRecord(); + person.save(); }); assert.equal(group.get('people.length'), 1, 'expected 1 related records after delete'); }); -test('deleting a record that is part of a hasMany removes it from the hasMany recordArray when adapter deleteRecord function returns a payload', function(assert) { +test('We properly unload a record when destroyRecord is called', function(assert) { let group; - let person; const Group = DS.Model.extend({ - people: DS.hasMany('person', { inverse: null, async: false }) + name: attr(), }); - Group.toString = () => { return 'Group'; } + Group.toString = () => { + return 'Group'; + }; + env.adapter.shouldBackgroundReloadRecord = () => false; env.adapter.deleteRecord = function() { return EmberPromise.resolve({ data: { id: '1', type: 'group', - attributes: {} - } + attributes: { + name: 'Deleted Checkers', + }, + }, }); }; @@ -164,49 +169,23 @@ test('deleting a record that is part of a hasMany removes it from the hasMany re data: { type: 'group', id: '1', - relationships: { - people: { - data: [ - { type: 'person', id: '1' }, - { type: 'person', id: '2' } - ] - } - } - }, - included: [ - { - type: 'person', - id: '1', - attributes: { - name: 'Adam Sunderland' - } + attributes: { + name: 'Checkers', }, - { - type: 'person', - id: '2', - attributes: { - name: 'Dave Sunderland' - } - } - ] + }, }); group = env.store.peekRecord('group', '1'); - person = env.store.peekRecord('person', '1'); }); - // Sanity Check we are in the correct state. - assert.equal(group.get('people.length'), 2, 'expected 2 related records before delete'); - assert.equal(person.get('name'), 'Adam Sunderland', 'expected related records to be loaded'); + assert.equal(group.get('name'), 'Checkers', 'We have the right group'); const promise = run(() => group.destroyRecord()); return promise.then(() => { - // qunit complains if i pass the record to QUnit due to some assertion in ember - // iterating over the properties. - const hasGroup = env.store.peekRecord('group', '1') ? true : false; - assert.equal(hasGroup, false); - assert.equal(person.get('group'), null, 'expected relationship to be null'); + const deletedGroup = env.store.peekRecord('group', '1'); + + assert.equal(!!deletedGroup, false, 'expected to no longer have group 1'); }); }); @@ -246,7 +225,8 @@ test('records can be deleted during record array enumeration', function(assert) run(function() { all.forEach(function(record) { - record.destroyRecord(); + record.deleteRecord(); + record.save(); }); }); diff --git a/tests/integration/relationships/has-many-test.js b/tests/integration/relationships/has-many-test.js index 2a44b221369..c3c02bd2468 100644 --- a/tests/integration/relationships/has-many-test.js +++ b/tests/integration/relationships/has-many-test.js @@ -3626,30 +3626,15 @@ test('deleteRecord + unloadRecord fun', function(assert) { ]); return run(() => { - return env.store - .peekRecord('post', 'post-2') - .destroyRecord() - .then(record => { - return env.store.unloadRecord(record); - }); + return env.store.peekRecord('post', 'post-2').destroyRecord(); }) .then(() => { assert.deepEqual(posts.map(x => x.get('id')), ['post-1', 'post-3', 'post-4', 'post-5']); - return env.store - .peekRecord('post', 'post-3') - .destroyRecord() - .then(record => { - return env.store.unloadRecord(record); - }); + return env.store.peekRecord('post', 'post-3').destroyRecord(); }) .then(() => { assert.deepEqual(posts.map(x => x.get('id')), ['post-1', 'post-4', 'post-5']); - return env.store - .peekRecord('post', 'post-4') - .destroyRecord() - .then(record => { - return env.store.unloadRecord(record); - }); + return env.store.peekRecord('post', 'post-4').destroyRecord(); }) .then(() => { assert.deepEqual(posts.map(x => x.get('id')), ['post-1', 'post-5']); diff --git a/tests/unit/model-test.js b/tests/unit/model-test.js index 9a8c00c256c..b10d125f6ea 100644 --- a/tests/unit/model-test.js +++ b/tests/unit/model-test.js @@ -688,7 +688,8 @@ test('supports canonical updates via pushedData in root.deleted.saved', function ); run(() => { - record.destroyRecord().then(() => { + record.deleteRecord(); + record.save().then(() => { let currentState = record._internalModel.currentState; assert.ok( @@ -749,7 +750,8 @@ test('Does not support dirtying in root.deleted.saved', function(assert) { ); run(() => { - record.destroyRecord().then(() => { + record.deleteRecord(); + record.save().then(() => { let currentState = record._internalModel.currentState; assert.ok(