marmelab / ng-admin

Add an AngularJS admin GUI to any RESTful API
http://ng-admin-book.marmelab.com/
MIT License
3.95k stars 725 forks source link

[RFR] Handle CRUD form error through http error service #1300

Closed Phocea closed 7 years ago

Phocea commented 7 years ago

This PR is aim to make use of the new HttpErrorService to display CRUD form http error responses. For backward compatibility, the code still pick up customization done via entity.errorMessage method. However this step is now redudant since it can be done inside an decorator

TODO

Phocea commented 7 years ago

@Kmaschta @jpetitcolas @fzaninotto Your thoughts on this PR. Would it also be acceptable to start marking entity.getErrorMessage() as deprecated to only keep ony way of customizing error messages using a decorator (and simplify ng-admin). Or we can keep and document both ways.

Kmaschta commented 7 years ago

I think that this change should be deployed with the 1.0.

Phocea commented 7 years ago

I agree it would be neater to submit this in the 1.0 together with the HttpErrorService. Can we postpone while we get more feedback on this?

Kmaschta commented 7 years ago

Have you something to add to this PR or can we merge it?

Phocea commented 7 years ago

I need to update the documentation. Do we keep the dual way of customizing the error message (decorator or entity.errorMessage(), or do we mark the later as deprecated?

Also, since the HttpErrorService is used for both CRUD and stateChangeError, then I think it would make more sense for the defaultError to only gice the STATE_CHANGE_ERROR of the toState and such parameters exists, otherwise just display a GLOBAL_ERROR of some sort

Phocea commented 7 years ago

@Kmaschta, @jpetitcolas Any of you guys have a chance to review and merge those changes ?

Phocea commented 7 years ago

@Kmaschta I agree, but the more we wait the more troublesome that BC could be. Right now I doubt if its going to force people to make code changes...except from me :)

jpetitcolas commented 7 years ago

I agree with @Kmaschta: even if we didn't explicitly encourage to use HttpErrorHandler directly, some people may use them, through a decorated handler for instance. To respect semantic versioning, I would recommend to keep compatibility as currently, and to update it when switching to a 2.0 version.

Unless we release discretly (without a blog post) a new major version after each BC change? @fzaninotto, what do you think about it?

Kmaschta commented 7 years ago

As discussed with @fzaninotto & @jpetitcolas, we'll most likely prepare a 2.0. Meanwhile, we can merge all the BC features on a new branch: next.

jpetitcolas commented 7 years ago

Oops, my bad. Merged too fast. Read "we can merge" from @Kmaschta. @Phocea, can you fix your review in a new PR? :)

jromero-onclick commented 7 years ago

Hello at this moment should I have to decorate HttpErrorService for accomodate form errors with my api or not? I did not found the steps on the doc

Phocea commented 7 years ago

Hello, Documentation is here: https://github.com/marmelab/ng-admin/blob/master/doc/Theming.md#customizing-http-error-messages

Until those changes are merged on main you will only be able to accomodated for stateChange errors. For API CRUD errors you have to wait for this fix. Should not be too long now