Skip to content
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

Add support for ajax errors. #35

Merged
merged 3 commits into from
Dec 8, 2014

Conversation

benkonrath
Copy link
Collaborator

This improvement make the adapter require Ember Data v1.0.0-beta.12 as it uses the error serialization support that was recently added to Ember Data.

The CRUD integration tests have also been cleaned up a bit.
The commit relies on the error serialization that was added in Ember
Data v1.0.0-beta.12.

emberjs/data@4884933
@benkonrath
Copy link
Collaborator Author

It's also possible to write some unit tests for this similar to what is done for the active model adapter.

https://github.com/emberjs/data/blob/master/packages/activemodel-adapter/tests/integration/active_model_adapter_test.js#L23

@benkonrath
Copy link
Collaborator Author

@holandes22 If you have a chance it would be great if you could try out this feature. This is based on something I'm already using but it's always nice to get another person to try it out as well.

@holandes22
Copy link
Collaborator

@benkonrath sure thing. Sorry I've been absent lately, completely swamped with work. Been wanting to try ED beta 12 + DRF 3.0, so was planning to clear some space over the weekend, and I'll give this a go then too.

@benkonrath
Copy link
Collaborator Author

@holandes22 No worries, I know the feeling. :)

@dustinfarris
Copy link
Owner

What is the status on this?

@benkonrath
Copy link
Collaborator Author

It's waiting for review. The open question is: Are the tests ok? Or should I add some unit tests as well or instead of what I have.

@dustinfarris
Copy link
Owner

I like how the activemodel adapter unit tests you linked to do a quick spot check on ajaxError method output. If we can mimic that here I think this is otherwise good to go. Great work on the integration tests.

Also:

* Modified ajaxError() to make the behaviour more explicit when the
  responseText can't be parsed.
* Improved comments.
@benkonrath
Copy link
Collaborator Author

The active model tests are slightly outdated because you can no longer test for the camelCase transformation in ajaxError(). In any case, I've added unit tests for our ajaxError() which gives it full unit test coverage. The integration tests check that the field errors are correctly converted to camelCase and I've made that more explicit in the comments.

dustinfarris added a commit that referenced this pull request Dec 8, 2014
@dustinfarris dustinfarris merged commit c3fec14 into dustinfarris:version-1.0 Dec 8, 2014
@benkonrath benkonrath deleted the ajax-errors branch December 12, 2014 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants