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

Fix error messages on failed deletes, updates #1130

Conversation

RichardBradley
Copy link
Contributor

This is a fix for items 1 & 2 described at #1129 (comment)

It does not fix item 3.

New test results:

Given an entity where the REST backend will return 400 "cannot delete due to xyz" for a "DELETE" request
When I view the "edit" page for that entity
And I click the "delete" button
And I click "Yes" to "are you sure?"
Then I should see an error banner including "cannot delete due to xyz"
 # this passes now (and fails before these changes)

Given an entity where the REST backend will return 400 "cannot delete due to xyz" for a "DELETE" request
When I view the "list" page for that entity tytpe
And I select that entity
And I click the "delete" button
And I click "Yes" to "are you sure?"
Then I should see an error banner including "cannot delete due to xyz"
 # this still fails due to item #3
 # I see a success banner

@jpetitcolas
Copy link
Contributor

Thanks for your PR. Just rebasing it and fixing tests in a new one to be able to merge it today (we plan to release a new alpha later today). :)

@fzaninotto fzaninotto changed the title #1129 Fix error messages on failed deletes, updates Fix error messages on failed deletes, updates Dec 1, 2016
@Phocea
Copy link
Contributor

Phocea commented Dec 10, 2016

Hey guys. I was just think how close this error handling is from the [PR] (#1273) I submitted.
PR is for http error on state change. This one is affecting handling of HTTP error on CRUD operations.

Cant we think of a common way to handle this rather than to have 2 different methods?

  • State change can now be customised using a decorator
  • CRUD errors can be customised using the entity.errorMessage(). I have not tried yet, but couldnt we have errorMessage use the HttpErrorService by default now ?

@jpetitcolas
Copy link
Contributor

It seems these changes are already on master. Closing the issue.

@Phocea: indeed, it sounds like a good idea, yet let's apply it later. Short term objective is to release 1.0 version. :)

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.

3 participants