DirtyState.invalid handle pushedData event

[turboMaCk]

I think that this should by handled similar as in uncommited.
It's currently causing this error:

route: xz Attempted to handle event `pushedData` on <app@model:class::instance> while in state root.loaded.updated.invalid.

[bmac]

Sounds reasonable to me. @turboMaCk would you like to submit a pr with this change?

[turboMaCk]

I'd like to, but build fails for my for some reason.

Build failed.
File: activemodel-adapter/lib/setup-container.js
undefined is not a function

[turboMaCk]

Ok. The problem is in this code checking script: require-spaces-after-closing-parenthesis-in-function-declaration.js looks like file has no method getTokenPosByRangeStart(). I do not know if this should be enviroment specific problem or if it is bug in build configuration but It will be better if someone will look at this.

[bmac]

Oops sorry about that @turboMaCk. Looks like one of our dependencies updated and changed their api. I'll look into it now.

[bmac]

@turboMaCk master should be fixed now. Thanks for reporting the issue and helping debugging the issue.

[turboMaCk]

Sorry but I've found one more issue with rollbacking invalid records. It should not be dirty after rollback, righ?

[bmac]

I believe so. @igorT can you confirm?

Unrelated. How did you upgrade this issue into a pr?

[turboMaCk]

Check this tool for using git + github api. And there is stackoverflow thread about this feature.

[turboMaCk]

OT: @bmac Don't you know if there is some way to use my fork with ember-cli?

[devinus]

I'm having this problem as well. But how does this happen when my code does model.get('errors').clear() by itself?

[devinus]

I actually don't understand at all how I'm getting this error when I use this.store.find('type'). I've cleared the errors on my model and I'm still getting this.

[turboMaCk]

@devinus The problem is in root state maneger of ember data. Cleaning errors doesn't change record state which cause this issue. Try to use my fork of ember-data https://github.com/turboMaCk/data until this will be merged.

@bmac Can you please review this pull request and merge it?

[devinus]

@turboMaCk Is there any workaround until it's merged? Why can't I forcefully change the models state? I thought model.get('errors').clear() would change the state.

[devinus]

For anybody else coming to this, you unfortunately might have to do what I did for now:

  beforeModel() {
    // https://github.com/emberjs/data/pull/2943
    this.store.unloadAll('type');
  },

  model() {
    return this.store.find('type');
  }

[turboMaCk]

@devinus I don't want to hack this in application layer. For mo It make more sense to use custom build of ember data and than switch to official one after this will be merged – no need to changes app code. And I'm also not sure if unloadAll do not cause revert on all record in collection which I do not want to happened. I want to keep all state as it and only handle fetching on invalid records right way.

[pangratz]

This will break the case when a new record is saved, has errors and is then rolled back. By transitioning into the loaded.saved state, this record is now marked as created, but it should be deleted instead...

Failing test case:

test("invalid new record can be rollbacked", function() {
  var person;
  var adapter = DS.RESTAdapter.extend({
    ajax: function(url, type, hash) {
      var adapter = this;

      return new Ember.RSVP.Promise(function(resolve, reject) {
        /* If InvalidError is passed back in the reject it will throw the
           exception which will bubble up the call stack (crashing the test)
           instead of hitting the failure route of the promise.
           So wrapping the reject in an Ember.run.next makes it so save
           completes without failure and the failure hits the failure route
           of the promise instead of crashing the save. */
        Ember.run.next(function() {
          reject(adapter.ajaxError({ name: 'is invalid' }));
        });
      });
    },

    ajaxError: function(jqXHR) {
      return new DS.InvalidError(jqXHR);
    }
  });

  env = setupStore({ person: Person, adapter: adapter });

  run(function() {
    person = env.store.createRecord('person', { id: 1 });
  });

  equal(person.get('isNew'), true, "must be new");
  equal(person.get('isDirty'), true, "must be dirty");

  run(function() {
    person.save().then(null, async(function() {
      equal(person.get('isValid'), false);
      person.rollback();

      // assertions taken from the test 'new record can be rollbacked' http://git.io/vTsKR
      equal(person.get('isNew'), false, "must not be new");
      equal(person.get('isDirty'), false, "must not be dirty");
      equal(person.get('isDeleted'), true, "must be deleted");
    }));
  });
});

[pangratz]

Actually it looks like the above test fails on current master as well. I will investigate and suspect the test setup is not correct or the behavior of rolling back an invalid record is currently not supported.

But still, getting the state of the new, invalid person after it is rolled back via person.currentState.stateName is root.loaded.saved, where it should be root.deleted.saved or loaded.created.uncommitted ...

[pangratz]

or the behavior of rolling back an invalid record is currently not supported

This is indeed the current behavior: rolling back a new record which is marked as invalid puts the record in the loaded.created.uncommitted state. Is this the intended behavior? There are currently no tests for this case in rollback-test.js so it needs to be discussed what should be the intended behavior here. Sorry for hijacking this PR for this issue, I will gladly open a new one if this is desired ...

[turboMaCk]

I agree that we need add test case for rollbacking new uncommitted invalid record.
For me it make sense to handle it same as rollbacking uncommitted valid record.

[pangratz]

For me it make sense to handle it same as rollbacking uncommitted valid record.

yep, that would be my vote too

[turboMaCk]

@pangratz I fix it this way but I still think that this should be done in some better way but my solution might be useful for anybody who needs to fix this right know. We should always refactor and change logic after final decision will be made.

[pangratz]

@turboMaCk not needed, I have already a fix available, just haven't made the PR yet.

Edit: #3083

[devinus]

Anything that allows me to use the store after I have a DS.InvalidError would be welcome. I'm surprised this hasn't been caught yet, considering so many people must be using the ActiveModelAdapter.

[turboMaCk]

@devinus You can try to build my fork yourself, and replace ember-data build in your app with this custom build untill this will be finaly fixed and merged to master.

$ git clone -b push-invalid-record git@github.com:turboMaCk/data.git
$ npm install
$ npm run build:production

The builds will be placed in the dist/ directory.

[devinus]

@turboMaCk Indeed, I'm just concerned about the issues @pangratz raised.

[turboMaCk]

@devinus my last commit solve it. But you should merge this commit #3083 which is more clean.

[turboMaCk]

@devinus https://github.com/turboMaCk/data on master has both my and @pangratz fixes merged if you are interested....

[jcope2013]

any timeline on this being merged in?

[tchak]

@turboMaCk we merged @pangratz's PR. Can you leave only the pushedData part and the test in this PR? I can do it but if you can get to it before I do it would be nice.

[bmac]

Hi @turboMaCk I've rebased and committed this pr as a5e4df6. I kept you as the author. Thanks for pull request!

[turboMaCk]

@bmac @tchak sorry for late response and thank you!