fix(core): Add custom 400 and 404 error messages

[hyperreality]

fix(core): Add custom 400 and 404 error messages

Fixes #1504

---

One remaining problem is that ideally, the bad URL should remain the same (i.e. /articles/noarticlehere rather than /not-found) and still display an error message. I was able to get halfway there by deleting the url property from the error routes, but then you have another issue where the valid page before getting the error cannot be reached in the browser history. The 404 article page is intercepted before it is displayed, so then the browser would think that the page before that and the error page were the same entity, rather than the bad article page and the error. But that's for another PR.

[coveralls]

Coverage decreased (-0.1%) to 72.929% when pulling 50879f0 on hyperreality:upstream into 8b54669 on meanjs:master.

[hyperreality]

A good point being thrown up by the E2E testing there. We already have loads of bad requests that don't expect to be redirected to another page, such as in the user sign up:

return res.status(400).send({
    message: errorHandler.getErrorMessage(err)
});

This could be avoided by removing the intercepting of 400 bad requests I just added and change the 'Invalid Article ID' error to another 404, what do you think @mleanos?

[mleanos]

I think the concern of some 400 responses not requiring a redirect, was what someone brought up a while back. I couldn't find that conversation. Either way you're right.

In the case of a failed sign up, 400 is not appropriate. What about 409 or 422?

Another option might be to use 400, but also pass a flag (something like: bypassInterceptorRedirect) that we can check in our interceptors to decide if we should redirect the user. I think this option should only be used if we can't come up with a better HTTP response code than 400.

Side Note: We seem to be using 400 as an error response in some cases where it's probably not the best code to return. I can't recall where these are. However, I just want to point out that we should try to use more appropriate codes in certain situations.

[mleanos]

Overall, this looks pretty good to me. We just need to figure out how to handle the Sign Up error response, and keep the user on that view. Thus, solving the E2E tests that are failing.

[mleanos]

We need to handle the 400 error response on the Sign Up, so that the user stays on that view & can fix the issues.

[mleanos]

ping @hyperreality

[hyperreality]

Grepping through the project finds loads of 400 errors. Perhaps we should ignore them for now and look at improving the error status codes at some point in the future? I actually think that 403 is the most appropriate code for a failed login, but that's also intercepted by the code already.

I wonder if the bypassInterceptorRedirect type solution is a commonly used option in projects.

[mleanos]

A failed login should be 403. However, a failed Sign Up might be a 409 or 422.

http://www.bennadel.com/blog/2434-http-status-codes-for-invalid-data-400-vs-422.htm

I feel like 422 is more appropriate in this case.

[mleanos]

As for the bypassInterceptorRedirect, it shouldn't be a common solution. It's probably not a good solution, but a last resort option; then again maybe we shouldn't consider it.

[mleanos]

There's just one last issue we need to resolve; we need to decide on how to handle the current 400 HTTP response codes in the Sign-Up & Sign-In views. See this comment for more context. My opinion is that they should be returning 422; this is described here http://www.bennadel.com/blog/2434-http-status-codes-for-invalid-data-400-vs-422.htm

@meanjs/contributors These changes are pretty important. I'd really like to see this in for 0.5.0. Let's make a decision on how to handle the 400 error codes discussed above, and finalize this PR.

@hyperreality If you don't have time to finish this, I can take it over & create a new PR based off of the work done here. I'll create a branch & merge these changes in, to keep these commits in the history.

[hyperreality]

Hey @mleanos, I agree with all your decisions. Yes, I would appreciate if you could take over this PR, I'm extremely busy atm.

[mleanos]

Closing in favor of #1547