Better http error handling

[Phocea]

As discussed with @fzaninotto, reopening this PR. I have left the original implementation with a global delcared in the vendor.js since this is the solution I tested.

On latest build I am getting the error _.includes is not a function when navigating into one of my custom pages.
Following several threads on Restangular Git (one of them being mgonto/restangular#1225). I found out that Restangular is now compatible with lodash 4.

ng-admin using underscore is causing incompatibilty since the new webpack dependency version has been merged. Making use of lodash fixes the problems

[jpetitcolas]

I just added unit tests for HttpErrorService. Yet I noticed a difference between error message structure. On 403, you look for a error.data.message and for other errors for a error.message. Is it an expected behavior?

[Phocea]

Yes good remark. Indeed it should also be error.data.message. This was part of the original fix and has been reported a few time.
I will change it in the code. Doesnt seem to affect your test

[jpetitcolas]

No, test is not impacted, yet you should also change the mock to be as closest as possible from original behavior. :)

[Phocea]

@jpetitcolas more question. Your test wont go trough the HttpErrorHandler method.
Even thought I agree that this make the test more "unitary" and testing only the service, we do loose coverage on the HttpErrororHandler method.
Not a big deal, but shall we have 2 tests then, or one extra method in the test to perform an emit ?

[jpetitcolas]

@Phocea: you're right: we need some unit tests on the handler too.

If we want to test the whole part together, that's more end to end testing purpose in my opinion. That would be ideal, yet it is longer to implement, and not highly critical. I propose we don't block this PR for 1.0 release because of an E2E test lack.

[Phocea]

Agreed to, it's a one liner with no custom code.
Just for my knowledge, would it be considered bad practice to add a unit test doing an emit (like in my original test), inside the same spec file?

[jpetitcolas]

@Phocea, we prefer to have an equivalence between our tested and testing files. One code file, one test file. So, even if there is more bootstrap, yes, please, create another file. :)

By the way, if you want to test an $emit, do not forget to $scope.$digest(). Otherwise, it won't be emitted. :)

[Phocea]

@jpetitcolas ok noted.
Actually, I tested the test in a plunkr and it worked with or without the digest, so I removed it!

[Phocea]

Guys, can this be merged now ?

[Kmaschta]

What about the tests?
Travis should pass E2E tests since the WebDriver is up-to-date.

[Phocea]

@Kmaschta Unit test is done through HttpErrorServiceTest.spec.js
Or are you talking about the E2E still not going through?

[jpetitcolas]

@Phocea: some tests are failing on Travis:

Http Error Service should display generic translated error notification if neither 404 nor 403 FAILED
Expected spy $translate to have been called with [ 'STATE_CHANGE_ERROR', Object({ message: 'Unknown error' }) ] but it was never called.

I let you take a look on it?

[Phocea]

@jpetitcolas Gotcha. Sorry did not notice it was the unit test failing after the error.data.message change!

[Phocea]

@Kmaschta all good for you now ? Since Jonathan and myself pushed some commit I guess its down to you for the merge :)

[Kmaschta]

The E2E tests still failing ... but it's the cause of the protractor config this time. I'll check that soon.

[Phocea]

@Kmaschta Let me know if you want me to try rebasing again. I can only use GitHub right now so its a bit cumbersome, but dont mind updating my fork and resubmitting again if it helps

[Kmaschta]

We can change the target branch of this PR, we'll find a workaround.