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

Work in progres.  More to come....
-- 
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 19:59:36 +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.
  *
@@ -31,15 +28,22 @@
         bindUI: function() {
             // call PrettyOverlay's bindUI
             this.constructor.superclass.bindUI.call(this);
-        },
-
-        _setupDismissDiff: function() {
-            var cancel_icon = Y.one('.close-button');
-
-            // Setup a click handler to dismiss the diff overlay.
-            cancel_icon.on('click', function(e) {
-                Y.one('.yui3-diff-overlay').setStyle('display', 'none');
-            });
+            this.on('cancel', function(e) {
+                this.hide();
+            }, this);
+        },
+
+
+        /*
+         * Override widget's hide/show methods, since DiffOverlay
+         * doesn't provide CSS to handle .visible objects.
+         */
+        hide: function() {
+            this.get('boundingBox').setStyle('display', 'none');
+        },
+
+        show: function() {
+            this.get('boundingBox').setStyle('display', 'block');
         }
     });
 
@@ -60,7 +64,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 +112,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,7 +123,7 @@
         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);
             });
     }
 }
@@ -131,14 +138,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 19:59:36 +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 19:59:36 +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();
+    });
+});