From d17fe0362c9844921ed2fbc3f9b25dd559512e3c Mon Sep 17 00:00:00 2001 From: Michael Krotscheck Date: Wed, 11 Jun 2014 11:21:06 -0700 Subject: [PATCH] Error message & notification handling This patch adds the notification module, a mostly-self contained way of surfacing errors, warnings and notifications to the user. These can either be initiated by the application, or can be received from the server. Among other things, the impact of this will be that the user will be notified if one of our HTTP requests fails. This fixes the situation where storyboard would 'fail silently' when someone is trying to accomplish something. - New notification module. - Notifications directive, for a list of all notifications that made it past filters. - New Notification service that handles all error broadcasts in the system. - New severity directive. - HTTP Error broadcaster now uses the notification service. - Session now uses Notification service to handle 401 errors. - Added new notification filters to strip out 200's and template load requests. Change-Id: I4aaa50404560d1c1f14f639e3ac68b2ac2d4380c --- src/app/auth/service/session.js | 31 ++-- .../controller/notifications_controller.js | 69 +++++++++ .../notification/directive/notifications.js | 30 ++++ src/app/notification/module.js | 21 +++ src/app/notification/provider/severity.js | 27 ++++ .../service/notification_service.js | 136 ++++++++++++++++++ .../services/http/http_error_broadcaster.js | 19 ++- src/app/services/module.js | 2 +- .../notification/http_message_filter.js | 92 ++++++++++++ src/app/storyboard/module.js | 4 +- src/app/templates/error/notifications.html | 44 ++++++ src/index.html | 9 +- src/theme/base/notification.less | 51 +++++++ src/theme/main.less | 1 + src/theme/storyboard/theme.less | 7 + .../http/http_error_broadcaster_test.js | 36 ++--- 16 files changed, 531 insertions(+), 48 deletions(-) create mode 100644 src/app/notification/controller/notifications_controller.js create mode 100644 src/app/notification/directive/notifications.js create mode 100644 src/app/notification/module.js create mode 100644 src/app/notification/provider/severity.js create mode 100644 src/app/notification/service/notification_service.js create mode 100644 src/app/services/notification/http_message_filter.js create mode 100644 src/app/templates/error/notifications.html create mode 100644 src/theme/base/notification.less diff --git a/src/app/auth/service/session.js b/src/app/auth/service/session.js index e167f03e..c3ea4323 100644 --- a/src/app/auth/service/session.js +++ b/src/app/auth/service/session.js @@ -20,7 +20,7 @@ */ angular.module('sb.auth').factory('Session', function (SessionState, AccessToken, $rootScope, $log, $q, $state, User, - RefreshManager) { + RefreshManager, Notification) { 'use strict'; /** @@ -68,10 +68,10 @@ angular.module('sb.auth').factory('Session', * validate a token after a long break in using StoryBoard. * Even if refresh is not necessary right now the tryRefresh method * will just resolve immediately. - */ + */ var deferred = $q.defer(); - RefreshManager.tryRefresh().then(function() { + RefreshManager.tryRefresh().then(function () { var id = AccessToken.getIdToken(); User.read({id: id}, @@ -109,16 +109,21 @@ angular.module('sb.auth').factory('Session', */ initializeSession(); - // If we ever encounter a 401 error, make sure the session is destroyed. - $rootScope.$on('http_401', function ($log) { - RefreshManager.tryRefresh().then( - function () { - $log.info('Token refreshsed on 401'); - }, function() { - $log.info('Could not refresh token. Destroying session'); - destroySession(); - }); - }); + // We're using -1 as the priority, to ensure that this is intercepted + // before anything else happens. + Notification.intercept(function (message) { + if (message.type === 'http' && message.message === 401) { + RefreshManager.tryRefresh().then( + function () { + $log.info('Token refreshed on 401'); + }, function () { + $log.info('Could not refresh token. ' + + 'Destroying session'); + destroySession(); + }); + return true; // Stop processing this notification. + } + }, -1); // Expose the methods for this service. return { diff --git a/src/app/notification/controller/notifications_controller.js b/src/app/notification/controller/notifications_controller.js new file mode 100644 index 00000000..9cd2b0af --- /dev/null +++ b/src/app/notification/controller/notifications_controller.js @@ -0,0 +1,69 @@ +/* + * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +/** + * This module acts as the central routing point for all errors that occur + * within storyboard. + */ +angular.module('sb.notification').controller('NotificationsController', + function ($scope, Notification) { + 'use strict'; + + var defaultDisplayCount = 5; + + $scope.displayCount = defaultDisplayCount; + + $scope.notifications = []; + + /** + * Remove a notification from the display list. + * + * @param notification + */ + $scope.remove = function (notification) { + var idx = $scope.notifications.indexOf(notification); + if (idx > -1) { + $scope.notifications.splice(idx, 1); + } + + // If the notification list length drops below default, make + // sure we reset the limit. + if ($scope.notifications.length <= defaultDisplayCount) { + $scope.displayCount = defaultDisplayCount; + } + }; + + /** + * Reveal more notifications, either current count + 5 or the total + * number of messages, whichever is smaller. + */ + $scope.showMore = function () { + // Set this to something big. + $scope.displayCount = Math.min($scope.notifications.length, + $scope.displayCount + 5); + }; + + /** + * Set up a notification subscriber, and make sure it's removed when + * the scope is destroyed. + */ + $scope.$on('$destroy', Notification.subscribe( + function (notification) { + $scope.notifications.push(notification); + } + ) + ); + }); \ No newline at end of file diff --git a/src/app/notification/directive/notifications.js b/src/app/notification/directive/notifications.js new file mode 100644 index 00000000..5830e50e --- /dev/null +++ b/src/app/notification/directive/notifications.js @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +/** + * This directive is a notification list renderer with all the trimmings. + * Errors broadcast throughout the system will be collected and displayed here. + */ +angular.module('sb.notification').directive('notifications', + function () { + 'use strict'; + + return { + restrict: 'E', + templateUrl: 'app/templates/error/notifications.html', + controller: 'NotificationsController' + }; + }); \ No newline at end of file diff --git a/src/app/notification/module.js b/src/app/notification/module.js new file mode 100644 index 00000000..07e24221 --- /dev/null +++ b/src/app/notification/module.js @@ -0,0 +1,21 @@ +/* + * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +/** + * This module acts as the central routing point for all errors that occur + * within storyboard. + */ +angular.module('sb.notification', []); \ No newline at end of file diff --git a/src/app/notification/provider/severity.js b/src/app/notification/provider/severity.js new file mode 100644 index 00000000..59d04ce7 --- /dev/null +++ b/src/app/notification/provider/severity.js @@ -0,0 +1,27 @@ +/* + * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + + +/** + * A list of severity levels used within this module. + */ + +angular.module('sb.notification').constant('Severity', { + ERROR: 'error', + WARNING: 'warning', + INFO: 'info', + SUCCESS: 'success' +}); \ No newline at end of file diff --git a/src/app/notification/service/notification_service.js b/src/app/notification/service/notification_service.js new file mode 100644 index 00000000..f3a0d5e5 --- /dev/null +++ b/src/app/notification/service/notification_service.js @@ -0,0 +1,136 @@ +/* + * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +/** + * The centralized notification service, aka the central routing point for all + * broadcast notifications. You use it by registering interceptors and + * subscribers, and handling any messages that are sent(). + * + * Interceptors are intended to be both filters and decorators, where + * individual components can handle messages before either terminating + * the dispatch chain, or passing them on to the next interceptor. In this + * fashion it is easy to handle specific messages in one context while + * other messages in another. + * + * Subscribers are processors that handle all messages vetted by our + * interceptors. + */ +angular.module('sb.notification').factory('Notification', + function ($log, Severity) { + 'use strict'; + + var subscribers = []; + var interceptors = []; + + return { + /** + * Send a notification. + * + * @param type A type identifier, such as a string. Use this for + * your subscribers to determine what kind of a message you're + * working with. + * @param message A human readable message for this notification. + * @param severity The severity of this message, any of the + * constants provided in Severity. + * @param cause The cause of this message, perhaps a large amount + * of debugging information. + * @param callback If this message prompts the user to do + * something, then pass a function here and it'll be rendered + * in the message. + * @param callbackLabel A custom label for the callback. + */ + send: function (type, message, severity, cause, callback, + callbackLabel) { + // Return the type. + if (!type || !message) { + $log.warn('Invoked Notification.send() without a type' + + ' or message.'); + return; + } + + // sanitize our data. + var n = { + 'type': type, + 'message': message, + 'severity': severity || Severity.INFO, + 'cause': cause || null, + 'callback': callback || null, + 'callbackLabel': callbackLabel || null, + 'date': new Date() + }; + + // Iterate through the interceptors. + for (var i = 0; i < interceptors.length; i++) { + if (!!interceptors[i].method(n)) { + return; + } + } + + // Iterate through the subscribers. + for (var j = 0; j < subscribers.length; j++) { + subscribers[j](n); + } + }, + + /** + * Add a message interceptor to the notification system, in order + * to determine which messages you'd like to handle. + * + * @param interceptor A method that accepts a notification. You can + * return true from the interceptor method to indicate that this + * message has been handled and should not be processed further. + * @param priority An optional priority (default 999). + * Interceptors with a lower priority will go first. + * @returns {Function} A method that may be called to remove the + * interceptor at a later time. + */ + intercept: function (interceptor, priority) { + + var i = { + 'priority': priority || 999, + 'method': interceptor + }; + + // Add and sort the interceptors. We're using unshift here so + // that the sorting algorithm ends up being a single-pass + // bubble sort. + interceptors.unshift(i); + interceptors.sort(function (a, b) { + return a.priority - b.priority; + }); + + return function () { + interceptors.remove(i); + }; + }, + + /** + * Subscribe to all messages that make it through our interceptors. + * + * @param subscriber A subscriber method that receives a + * notification. + * @returns {Function} A method that may be called to remove the + * subscriber at a later time. + */ + subscribe: function (subscriber) { + subscribers.push(subscriber); + + return function () { + subscribers.remove(subscriber); + }; + } + }; + }); \ No newline at end of file diff --git a/src/app/services/http/http_error_broadcaster.js b/src/app/services/http/http_error_broadcaster.js index f24bc7ae..0107fad1 100644 --- a/src/app/services/http/http_error_broadcaster.js +++ b/src/app/services/http/http_error_broadcaster.js @@ -16,8 +16,8 @@ /** * An HTTP request interceptor that broadcasts response status codes to the - * rest of the application as events. These events are broadcast before the - * error response itself is passed back to the receiving closure, so please + * rest of the application as notifications. These events are broadcast before + * the error response itself is passed back to the receiving closure, so please * keep that in mind as you base your application logic on it. * * @author Michael Krotscheck @@ -25,24 +25,24 @@ angular.module('sb.services') // Create an HTTP Error Broadcaster that intercepts requests and lets the // rest of the application know about what happened. - .factory('httpErrorBroadcaster', function ($q, $rootScope) { + .factory('httpErrorBroadcaster', + function ($q, $rootScope, Notification, Severity) { 'use strict'; - function sendEvent(status, body) { + function sendEvent(severity, response) { // Only send an event if a status is passed. - if (!!status) { - $rootScope.$broadcast('http_' + status, body || {}); + if (!!response.status) { + Notification.send('http', response.status, severity, response); } } - return { /** * Handle a success response. */ response: function (response) { if (!!response) { - sendEvent(response.status); + sendEvent(Severity.SUCCESS, response); } return response; }, @@ -51,9 +51,8 @@ angular.module('sb.services') * Handle a fail response. */ responseError: function (response) { - if (!!response) { - sendEvent(response.status, response.data); + sendEvent(Severity.ERROR, response); } return $q.reject(response); diff --git a/src/app/services/module.js b/src/app/services/module.js index 3e2afeb8..38cf2143 100644 --- a/src/app/services/module.js +++ b/src/app/services/module.js @@ -19,4 +19,4 @@ * used by the storyboard client. Its resources are available via injection to * any module that declares it as a dependency. */ -angular.module('sb.services', ['ngResource']); \ No newline at end of file +angular.module('sb.services', ['ngResource', 'sb.notification']); \ No newline at end of file diff --git a/src/app/services/notification/http_message_filter.js b/src/app/services/notification/http_message_filter.js new file mode 100644 index 00000000..04d3cb01 --- /dev/null +++ b/src/app/services/notification/http_message_filter.js @@ -0,0 +1,92 @@ +/* + * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +/** + * Notification interceptors for this library. + */ +angular.module('sb.services') + .run(function (Notification) { + 'use strict'; + + /** + * Template load requests are done via $http, so we need to filter + * those out first. + */ + function filterTemplateRequests(message) { + if (message.type !== 'http') { + return; + } + + var request = message.cause; + var url = request.config.url; + + if (url.substr(-5) === '.html') { + return true; + } + } + + /** + * A notification interceptor that filters successful HTTP requests. + * It's registered at priority 999 (the lowest) so that other + * interceptors can get access to this message first (ex: statistics). + */ + function filterSuccessful(message) { + var response = message.cause; + if (message.type !== 'http' || !response) { + return; + } + + // All 200 requests are filtered out. + if (response.status === 200) { + return true; + } + } + + /** + * A notification interceptor that rewrites HTTP status codes to + * human readable messages. + */ + function rewriteHttpStatus(message) { + + if (message.type !== 'http') { + // Do nothing. + return; + } + + var httpStatus = message.message; + var request = message.cause; + + if (!httpStatus || !request || !request.data) { + return; + } + var data = request.data; + var method = request.config.method; + var url = request.config.url; + + message.message = httpStatus + ': ' + method + ' ' + url + ': '; + + if (data.hasOwnProperty('faultstring')) { + message.message += data.faultstring; + } else { + message.message += 'No error details available.'; + } + } + + // Apply the interceptors. + Notification.intercept(filterTemplateRequests, -1); + Notification.intercept(filterSuccessful, 999); + Notification.intercept(rewriteHttpStatus, 1000); + }); \ No newline at end of file diff --git a/src/app/storyboard/module.js b/src/app/storyboard/module.js index 15280651..05ec0380 100644 --- a/src/app/storyboard/module.js +++ b/src/app/storyboard/module.js @@ -24,8 +24,8 @@ */ angular.module('storyboard', [ 'sb.services', 'sb.templates', 'sb.dashboard', 'sb.pages', 'sb.projects', - 'sb.auth', 'sb.story', 'sb.profile', 'ui.router', 'ui.bootstrap', - 'monospaced.elastic']) + 'sb.auth', 'sb.story', 'sb.profile', 'sb.notification', 'ui.router', + 'ui.bootstrap', 'monospaced.elastic']) .config(function ($provide, $urlRouterProvider, $locationProvider, $httpProvider, msdElasticConfig) { 'use strict'; diff --git a/src/app/templates/error/notifications.html b/src/app/templates/error/notifications.html new file mode 100644 index 00000000..51abc7b4 --- /dev/null +++ b/src/app/templates/error/notifications.html @@ -0,0 +1,44 @@ +
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ Showing {{displayCount}} of {{notifications.length}} + messages. + + + Show More + +
+
+ + + \ No newline at end of file diff --git a/src/index.html b/src/index.html index e43fe3c5..8a461235 100644 --- a/src/index.html +++ b/src/index.html @@ -47,9 +47,10 @@ -
-
-
- + +
+
+
+ diff --git a/src/theme/base/notification.less b/src/theme/base/notification.less new file mode 100644 index 00000000..813238e6 --- /dev/null +++ b/src/theme/base/notification.less @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2014 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +/** + * Styles specific to error display. + */ +.notification-container { + position: absolute; + z-index: 2000; + + @media (max-width: @screen-xs-max) { + top: @error-container-margin-vertical; + right: @error-container-margin-horizontal; + left: @error-container-margin-horizontal; + } + + @media (min-width: @screen-sm-min) and (max-width: @screen-sm-max) { + top: @error-container-margin-vertical; + right: @error-container-margin-horizontal; + width: @error-container-max-width; + } + + @media (min-width: @screen-md-min) and (max-width: @screen-md-max) { + top: @error-container-margin-vertical; + right: @error-container-margin-horizontal; + width: @error-container-max-width; + } + + @media (min-width: @screen-lg-min) { + top: @error-container-margin-vertical; + right: @error-container-margin-horizontal; + width: @error-container-max-width; + } + + .alert { + box-shadow: 3px 3px 10px rgba(0, 0, 0, .5) + } +} \ No newline at end of file diff --git a/src/theme/main.less b/src/theme/main.less index b25ff5e7..d31f159c 100644 --- a/src/theme/main.less +++ b/src/theme/main.less @@ -31,6 +31,7 @@ // Add our own custom icon font. @import './base/custom_font_icons.less'; // Module specific styles +@import './base/notification.less'; @import './base/body.less'; @import './base/logged_in.less'; @import './base/auth.less'; diff --git a/src/theme/storyboard/theme.less b/src/theme/storyboard/theme.less index 761415ba..2835adfa 100644 --- a/src/theme/storyboard/theme.less +++ b/src/theme/storyboard/theme.less @@ -29,6 +29,13 @@ @font-size-h5: (@font-size-base); @font-size-h6: (ceil(@font-size-base * 0.85)); + +// Error messages +// -------------- +@error-container-margin-horizontal: 10px; +@error-container-margin-vertical: 10px; +@error-container-max-width: 400px; + // Navbar // ------------------------- // Basics of a navbar diff --git a/test/unit/services/http/http_error_broadcaster_test.js b/test/unit/services/http/http_error_broadcaster_test.js index 1e646c77..ddb983e2 100644 --- a/test/unit/services/http/http_error_broadcaster_test.js +++ b/test/unit/services/http/http_error_broadcaster_test.js @@ -21,7 +21,7 @@ describe('httpErrorBroadcaster', function () { 'use strict'; - var $rootScope, $httpBackend, $resource, MockResource; + var notification, $httpBackend, $resource, MockResource; var errorResponse = { error_code: 404, @@ -35,31 +35,31 @@ describe('httpErrorBroadcaster', function () { inject(function ($injector) { // Capture various providers for later use. - $rootScope = $injector.get('$rootScope'); + notification = $injector.get('Notification'); $httpBackend = $injector.get('$httpBackend'); $resource = $injector.get('$resource'); MockResource = $resource('/foo/:id', {id: '@id'}); }); - // Start listening to the broadcast method. - spyOn($rootScope, '$broadcast'); }); - it('should capture events on the $rootScope', function (done) { + it('should dispatch events to the Notification framework', + function () { + // Start listening to the broadcast method. + spyOn(notification, 'send').and.callFake( + function(type, code, severity) { + expect(type).toEqual('http'); + expect(code).toEqual(553); + expect(severity).toEqual('error'); + } + ); - // Prepare the HTTP Backend - $httpBackend.when('GET', '/foo/99') - .respond(553, JSON.stringify(errorResponse)); + // Prepare the HTTP Backend + $httpBackend.when('GET', '/foo/99') + .respond(553, JSON.stringify(errorResponse)); - // Handle a result. - function handleResult() { - expect($rootScope.$broadcast) - .toHaveBeenCalledWith('http_553', errorResponse); - done(); - } - - MockResource.get({'id': 99}, handleResult, handleResult); - $httpBackend.flush(); - }); + MockResource.get({'id': 99}); + $httpBackend.flush(); + }); });