fix transitioning into invalid state

[arenoir]

Fixes issue #3851.

Removes events from error class that was causing side effects. I don't think the error class should be responsible for updating the model state.

[bmac]

@arenoir I'd like to get a better understanding of what you are suggesting here. Are you saying rollbackAttributes should be responsible for transitioning a model out of the invalid state?

[arenoir]

I think changing the state should be more explicit. The rollback method to me says put this record back to the state it was in when instantiated.

[pangratz]

To be semver compatible, this change entails an increase of the major version since it changes the current behavior. Maybe it should be possible to opt out of this behavior via an environment flag and add deprecation warning that the current behavior is about to change in 3.0?

---

Apart from that, I in general agree with

I don't think the error class should be responsible for updating the model state.

I think ember data would need a didUpdateAttribute hook so the current situation could be implemented nicely once the errors for an attribute are not cleared automatically when it changes:

DS.Model.extend({
  didUpdateAttribute: function(name, oldValue, newValue) {
    var errors = this.get('errors');
    errors.clearErrorsFor(attributeName);

    if (errors.get('isEmpty')) {
      // currently not public ...
      this.send('becameValid');
    }
   }
});

[arenoir]

@pangratz I would argue that this behavior is a bug and not documented? However I know you are correct; it is a breaking change.

I like the idea of a didUpdateAttribute hook and that would restore the behavior thus allowing us to remove the side effect causing event bindings. I will try to implement it. In the mean time I am using this pull request in my application as it fixes a bigger breaking change.

[arenoir]

@pangratz @bmac, I have restored the functionality to become valid when errors are removed. I believe this is a better solution than sending events from the error class.

[bmac]

Thanks for the update @arenoir. This code looks good.

cc @igorT @tchak for review.

[pangratz]

I don't know how many apps/addons rely on those events being triggered when registered via model.get('errors').on('becameValid') or model.get('errors').registerHandlers(). Since Ember.Evented is removed in this PR, code which relies on the on method being available on the DS.Errors throws an error.

The registerHandler method isn't advertised but it also hasn't been explicitly marked as @private so I technically this would break public API, I guess. It also hasn't been explicitly marked as @public either, so there's that. If I am misinterpreting semver here, please ignore me 😔 .

Again, just to be clear: I am 👍 👍 on removing those events and eventually make clearing the errors for an attribute an opt in solution, but I just want to point out possible implications for semver.

A possible, compatible solution could be to overwrite the on method and throw a deprecation...

[bmac]

I think @pangratz is correct. @arenoir do you mind adding registerHandlers back with a deprecation warning and continuing to use the Ember.Evented mixin even though the behavior is no longer needed?

[tchak]

The behivor is not dicumented but it is definitly used by some applications. Basicly the use case is to be able to implement client side validation in terms of errors object. Pushing an error in to errors object transitions the record in to invalid state.

I am fine with deprecating it and removing in next major version. But I am not sure where the deprecation should be. For example in my app, I don't care about events at all. What I do rely on, is the fact that if I am pushing an error in to errors object, the record became invalid.

[pangratz]

I believe this is a better solution than sending events from the error class.

💯 agree

[arenoir]

@bmac okay I have added the Evented functionality back and added my attempt to deprecate it. The problem is we don't want to deprecate add, remove, clear but only warn that they will no longer be responsible for modifying a records state.

[tchak]

@arenoir I am not sure how I feel about this... It feels like manual errors management have valid use cases... And if we decide that we want to remove this functionality we should simplify the errors object.

In any case, this probably should go behind a flag to begin with.

[arenoir]

@tchak, I agree with the use case of manually adding errors but I don't think doing so should update the state of the record.

If the server has put the record into the invalid state clearing the errors shouldn't change it to 'uncommitted' state. As it stands now calling willCommit from the 'invalid' state first clears the errors inconsequentially changing the state to 'uncommitted' and then transitions the state to 'inflight'.

[tchak]

@arenoir I am fine with this reasoning.
A thew observations:

• if we doing so, I think we need a clearly documented api to manually mark a record as invalid.
• we do agree that _add and co methods are temporary, until we can remove deprecations?
• Speaking of deprecations, shouldn't they be warnings?

[arenoir]

@tchak I agree there should be a public api to change a records state. The _add , _remove and _clear private methods are temporary. Once we remove the Evented functionality from the error class the temporary methods can be removed. As far as warnings vs deprecations I am in favor of a warning as the methods are not going away their behavior is just changing a bit.

I think this will fix some other inconsistent bugs like #3707.

[tchak]

@arenoir 👍 for warnings. For the rest, I am 👍 on merging it.

[arenoir]

@tchak I switched to warnings and rebased.

[tchak]

I am happy with this PR.
@bmac @igorT can we merge it as is, or dose it have to go behind a feature flag or something? It's not really a feature and it do not actually change current behaviour. Just adding some warnings.

[tchak]

LGTM. @bmac is it ok with you to merge this ?

[bmac]

Sorry @arenoir. I missed @tchak ping. Do you mind rebasing this pr so it can be merged?

[arenoir]

@bmac okay I rebased with master. Sorry It took so long I was on holiday.

[bmac]

Thanks @arenoir

[forgetboy]

Going through the conversation, it seems, that add, remove and clear methods shouldn't have been marked deprecated. Only the warnings need to exist. @arenoir @tchak could you verify this? Thanks.