Merge lp:~blake-rouse/maas/error-overlay-directive into lp:~maas-committers/maas/trunk
- error-overlay-directive
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Blake Rouse | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 3844 | ||||
Proposed branch: | lp:~blake-rouse/maas/error-overlay-directive | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Diff against target: |
807 lines (+497/-162) 13 files modified
src/maasserver/context_processors.py (+2/-0) src/maasserver/static/js/angular/controllers/error.js (+0/-37) src/maasserver/static/js/angular/controllers/tests/test_error.js (+0/-83) src/maasserver/static/js/angular/directives/error_overlay.js (+173/-0) src/maasserver/static/js/angular/directives/error_toggle.js (+67/-0) src/maasserver/static/js/angular/directives/tests/test_error_overlay.js (+160/-0) src/maasserver/static/js/angular/directives/tests/test_error_toggle.js (+74/-0) src/maasserver/static/js/angular/factories/region.js (+2/-0) src/maasserver/static/js/angular/factories/tests/test_region.js (+8/-0) src/maasserver/static/js/angular/maas.js (+0/-4) src/maasserver/static/js/angular/services/error.js (+6/-11) src/maasserver/static/js/angular/services/tests/test_error.js (+3/-25) src/maasserver/templates/maasserver/index.html (+2/-2) |
||||
To merge this branch: | bzr merge lp:~blake-rouse/maas/error-overlay-directive | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Raphaël Badin (community) | Approve | ||
Andres Rodriguez (community) | Needs Information | ||
Review via email: mp+255426@code.launchpad.net |
Commit message
Add maas-error-overlay and maas-error-toggle directives.
Used to show an error or connecting information to the region controller over the websocket. It overrides ng-view with the error header and message to show the client an error has occurred or that a connection has not been made or has been lost to the region.
Description of the change
The loading spinner that is show next to the title is currently misaligned. A following branch from Rich will fix this issue, as the spinner was never designed to go in this new location.
Andres Rodriguez (andreserl) wrote : | # |
If this is just by the designs in the bug, shouldn't we be everything but the node list? I mean, a white page with a message is just not so cool.
And meant for this to be needs information.
Blake Rouse (blake-rouse) wrote : | # |
So this will occur only on the first load of a page with the websocket. Meaning when you load the node listing it will occur. Most of the time the connection is made so fast that it doesn't even show. It will only show if the connection cannot be made, the connection is lost, or a fatal error occurs. Now switching between the node listing to nodes details to node events or other pages that use angular do not reload the page at all. Its the same page just a different view and its the same websocket connection. The end goal would do this for the entire web UI, so the page would only need to be loaded once and the websocket connection would never need to be created again.
You can't show something like failed to load node listing, because this can happen anywhere. Chances are uses will never see this view, but when something happens where the connection has failed it much better to show why then just leave them guessing.
There are three different messages that can be displayed from this directive.
1. Connecting
This occurs when the websocket is trying to connect to the region, and a connection has never been made to the region. This will only occur if the user does not have access to the 5240 port. The url is shown so they know where its trying to connect.
http://
2. Connection to region lost, reconnection...
Occurs when the websocket connection goes down. Normally because the region has crashed or they have lost a route to that host or that port. This will continue to be shown until the connection is remade. The client will keep try over and over again until the connection is made.
http://
3. Erro occurred communicating with region
Only occurs when something really wrong has happened and the region did not respond as expected. This is very rare, but is needed to alter the user of a very bad error.
http://
You will notice that in 1 and 2 the spinner is misaligned, Rich is looking into fixing this for me.
Raphaël Badin (rvb) wrote : | # |
Now, I think Andres has a point, the terminology "region" is a bit confusing I think. I would just have "Connecting…". This way it will be obvious that it's connecting to the *server*.
Looking at screenshot #2: do you mean that if the connection drops, the entire page becomes white with the "Connection lost…" message? If that's the case, then it's weird… if the network goes down and then comes back up, the page will "flicker" in the most unpleasant way.
Andres Rodriguez (andreserl) wrote : | # |
Also... Displaying that we failed to connect to websocket or showing the
url and stuff like that is just not nice. I dont think the user cares or
not.
For example, in website like Facebook, when there's a problem you get a
message like "sorry, something went wrong try again".
If we display error w to connect to websocket ws://localhost/xyz is just
not nice. I think we should just tell them that *something* went wrong.
On Apr 10, 2015 10:27 AM, "Raphaël Badin" <email address hidden>
wrote:
> Review: Needs Information
>
> Now, I think Andres has a point, the terminology "region" is a bit
> confusing I think. I would just have "Connecting…". This way it will be
> obvious that it's connecting to the *server*.
>
> Looking at screenshot #2: do you mean that if the connection drops, the
> entire page becomes white with the "Connection lost…" message? If that's
> the case, then it's weird… if the network goes down and then comes back up,
> the page will "flicker" in the most unpleasant way.
> --
>
> https:/
> You are reviewing the proposed merge of
> lp:~blake-rouse/maas/error-overlay-directive into lp:maas.
>
Blake Rouse (blake-rouse) wrote : | # |
I will update it to "Connecting...", but I think he was talking more specifically about the detailed error message as well.
Yes the entire page changes to that. This is the way design implemented it, I wanted to just do a modal overlay, but they said this works better. Anyway its not that fast, if the connection goes down it will take a moment for it to reconnect. If the connection goes down and back up quickly you will not see the error. I will update the directive to only show if disconnected for more than 1 second.
I will generalize the errors displayed and just log the detailed errors to the browser console.
Blake Rouse (blake-rouse) wrote : | # |
Okay I have cleaned this up to be how we discussed at the sprint. It is ready to get a final review.
Raphaël Badin (rvb) wrote : | # |
Looks good. Thanks for the changes.
Preview Diff
1 | === modified file 'src/maasserver/context_processors.py' |
2 | --- src/maasserver/context_processors.py 2015-04-09 18:02:51 +0000 |
3 | +++ src/maasserver/context_processors.py 2015-04-24 19:35:27 +0000 |
4 | @@ -55,6 +55,8 @@ |
5 | 'js/angular/services/error.js', |
6 | 'js/angular/services/validation.js', |
7 | 'js/angular/services/browser.js', |
8 | + 'js/angular/directives/error_overlay.js', |
9 | + 'js/angular/directives/error_toggle.js', |
10 | 'js/angular/directives/call_to_action.js', |
11 | 'js/angular/directives/power_parameters.js', |
12 | 'js/angular/directives/os_select.js', |
13 | |
14 | === removed file 'src/maasserver/static/js/angular/controllers/error.js' |
15 | --- src/maasserver/static/js/angular/controllers/error.js 2015-03-17 17:57:17 +0000 |
16 | +++ src/maasserver/static/js/angular/controllers/error.js 1970-01-01 00:00:00 +0000 |
17 | @@ -1,37 +0,0 @@ |
18 | -/* Copyright 2015 Canonical Ltd. This software is licensed under the |
19 | - * GNU Affero General Public License version 3 (see the file LICENSE). |
20 | - * |
21 | - * MAAS Error Controller |
22 | - */ |
23 | - |
24 | -angular.module('MAAS').controller('ErrorController', [ |
25 | - '$scope', '$rootScope', '$location', 'ErrorService', function($scope, |
26 | - $rootScope, $location, ErrorService) { |
27 | - |
28 | - // Set the title and page. |
29 | - $rootScope.title = "Error"; |
30 | - $rootScope.page = ""; |
31 | - |
32 | - // Get the error message and clear it in the service. |
33 | - $scope.error = ErrorService._error; |
34 | - ErrorService._error = null; |
35 | - |
36 | - // Get the url to return to when back is clicked. |
37 | - $scope.backUrl = ErrorService._backUrl; |
38 | - ErrorService._backUrl = null; |
39 | - |
40 | - // If the error is not a string then the user should not be here. |
41 | - if(!angular.isString($scope.error)) { |
42 | - // Go back to index. |
43 | - $location.path('/'); |
44 | - } |
45 | - |
46 | - // Go back to previous page. |
47 | - $scope.goBack = function() { |
48 | - if(angular.isString($scope.backUrl)) { |
49 | - $location.path($scope.backUrl); |
50 | - } else { |
51 | - $location.path('/'); |
52 | - } |
53 | - }; |
54 | - }]); |
55 | |
56 | === removed file 'src/maasserver/static/js/angular/controllers/tests/test_error.js' |
57 | --- src/maasserver/static/js/angular/controllers/tests/test_error.js 2015-03-18 13:13:02 +0000 |
58 | +++ src/maasserver/static/js/angular/controllers/tests/test_error.js 1970-01-01 00:00:00 +0000 |
59 | @@ -1,83 +0,0 @@ |
60 | -/* Copyright 2015 Canonical Ltd. This software is licensed under the |
61 | - * GNU Affero General Public License version 3 (see the file LICENSE). |
62 | - * |
63 | - * Unit tests for ErrorController. |
64 | - */ |
65 | - |
66 | -describe("ErrorController", function() { |
67 | - |
68 | - // Load the MAAS module. |
69 | - beforeEach(module("MAAS")); |
70 | - |
71 | - // Grab the needed angular pieces. |
72 | - var $controller, $rootScope, $location, $scope; |
73 | - beforeEach(inject(function($injector) { |
74 | - $controller = $injector.get("$controller"); |
75 | - $rootScope = $injector.get("$rootScope"); |
76 | - $location = $injector.get("$location"); |
77 | - $scope = $rootScope.$new(); |
78 | - })); |
79 | - |
80 | - // Load the ErrorService. |
81 | - var ErrorService; |
82 | - beforeEach(inject(function($injector) { |
83 | - ErrorService = $injector.get("ErrorService"); |
84 | - })); |
85 | - |
86 | - // Makes the ErrorController |
87 | - function makeController() { |
88 | - return $controller("ErrorController", { |
89 | - $scope: $scope, |
90 | - $rootScope: $rootScope, |
91 | - $location: $location, |
92 | - ErrorService: ErrorService |
93 | - }); |
94 | - } |
95 | - |
96 | - it("sets $rootScope title and page", function() { |
97 | - var controller = makeController(); |
98 | - expect($rootScope.title).toBe("Error"); |
99 | - expect($rootScope.page).toBe(""); |
100 | - }); |
101 | - |
102 | - it("sets error from ErrorService and clears error", function() { |
103 | - var error = makeName("error"); |
104 | - ErrorService._error = error; |
105 | - var controller = makeController(); |
106 | - expect($scope.error).toBe(error); |
107 | - expect(ErrorService._error).toBeNull(); |
108 | - }); |
109 | - |
110 | - it("sets backUrl from ErrorService and clears backUrl", function() { |
111 | - var backUrl = makeName("url"); |
112 | - ErrorService._backUrl = backUrl; |
113 | - var controller = makeController(); |
114 | - expect($scope.backUrl).toBe(backUrl); |
115 | - expect(ErrorService._backUrl).toBeNull(); |
116 | - }); |
117 | - |
118 | - it("calls $location.path if missing error", function() { |
119 | - spyOn($location, "path"); |
120 | - var controller = makeController(); |
121 | - expect($location.path).toHaveBeenCalledWith("/"); |
122 | - }); |
123 | - |
124 | - describe("goBack", function() { |
125 | - |
126 | - it("calls $location.path with backUrl when set", function() { |
127 | - var controller = makeController(); |
128 | - $scope.backUrl = makeName("url"); |
129 | - spyOn($location, "path"); |
130 | - $scope.goBack(); |
131 | - expect($location.path).toHaveBeenCalledWith($scope.backUrl); |
132 | - }); |
133 | - |
134 | - it("calls $location.path with index path if backUrl not set", |
135 | - function() { |
136 | - var controller = makeController(); |
137 | - spyOn($location, "path"); |
138 | - $scope.goBack(); |
139 | - expect($location.path).toHaveBeenCalledWith("/"); |
140 | - }); |
141 | - }); |
142 | -}); |
143 | |
144 | === added file 'src/maasserver/static/js/angular/directives/error_overlay.js' |
145 | --- src/maasserver/static/js/angular/directives/error_overlay.js 1970-01-01 00:00:00 +0000 |
146 | +++ src/maasserver/static/js/angular/directives/error_overlay.js 2015-04-24 19:35:27 +0000 |
147 | @@ -0,0 +1,173 @@ |
148 | +/* Copyright 2015 Canonical Ltd. This software is licensed under the |
149 | + * GNU Affero General Public License version 3 (see the file LICENSE). |
150 | + * |
151 | + * Error overlay. |
152 | + * |
153 | + * Directive overrides the entire transcluded element if an error occurs or |
154 | + * connection to the region over the websocket fails or becomes disconnected. |
155 | + */ |
156 | + |
157 | +angular.module('MAAS').run(['$templateCache', function ($templateCache) { |
158 | + // Inject the error_overlay.html into the template cache. |
159 | + $templateCache.put('directive/templates/error_overlay.html', [ |
160 | + '<header id="error-header" class="page-header" data-ng-show="show()">', |
161 | + '<div class="inner-wrapper">', |
162 | + '<h1 class="page-header__title eight-col">', |
163 | + '<span class="loader" data-ng-hide="clientError"></span> ', |
164 | + '{$ getTitle() $}', |
165 | + '</h1>', |
166 | + '<div class="page-header__actions four-col last-col">', |
167 | + '<div class="page-header__cta two-col no-margin-bottom ', |
168 | + 'last-col">', |
169 | + '<button class="two-col cta-ubuntu"', |
170 | + 'data-ng-click="reload()"', |
171 | + 'data-ng-show="clientError">Reload</button>', |
172 | + '</div>', |
173 | + '</div>', |
174 | + '<div class="page-header__dropdown ng-hide" ', |
175 | + 'data-ng-show="error">', |
176 | + '<div class="page-header__feedback">', |
177 | + '<p class="page-header__feedback-message info">', |
178 | + '{$ error $}', |
179 | + '</p>', |
180 | + '</div>', |
181 | + '</div>', |
182 | + '</div>', |
183 | + '</header>', |
184 | + '<div class="ng-hide" data-ng-hide="show()">', |
185 | + '<div ng-transclude></div>', |
186 | + '</div>' |
187 | + ].join('')); |
188 | + |
189 | + // Preload the svg and png error icon. Its possible that it has never been |
190 | + // loaded by the browser and if the region connection goes down and the |
191 | + // directive gets shown with an error the icon will be missing. |
192 | + // |
193 | + // Note: This is skipped if unit testing because it will throw 404 errors |
194 | + // continuously. |
195 | + if(!angular.isDefined(window.jasmine)) { |
196 | + var image = new Image(); |
197 | + image.src = "static/img/icons/error.svg"; |
198 | + image = new Image(); |
199 | + image.src = "static/img/icons/error.png"; |
200 | + } |
201 | +}]); |
202 | + |
203 | +angular.module('MAAS').directive('maasErrorOverlay', [ |
204 | + '$window', '$timeout', 'RegionConnection', 'ErrorService', |
205 | + function($window, $timeout, RegionConnection, ErrorService) { |
206 | + return { |
207 | + restrict: "A", |
208 | + transclude: true, |
209 | + scope: true, |
210 | + templateUrl: 'directive/templates/error_overlay.html', |
211 | + link: function(scope, element, attrs) { |
212 | + |
213 | + scope.connected = false; |
214 | + scope.showDisconnected = false; |
215 | + scope.clientError = false; |
216 | + scope.wasConnected = false; |
217 | + |
218 | + // Holds the promise that sets showDisconnected to true. Will |
219 | + // be cleared when the scope is destroyed. |
220 | + var markDisconnected; |
221 | + |
222 | + // Returns true when the overlay should be shown. |
223 | + scope.show = function() { |
224 | + // Always show if clientError. |
225 | + if(scope.clientError) { |
226 | + return true; |
227 | + } |
228 | + // Never show if connected. |
229 | + if(scope.connected) { |
230 | + return false; |
231 | + } |
232 | + // Never been connected then always show. |
233 | + if(!scope.wasConnected) { |
234 | + return true; |
235 | + } |
236 | + // Not connected. |
237 | + return scope.showDisconnected; |
238 | + }; |
239 | + |
240 | + // Returns the title for the header. |
241 | + scope.getTitle = function() { |
242 | + if(scope.clientError) { |
243 | + return "Error occurred"; |
244 | + } else if(scope.wasConnected) { |
245 | + return "Connection lost, reconnecting..."; |
246 | + } else { |
247 | + return "Connecting..."; |
248 | + } |
249 | + }; |
250 | + |
251 | + // Called to reload the page. |
252 | + scope.reload = function() { |
253 | + $window.location.reload(); |
254 | + }; |
255 | + |
256 | + // Called to when the connection status of the region |
257 | + // changes. Updates the scope connected and error values. |
258 | + var watchConnection = function() { |
259 | + // Do nothing if already a client error. |
260 | + if(scope.clientError) { |
261 | + return; |
262 | + } |
263 | + |
264 | + // Set connected and the time it changed. |
265 | + var connected = RegionConnection.isConnected(); |
266 | + if(connected !== scope.connected) { |
267 | + scope.connected = connected; |
268 | + if(!connected) { |
269 | + scope.showDisconnected = false; |
270 | + |
271 | + // Show disconnected after 1/2 second. This removes |
272 | + // the flicker that can occur, if it disconnecets |
273 | + // and reconnected quickly. |
274 | + markDisconnected = $timeout(function() { |
275 | + scope.showDisconnected = true; |
276 | + markDisconnected = undefined; |
277 | + }, 500); |
278 | + } |
279 | + } |
280 | + |
281 | + // Set error and whether of not the connection |
282 | + // has ever been made. |
283 | + scope.error = RegionConnection.error; |
284 | + if(!scope.wasConnected && connected) { |
285 | + scope.wasConnected = true; |
286 | + } |
287 | + }; |
288 | + |
289 | + // Watch the isConnected and error value on the |
290 | + // RegionConnection. |
291 | + scope.$watch(function() { |
292 | + return RegionConnection.isConnected(); |
293 | + }, watchConnection); |
294 | + scope.$watch(function() { |
295 | + return RegionConnection.error; |
296 | + }, watchConnection); |
297 | + |
298 | + // Called then the error value on the ErrorService changes. |
299 | + var watchError = function() { |
300 | + var error = ErrorService._error; |
301 | + if(angular.isString(error)) { |
302 | + scope.clientError = true; |
303 | + scope.error = ErrorService._error; |
304 | + } |
305 | + }; |
306 | + |
307 | + // Watch _error on the ErrorService. |
308 | + scope.$watch(function() { |
309 | + return ErrorService._error; |
310 | + }, watchError); |
311 | + |
312 | + // Cancel the timeout on scope destroy. |
313 | + scope.$on("$destroy", function() { |
314 | + if(angular.isDefined(markDisconnected)) { |
315 | + $timeout.cancel(markDisconnected); |
316 | + } |
317 | + }); |
318 | + } |
319 | + }; |
320 | + }]); |
321 | |
322 | === added file 'src/maasserver/static/js/angular/directives/error_toggle.js' |
323 | --- src/maasserver/static/js/angular/directives/error_toggle.js 1970-01-01 00:00:00 +0000 |
324 | +++ src/maasserver/static/js/angular/directives/error_toggle.js 2015-04-24 19:35:27 +0000 |
325 | @@ -0,0 +1,67 @@ |
326 | +/* Copyright 2015 Canonical Ltd. This software is licensed under the |
327 | + * GNU Affero General Public License version 3 (see the file LICENSE). |
328 | + * |
329 | + * Error toggle. |
330 | + * |
331 | + * Hides the element if an error occurs or no connection to the region |
332 | + * is present. |
333 | + */ |
334 | + |
335 | +angular.module('MAAS').directive('maasErrorToggle', [ |
336 | + '$timeout', 'RegionConnection', 'ErrorService', |
337 | + function($timeout, RegionConnection, ErrorService) { |
338 | + return { |
339 | + restrict: "A", |
340 | + link: function(scope, element, attrs) { |
341 | + |
342 | + // Holds timeout promise for setting ng-hide when |
343 | + // connection is lost. |
344 | + var disconnectedPromise; |
345 | + |
346 | + // Cancel the disconnected timeout. |
347 | + var cancelTimeout = function() { |
348 | + if(angular.isDefined(disconnectedPromise)) { |
349 | + $timeout.cancel(disconnectedPromise); |
350 | + disconnectedPromise = undefined; |
351 | + } |
352 | + }; |
353 | + |
354 | + // Called to when the connection status of the region |
355 | + // changes or the error on the ErrorService is set. |
356 | + // The element is shown when connected and no errors. |
357 | + var watchConnectionAndError = function() { |
358 | + var connected = RegionConnection.isConnected(); |
359 | + var error = ErrorService._error; |
360 | + if(connected && !angular.isString(error)) { |
361 | + cancelTimeout(); |
362 | + element.removeClass("ng-hide"); |
363 | + } else if(angular.isString(error)) { |
364 | + cancelTimeout(); |
365 | + element.addClass("ng-hide"); |
366 | + } else if(!connected) { |
367 | + // Hide the element after 1/2 second. This stops |
368 | + // flickering when the connection goes down and |
369 | + // reconnects quickly. |
370 | + cancelTimeout(); |
371 | + disconnectedPromise = $timeout(function() { |
372 | + element.addClass("ng-hide"); |
373 | + }, 500); |
374 | + } |
375 | + }; |
376 | + |
377 | + // Watch the RegionConnection.isConnected() and |
378 | + // ErrorService._error value. |
379 | + scope.$watch(function() { |
380 | + return RegionConnection.isConnected(); |
381 | + }, watchConnectionAndError); |
382 | + scope.$watch(function() { |
383 | + return ErrorService._error; |
384 | + }, watchConnectionAndError); |
385 | + |
386 | + // Cancel disconnect timeout on destroy. |
387 | + scope.$on("$destroy", function() { |
388 | + cancelTimeout(); |
389 | + }); |
390 | + } |
391 | + }; |
392 | + }]); |
393 | |
394 | === added file 'src/maasserver/static/js/angular/directives/tests/test_error_overlay.js' |
395 | --- src/maasserver/static/js/angular/directives/tests/test_error_overlay.js 1970-01-01 00:00:00 +0000 |
396 | +++ src/maasserver/static/js/angular/directives/tests/test_error_overlay.js 2015-04-24 19:35:27 +0000 |
397 | @@ -0,0 +1,160 @@ |
398 | +/* Copyright 2015 Canonical Ltd. This software is licensed under the |
399 | + * GNU Affero General Public License version 3 (see the file LICENSE). |
400 | + * |
401 | + * Unit tests for error overlay. |
402 | + */ |
403 | + |
404 | +describe("maasErrorOverlay", function() { |
405 | + |
406 | + // Load the MAAS module. |
407 | + beforeEach(module("MAAS")); |
408 | + |
409 | + // Get required angular pieces and create a new scope before each test. |
410 | + var $scope, $window, $timeout; |
411 | + beforeEach(inject(function($rootScope, $injector) { |
412 | + $scope = $rootScope.$new(); |
413 | + $window = $injector.get("$window"); |
414 | + $timeout = $injector.get("$timeout"); |
415 | + })); |
416 | + |
417 | + // Load the RegionConnection and ErrorService. |
418 | + var RegionConnection; |
419 | + beforeEach(inject(function($injector) { |
420 | + RegionConnection = $injector.get("RegionConnection"); |
421 | + ErrorService = $injector.get("ErrorService"); |
422 | + })); |
423 | + |
424 | + // Return the compiled directive. |
425 | + function compileDirective() { |
426 | + var directive; |
427 | + var html = '<div><div data-maas-error-overlay>' + |
428 | + '<div id="content"></div>' + |
429 | + '</div></div>'; |
430 | + |
431 | + // Compile the directive. |
432 | + inject(function($compile) { |
433 | + directive = $compile(html)($scope); |
434 | + }); |
435 | + |
436 | + // Perform the digest cycle to finish the compile. |
437 | + $scope.$digest(); |
438 | + return directive.find("span"); |
439 | + } |
440 | + |
441 | + it("sets connected to value of isConnected", function() { |
442 | + spyOn(RegionConnection, "isConnected").and.returnValue(true); |
443 | + var directive = compileDirective(); |
444 | + expect(directive.scope().connected).toBe(true); |
445 | + }); |
446 | + |
447 | + it("sets wasConnected to true once connected", function() { |
448 | + spyOn(RegionConnection, "isConnected").and.returnValue(true); |
449 | + var directive = compileDirective(); |
450 | + expect(directive.scope().wasConnected).toBe(true); |
451 | + }); |
452 | + |
453 | + it("keeps wasConnected to true if becomes disconnected", function() { |
454 | + var spy = spyOn(RegionConnection, "isConnected"); |
455 | + spy.and.returnValue(true); |
456 | + var directive = compileDirective(); |
457 | + spy.and.returnValue(false); |
458 | + $scope.$digest(); |
459 | + expect(directive.scope().wasConnected).toBe(true); |
460 | + }); |
461 | + |
462 | + it("keeps clientError to true if error in ErrorService", function() { |
463 | + ErrorService._error = makeName("error"); |
464 | + var directive = compileDirective(); |
465 | + expect(directive.scope().clientError).toBe(true); |
466 | + }); |
467 | + |
468 | + it("sets error to error in ErrorService", function() { |
469 | + var error = makeName("error"); |
470 | + ErrorService._error = error; |
471 | + var directive = compileDirective(); |
472 | + expect(directive.scope().error).toBe(error); |
473 | + }); |
474 | + |
475 | + it("sets error to error in RegionConnection", function() { |
476 | + var error = makeName("error"); |
477 | + RegionConnection.error = error; |
478 | + var directive = compileDirective(); |
479 | + expect(directive.scope().error).toBe(error); |
480 | + }); |
481 | + |
482 | + it("doesnt sets error to error in RegionConnection if already error in " + |
483 | + "ErrorService", function() { |
484 | + var error = makeName("error"); |
485 | + ErrorService._error = error; |
486 | + RegionConnection.error = makeName("error"); |
487 | + var directive = compileDirective(); |
488 | + expect(directive.scope().error).toBe(error); |
489 | + }); |
490 | + |
491 | + describe("show", function() { |
492 | + |
493 | + it("returns true if not connected", function() { |
494 | + spyOn(RegionConnection, "isConnected").and.returnValue(false); |
495 | + var directive = compileDirective(); |
496 | + expect(directive.scope().show()).toBe(true); |
497 | + }); |
498 | + |
499 | + it("returns true if error in ErrorService", function() { |
500 | + ErrorService._error = makeName("error"); |
501 | + var directive = compileDirective(); |
502 | + expect(directive.scope().show()).toBe(true); |
503 | + }); |
504 | + |
505 | + it("returns false if connected and no error", function() { |
506 | + spyOn(RegionConnection, "isConnected").and.returnValue(true); |
507 | + var directive = compileDirective(); |
508 | + expect(directive.scope().show()).toBe(false); |
509 | + }); |
510 | + |
511 | + it("returns false if disconnected less than 1/2 second", function() { |
512 | + var spy = spyOn(RegionConnection, "isConnected"); |
513 | + spy.and.returnValue(true); |
514 | + var directive = compileDirective(); |
515 | + spy.and.returnValue(false); |
516 | + $scope.$digest(); |
517 | + expect(directive.scope().show()).toBe(false); |
518 | + }); |
519 | + |
520 | + it("returns true if disconnected more than 1/2 second", function() { |
521 | + var spy = spyOn(RegionConnection, "isConnected"); |
522 | + spy.and.returnValue(true); |
523 | + var directive = compileDirective(); |
524 | + spy.and.returnValue(false); |
525 | + $scope.$digest(); |
526 | + $timeout.flush(500); |
527 | + expect(directive.scope().show()).toBe(true); |
528 | + }); |
529 | + }); |
530 | + |
531 | + describe("getTitle", function() { |
532 | + |
533 | + it("returns error title", function() { |
534 | + ErrorService._error = makeName("error"); |
535 | + var directive = compileDirective(); |
536 | + expect(directive.scope().getTitle()).toBe( |
537 | + "Error occurred"); |
538 | + }); |
539 | + |
540 | + it("returns connection lost error", function() { |
541 | + var spy = spyOn(RegionConnection, "isConnected"); |
542 | + spy.and.returnValue(true); |
543 | + var directive = compileDirective(); |
544 | + spy.and.returnValue(false); |
545 | + $scope.$digest(); |
546 | + expect(directive.scope().getTitle()).toBe( |
547 | + "Connection lost, reconnecting..."); |
548 | + }); |
549 | + |
550 | + it("returns connecting", function() { |
551 | + spyOn(RegionConnection, "isConnected").and.returnValue(false); |
552 | + var directive = compileDirective(); |
553 | + expect(directive.scope().getTitle()).toBe( |
554 | + "Connecting..."); |
555 | + }); |
556 | + }); |
557 | +}); |
558 | |
559 | === added file 'src/maasserver/static/js/angular/directives/tests/test_error_toggle.js' |
560 | --- src/maasserver/static/js/angular/directives/tests/test_error_toggle.js 1970-01-01 00:00:00 +0000 |
561 | +++ src/maasserver/static/js/angular/directives/tests/test_error_toggle.js 2015-04-24 19:35:27 +0000 |
562 | @@ -0,0 +1,74 @@ |
563 | +/* Copyright 2015 Canonical Ltd. This software is licensed under the |
564 | + * GNU Affero General Public License version 3 (see the file LICENSE). |
565 | + * |
566 | + * Unit tests for error toggle. |
567 | + */ |
568 | + |
569 | +describe("maasErrorToggle", function() { |
570 | + |
571 | + // Load the MAAS module. |
572 | + beforeEach(module("MAAS")); |
573 | + |
574 | + // Get required angular pieces and create a new scope before each test. |
575 | + var $scope, $timeout; |
576 | + beforeEach(inject(function($rootScope, $injector) { |
577 | + $scope = $rootScope.$new(); |
578 | + $timeout = $injector.get("$timeout"); |
579 | + })); |
580 | + |
581 | + // Load the RegionConnection and ErrorService. |
582 | + var RegionConnection; |
583 | + beforeEach(inject(function($injector) { |
584 | + RegionConnection = $injector.get("RegionConnection"); |
585 | + ErrorService = $injector.get("ErrorService"); |
586 | + })); |
587 | + |
588 | + // Return the compiled directive. |
589 | + function compileDirective() { |
590 | + var directive; |
591 | + var html = '<div><span data-maas-error-toggle></span></div>'; |
592 | + |
593 | + // Compile the directive. |
594 | + inject(function($compile) { |
595 | + directive = $compile(html)($scope); |
596 | + }); |
597 | + |
598 | + // Perform the digest cycle to finish the compile. |
599 | + $scope.$digest(); |
600 | + return directive.find("span"); |
601 | + } |
602 | + |
603 | + it("doesnt hide element instantly if region not connected", function() { |
604 | + spyOn(RegionConnection, "isConnected").and.returnValue(false); |
605 | + var directive = compileDirective(); |
606 | + expect(directive.hasClass("ng-hide")).toBe(false); |
607 | + }); |
608 | + |
609 | + it("hides element if region not connected after 1/2 second", function() { |
610 | + spyOn(RegionConnection, "isConnected").and.returnValue(false); |
611 | + var directive = compileDirective(); |
612 | + $timeout.flush(500); |
613 | + expect(directive.hasClass("ng-hide")).toBe(true); |
614 | + }); |
615 | + |
616 | + it("hides element if error in ErrorService", function() { |
617 | + ErrorService._error = makeName("error"); |
618 | + var directive = compileDirective(); |
619 | + expect(directive.hasClass("ng-hide")).toBe(true); |
620 | + }); |
621 | + |
622 | + it("shows element if connected and no error", function() { |
623 | + spyOn(RegionConnection, "isConnected").and.returnValue(true); |
624 | + var directive = compileDirective(); |
625 | + expect(directive.hasClass("ng-hide")).toBe(false); |
626 | + }); |
627 | + |
628 | + it("shows element if becomes connected", function() { |
629 | + var spy = spyOn(RegionConnection, "isConnected"); |
630 | + spy.and.returnValue(false); |
631 | + var directive = compileDirective(); |
632 | + spy.and.returnValue(true); |
633 | + $scope.$digest(); |
634 | + expect(directive.hasClass("ng-hide")).toBe(false); |
635 | + }); |
636 | +}); |
637 | |
638 | === modified file 'src/maasserver/static/js/angular/factories/region.js' |
639 | --- src/maasserver/static/js/angular/factories/region.js 2015-03-30 07:41:18 +0000 |
640 | +++ src/maasserver/static/js/angular/factories/region.js 2015-04-24 19:35:27 +0000 |
641 | @@ -34,6 +34,7 @@ |
642 | this.connected = false; |
643 | this.autoReconnect = true; |
644 | this.retryTimeout = 5000; |
645 | + this.error = null; |
646 | |
647 | // Defer used for defaultConnect. If defaultConnect is called |
648 | // quickly only the first one will start the connection. The |
649 | @@ -135,6 +136,7 @@ |
650 | }; |
651 | this.websocket.onclose = function(evt) { |
652 | self.connected = false; |
653 | + self.error = "Unable to connect to: " + self.url.split("?")[0]; |
654 | angular.forEach(self.handlers.close, function(func) { |
655 | func(evt); |
656 | }); |
657 | |
658 | === modified file 'src/maasserver/static/js/angular/factories/tests/test_region.js' |
659 | --- src/maasserver/static/js/angular/factories/tests/test_region.js 2015-03-27 09:14:17 +0000 |
660 | +++ src/maasserver/static/js/angular/factories/tests/test_region.js 2015-04-24 19:35:27 +0000 |
661 | @@ -266,6 +266,14 @@ |
662 | expect(RegionConnection.connect).toHaveBeenCalledWith(url); |
663 | }); |
664 | |
665 | + it("onclose sets error", function() { |
666 | + RegionConnection.connect(url); |
667 | + webSocket.onclose(); |
668 | + $timeout.flush(); |
669 | + expect(RegionConnection.error).toBe( |
670 | + "Unable to connect to: " + url); |
671 | + }); |
672 | + |
673 | it("calls onMessage when onmessage called", function() { |
674 | var sampleData = { sample: "data" }; |
675 | spyOn(RegionConnection, "onMessage"); |
676 | |
677 | === modified file 'src/maasserver/static/js/angular/maas.js' |
678 | --- src/maasserver/static/js/angular/maas.js 2015-03-30 16:05:41 +0000 |
679 | +++ src/maasserver/static/js/angular/maas.js 2015-04-24 19:35:27 +0000 |
680 | @@ -39,10 +39,6 @@ |
681 | templateUrl: 'static/partials/node-events.html', |
682 | controller: 'NodeEventsController' |
683 | }). |
684 | - when('/error', { |
685 | - templateUrl: 'static/partials/error.html', |
686 | - controller: 'ErrorController' |
687 | - }). |
688 | otherwise({ |
689 | redirectTo: '/nodes' |
690 | }); |
691 | |
692 | === modified file 'src/maasserver/static/js/angular/services/error.js' |
693 | --- src/maasserver/static/js/angular/services/error.js 2015-03-17 17:57:17 +0000 |
694 | +++ src/maasserver/static/js/angular/services/error.js 2015-04-24 19:35:27 +0000 |
695 | @@ -4,22 +4,17 @@ |
696 | * MAAS Error Service |
697 | */ |
698 | |
699 | -angular.module('MAAS').service('ErrorService', ['$location', |
700 | - function($location) { |
701 | +angular.module('MAAS').service('ErrorService', function() { |
702 | |
703 | - // Holds the raised error and url when the error was raised. |
704 | + // Holds the client error. |
705 | this._error = null; |
706 | - this._backUrl = null; |
707 | |
708 | - // Raise this error in the UI. Will cause the page to redirect to the |
709 | - // error page. |
710 | + // Raise this error in the UI. |
711 | this.raiseError = function(error) { |
712 | - // Possible that this method is called more than once, before the |
713 | - // location is changed. Only take the first error. |
714 | + // Possible that this method is called more than once. |
715 | + // Only take the first error. |
716 | if(!angular.isString(this._error)) { |
717 | this._error = error; |
718 | - this._backUrl = $location.url(); |
719 | } |
720 | - $location.path('/error'); |
721 | }; |
722 | - }]); |
723 | + }); |
724 | |
725 | === modified file 'src/maasserver/static/js/angular/services/tests/test_error.js' |
726 | --- src/maasserver/static/js/angular/services/tests/test_error.js 2015-03-17 17:57:17 +0000 |
727 | +++ src/maasserver/static/js/angular/services/tests/test_error.js 2015-04-24 19:35:27 +0000 |
728 | @@ -9,54 +9,32 @@ |
729 | // Load the MAAS module. |
730 | beforeEach(module("MAAS")); |
731 | |
732 | - // Grab the needed angular pieces. |
733 | - var $location; |
734 | - beforeEach(inject(function($injector) { |
735 | - $location = $injector.get("$location"); |
736 | - })); |
737 | - |
738 | // Load the ErrorService. |
739 | var ErrorService; |
740 | beforeEach(inject(function($injector) { |
741 | ErrorService = $injector.get("ErrorService"); |
742 | })); |
743 | |
744 | - it("initializes _error and _backUrl to null", function() { |
745 | + it("initializes _error to null", function() { |
746 | expect(ErrorService._error).toBeNull(); |
747 | - expect(ErrorService._backUrl).toBeNull(); |
748 | }); |
749 | |
750 | describe("raiseError", function() { |
751 | |
752 | - it("sets _error and _backUrl and calls $location.path", function() { |
753 | + it("sets _error", function() { |
754 | var error = makeName("error"); |
755 | - var url = makeName("url"); |
756 | - spyOn($location, "url").and.returnValue(url); |
757 | - spyOn($location, "path"); |
758 | ErrorService.raiseError(error); |
759 | expect(ErrorService._error).toBe(error); |
760 | - expect(ErrorService._backUrl).toBe(url); |
761 | - expect($location.path).toHaveBeenCalledWith("/error"); |
762 | }); |
763 | |
764 | - it("only sets _error and _backUrl once", function() { |
765 | + it("only sets _error once", function() { |
766 | var errors = [ |
767 | makeName("error"), |
768 | makeName("error") |
769 | ]; |
770 | - var urls = [ |
771 | - makeName("url"), |
772 | - makeName("url") |
773 | - ]; |
774 | - var i = 0; |
775 | - spyOn($location, "url").and.callFake(function() { |
776 | - return urls[i++]; |
777 | - }); |
778 | - spyOn($location, "path"); |
779 | ErrorService.raiseError(errors[0]); |
780 | ErrorService.raiseError(errors[1]); |
781 | expect(ErrorService._error).toBe(errors[0]); |
782 | - expect(ErrorService._backUrl).toBe(urls[0]); |
783 | }); |
784 | }); |
785 | }); |
786 | |
787 | === modified file 'src/maasserver/templates/maasserver/index.html' |
788 | --- src/maasserver/templates/maasserver/index.html 2015-04-14 13:18:41 +0000 |
789 | +++ src/maasserver/templates/maasserver/index.html 2015-04-24 19:35:27 +0000 |
790 | @@ -81,7 +81,7 @@ |
791 | </div> |
792 | </header> |
793 | <main id="body"> |
794 | - <div id="main-content"> |
795 | + <div id="main-content" data-maas-error-overlay> |
796 | {% if user.is_authenticated %} |
797 | <ul id="flash-messages" class="flash-messages hidden"> |
798 | {% for persistent_error in persistent_errors %} |
799 | @@ -100,7 +100,7 @@ |
800 | <div class="push"></div> |
801 | </main> |
802 | </div> |
803 | - <div class="footer-wrapper"> |
804 | + <div class="footer-wrapper" data-maas-error-toggle> |
805 | <footer class="global inner-wrapper clearfix"> |
806 | <div class="legal clearfix"> |
807 | <div class="legal-inner"> |
So, if I understand correctly, when we load the page for the first time we see a blank page when it is trying to connect to the websocket. My questions are, does this happen every single time we reload the page?
Now, do we have screenshots that show how these messages are shown? (I want to know if it is by the design attached on the bug - meaning just a message at the top with a blank page or what?).
Now, I've put a few comments inline but do we really care about the user known that we are connecting to the region? From a user perspective, I could be asking, what does connecting to the region mean? What is it doing?