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

Allow errors on arbitrary properties, not just defined attributes or relationships. #1984

Merged
merged 1 commit into from Feb 13, 2015

Conversation

Projects
None yet
10 participants
@alexspeller
Contributor

alexspeller commented May 29, 2014

Addresses #1877

@tchak @igorT ping

@iwarshak

This comment has been minimized.

Show comment
Hide comment
@iwarshak

iwarshak May 29, 2014

Thanks for this @alexspeller - my pull req for this (#1979) works, but doesn't have tests and doesn't pass Travis

iwarshak commented May 29, 2014

Thanks for this @alexspeller - my pull req for this (#1979) works, but doesn't have tests and doesn't pass Travis

@alexspeller

This comment has been minimized.

Show comment
Hide comment
@alexspeller

alexspeller May 29, 2014

Contributor

Oh, I didn't even see #1979 - I think the reason it failed was because forEach will fail on non-ember arrays if prototype extensions are off? Anways the approach looks basically the same in either case.

Contributor

alexspeller commented May 29, 2014

Oh, I didn't even see #1979 - I think the reason it failed was because forEach will fail on non-ember arrays if prototype extensions are off? Anways the approach looks basically the same in either case.

@iwarshak

This comment has been minimized.

Show comment
Hide comment
@iwarshak

iwarshak Jun 2, 2014

@alexspeller no worries, mine doesn't have tests. 👍

iwarshak commented Jun 2, 2014

@alexspeller no worries, mine doesn't have tests. 👍

watsonbox added a commit to watsonbox/facture that referenced this pull request Jun 17, 2014

Display server-side errors when saving invoices
Allow errors on arbitrary properties, not just defined attributes or relationships
See emberjs/data#1984
@iwarshak

This comment has been minimized.

Show comment
Hide comment
@iwarshak

iwarshak Jul 3, 2014

@alexspeller any thoughts on when this might get merged in?

iwarshak commented Jul 3, 2014

@alexspeller any thoughts on when this might get merged in?

@iwarshak

This comment has been minimized.

Show comment
Hide comment
@iwarshak

iwarshak Jul 22, 2014

@tchak @igorT would you mind looking at this?

iwarshak commented Jul 22, 2014

@tchak @igorT would you mind looking at this?

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Aug 5, 2014

Member

As I already said I would like for this behavior to be optional... But if I am the only one to believe this, I suppose I will abandon my fight :)

Member

tchak commented Aug 5, 2014

As I already said I would like for this behavior to be optional... But if I am the only one to believe this, I suppose I will abandon my fight :)

@kurko

This comment has been minimized.

Show comment
Hide comment
@kurko

kurko Aug 11, 2014

Contributor

I heard your argument on the original issue, but I fail to see how this would bring inconsistencies. It's the backend that enforces schemas, so I believe the client should be flexible in that regard. Nonetheless, I'd love to see the PR you promised :D

Contributor

kurko commented Aug 11, 2014

I heard your argument on the original issue, but I fail to see how this would bring inconsistencies. It's the backend that enforces schemas, so I believe the client should be flexible in that regard. Nonetheless, I'd love to see the PR you promised :D

@iwarshak

This comment has been minimized.

Show comment
Hide comment
@iwarshak

iwarshak Aug 21, 2014

Until this gets merged in, you can add this to your app to get this functionality.

DS.Model.reopen({
    adapterDidInvalidate: function(errors) {
      var recordErrors = this.get('errors');
      for (var key in errors) {
        if (!errors.hasOwnProperty(key)) continue;
        recordErrors.add(key, errors[key]);
      }
    }
});

iwarshak commented Aug 21, 2014

Until this gets merged in, you can add this to your app to get this functionality.

DS.Model.reopen({
    adapterDidInvalidate: function(errors) {
      var recordErrors = this.get('errors');
      for (var key in errors) {
        if (!errors.hasOwnProperty(key)) continue;
        recordErrors.add(key, errors[key]);
      }
    }
});
@ahacking

This comment has been minimized.

Show comment
Hide comment
@ahacking

ahacking Oct 15, 2014

Contributor

I just commented on #1977 as I intend to fix some problems that Ember Data currently has with error handling in my project:

  • mapping payload keys to model keys
  • errors on embedded models

I think if the serializer is enhanced to process/extract errors and allows errors to pass through then that is all that needs to be done. It is the right place to put this concern as serializers currently control the what and how of model serialization and InvalidErrors should receive a similar treatment to other model attributes.

The adapterDidInvalidate call should just apply what the serializer has sanctioned as per what @iwarshak demonstrates above.

Contributor

ahacking commented Oct 15, 2014

I just commented on #1977 as I intend to fix some problems that Ember Data currently has with error handling in my project:

  • mapping payload keys to model keys
  • errors on embedded models

I think if the serializer is enhanced to process/extract errors and allows errors to pass through then that is all that needs to be done. It is the right place to put this concern as serializers currently control the what and how of model serialization and InvalidErrors should receive a similar treatment to other model attributes.

The adapterDidInvalidate call should just apply what the serializer has sanctioned as per what @iwarshak demonstrates above.

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT Nov 24, 2014

Member

ping @tchak

Member

igorT commented Nov 24, 2014

ping @tchak

@pmdarrow

This comment has been minimized.

Show comment
Hide comment
@pmdarrow

pmdarrow Dec 19, 2014

Any update on whether this is going to merged or not?

pmdarrow commented Dec 19, 2014

Any update on whether this is going to merged or not?

@ahacking

This comment has been minimized.

Show comment
Hide comment
@ahacking

ahacking Jan 2, 2015

Contributor

IMO this should be merged.

Since PR #2392 has been merged, error payload processing is now a serializer concern. There should be no reason not to include all errors that the serializer has sanctioned.

ping @igorT, @alexspeller, @tchak

Contributor

ahacking commented Jan 2, 2015

IMO this should be merged.

Since PR #2392 has been merged, error payload processing is now a serializer concern. There should be no reason not to include all errors that the serializer has sanctioned.

ping @igorT, @alexspeller, @tchak

@alexspeller

This comment has been minimized.

Show comment
Hide comment
@alexspeller

alexspeller Jan 2, 2015

Contributor

If this will be merged I will update the PR. But if it will be unmerged for another 8 months there is no point as it will have to be rebased again. So if someone on the core team does want this, please ping me when you are ready so that I can make the changes.

Contributor

alexspeller commented Jan 2, 2015

If this will be merged I will update the PR. But if it will be unmerged for another 8 months there is no point as it will have to be rebased again. So if someone on the core team does want this, please ping me when you are ready so that I can make the changes.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jan 3, 2015

Member

@igorT @alexspeller I am 👍 on merging this

Member

tchak commented Jan 3, 2015

@igorT @alexspeller I am 👍 on merging this

@alexspeller

This comment has been minimized.

Show comment
Hide comment
@alexspeller

alexspeller Jan 3, 2015

Contributor

Awesome, I will rebase shortly

Contributor

alexspeller commented Jan 3, 2015

Awesome, I will rebase shortly

@alexspeller

This comment has been minimized.

Show comment
Hide comment
@alexspeller

alexspeller Jan 7, 2015

Contributor

@tchak rebased and tests all passing :)

Contributor

alexspeller commented Jan 7, 2015

@tchak rebased and tests all passing :)

@alexspeller

This comment has been minimized.

Show comment
Hide comment
@alexspeller
Contributor

alexspeller commented Jan 24, 2015

@tchak @igorT ping

@bjornstar

This comment has been minimized.

Show comment
Hide comment
@bjornstar

bjornstar Jan 29, 2015

Can this PR get some love?

bjornstar commented Jan 29, 2015

Can this PR get some love?

fivetanley added a commit that referenced this pull request Feb 13, 2015

Merge pull request #1984 from alexspeller/arbitrary-errors
Allow errors on arbitrary properties, not just defined attributes or relationships.

@fivetanley fivetanley merged commit 589156f into emberjs:master Feb 13, 2015

1 of 2 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment