Meta Rollback Issue #1288

Closed
wycats opened this Issue Sep 13, 2013 · 23 comments

Comments

Projects
None yet
Owner

wycats commented Sep 13, 2013

  • Async and rollback (#1283)
  • Resetting after failure (#1271)
  • Rollback and invalid (#1354)

I'll be working on enhanced rollback semantics and tracking the originating issues here.

Thanks @wycats, looking forward

Member

bradleypriest commented Sep 13, 2013

Also very much looking forward to this.
This is how I'm rolling back at the moment.

Models:

App.Order = DS.Model.extend({
  lineItems: DS.hasMany('lineItems'),
  initialize: ( ->
    this.set('deletedLineItems', [])
  ).on('init')
  // Manually adding deleted line items to this array from the router.
})

App.LineItem = DS.Model.extend({
  order: DS.belongsTo('order'),
  deleteAndRemoveRecord: function() {
    var self = this;
    this.get('order.lineItems').then(function(collection) {
      collection.removeObject(self);
      self.deleteRecord();
    });
  }
})

Rollback

record.get('lineItems').then(function(items) {
  if (record.get('isDirty')) {
    record.rollback();
  }
  items.filterProperty('isNew').invoke('deleteAndRemoveRecord');
  items.rejectProperty('isNew').filterProperty('isDirty').invoke('rollback');
  deletedLineItems = record.get('deletedLineItems');
  deletedLineItems.invoke('rollback');
  record.set('deletedLineItems', []);
  items.pushObjects(deletedLineItems);
  itemIds = items.rejectProperty('isNew').toArray().sort(function(a, b) {
    return a.get('position') - b.get('position');
  }).mapBy('id');
  hash = {
    id: record.get("id")
  };
  hash["lineItems"] = itemIds;
  record.store.update(record.constructor.typeKey, hash);
});

I'm working around several issues here, so let me know if I can help out with this somehow.

Owner

wycats commented Sep 13, 2013

Can you enumerate the issues you're working around?

As of now I'm having issue of a rollback on model. To work around I keep the original state with me, and in case of any failure, I reset the model accordingly.

Few people facing issue with lost relationship. I have several hasMany relationships. For this I'm saving the child record first (on server I save the child record and add a relationship to parent ), on success I push the new record to the parent. So far I'm not bleeding on this, because I'm using Appacitive, which allows creation relationship like this. So even if user refresh the page, I've all the relationships in place.

Do let me know if I'm doing anything wrong in either of case.

Member

bradleypriest commented Sep 13, 2013

These are the main ones, will check if there are more:

  • Add a new child to a collection, then delete it, it is not removed from the collection
  • Create a new parent with child items, save parent, collection is set to an empty array by server, losing the children see this commit from a previous PR.
  • Delete a child from a collection, rollback child, not reinstated to collection.

decasia commented Sep 17, 2013

I'd like to hear more about the lost relationships that @amarpalsapure mentions. I'm seeing an issue where child relationships disappear from a parent after saving an array of child models, if any of the child models returns 422 for a validation error. Navigating to a different route and then back to the edit route seems to get the relationships to reappear. Are we expected to manually update hasMany relationships after saving child records at this point?

Owner

wycats commented Sep 24, 2013

Merging in #1354.

Would there be a possibility that you could work on this use case?

  1. Delete record
  2. Record fails the save
  3. Rollback record and have the state the same as before step 1.
Owner

wycats commented Sep 24, 2013

@twosick Yes. I think that makes sense.

Contributor

matthooks commented Sep 26, 2013

At least as of commit 7e0d0d1 .rollback() is not resetting the model after failure any more.

Contributor

matthooks commented Sep 26, 2013

Sorry -- looks like it's actually as of commit e2e3b8a, where in flight attributes are only reset if isError is truthy.

So, rollback() will still reset the model after a 5xx response but it's definitely no longer resetting after a 422.

WMeldon commented Sep 30, 2013

After a more time than spent on this than I'm proud to admit, I think I've gained some understanding of how to 'hard rollback' a model after a server side validation error.

I'm currently playing around with the ActiveModelAdapter and this is the code I got to successfully rollback a model that the server marked as invalid.

    cancelForm: function (){
      var model = this.get('model'),
          self = this;

      if(!model.get('isValid')) {
        var state = model.get('currentState');
        state.rollback(model);
        model.save();
      }
      else {
        model.rollback();
      }
      this.set('isEditing', false);
    },

If the model is invalid, the state has to be rolled back (which causes the model to roll back) because the model isn't allowed to change while 'inFlight'. I still don't entirely get it though, and it's not the most intuitive system, but I figure it could help.

CodeOfficer referenced this issue in dgeb/ember_data_example Oct 2, 2013

Open

Just curious if this is going to be updated... #43

@WMeldon This works for me except the record is still in error state when you return. So all validation errors remain, despite the record being in valid condition.

Any ideas?

edit: this works

var state = record.get('currentState');
state.isValid = true;
state.isError = false;
state.rollback(record);
record.set('errors', null);
record.save();

WMeldon commented Oct 11, 2013

@ianbishop I have the exact same problem. Short of overriding the isValid flag with some kind of spoof boolean, I don't see a way to do this right. My current course of action is "wait for @wycats ".

I just want to add a slightly different use case beyond "automatically roll back after failure." In my app, when a failure occurs, I let the user edit and try again, but if the user navigates away from the form before successfully saving, I want to roll back. I might accomplish this by checking isDirty and calling rollback from the appropriate route's willTransition callback.

@samwgoldman see @WMeldon and I's solutions, it's for this use case.

Here is my actual code in my route:

actions: {
    willTransition: function(transition) {
      var record = this.controller.get('model');

      if (record.get('isDeleted')) {
        return true;
      }
      else if (!record.get('isValid')) {
        var state = record.get('currentState');
        state.isValid = true;
        state.isError = false;
        state.rollback(record);
        record.set('errors', null);
        record.save();
      } else if (record.get('isDirty')) {
        record.rollback();
      }
    }
  }
Contributor

efigarolam commented Feb 7, 2014

I had a similar issue, I was trying to delete a record, but when the record was not deleted because of a validation, the record.rollback() method was not working because the server returned a 422.

I solved it with the following code:

deleteTeam: (teamId) ->
  controller = @controllerFor('teams')
  model = @modelFor('teams')

  if confirm('Are you sure?')
    @store.find('team', teamId).then (team) ->
      team.deleteRecord()
      team.save().then (->), ((response) ->
        model.pushObject @
        @.transitionTo('loaded.updated.uncommitted')
        controller.showAlertMessage(false, response.errors.messages)
      ).bind(team)

Hope this help to somebody, I lost 3 hours working around this...

I don't know if this the right place, but I also want the behavior commented by @4virtuals. For this I did the following modifications:

DS.RootState.loaded.saved.deleteRecord = (record) ->
    record.transitionTo('deleted.uncommitted')
DS.RootState.deleted.inFlight.didCommit = (record) ->
    record.transitionTo('saved')
    record.clearRelationships()
    record.send('invokeLifecycleCallbacks')

DS.RecordArrayManager.reopen
    updateRecordArrays: ->
        Ember.EnumerableUtils.forEach(@changedRecords, (record) ->
            if (Ember.get(record, 'isDeleted') and !Ember.get(record, 'isDirty') and !Ember.get(record, 'isSaving'))
                @_recordWasDeleted(record)
            else
                @_recordWasChanged(record)
        , @)

        @changedRecords.length = 0;

The modifications in the RootState clean the relationships of the deleted object when it's commited in the backend (I'm using the RESTAdapter and don't know if there is any semantical difference for other adapters).

The modification in updateRecordArrays is to prevent the deleted object from being removed of hasMany relationships when it's deleted, but not persisted, according to https://github.com/emberjs/data/blob/75ab804301793873a7d374039c691d82c430aa30/packages/ember-data/lib/system/model/model.js#L129

I don't look carefully about transactions from other states to the deleted, so this code can be incomplete, but I want to know if this a desired behavior, before going any further.

kumavis commented Oct 16, 2014

+1 to the ability to rollback relations

👍 rollback should restore relations.

kumavis commented Oct 17, 2014

Curious where we are on the path forward for this:
Are we still deciding the desired behaviour?
Are we waiting for a PR?
Is this pending some other refactor or feature branch?

A little context would make it easier to contribute.

Contributor

jcope2013 commented Oct 31, 2014

+1

I am trying to delete/rollback a relationship like this but oddly say if I have 9 new line items created (all done inside a modal) and then hit cancel on the modal, the code belows removes like 6 of them from the data store and leaves 3 left in the data store for Line Items, but if I create like 3 line items instead and hit cancel, it removes all 3 from the data store properly so now I need a new approach, I would love if there was an easier way to rollback/deleteRecord with corresponding affected relationships

if (cancelledItem.get('isNew')) {
          cancelledItem.constructor.eachRelationship(function (key, relationship) {
          if (relationship.kind === 'hasMany') {
              relatedModels = cancelledItem.get(key);
              if (relatedModels.content.length > 0) {
                 relatedModels.forEach(function(item) {
                      if (item.get('isNew')) {
                           item.deleteRecord();
                      }
                  });
                }
              }
            });
      cancelledItem.deleteRecord();
 } 
Contributor

sly7-7 commented Dec 12, 2014

I'm closing this, since I think this is superceeded by #2545

sly7-7 closed this Dec 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment