adopted-ember-addons / ember-changeset

Ember.js flavored changesets, inspired by Ecto
http://bit.ly/ember-changeset-demo
MIT License
431 stars 141 forks source link

Can no longer access model errors after failed save #564

Open lindyhopchris opened 3 years ago

lindyhopchris commented 3 years ago

Version

ember-changeset@3.10.2 validated-changeset@0.10.2

Test Case

None

Steps to reproduce

Set up a scenario where the server rejects a save with validation errors, as per this example in the readme:

changeset
  .save()
  .then(() => {
    /* ... */
  })
  .catch(() => {
    get(this, 'model.errors').forEach(({ attribute, message }) => {
      changeset.addError(attribute, message);
    });
  });

Expected Behavior

model.errors contains errors

Actual Behavior

model.errors has no errors as original value has been restored on the model, wiping the errors out.

I believe this has been caused by this change: https://github.com/validated-changeset/validated-changeset/pull/80

When the original value of an attribute is restored onto the model, the model clears the errors for that attribute. That means by the time the error is caught, model.errors contains no error messages.

It is therefore no longer possible to add model errors to a changeset.

The exception that Ember Data throws does not have the parsed errors on it - it has the raw JSON:API response. So there is no way to access the parsed Ember Data errors and push them into the changeset.

snewcomer commented 3 years ago

👋 @lindyhopchris Do you happen to have a reproduction? I added a test here and all seems ok. We only rollback changes made through the changeset. Does DS.errors get set on the changeset as a result of a failed save? I would have assumed it would only exist on the model (content as referred in our codebase).

lindyhopchris commented 3 years ago

@snewcomer thanks for getting back to me!

your test actually shows what the problem is. this line:

expect(dummyModel.name).toEqual('previous');

that shows that the value on the dummy model has been set back to the value it was before the changeset was saved. I.e. the sequence is:

However, that's the problem - setting the name back to previous on a real Ember Data model removes the error message for the name property.

I.e. for the Ember Data model to still have the error message for name, the assertion in the finally block needs to be:

expect(dummyModel.name).toEqual('new');

lindyhopchris commented 3 years ago

I can't find any reference to it in the guides or the API docs, but basically when you set an attribute on an Ember Data model, if there is an error message for that attribute, the error message gets cleared. That's why setting the name value back to its original value clears the error message for the name attribute. Hope I've explained that ok!

snewcomer commented 3 years ago

Oh you are right. I naively assumed the errors from the API would be set and persist regardless of the DS.Model value. So it must clear out the errors when the value is set to the previous as you have described.

We are a little at odds because the underlying model is only expected to only update the underlying model upon a verified "proper" state on the changeset. I'll have to think about this!

lindyhopchris commented 3 years ago

Yeah totally get why you're reverting the model after the save fails... does kind of make sense. Not sure what to suggest so interested to hear your thoughts once you've had a chance to think about it!