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

Dont remove deleted but uncommited records from record arrays #3247

Conversation

ianstarz
Copy link
Contributor

@ianstarz ianstarz commented Jun 7, 2015

Before this commit records were deleted from record arrays just after
deleting them, but before they were committed. This can be an issue if
you want to keep records visible in the UI until they're actually
removed.

@@ -60,7 +60,8 @@ export default Ember.Object.extend({
*/
updateRecordArrays: function() {
forEach(this.changedRecords, function(record) {
if (record.isDeleted()) {
// TODO: it can be refactored after #2862 && #2859 are closed
if (get(record, 'record.isDestroyed') || get(record, 'record.isDestroying') || (record.isDeleted() && !record.isDirty())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igor @drogus I'm still having trouble figuring out how to form this conditional, but only 4 failing tests now...

@ianstarz
Copy link
Contributor Author

ianstarz commented Jun 7, 2015

There's still 4 failing tests for this, but otherwise a lot of the work to rebase and cleanup/fix other tests is done. Taking a break now, so I just wanted to open this and update you guys.

screen shot 2015-06-07 at 15 30 33

@ianstarz
Copy link
Contributor Author

ianstarz commented Jun 7, 2015

This is the updated PR for the work done in #2867

drogus and others added 2 commits June 15, 2015 19:25
Before this commit records were deleted from record arrays just after
deleting them, but before they were commited. This can be an issue if
you want to keep records visible in the UI until they're actually
removed.
…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
@ianstarz ianstarz force-pushed the dont-remove-deleted-but-uncommited-records-from-record-arrays branch from 7b44c3e to a8599de Compare June 16, 2015 01:28

// drogus' attempt
// if (get(record, 'record.isDestroyed') || get(record, 'record.isDestroying') || (record.isDeleted() && !record.isDirty())) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igorT here is where I was talking about

@bmac
Copy link
Member

bmac commented Jul 13, 2015

Hi @ianstarz. Thanks for this work. I ended up taking a stab at this too. #3539

For the updateRecordArrays code in record-array-manager.js I ended up checking if the record was in the root.deleted.saved state to determine if the record should be removed from the record array.

I also left the disconnectRelationships/reconnectRelationships on the model and moved the places where it was called such that it is now only called when the record transitions into the deleted.saved state.

@ianstarz
Copy link
Contributor Author

@bmac it seemed like inspecting the state path was the right way to go. Thanks for picking this up! I'm still getting familiar with the code base, and was feeling pretty stuck.

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

Successfully merging this pull request may close these issues.

3 participants