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

[Henning Eggers]

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

= 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

[Данило Шеган (danilo)]

review: Approve

=== 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 @@
     def _process_error(self, data=None, builds=None, informational=None,
                        errors=None, reason="Validation"):
         """Set up the response and json data to return to the caller."""
-        self.request.response.setStatus(400, reason)
+        self.request.response.setStatus(200, reason)
         self.request.response.setHeader('Content-type', 'application/json')
         return_data = dict(builds=builds, errors=errors)
         if informational:

=== 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 @@
 // developer to specify onSuccess and onError hooks. It is quite generic and
 // perhaps could be moved to an infrastructure class.
 
-RequestResponseHandler = function () {};
-RequestResponseHandler.prototype = {
-    clearProgressUI: function () {},
-    showError: function (header, error_msg) {},
-    getErrorHandler: function (errorCallback) {
-        var self = this;
-        return function (id, response) {
-            self.clearProgressUI();
-            // If it was a timeout...
-            if (response.status === 503) {
-                self.showError(
-                    'Timeout error, please try again in a few minutes.',
-                    null);
-            } else {
-                if (errorCallback !== null) {
-                    errorCallback(self, id, response);
-                } else {
-                    self.showError(response.responseText, null);
-                }
-            }
-        };
-    },
-    getSuccessHandler: function (successCallback) {
-        var self = this;
-        return function (id, response) {
-            self.clearProgressUI();
-            successCallback(self, id, response);
-        };
-    }
+var RequestResponseHandler = function () {};
+
+RequestResponseHandler.prototype = new Y.lp.client.ErrorHandler();
+
+RequestResponseHandler.prototype.getSuccessHandler = function(callback) {
+    var self = this;
+    return function (id, response) {
+        self.clearProgressUI();
+        callback(self, id, response);
+    };
 };
 
 namespace.RequestResponseHandler = RequestResponseHandler;
@@ -115,28 +96,8 @@
     request_build_response_handler.clearProgressUI = function() {
         destroy_temporary_spinner();
     };
-    request_build_response_handler.showError = function(header, error_msgs) {
-        if (header === null) {
-            if (error_msgs === null) {
-                header = "An error occurred, please contact an " +
-                         "administrator.";
-            } else {
-                header = "The following errors occurred:";
-            }
-        }
-        var error_html = header;
-        if (error_msgs !== null) {
-            error_html += "<ul>";
-            if (typeof(error_msgs) === "string"){
-                error_msgs = [error_msgs];
-            }
-            Y.each(error_msgs, function(error_msg){
-                error_html += "<li>" + error_msg.replace(/<([^>]+)>/g,'') +
-                              "</li>";
-            });
-            error_html += "</ul><p/>";
-        }
-        request_build_overlay.error_node.set('innerHTML', error_html);
+    request_build_response_handler.showError = function(errormessage) {
+        display_errors(null, errormessage);
     };
 };
 
@@ -191,12 +152,8 @@
             method: "POST",
             headers: {'Accept': 'application/xhtml'},
             on: {
-                failure: request_daily_build_response_handler.getErrorHandler(
-                    function(handler, id, response) {
-                        var server_error = 'Server error, ' +
-                                           'please contact an administrator.';
-                        handler.showError(server_error, null);
-                    }),
+                failure:
+                    request_daily_build_response_handler.getFailureHandler(),
                 success:
                     request_daily_build_response_handler.getSuccessHandler(
                     function(handler, id, response) {
@@ -232,13 +189,9 @@
     request_daily_build_response_handler.clearProgressUI = function() {
         destroy_temporary_spinner();
     };
-    request_daily_build_response_handler.showError = function(header, error) {
-        var error_msg = header;
-        if (error !== null) {
-            error_msg += " " + error;
-        }
-        display_message(error_msg, 'build-error');
-        Y.log(error_msg);
+    request_daily_build_response_handler.showError = function(message) {
+        display_message(message, 'build-error');
+        Y.log(message);
     };
 };
 
@@ -294,6 +247,35 @@
     return true;
 };
 
+var get_new_builds_message = function(build_html, current_builds) {
+    var nr_new;
+    if (build_html === null) {
+        return null;
+    }
+    nr_new = display_build_records(build_html, current_builds);
+    if (nr_new > 1) {
+        return Y.Lang.substitute(MANY_BUILDS_MESSAGE, {nr_new: nr_new});
+    }
+    return ONE_BUILD_MESSAGE;
+};
+
+var get_info_header = function(new_builds_message, pending_build_info) {
+    var info_header = Y.Node.create('<div></div>')
+        .addClass('popup-build-informational');
+    if (new_builds_message !== null) {
+        info_header.append(Y.Node.create('<p></p>')
+                .set('text', new_builds_message));
+    }
+    if (pending_build_info !== null) {
+        info_header.append(Y.Node.create('<p></p>')
+                .set('text', pending_build_info));
+    }
+    if (info_header.hasChildNodes()) {
+        return info_header;
+    }
+    return null;
+};
+
 /*
  * The form submit function
  */
@@ -312,79 +294,70 @@
         method: "POST",
         headers: {'Accept': 'application/json; application/xhtml'},
         on: {
-            failure: request_build_response_handler.getErrorHandler(
-                function(handler, id, response) {
-                    var errors = [],
-                        error_header = null,
-                        error_header_text = "",
-                        build_info, build_html, error_info,
-                        nr_new, new_builds_message, field_name;
-                    if( response.status >= 500 ) {
-                        // There's some error content we need to display.
-                        errors = [
-                            response.status + ' ' + response.statusText];
-                    } else {
-                        // The response will be a json data object containing
-                        // info about what builds there are, informational
-                        // text about any builds already pending, plus any
-                        // errors. The FormOverlay infrastructure only
-                        // supports displaying errors so we will construct
-                        // our own HTML where the informational text will be
-                        // appropriately displayed.
-                        build_info = Y.JSON.parse(response.responseText);
-
-                        // The result of rendering the +builds view
-                        build_html = build_info.builds;
-                        // Any builds already pending (informational only)
-                        pending_build_info = build_info.already_pending;
-                        // Other more critical errors
-                        error_info = build_info.errors;
-
-                        if (build_html !== null) {
-                            nr_new = display_build_records(
-                                    build_html, current_builds);
-                            new_builds_message = ONE_BUILD_MESSAGE;
-                            if (nr_new > 1) {
-                                new_builds_message =
-                                        Y.Lang.substitute(
-                                                MANY_BUILDS_MESSAGE,
-                                                {nr_new: nr_new});
-                            }
-                            error_header_text = new_builds_message;
-                        }
-                        if (pending_build_info !== null) {
-                            if (error_header_text !== "" ) {
-                                error_header_text += "<p/>" +
-                                pending_build_info;
-                            } else {
-                                error_header_text = pending_build_info;
-                            }
-                        }
-                        for (field_name in error_info) {
-                            if (error_info.hasOwnProperty(field_name)) {
-                                errors.push(error_info[field_name]);
-                            }
-                        }
-                        if (error_header_text !== "") {
-                            error_header =
-                                "<p><div class='popup-build-informational'>"
-                                + error_header_text + "</p></div>";
-                            if (Y.Object.size(errors)>0) {
-                                error_header +=
-                                    "<p/>" + "There were also some errors:";
-                            }
-                        } else {
-                            error_header = "There were some errors:";
-                        }
-                    }
-                    handler.showError(error_header, errors);
-                }),
+            failure: request_build_response_handler.getFailureHandler(),
             success: request_build_response_handler.getSuccessHandler(
                 function(handler, id, response) {
+                var errors = [],
+                    error_header = null,
+                    error_header_text = "",
+                    build_info, build_html, error_info,
+                    nr_new, info_header, field_name;
+                // The content type is used to tell a fully successful
+                // request from a partially successful one. Successful
+                // responses simply return the HTML snippet for the builds
+                // table. If this ever causes problems, the view should
+                // be changed to always return JSON and to provide an
+                // attribute that identifies the type of response.
+                content_type = response.getResponseHeader('Content-type');
+                if( content_type !== 'application/json' ) {
+                    // We got the HTML for the builds back, we're done.
                     request_build_overlay.hide();
                     display_build_records(
                             response.responseText, current_builds);
-                })
+                } else {
+                    // The response will be a json data object containing
+                    // info about what builds there are, informational
+                    // text about any builds already pending, plus any
+                    // errors. The FormOverlay infrastructure only
+                    // supports displaying errors so we will construct
+                    // our own HTML where the informational text will be
+                    // appropriately displayed.
+                    build_info = Y.JSON.parse(response.responseText);
+
+                    // The result of rendering the +builds view
+                    build_html = build_info.builds;
+                    // Any builds already pending (informational only)
+                    pending_build_info = build_info.already_pending;
+                    // Other more critical errors
+                    error_info = build_info.errors;
+
+                    info_header = get_info_header(
+                        get_new_builds_message(
+                            build_html, current_builds),
+                            pending_build_info);
+
+                    for (field_name in error_info) {
+                        if (error_info.hasOwnProperty(field_name)) {
+                            errors.push(error_info[field_name]);
+                        }
+                    }
+                    error_container = Y.Node.create('<div></div>');
+                    if (info_header !== null) {
+                        error_container.append(info_header);
+                        if (errors.length > 0) {
+                            error_container.append(
+                                Y.Node.create('<p></p>')
+                                    .set(
+                                        'text',
+                                        "There were also some errors:"));
+                        }
+                    } else {
+                        error_container.set(
+                            'text', "There were some errors:");
+                    }
+                    display_errors(error_container, errors);
+                }
+            })
         },
         form: {
             id: request_build_overlay.form_node,
@@ -432,6 +405,31 @@
             "label[for^='field.distroseries.']");
 };
 
+var display_errors = function(container, error_msgs) {
+    var errors_list, header;
+    if (container === null) {
+        if (error_msgs === null) {
+            header = "An error occurred, please contact an administrator.";
+        } else {
+            header = "The following errors occurred:";
+        }
+        container = Y.Node.create('<div></div>')
+            .set('text', header);
+    }
+    if (error_msgs !== null) {
+        errors_list = Y.Node.create('<ul></ul>');
+        if (Y.Lang.isString(error_msgs)){
+            error_msgs = [error_msgs];
+        }
+        Y.each(error_msgs, function(error_msg){
+            errors_list.append(Y.Node.create('<li></li>')
+                .set('text', error_msg));
+        });
+        container.append(errors_list);
+    }
+    request_build_overlay.error_node.setContent(container);
+};
+
 /*
  * Callback used to enable/disable distro series selection when a different
  * ppa is selected.

=== 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 @@
     },
 
 
-    _testRequestbuildFailure: function(status, expected_message) {
-        var mockio = this._makeRequest();
-        mockio.respond({status: status});
+    _testRequestbuildFailure: function(status, expected_message, oops) {
+        var mockio = this._makeRequest(),
+            response = {status: status},
+            error;
+        if (oops !== undefined) {
+            response.responseHeaders = {'X-Lazr-OopsId': oops};
+        }
+        mockio.respond(response);
 
         // No build targets.
         Y.Assert.areSame(
             0, Y.one("#builds-target").all('.package-build').size());
         // The message is being displayed as an error.
-        var error = Y.one("#new-builds-info");
+        error = Y.one("#new-builds-info");
         Y.Assert.isTrue(error.hasClass('build-error'));
         Y.Assert.areSame(expected_message + "Dismiss", error.get('text'));
         // The message can be dismissed.
@@ -90,10 +95,11 @@
            503, "Timeout error, please try again in a few minutes.");
     },
 
-
     test_requestdailybuild_failure_500: function() {
-        this._testRequestbuildFailure(
-           500, "Server error, please contact an administrator.");
+        var oops_id = "OOPS-TESTING",
+            message = "Server error, please contact an administrator. " +
+                "OOPS ID:" + oops_id;
+        this._testRequestbuildFailure(500, message, oops_id);
     }
 
 }));
@@ -171,10 +177,12 @@
 
     test_requestbuilds_failure_500: function() {
         var mockio = this._makeRequest(),
-            status = 500,
-            status_text = "Internal Server Error",
-            expected_error = status + ' ' + status_text;
-        mockio.failure({status: status, statusText: status_text});
+            oops_id = "OOPS-TESTING",
+            message = "Server error, please contact an administrator. " +
+                "OOPS ID:" + oops_id;
+        mockio.failure({
+            'responseHeaders': {'X-Lazr-OopsId': oops_id}
+            });
 
         // The form stays visible and no builds are displayed.
         Y.Assert.isFalse(
@@ -184,14 +192,14 @@
             0, Y.one("#builds-target").all('.package-build').size());
         // The error message is displayed.
         Y.Assert.areSame(
-            expected_error,
+            message,
             Y.one(".yui3-lazr-formoverlay-errors li").get('text'));
         // The submit button is disabled.
         Y.Assert.isTrue(
             Y.one("[name='field.actions.request']").get('disabled'));
     },
 
-    test_requestbuilds_failure_400: function() {
+    test_requestbuilds_build_collision: function() {
         var mockio = this._makeRequest(),
             success_message = "2 new recipe builds have been queued.",
             informational_message = "An identical build ...",
@@ -201,11 +209,10 @@
                 already_pending: informational_message,
                 errors: [error_message]
             };
-        mockio.failure({
-            status: 400,
+        mockio.success({
             statusText: "Request Build",
             responseText: Y.JSON.stringify(response_text),
-            responseHeaders: {'Content-type:': 'application/json'}
+            responseHeaders: {'Content-type': 'application/json'}
             });
 
         // The form stays visible and the builds are displayed.