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

Http error handling #1273

Merged
merged 11 commits into from
Dec 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions doc/Theming.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,3 +181,37 @@ entity.errorMessage(function (response) {
return 'Global error: ' + response.status + '(' + response.data + ')';
});
```
## Customizing HTTP Error Messages

If you want to override, patch or extend the way HTTP errors are handled by the ng-admin [Http Error Service](../src/javascripts/ng-admin/Main/component/provider/HttpErrorService.js) then you may use an [Angular decorator](https://docs.angularjs.org/guide/decorators), as per the below example:

Create a decorator, HttpErrorDecorator.js

```js
// Change HTTP 403 error notification to display them as information
// and not errors ('humane-flatty-info' instead of 'humane-flatty-error')
export const HttpErrorDecorator = ($delegate, $translate, notification) => {
$delegate.handle403Error = error => {
$translate('STATE_FORBIDDEN_ERROR', {
message: error.data.message,
}).then(text => notification.log(text, {
addnCls: 'humane-flatty-info',
}));

throw error;
};

return $delegate;
}

HttpErrorDecorator.$inject = ['$delegate', '$translate', 'notification'];

export default HttpErrorDecorator;

```

Bind the decorator to your application:

```js
myApp.decorator('HttpErrorService',require('HttpErrorDecorator'));
```
3 changes: 2 additions & 1 deletion src/javascripts/ng-admin/Main/MainModule.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ var MainModule = angular.module('main', ['ui.router', 'restangular', 'pascalprec
MainModule.controller('AppController', require('./component/controller/AppController'));
MainModule.controller('DashboardController', require('./component/controller/DashboardController'));
MainModule.provider('NgAdminConfiguration', require('./component/provider/NgAdminConfiguration'));
MainModule.provider('HttpErrorService', require('./component/provider/HttpErrorService'));

MainModule.filter('orderElement', require('./component/filter/OrderElement'));
MainModule.filter('stripTags', require('./component/filter/StripTags'));
Expand All @@ -19,5 +20,5 @@ MainModule.config(require('./config/http'));
MainModule.config(require('./config/routing'));
MainModule.config(require('./config/translate'));

MainModule.run(require('./run/ErrorHandler'));
MainModule.run(require('./run/HttpErrorHandler'));
MainModule.run(require('./run/Loader'));
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, be consistent and give the same API for all the handlers: (event, error) or (error, event).
I'm even in favor of (error, event, toState, toParams, fromState, fromParams).

It will be more customable for the next developers and the doc will be more clear too.

Copy link
Contributor Author

@Phocea Phocea Dec 20, 2016

Choose a reason for hiding this comment

The 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)
Nothing is mentionned about this in google guidelines.

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 };
1 change: 1 addition & 0 deletions src/javascripts/ng-admin/Main/config/translate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A server 403 error occured to A permission error occured?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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))

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.',
});
Expand Down
13 changes: 0 additions & 13 deletions src/javascripts/ng-admin/Main/run/ErrorHandler.js

This file was deleted.

7 changes: 7 additions & 0 deletions src/javascripts/ng-admin/Main/run/HttpErrorHandler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function HttpErrorHandler($rootScope, HttpErrorService) {
$rootScope.$on("$stateChangeError", (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 HttpErrorServiceProvider = 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', HttpErrorServiceProvider);

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' });
});
});