-
Notifications
You must be signed in to change notification settings - Fork 725
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
Http error handling #1273
Http error handling #1273
Changes from 9 commits
54ff59a
8148b30
626b509
b066920
6db5c58
d5e07d2
8ee4514
5125c31
3588e0e
2b03d41
ece3ae5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
const httpErrorService = ($state, $translate, notification) => ({ | ||
handleError: function(event, toState, toParams, fromState, fromParams, error) { | ||
switch (error.status) { | ||
case 404: | ||
this.handle404Error(event, error); | ||
break; | ||
case 403: | ||
this.handle403Error(error); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, be consistent and give the same API for all the handlers: It will be more customable for the next developers and the doc will be more clear too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you do this I get sonar style check errors because the parameters are not used in the method (Unused function parameters should be removed javascript : UnusedFunctionArgument) Dev can override this in the decorator anyway |
||
break; | ||
default: | ||
this.handleDefaultError(error); | ||
break; | ||
} | ||
}, | ||
|
||
handle404Error: function(event) { | ||
event.preventDefault(); | ||
$state.go('ma-404'); | ||
}, | ||
|
||
handle403Error: function(error) { | ||
$translate('STATE_FORBIDDEN_ERROR', { message: error.data.message }).then(this.displayError); | ||
throw error; | ||
}, | ||
|
||
handleDefaultError: function(error) { | ||
$translate('STATE_CHANGE_ERROR', { message: error.data.message }).then(this.displayError); | ||
throw error; | ||
}, | ||
|
||
displayError: text => notification.log(text, { addnCls: 'humane-flatty-error' }), | ||
}); | ||
|
||
httpErrorService.$inject = ['$state', '$translate', 'notification']; | ||
|
||
export default { $get: httpErrorService }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ export default function translate($translateProvider) { | |
'NEXT': 'Next »', | ||
'DETAIL': 'Detail', | ||
'STATE_CHANGE_ERROR': 'State change error: {{ message }}', | ||
'STATE_FORBIDDEN_ERROR': 'A server 403 error occured: {{ message }}', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well 403 error could be something else than permission, so not sure. I will let you debate with @jpetitcolas as this was is wording for the error when we started discussing this issue (#1031 (comment)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let use current message, as it will be useful for users to report problem to their developper. Moreover, it is easily overridable now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok |
||
'NOT_FOUND': 'Not Found', | ||
'NOT_FOUND_DETAILS': 'The page you are looking for cannot be found. Take a break before trying again.', | ||
}); | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export default function httpErrorHandler($rootScope, HttpErrorService) { | ||
$rootScope.$on("$stateChangeError", function handleError(event, toState, toParams, fromState, fromParams, error) { | ||
HttpErrorService.handleError(event, toState, toParams, fromState, fromParams, error); | ||
}); | ||
} | ||
|
||
httpErrorHandler.$inject = ['$rootScope','httpErrorService']; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
const HttpErrorService = require('../../../../../ng-admin/Main/component/provider/HttpErrorService'); | ||
|
||
describe('Http Error Service', () => { | ||
let $state; | ||
let $translate; | ||
let httpErrorService; | ||
let notification; | ||
|
||
angular.module('test_httpErrorService', []) | ||
.service('$state', () => ({ | ||
go: jasmine.createSpy('$state.go'), | ||
})) | ||
.service('$translate', () => ( | ||
jasmine.createSpy('$translate').and.returnValue({ then: cb => cb('translated') }) | ||
)) | ||
.service('notification', () => ({ | ||
log: jasmine.createSpy('notification.log'), | ||
})) | ||
.provider('HttpErrorService', HttpErrorService); | ||
|
||
beforeEach(angular.mock.module('test_httpErrorService')); | ||
|
||
beforeEach(inject(function (_$state_, _$translate_, _HttpErrorService_, _notification_) { | ||
$state = _$state_; | ||
$translate = _$translate_; | ||
httpErrorService = _HttpErrorService_; | ||
notification = _notification_; | ||
})); | ||
|
||
it('should redirect to `ma-404` state when receiving a 404 error', () => { | ||
const event = { | ||
preventDefault: jasmine.createSpy(), | ||
}; | ||
|
||
const error = { status: 404 }; | ||
httpErrorService.handleError(event, '', '', '', '', error); | ||
|
||
expect(event.preventDefault).toHaveBeenCalled(); | ||
expect($state.go).toHaveBeenCalledWith('ma-404'); | ||
}); | ||
|
||
it('should display a translated forbidden error notification in case of 403', () => { | ||
const error = { | ||
status: 403, | ||
data: { | ||
message: 'Forbidden access', | ||
}, | ||
}; | ||
|
||
try { | ||
httpErrorService.handleError('', '', '', '', '', error); | ||
expect(true).toBe(false); | ||
} catch (e) { | ||
// should throw an exception in case of 403 | ||
} | ||
|
||
expect($translate).toHaveBeenCalledWith('STATE_FORBIDDEN_ERROR', { message: 'Forbidden access' }); | ||
expect(notification.log).toHaveBeenCalledWith('translated', { addnCls: 'humane-flatty-error' }); | ||
}); | ||
|
||
it('should display generic translated error notification if neither 404 nor 403', () => { | ||
const error = { | ||
status: 500, | ||
data: { | ||
message: 'Unknown error', | ||
} | ||
}; | ||
|
||
try { | ||
httpErrorService.handleError('', '', '', '', '', error); | ||
expect(true).toBe(false); | ||
} catch (e) { | ||
// should throw an exception in case of 500 | ||
} | ||
|
||
expect($translate).toHaveBeenCalledWith('STATE_CHANGE_ERROR', { message: 'Unknown error' }); | ||
expect(notification.log).toHaveBeenCalledWith('translated', { addnCls: 'humane-flatty-error' }); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/httpErrorService/HttpErrorService/