Merge lp:~henninge/launchpad/bug-824435-failure-reporting-4 into lp:launchpad

Proposed by Henning Eggers
Status: Merged
Approved by: Данило Шеган
Approved revision: no longer in the source branch.
Merged at revision: 13837
Proposed branch: lp:~henninge/launchpad/bug-824435-failure-reporting-4
Merge into: lp:launchpad
Diff against target: 424 lines (+154/-149)
3 files modified
lib/lp/code/browser/sourcepackagerecipe.py (+1/-1)
lib/lp/code/javascript/requestbuild_overlay.js (+130/-132)
lib/lp/code/javascript/tests/test_requestbuild_overlay.js (+23/-16)
To merge this branch: bzr merge lp:~henninge/launchpad/bug-824435-failure-reporting-4
Reviewer Review Type Date Requested Status
Данило Шеган (community) Approve
Review via email: mp+73507@code.launchpad.net

Commit message

[r=danilo][bug=824435] Include OOPS-ID in error output of requestbuild overlay.

Description of the change

= Summary =

This branch finally fixes bug 824435 by making sure that the OOPS ID
returned by server failures is displayed in the UI. This makes it
possible for users to report useful information to the LP team.

== Proposed fix ==

Use lp.cient.ErrorHandler as the base for RequestResponseHandler which
provides a getFailureHandler method which in turn produces an io handler
that is able to extract OOPS information from the server response.

"Partial" errors are not "Bad requests" because they were may have been
able to request some builds while others had already been requested.
This is still normal processing and therefore a successful response. So
it should return status 200. The success handler will have the content
type information to figure out which type of response it got.

Factor out the error display into its own functio because it will be
used from the failure handler as well as the success handler.

Improve markup creation by using YUI methods.

== Pre-implementation notes ==

On-going effort.

== Implementation details ==

I refactored the code a bit by creating the get_new_builds_message and
get_info_header functions, to make the code in the success handler more
concise.

== Tests ==

I updated the tests to check for an OOPS id beint displayed with the
error messages for both request daily build and request builds.

firefox lib/lp/code/javascript/tests/test_requestbuild_overlay.html

== Demo and Q/A ==

Verify that the succesful and partially succesful handlers still work.

For QA, create a new recipe on any old branch, using the stand settings.
Open the recipe page in two tabs and click "Request build(s)" in both.
Select one distroseries in the first and two in the second tab, where
the two include the one in the first. Got that? ;-)
Submit the first, it will close successfully.
Submit the second, you will get an informational message that "An
identical build is already pending for ..."

Verify that server errors are displayed.

The only idea I have for that is to have a LOSA cowboy a patch onto
qastaging that places and odd "assert fail" in
SourcePackageRecipeRequestBuildsAjaxView.request_action.
Request builds using the "Bulid now" button and the "Request build(s)"
overlay and you should get an error message that includes an OOPS-ID.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/javascript/requestbuild_overlay.js
  lib/lp/code/browser/sourcepackagerecipe.py
  lib/lp/code/javascript/tests/test_requestbuild_overlay.js

To post a comment you must log in.
Revision history for this message
Данило Шеган (danilo) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py 2011-07-15 03:13:14 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py 2011-08-31 11:10:17 +0000
@@ -479,7 +479,7 @@
479 def _process_error(self, data=None, builds=None, informational=None,479 def _process_error(self, data=None, builds=None, informational=None,
480 errors=None, reason="Validation"):480 errors=None, reason="Validation"):
481 """Set up the response and json data to return to the caller."""481 """Set up the response and json data to return to the caller."""
482 self.request.response.setStatus(400, reason)482 self.request.response.setStatus(200, reason)
483 self.request.response.setHeader('Content-type', 'application/json')483 self.request.response.setHeader('Content-type', 'application/json')
484 return_data = dict(builds=builds, errors=errors)484 return_data = dict(builds=builds, errors=errors)
485 if informational:485 if informational:
486486
=== modified file 'lib/lp/code/javascript/requestbuild_overlay.js'
--- lib/lp/code/javascript/requestbuild_overlay.js 2011-08-25 13:19:31 +0000
+++ lib/lp/code/javascript/requestbuild_overlay.js 2011-08-31 11:10:17 +0000
@@ -34,35 +34,16 @@
34// developer to specify onSuccess and onError hooks. It is quite generic and34// developer to specify onSuccess and onError hooks. It is quite generic and
35// perhaps could be moved to an infrastructure class.35// perhaps could be moved to an infrastructure class.
3636
37RequestResponseHandler = function () {};37var RequestResponseHandler = function () {};
38RequestResponseHandler.prototype = {38
39 clearProgressUI: function () {},39RequestResponseHandler.prototype = new Y.lp.client.ErrorHandler();
40 showError: function (header, error_msg) {},40
41 getErrorHandler: function (errorCallback) {41RequestResponseHandler.prototype.getSuccessHandler = function(callback) {
42 var self = this;42 var self = this;
43 return function (id, response) {43 return function (id, response) {
44 self.clearProgressUI();44 self.clearProgressUI();
45 // If it was a timeout...45 callback(self, id, response);
46 if (response.status === 503) {46 };
47 self.showError(
48 'Timeout error, please try again in a few minutes.',
49 null);
50 } else {
51 if (errorCallback !== null) {
52 errorCallback(self, id, response);
53 } else {
54 self.showError(response.responseText, null);
55 }
56 }
57 };
58 },
59 getSuccessHandler: function (successCallback) {
60 var self = this;
61 return function (id, response) {
62 self.clearProgressUI();
63 successCallback(self, id, response);
64 };
65 }
66};47};
6748
68namespace.RequestResponseHandler = RequestResponseHandler;49namespace.RequestResponseHandler = RequestResponseHandler;
@@ -115,28 +96,8 @@
115 request_build_response_handler.clearProgressUI = function() {96 request_build_response_handler.clearProgressUI = function() {
116 destroy_temporary_spinner();97 destroy_temporary_spinner();
117 };98 };
118 request_build_response_handler.showError = function(header, error_msgs) {99 request_build_response_handler.showError = function(errormessage) {
119 if (header === null) {100 display_errors(null, errormessage);
120 if (error_msgs === null) {
121 header = "An error occurred, please contact an " +
122 "administrator.";
123 } else {
124 header = "The following errors occurred:";
125 }
126 }
127 var error_html = header;
128 if (error_msgs !== null) {
129 error_html += "<ul>";
130 if (typeof(error_msgs) === "string"){
131 error_msgs = [error_msgs];
132 }
133 Y.each(error_msgs, function(error_msg){
134 error_html += "<li>" + error_msg.replace(/<([^>]+)>/g,'') +
135 "</li>";
136 });
137 error_html += "</ul><p/>";
138 }
139 request_build_overlay.error_node.set('innerHTML', error_html);
140 };101 };
141};102};
142103
@@ -191,12 +152,8 @@
191 method: "POST",152 method: "POST",
192 headers: {'Accept': 'application/xhtml'},153 headers: {'Accept': 'application/xhtml'},
193 on: {154 on: {
194 failure: request_daily_build_response_handler.getErrorHandler(155 failure:
195 function(handler, id, response) {156 request_daily_build_response_handler.getFailureHandler(),
196 var server_error = 'Server error, ' +
197 'please contact an administrator.';
198 handler.showError(server_error, null);
199 }),
200 success:157 success:
201 request_daily_build_response_handler.getSuccessHandler(158 request_daily_build_response_handler.getSuccessHandler(
202 function(handler, id, response) {159 function(handler, id, response) {
@@ -232,13 +189,9 @@
232 request_daily_build_response_handler.clearProgressUI = function() {189 request_daily_build_response_handler.clearProgressUI = function() {
233 destroy_temporary_spinner();190 destroy_temporary_spinner();
234 };191 };
235 request_daily_build_response_handler.showError = function(header, error) {192 request_daily_build_response_handler.showError = function(message) {
236 var error_msg = header;193 display_message(message, 'build-error');
237 if (error !== null) {194 Y.log(message);
238 error_msg += " " + error;
239 }
240 display_message(error_msg, 'build-error');
241 Y.log(error_msg);
242 };195 };
243};196};
244197
@@ -294,6 +247,35 @@
294 return true;247 return true;
295};248};
296249
250var get_new_builds_message = function(build_html, current_builds) {
251 var nr_new;
252 if (build_html === null) {
253 return null;
254 }
255 nr_new = display_build_records(build_html, current_builds);
256 if (nr_new > 1) {
257 return Y.Lang.substitute(MANY_BUILDS_MESSAGE, {nr_new: nr_new});
258 }
259 return ONE_BUILD_MESSAGE;
260};
261
262var get_info_header = function(new_builds_message, pending_build_info) {
263 var info_header = Y.Node.create('<div></div>')
264 .addClass('popup-build-informational');
265 if (new_builds_message !== null) {
266 info_header.append(Y.Node.create('<p></p>')
267 .set('text', new_builds_message));
268 }
269 if (pending_build_info !== null) {
270 info_header.append(Y.Node.create('<p></p>')
271 .set('text', pending_build_info));
272 }
273 if (info_header.hasChildNodes()) {
274 return info_header;
275 }
276 return null;
277};
278
297/*279/*
298 * The form submit function280 * The form submit function
299 */281 */
@@ -312,79 +294,70 @@
312 method: "POST",294 method: "POST",
313 headers: {'Accept': 'application/json; application/xhtml'},295 headers: {'Accept': 'application/json; application/xhtml'},
314 on: {296 on: {
315 failure: request_build_response_handler.getErrorHandler(297 failure: request_build_response_handler.getFailureHandler(),
316 function(handler, id, response) {
317 var errors = [],
318 error_header = null,
319 error_header_text = "",
320 build_info, build_html, error_info,
321 nr_new, new_builds_message, field_name;
322 if( response.status >= 500 ) {
323 // There's some error content we need to display.
324 errors = [
325 response.status + ' ' + response.statusText];
326 } else {
327 // The response will be a json data object containing
328 // info about what builds there are, informational
329 // text about any builds already pending, plus any
330 // errors. The FormOverlay infrastructure only
331 // supports displaying errors so we will construct
332 // our own HTML where the informational text will be
333 // appropriately displayed.
334 build_info = Y.JSON.parse(response.responseText);
335
336 // The result of rendering the +builds view
337 build_html = build_info.builds;
338 // Any builds already pending (informational only)
339 pending_build_info = build_info.already_pending;
340 // Other more critical errors
341 error_info = build_info.errors;
342
343 if (build_html !== null) {
344 nr_new = display_build_records(
345 build_html, current_builds);
346 new_builds_message = ONE_BUILD_MESSAGE;
347 if (nr_new > 1) {
348 new_builds_message =
349 Y.Lang.substitute(
350 MANY_BUILDS_MESSAGE,
351 {nr_new: nr_new});
352 }
353 error_header_text = new_builds_message;
354 }
355 if (pending_build_info !== null) {
356 if (error_header_text !== "" ) {
357 error_header_text += "<p/>" +
358 pending_build_info;
359 } else {
360 error_header_text = pending_build_info;
361 }
362 }
363 for (field_name in error_info) {
364 if (error_info.hasOwnProperty(field_name)) {
365 errors.push(error_info[field_name]);
366 }
367 }
368 if (error_header_text !== "") {
369 error_header =
370 "<p><div class='popup-build-informational'>"
371 + error_header_text + "</p></div>";
372 if (Y.Object.size(errors)>0) {
373 error_header +=
374 "<p/>" + "There were also some errors:";
375 }
376 } else {
377 error_header = "There were some errors:";
378 }
379 }
380 handler.showError(error_header, errors);
381 }),
382 success: request_build_response_handler.getSuccessHandler(298 success: request_build_response_handler.getSuccessHandler(
383 function(handler, id, response) {299 function(handler, id, response) {
300 var errors = [],
301 error_header = null,
302 error_header_text = "",
303 build_info, build_html, error_info,
304 nr_new, info_header, field_name;
305 // The content type is used to tell a fully successful
306 // request from a partially successful one. Successful
307 // responses simply return the HTML snippet for the builds
308 // table. If this ever causes problems, the view should
309 // be changed to always return JSON and to provide an
310 // attribute that identifies the type of response.
311 content_type = response.getResponseHeader('Content-type');
312 if( content_type !== 'application/json' ) {
313 // We got the HTML for the builds back, we're done.
384 request_build_overlay.hide();314 request_build_overlay.hide();
385 display_build_records(315 display_build_records(
386 response.responseText, current_builds);316 response.responseText, current_builds);
387 })317 } else {
318 // The response will be a json data object containing
319 // info about what builds there are, informational
320 // text about any builds already pending, plus any
321 // errors. The FormOverlay infrastructure only
322 // supports displaying errors so we will construct
323 // our own HTML where the informational text will be
324 // appropriately displayed.
325 build_info = Y.JSON.parse(response.responseText);
326
327 // The result of rendering the +builds view
328 build_html = build_info.builds;
329 // Any builds already pending (informational only)
330 pending_build_info = build_info.already_pending;
331 // Other more critical errors
332 error_info = build_info.errors;
333
334 info_header = get_info_header(
335 get_new_builds_message(
336 build_html, current_builds),
337 pending_build_info);
338
339 for (field_name in error_info) {
340 if (error_info.hasOwnProperty(field_name)) {
341 errors.push(error_info[field_name]);
342 }
343 }
344 error_container = Y.Node.create('<div></div>');
345 if (info_header !== null) {
346 error_container.append(info_header);
347 if (errors.length > 0) {
348 error_container.append(
349 Y.Node.create('<p></p>')
350 .set(
351 'text',
352 "There were also some errors:"));
353 }
354 } else {
355 error_container.set(
356 'text', "There were some errors:");
357 }
358 display_errors(error_container, errors);
359 }
360 })
388 },361 },
389 form: {362 form: {
390 id: request_build_overlay.form_node,363 id: request_build_overlay.form_node,
@@ -432,6 +405,31 @@
432 "label[for^='field.distroseries.']");405 "label[for^='field.distroseries.']");
433};406};
434407
408var display_errors = function(container, error_msgs) {
409 var errors_list, header;
410 if (container === null) {
411 if (error_msgs === null) {
412 header = "An error occurred, please contact an administrator.";
413 } else {
414 header = "The following errors occurred:";
415 }
416 container = Y.Node.create('<div></div>')
417 .set('text', header);
418 }
419 if (error_msgs !== null) {
420 errors_list = Y.Node.create('<ul></ul>');
421 if (Y.Lang.isString(error_msgs)){
422 error_msgs = [error_msgs];
423 }
424 Y.each(error_msgs, function(error_msg){
425 errors_list.append(Y.Node.create('<li></li>')
426 .set('text', error_msg));
427 });
428 container.append(errors_list);
429 }
430 request_build_overlay.error_node.setContent(container);
431};
432
435/*433/*
436 * Callback used to enable/disable distro series selection when a different434 * Callback used to enable/disable distro series selection when a different
437 * ppa is selected.435 * ppa is selected.
438436
=== modified file 'lib/lp/code/javascript/tests/test_requestbuild_overlay.js'
--- lib/lp/code/javascript/tests/test_requestbuild_overlay.js 2011-08-26 07:41:59 +0000
+++ lib/lp/code/javascript/tests/test_requestbuild_overlay.js 2011-08-31 11:10:17 +0000
@@ -66,15 +66,20 @@
66 },66 },
6767
6868
69 _testRequestbuildFailure: function(status, expected_message) {69 _testRequestbuildFailure: function(status, expected_message, oops) {
70 var mockio = this._makeRequest();70 var mockio = this._makeRequest(),
71 mockio.respond({status: status});71 response = {status: status},
72 error;
73 if (oops !== undefined) {
74 response.responseHeaders = {'X-Lazr-OopsId': oops};
75 }
76 mockio.respond(response);
7277
73 // No build targets.78 // No build targets.
74 Y.Assert.areSame(79 Y.Assert.areSame(
75 0, Y.one("#builds-target").all('.package-build').size());80 0, Y.one("#builds-target").all('.package-build').size());
76 // The message is being displayed as an error.81 // The message is being displayed as an error.
77 var error = Y.one("#new-builds-info");82 error = Y.one("#new-builds-info");
78 Y.Assert.isTrue(error.hasClass('build-error'));83 Y.Assert.isTrue(error.hasClass('build-error'));
79 Y.Assert.areSame(expected_message + "Dismiss", error.get('text'));84 Y.Assert.areSame(expected_message + "Dismiss", error.get('text'));
80 // The message can be dismissed.85 // The message can be dismissed.
@@ -90,10 +95,11 @@
90 503, "Timeout error, please try again in a few minutes.");95 503, "Timeout error, please try again in a few minutes.");
91 },96 },
9297
93
94 test_requestdailybuild_failure_500: function() {98 test_requestdailybuild_failure_500: function() {
95 this._testRequestbuildFailure(99 var oops_id = "OOPS-TESTING",
96 500, "Server error, please contact an administrator.");100 message = "Server error, please contact an administrator. " +
101 "OOPS ID:" + oops_id;
102 this._testRequestbuildFailure(500, message, oops_id);
97 }103 }
98104
99}));105}));
@@ -171,10 +177,12 @@
171177
172 test_requestbuilds_failure_500: function() {178 test_requestbuilds_failure_500: function() {
173 var mockio = this._makeRequest(),179 var mockio = this._makeRequest(),
174 status = 500,180 oops_id = "OOPS-TESTING",
175 status_text = "Internal Server Error",181 message = "Server error, please contact an administrator. " +
176 expected_error = status + ' ' + status_text;182 "OOPS ID:" + oops_id;
177 mockio.failure({status: status, statusText: status_text});183 mockio.failure({
184 'responseHeaders': {'X-Lazr-OopsId': oops_id}
185 });
178186
179 // The form stays visible and no builds are displayed.187 // The form stays visible and no builds are displayed.
180 Y.Assert.isFalse(188 Y.Assert.isFalse(
@@ -184,14 +192,14 @@
184 0, Y.one("#builds-target").all('.package-build').size());192 0, Y.one("#builds-target").all('.package-build').size());
185 // The error message is displayed.193 // The error message is displayed.
186 Y.Assert.areSame(194 Y.Assert.areSame(
187 expected_error,195 message,
188 Y.one(".yui3-lazr-formoverlay-errors li").get('text'));196 Y.one(".yui3-lazr-formoverlay-errors li").get('text'));
189 // The submit button is disabled.197 // The submit button is disabled.
190 Y.Assert.isTrue(198 Y.Assert.isTrue(
191 Y.one("[name='field.actions.request']").get('disabled'));199 Y.one("[name='field.actions.request']").get('disabled'));
192 },200 },
193201
194 test_requestbuilds_failure_400: function() {202 test_requestbuilds_build_collision: function() {
195 var mockio = this._makeRequest(),203 var mockio = this._makeRequest(),
196 success_message = "2 new recipe builds have been queued.",204 success_message = "2 new recipe builds have been queued.",
197 informational_message = "An identical build ...",205 informational_message = "An identical build ...",
@@ -201,11 +209,10 @@
201 already_pending: informational_message,209 already_pending: informational_message,
202 errors: [error_message]210 errors: [error_message]
203 };211 };
204 mockio.failure({212 mockio.success({
205 status: 400,
206 statusText: "Request Build",213 statusText: "Request Build",
207 responseText: Y.JSON.stringify(response_text),214 responseText: Y.JSON.stringify(response_text),
208 responseHeaders: {'Content-type:': 'application/json'}215 responseHeaders: {'Content-type': 'application/json'}
209 });216 });
210217
211 // The form stays visible and the builds are displayed.218 // The form stays visible and the builds are displayed.