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

Deleted records are not accessible #1737

Closed
simonweil opened this issue Feb 6, 2014 · 24 comments
Closed

Deleted records are not accessible #1737

simonweil opened this issue Feb 6, 2014 · 24 comments

Comments

@simonweil
Copy link

When running deleteRecord() the record is removed from the content of the controller and therefore can't access the record to run the save() command to persist the deletion.

Another issue I came across is that after deleting the record I have no indication that I have records that need persisting if I do now wish to persist them right after I run the deleteRecord() command.

Is this really a problem or am I missing something?

@jherdman
Copy link

jherdman commented Feb 7, 2014

I'm not sure. Let's dig into this a bit, I may be experiencing a related issue. Here's an action in a route for deleting some record:

// In some route...
actions: {
  deletePost: function (post) {
    post.deleteRecord();
    post.save().then(
      function () {
        console.log("I succeeded! I'm removed from the store, and the API is happy.");
      },
      function () {
        console.err("Oh snap! The API is angry. I'm not in the store though!");
        console.err("isDeleted:", post.get('isDeleted'), "; isDirty:", post.get('isDirty'));
        post.rollback();
        console.err("Oh snap! I'm still not in the store.");
        console.err("isDeleted:", post.get('isDeleted'), "; isDirty:", post.get('isDirty'));
      }
  }
}

When you model.deleteRecord(), you only mark the record as ready to delete — you must model.save() to persist the delete.

What I'm finding is that you can't rollback a delete. Is that what your issue is too?

@stefanpenner
Copy link
Member

@jherdman can you show where u are creating and assigning to the model variable?

@jherdman
Copy link

jherdman commented Feb 7, 2014

Oops! That was a mistake in my code above. I've edited it to read "post" instead of model. My bad.

@simonweil
Copy link
Author

The main issue for me is that I do not want to persist the record right away but only when the user explicitly requests it but at that point the record isn't accessible in any way I found...

I can't even check if a record is waiting to be persisted (for deletion) as the record is gone.

The only way I found was to keep my own indication but that is an ugly workaround...

@jherdman
Copy link

jherdman commented Feb 8, 2014

Hmm... We definitely have separate issues then. FYI you can accomplish your usage like this:

actions: {
  deleteRecord: function (myRecord) {
    myRecord.deleteRecord();
    if (confirm("Are you sure about this?")) {
      myRecord.save(); // persisting happens here
    }
  }
}

@simonweil
Copy link
Author

The use case may be different but the root cause seems the same to me.

I'm aware of this way of implementation but in my case the confirmation needs to be in a latter interaction with the user and not right away.

@bradleypriest
Copy link
Member

@simonweil it sounds like for your use-case you should be using a custom flag on the object to mark it for deletion rather than actually deleting in the first instance and then on confirm actually perform the deletion?

@simonweil
Copy link
Author

This solution would work but it's got a big downside, every place I present the content the deleted records would need to be filtered out. This is quite ugly IMO.

And anyway, EmberData holds this info, it's just inaccessible to me, so it's a workaroumd and not a very nice one...

What I think of doing is keeping some sort of reference to the record for usage later - a solution I don't really like either.

@bradleypriest
Copy link
Member

@simonweil yep, you'll need to filter but that kinda comes with your application by the sounds of it.
In my head if you're not deleting the records straight away, you're not really deleting the records, just marking them.

I'm gonna close this ticket, @jherdman yours is a separate issue, can you see if there is an issue open or create one otherwise.

Cheers.

@simonweil
Copy link
Author

It's up to you, but IMHO this is a general issue that needs addressing.

What is the purpose of deleteRecord if the save must happen right after?
If this is the case then destroyRecord is the only real option to use.

If there is a record "marked" for deletion but needs another step to actually delete it, then there must be a way to reach the record otherwise as I wrote it's an option that is not usable.

Please reconsider.

@bradleypriest
Copy link
Member

IMO it's a rare case where you're not deleting and saving within the same action.
To be fair in my app there is a place where I'm pushing an item into 'deletedItems' before calling deleteRecord for a very similar reason, however I felt that was a reasonable workaround.

I'm reopening, I'd love to hear some thoughts on the API

@bradleypriest bradleypriest reopened this Feb 11, 2014
@jaaksarv
Copy link
Contributor

I have many use cases where delete is happening before save. For example I open up a dialog for editing or creating one object that has bunch of sub-objects. User can change name of the main object, add and delete sub-objects, change sub-object names in one dialog. Then user can either click "Save" or "Cancel". This is very common pattern in many admin UI-s.
On Save I also save deleted sub-objects. On cancel everything will get rolled back. I keep deleted sub-objects in separate deletedSubObjects array so I know which deletes need to be persisted with "save" command.
Downside is that I have to manually track deleted sub-objects. I also have separate logic for sub-objects that are created and then deleted in same transaction as they don's need to be save-d (or rolled back). But it's not so big work and I hard to come up with better solution that fits everyone needs. So I'm happy with current situation.

@timohofmeijer
Copy link

The docs also state that a deleted record can be rolled back:

https://github.com/emberjs/data/blob/v1.0.0-beta.6/packages/ember-data/lib/system/model/model.js#L514

/**
    Marks the record as deleted but does not save it. You must call
    `save` afterwards if you want to persist it. You might use this
    method if you want to allow the user to still `rollback()` a
    delete after it was made.

    Example

    ```javascript
    App.ModelDeleteRoute = Ember.Route.extend({
      actions: {
        softDelete: function() {
          this.get('model').deleteRecord();
        },
        confirm: function() {
          this.get('model').save();
        },
        undo: function() {
          this.get('model').rollback();
        }
      }
    });
    ```

    @method deleteRecord
  */

@ericop
Copy link

ericop commented May 29, 2014

I'm excited to see there is progress on this issue as it seems anything that you can save() you should be able rollback, as you guys have mentioned.

@duereg
Copy link
Contributor

duereg commented Jul 22, 2014

Any progress on this?

@cjc343
Copy link

cjc343 commented Nov 5, 2014

+1 for Save/Cancel deleteRecord use case

It's not a question of whether I can track deleted records, it's a question of whether I should need to track deleted records.

New and modified records are tracked for me and I call save when necessary. Why should deleted records be so different that I need to track pending deletions myself?

@quiaro
Copy link

quiaro commented Dec 1, 2014

+1

@igorT igorT mentioned this issue Dec 2, 2014
28 tasks
@glagola
Copy link

glagola commented Mar 26, 2015

+1

@averydev
Copy link

averydev commented Jun 9, 2015

+1

1 similar comment
@patternleaf
Copy link

+1

@dylanmensaert
Copy link

Any use case that does not persist records immediately can benefit from this feature.

@wojciech-wieronski
Copy link

+1

@wecc
Copy link
Contributor

wecc commented Aug 20, 2015

Closing this since #3539 has been merged 🎉

This is going to be available (as a breaking change) in Ember Data 2.0. Details will be explained in the upcoming ED 2.0 blog post.

@wecc wecc closed this as completed Aug 20, 2015
@simonweil
Copy link
Author

Great news, thank you :)

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