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

[Henning Eggers]

[r=abentley][bug=824435][incr] Remove Javascript alert and add test for requestdailybuild.

= Bug 824435 =

This is the first part of the fix. Does two things:

 1. Remove the Javascript alert
 2. Add a test as a fixture for the current situation

The next part will contain the actual fix.

== Proposed fix ==

The Javascript alert is a Bad Thing and I also don't know how to test
it. I replaced it with an in-place message which was already used for
displaying the result for the successful case.
I changed the behaviour of this message box by adding a "Dismiss"
button instead of having it disappear after 20 minutes. In case of the
error message it is a Good Idea to not have it disappear before you
can copy it to IRC. Also, since the page shifts around abit when the
message disappears, it used to be a bit surprising when that happened
by itself.

== Pre-implementation notes ==

I talked this through with Deryck and Aaron and they also helped me a
lot. There has been no pre-imp for the UI change, though.

== Implementation details ==

This is the second usage of IORecorder and I had to extend it to make
failure responses possible. The next step will see more extensions.

== Tests ==

The tests test the two possible errors that can currently be detected by
the failure handler: 503 and !503.

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

== Demo and Q/A ==

Create a new recipe without changing the standard settings. You will see
the "Buld now" link. Clicking on it should create new builds and the
success message will be displayed. When it is dismissed, the "Build now"
button is gone, too.

I am not sure how to trigger an error yet.

= Launchpad lint =

I will fix these in the next go.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/javascript/tests/test_requestbuild_overlay.html
  lib/lp/code/templates/sourcepackagerecipe-index.pt
  lib/lp/code/javascript/requestbuild_overlay.js
  lib/lp/app/javascript/testing/iorecorder.js
  lib/lp/code/javascript/tests/test_requestbuild_overlay.js

./lib/lp/code/javascript/requestbuild_overlay.js
      40: Expected '===' and instead saw '=='.
      45: Expected '!==' and instead saw '!='.
      70: Expected '===' and instead saw '=='.
      72: ['name'] is better written in dot notation.
     111: Expected '===' and instead saw '=='.
     112: Expected '===' and instead saw '=='.
     113: Expected '{' and instead saw 'header'.
     116: Expected '{' and instead saw 'header'.
     119: Expected '!==' and instead saw '!='.
     121: Expected '===' and instead saw '=='.
     227: Expected '!==' and instead saw '!='.
     228: Expected '{' and instead saw 'error_msg'.
     239: Use the array literal notation [].
     261: Expected '{' and instead saw 'return'.
     275: Expected ';' and instead saw 'if'.
     276: Expected '===' and instead saw '=='.
     290: Expected '{' and instead saw 'return'.
     324: ['builds'] is better written in dot notation.
     326: ['already_pending'] is better written in dot notation.
     328: ['errors'] is better written in dot notation.
     331: Expected '!==' and instead saw '!='.
     343: Expected '!==' and instead saw '!='.
     344: Expected '!==' and instead saw '!='.
     345: Expected '{' and instead saw 'error_header_text'.
     347: Expected '{' and instead saw 'error_header_text'.
     350: Move 'var' declarations to the top of the function.
     350: Stopping. (67% scanned).
       0: JSLINT had a fatal error.
     161: Line has trailing whitespace.
./lib/lp/code/javascript/tests/test_requestbuild_overlay.js
      21: ['context'] is better written in dot notation.
      99: Unexpected ','.
      43: Line has trailing whitespace.
      47: Line has trailing whitespace.

[Aaron Bentley (abentley)]

This is a syntax error in some browsers, so it should be fixed before landing:

./lib/lp/code/javascript/tests/test_requestbuild_overlay.js
      21: ['context'] is better written in dot notation.
      99: Unexpected ','.

Aside from that, this is landable.

200 is far from the only "success" code. It might be better to trigger success on < 400.

review: Needs Fixing

[Henning Eggers (henninge)]

I has screenshots!

http://people.canonical.com/~henninge/screenshots/requestbuild_overlay_error.png
http://people.canonical.com/~henninge/screenshots/requestbuild_overlay_success.png

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'lib/lp/app/javascript/testing/iorecorder.js'
--- lib/lp/app/javascript/testing/iorecorder.js	2011-07-27 18:10:47 +0000
+++ lib/lp/app/javascript/testing/iorecorder.js	2011-08-19 20:45:30 +0000
@@ -1,8 +1,10 @@
 /* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
 
 YUI.add('lp.testing.iorecorder', function(Y) {
-    namespace = Y.namespace("lp.testing.iorecorder");
-    function MockHttpResponse () {
+    var namespace = Y.namespace("lp.testing.iorecorder");
+
+    function MockHttpResponse (status) {
+        this.status = status;
         this.responseText = '[]';
         this.responseHeaders = {};
     }
@@ -26,12 +28,23 @@
     }
 
 
-    IORecorderRequest.prototype.success = function(value, headers){
-        this.response = new MockHttpResponse();
+    IORecorderRequest.prototype.respond = function(status, value, headers){
+        this.response = new MockHttpResponse(status);
         this.response.setResponseHeader(
             'Content-Type', headers['Content-Type']);
         this.response.responseText = value;
-        this.config.on.success(null, this.response, this.config['arguments']);
+        var callback;
+        if (status === 200) {
+            callback = this.config.on.success;
+        } else {
+            callback = this.config.on.failure;
+        }
+        callback(null, this.response, this.config['arguments']);
+    };
+
+
+    IORecorderRequest.prototype.success = function(value, headers){
+        this.respond(200, value, headers);
     };
 
 

=== modified file 'lib/lp/code/javascript/requestbuild_overlay.js'
--- lib/lp/code/javascript/requestbuild_overlay.js	2011-08-09 14:18:02 +0000
+++ lib/lp/code/javascript/requestbuild_overlay.js	2011-08-19 20:45:30 +0000
@@ -58,6 +58,8 @@
     }
 };
 
+namespace.RequestResponseHandler = RequestResponseHandler;
+
 namespace.connect_requestbuilds = function() {
 
     var request_build_handle = Y.one('#request-builds');
@@ -132,9 +134,32 @@
 var ONE_BUILD_MESSAGE = "1 new recipe build has been queued.";
 var MANY_BUILDS_MESSAGE = "{nr_new} new recipe builds have been queued.";
 
-namespace.connect_requestdailybuild = function() {
+namespace.connect_requestdailybuild = function(config) {
 
     var request_daily_build_handle = Y.one('#request-daily-build');
+    var display_message = function (message, css_class){
+        var build_message_node = Y.Node.create('<div></div>')
+            .set('id', 'new-builds-info')
+            .addClass(css_class)
+            .set('text', message)
+            .append(Y.Node.create('<br />'))
+            .append(Y.Node.create('<a></a>')
+                .set('href', '#')
+                .addClass('discreet')
+                .addClass('js-action')
+                .set('text', 'Dismiss'));
+        build_message_node.one('a').on('click', function(e) {
+            e.preventDefault();
+            build_message_node.hide();
+            if (css_class === 'build-error') {
+                request_daily_build_handle.removeClass("unseen");
+            }
+        });
+        request_daily_build_handle.insert(
+                build_message_node,
+                request_daily_build_handle);
+            
+    };
     request_daily_build_handle.on('click', function(e) {
         e.preventDefault();
 
@@ -154,7 +179,6 @@
             on: {
                 failure: request_daily_build_response_handler.getErrorHandler(
                     function(handler, id, response) {
-                        request_daily_build_handle.removeClass("unseen");
                         var server_error = 'Server error, ' +
                                            'please contact an administrator.';
                         handler.showError(server_error, null);
@@ -178,21 +202,18 @@
                                                 MANY_BUILDS_MESSAGE,
                                                 {nr_new: nr_new});
                         }
-                        var build_message_node = Y.Node.create([
-                            '<div id="new-builds-info" ' +
-                                    'class="build-informational">',
-                            new_builds_message,
-                            '</div>'].join(''));
-                        request_daily_build_handle.insert(
-                                build_message_node,
-                                request_daily_build_handle);
-                        Y.later(20000, build_message_node, 'hide', true);
+                        display_message(
+                            new_builds_message, 'build-informational');
                     }
                   )
             },
             data: qs
         };
-        Y.io(submit_url, y_config);
+        var io = Y.io;
+        if ( config !== undefined && Y.Lang.isValue(config.io)) {
+            io = config.io;
+        }
+        io(submit_url, y_config);
     });
 
     // Wire up the processing hooks
@@ -204,7 +225,7 @@
         var error_msg = header;
         if (error != null)
             error_msg += " " + error;
-        alert(error_msg);
+        display_message(error_msg, 'build-error');
         Y.log(error_msg);
     };
 };

=== added file 'lib/lp/code/javascript/tests/test_requestbuild_overlay.html'
--- lib/lp/code/javascript/tests/test_requestbuild_overlay.html	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_requestbuild_overlay.html	2011-08-19 20:45:30 +0000
@@ -0,0 +1,38 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
+<html>
+  <head>
+  <title>Launchpad lp.code.requestbuild_overlay module</title>
+
+  <!-- YUI and test setup -->
+  <script type="text/javascript"
+          src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
+  </script>
+  <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
+  <script type="text/javascript">
+    // The code expects this to be present.
+    var LP = {
+        cache: {},
+        links: {}
+    };
+  </script>
+  <script type="text/javascript"
+          src="../../../app/javascript/testing/testrunner.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/testing/iorecorder.js"></script>
+
+ <script type="text/javascript"
+          src="../../../app/javascript/client.js"></script>
+ <script type="text/javascript"
+          src="../../../app/javascript/extras/extras.js"></script>
+  <script type="text/javascript"
+          src="../../../app/javascript/anim/anim.js"></script>
+  <!-- The module under test -->
+  <script type="text/javascript" src="../requestbuild_overlay.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_requestbuild_overlay.js"></script>
+</head>
+<body class="yui3-skin-sam">
+<div id="testbed"></div>
+</body>
+</html>

=== added file 'lib/lp/code/javascript/tests/test_requestbuild_overlay.js'
--- lib/lp/code/javascript/tests/test_requestbuild_overlay.js	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_requestbuild_overlay.js	2011-08-19 20:45:30 +0000
@@ -0,0 +1,104 @@
+/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
+
+YUI().use('test', 'console', 'node-event-simulate',
+          'lp.testing.iorecorder', 'lp.testing.runner',
+          'lp.code.requestbuild_overlay', function(Y) {
+
+var suite = new Y.Test.Suite("lp.code.requestbuild_overlay Tests");
+
+var module = Y.lp.code.requestbuild_overlay;
+
+var builds_target_markup = '<table>' +
+    '<tr class="package-build" id="build-1"></tr>' +
+    '<tr class="package-build" id="build-2"></tr>' +
+    '</table>';
+
+suite.add(new Y.Test.Case({
+    name: "lp.code.requestbuild_overlay",
+
+    setUp: function() {
+        LP.cache['context'] = {
+            web_link: "http://code.launchpad.dev/~foobar/myrecipe"};
+        // Prepare testbed.
+        var testbed = Y.one("#testbed").set('innerHTML', '');
+        testbed.append(
+            Y.Node.create('<a></a>')
+                .set('id', 'request-daily-build')
+                .set('href', '#')
+                .addClass('unseen')
+                .set('text', 'Build now')
+        ).append(
+            Y.Node.create('<div></div>')
+                .set('id', 'builds-target')
+        );
+    },
+
+    _makeRequest: function() {
+        var recorder = new Y.lp.testing.iorecorder.IORecorder();
+        var build_now_link = Y.one('#request-daily-build');
+        build_now_link.removeClass('unseen');
+        module.connect_requestdailybuild(
+            {io: Y.bind(recorder.do_io, recorder)});
+        build_now_link.simulate('click');
+        
+        Y.Assert.areSame(1, recorder.requests.length);
+        return recorder.requests[0];
+    },
+    
+    test_requestbuild_success: function() {
+        var request = this._makeRequest();
+        request.success(
+            builds_target_markup,  {'Content-Type': 'application/xhtml'});
+
+        // The markup has been inserted.
+        Y.Assert.areSame(
+            2, Y.one("#builds-target").all('.package-build').size());
+        // The message is being displayed as informational.
+        var info = Y.one("#new-builds-info");
+        Y.Assert.isTrue(info.hasClass('build-informational'));
+        Y.Assert.areSame(
+            "2 new recipe builds have been queued.Dismiss",
+            info.get('text'));
+        // The message can be dismissed.
+        info.one('a').simulate('click');
+        Y.Assert.areSame('none', info.getStyle('display'));
+        // The build now button is hidden.
+        Y.Assert.isTrue(Y.one('#request-daily-build').hasClass('unseen'));
+    },
+
+
+    _testRequestbuildFailure: function(status, expected_message) {
+        var request = this._makeRequest();
+        request.respond(status, '',  {});
+
+        // 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");
+        Y.Assert.isTrue(error.hasClass('build-error'));
+        Y.Assert.areSame(expected_message + "Dismiss", error.get('text'));
+        // The message can be dismissed.
+        error.one('a').simulate('click');
+        Y.Assert.areSame('none', error.getStyle('display'));
+        // The build now button stays visible.
+        Y.Assert.isFalse(Y.one('#request-daily-build').hasClass('unseen'));
+    },
+
+
+    test_requestbuild_failure_503: function() {
+        this._testRequestbuildFailure(
+           503, "Timeout error, please try again in a few minutes.");
+    },
+
+
+    test_requestbuild_failure_500: function() {
+        this._testRequestbuildFailure(
+           500, "Server error, please contact an administrator.");
+    }
+
+}));
+
+Y.lp.testing.Runner.run(suite);
+
+});

=== modified file 'lib/lp/code/templates/sourcepackagerecipe-index.pt'
--- lib/lp/code/templates/sourcepackagerecipe-index.pt	2011-03-31 02:29:28 +0000
+++ lib/lp/code/templates/sourcepackagerecipe-index.pt	2011-08-19 20:45:30 +0000
@@ -20,7 +20,8 @@
     div#edit-description.yui3-editable_text-content {
       margin-top: -15px;
     }
-    .build-informational {
+    .build-informational,
+    .build-error {
         background: #d4e8ff url('/+icing/blue-fade-to-grey');
         border: solid #666;
         border-width: 1px 2px 2px 1px;
@@ -32,6 +33,10 @@
         padding-right: 5px;
         content: url('/@@/info');
     }
+    .build-error::before {
+        padding-right: 5px;
+        content: url('/@@/error');
+    }
     .popup-build-informational {
         color: black;
     }