launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/mp-diff-reload-741440 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #741440 in Launchpad itself: "merge proposal diff only shows once per page load"
  https://bugs.launchpad.net/launchpad/+bug/741440
  Bug #775610 in Launchpad itself: "Cannot close second javascript diff popup-window"
  https://bugs.launchpad.net/launchpad/+bug/775610

For more details, see:
https://code.launchpad.net/~deryck/launchpad/mp-diff-reload-741440/+merge/60112

This branch does some work to get overlays for showing diffs on bug pages behaving better.  The real fix here is to override hide and show methods on the widget, since DiffOverlay doesn't provide CSS rules to hide/show based on the widget infrastructure.  YUI widget methods normally add or remove a "visible" class in the show/hide methods.

The rest of the work here was done to make the testing story for the widget better.  I added a YUI test to make it easier to test whether the overlays were showing or hiding properly.  This can be run in the browser by pointing at:

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

There is only a test to cover this issue for now, but I'd like to do a follow branch to remove some of the stuff in the branch diff popup Windmill test and put it here.  Since it's better tested as a YUI test.

Also, in order to support the test better, I made one function available on the namespace and passed around the lp_client, rather than having a global lp_client object.  These are all better for the code anyway.

Once this is approved, I'll land it as is, though the Windmill test is failing, just because it adds value now and the Windmill tests are not running anyway.  Then I'll do the follow up branch.  I'd rather fix the Windmill test properly, rather than making minor changes for class name changes, which shouldn't be in the Windmill test anyway.

Thanks for the review!
-- 
https://code.launchpad.net/~deryck/launchpad/mp-diff-reload-741440/+merge/60112
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/mp-diff-reload-741440 into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.diff.js'
--- lib/lp/code/javascript/branchmergeproposal.diff.js	2011-03-23 16:28:51 +0000
+++ lib/lp/code/javascript/branchmergeproposal.diff.js	2011-05-05 20:13:53 +0000
@@ -12,9 +12,6 @@
 // Grab the namespace in order to be able to expose the connect method.
 var namespace = Y.namespace('lp.code.branchmergeproposal.diff');
 
-// The launchpad js client used.
-var lp_client;
-
 /*
  * The DiffOverlay object inherits from the lazr-js PerttyOverlay.
  *
@@ -24,7 +21,7 @@
  */
 var DiffOverlay = function() {
     DiffOverlay.superclass.constructor.apply(this, arguments);
-    Y.after(this._setupDismissDiff, this, 'syncUI')
+    Y.after(this._setupDismissDiff, this, 'syncUI');
 };
 
 Y.extend(DiffOverlay, Y.lazr.PrettyOverlay, {
@@ -33,13 +30,16 @@
             this.constructor.superclass.bindUI.call(this);
         },
 
-        _setupDismissDiff: function() {
-            var cancel_icon = Y.one('.close-button');
+        /*
+         * Override widget's hide/show methods, since DiffOverlay
+         * doesn't provide CSS to handle .visible objects.
+         */
+        hide: function() {
+            this.get('boundingBox').setStyle('display', 'none');
+        },
 
-            // Setup a click handler to dismiss the diff overlay.
-            cancel_icon.on('click', function(e) {
-                Y.one('.yui3-diff-overlay').setStyle('display', 'none');
-            });
+        show: function() {
+            this.get('boundingBox').setStyle('display', 'block');
         }
     });
 
@@ -60,7 +60,7 @@
  * If the diff fails to load, the user is taken to the librarian url just as
  * if Javascript was not enabled.
  */
-function display_diff(node, api_url, librarian_url) {
+function display_diff(node, api_url, librarian_url, lp_client) {
 
     // Look to see if we have rendered one already.
     if (rendered_overlays[api_url] !== undefined) {
@@ -108,7 +108,10 @@
  * Link up the onclick handler for the a.diff-link in the node to the function
  * that will popup the diff in the pretty overlay.
  */
-function link_popup_diff_onclick(node) {
+namespace.link_popup_diff_onclick = function(node, lp_client) {
+    if (lp_client === undefined) {
+        lp_client = new Y.lp.client.Launchpad();
+    }
     var a = node.one('a.diff-link');
     if (Y.Lang.isValue(a)) {
         a.addClass('js-action');
@@ -116,10 +119,10 @@
         var api_url = node.one('a.api-ref').getAttribute('href');
         a.on('click', function(e) {
                 e.preventDefault();
-                display_diff(a, api_url, librarian_url);
+                display_diff(a, api_url, librarian_url, lp_client);
             });
     }
-}
+};
 
 /*
  * Connect the diff links to their pretty overlay function.
@@ -131,14 +134,16 @@
     }
 
     // Setup the LP client.
-    lp_client = new Y.lp.client.Launchpad();
+    var lp_client = new Y.lp.client.Launchpad();
 
     // Listen for the branch-linked custom event.
-    Y.on('lp:branch-linked', link_popup_diff_onclick);
+    Y.on('lp:branch-linked', this.link_popup_diff_onclick);
     // var status_content = Y.one('#branch-details-status-value');
     var nl = Y.all('.popup-diff');
     if (nl) {
-        nl.each(link_popup_diff_onclick);
+        nl.each(function(node, key, node_list) {
+            namespace.link_popup_diff_onclick(node, lp_client);
+        });
     }
 };
 

=== added file 'lib/lp/code/javascript/tests/test_branchdiff.html'
--- lib/lp/code/javascript/tests/test_branchdiff.html	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchdiff.html	2011-05-05 20:13:53 +0000
@@ -0,0 +1,31 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd";>
+
+<html>
+  <head>
+  <title>Test page for branch diff JavaScript unit tests</title>
+
+  <!-- YUI 3.0 Setup -->
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/lazr.js"></script>
+  <script type="text/javascript" src="../../../app/javascript/client.js"></script>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssreset/reset.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssfonts/fonts.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssbase/base.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/javascript/test.css" />
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/lazr/build/lazr-sam.css" />
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../branchmergeproposal.diff.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_branchdiff.js"></script>
+  </head>
+
+<body class="yui3-skin-sam">
+    <div id="test-diff-popup">
+        <a class="diff-link" href="non-url">Test link for testing popup</a>
+        <a class="api-ref" style="display:none;"
+          href="/~foobar/example/branch/">api</a>
+    </div>
+</body>
+</html>

=== added file 'lib/lp/code/javascript/tests/test_branchdiff.js'
--- lib/lp/code/javascript/tests/test_branchdiff.js	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchdiff.js	2011-05-05 20:13:53 +0000
@@ -0,0 +1,70 @@
+/* Copyright 2010 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Tests for lp.code.branchmergeproposal.diff.
+ *
+ */
+
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw', combine: false
+    }).use('test', 'console', 'node-event-simulate', 'lp.client',
+           'lp.code.branchmergeproposal.diff', function(Y) {
+
+    var module = Y.lp.code.branchmergeproposal.diff;
+    var suite = new Y.Test.Suite("branchmergeproposal.diff Tests");
+
+    /*
+     * A Mock client that always calls success on get.
+     */
+    var MockClient = function() {};
+    MockClient.prototype = {
+        'get': function(uri, config) {
+            var content = Y.Node.create('<p>Sample diff.</p>');
+            config.on.success(content);
+        }
+    };
+
+    suite.add(new Y.Test.Case({
+        name: 'Test branch diff functions',
+
+        /*
+         * Diff overlays should reopen with multiple clicks.
+         */
+        test_diff_overlay_multiple_opens: function() {
+            // Setup mock client and initialize the link click handler.
+            var mock_client = new MockClient();
+            var link_node = Y.one('#test-diff-popup');
+            module.link_popup_diff_onclick(link_node, mock_client);
+            // Open the overlay once.
+            link_node.one('a.diff-link').simulate('click');
+            var overlay = Y.one('.yui3-pretty-overlay');
+            Y.Assert.isNotNull(overlay);
+            Y.Assert.areEqual(overlay.getStyle('display'), 'block');
+            // Close the overlay.
+            overlay.one('.close a').simulate('click');
+            Y.Assert.areEqual(overlay.getStyle('display'), 'none');
+            // Open it again.
+            link_node.one('a.diff-link').simulate('click');
+            Y.Assert.areEqual(overlay.getStyle('display'), 'block');
+        }
+
+        }));
+
+    var handle_complete = function(data) {
+        status_node = Y.Node.create(
+            '<p id="complete">Test status: complete</p>');
+        Y.one('body').appendChild(status_node);
+        };
+    Y.Test.Runner.on('complete', handle_complete);
+    Y.Test.Runner.add(suite);
+
+    var console = new Y.Console({newestOnTop: false});
+    console.render('#log');
+
+    // Start the test runner on Y.after to ensure all setup has had a
+    // chance to complete.
+    Y.after('domready', function() {
+        Y.Test.Runner.run();
+    });
+});