Merge lp:~rharding/launchpad/reportbug into lp:launchpad

[Richard Harding]

Update the buglisting updated navigation to scope css selctors.

= Summary =

I missed a point where the linked navigation gets updated by later events and
would render the report a bug link inert again.

So this is a follow up from:
https://code.launchpad.net/~rharding/launchpad/1024866_buglink/+merge/115117

== Implementation Notes ==

I noticed that we should be using the container of the ListingNavigator object
instance and moved the methods to using that to constrain the targeting.

Since linkify_navigation isn't on the instance, but a module method, we have
to pass in the instance info we need in the form of the container node.

== Tests ==

lib/lp/bugs/javascript/tests/test_listing_navigator.js
lib/lp/registry/javascript/sharing/tests/test_shareetable.html
lib/lp/registry/javascript/sharing/tests/test_pillarsharingview.html

[Francesco Banconi (frankban)]

Looks good, thanks Richard.

review: Approve (code*)

[Raphaël Badin (rvb)]

Looks good.

[0]

101 - '<a class="previous" href="http://example.org/">PreVious</span>' +
102 - '<div id="client-listing"></div>' +
103 + '<a class="previous" href="http://example.org/">PreVious</a>' +
104 + '<div id="client-listing"></div>' +

I've spotted the fix (s/span/a/) but the indentation seems off.

[1]

119 - Y.lp.app.listing_navigator.linkify_navigation('#sharee-table-listing');
120 + Y.lp.app.listing_navigator.linkify_navigation(target);

You explained to me on IRC why this doesn't require a test code change so FTR:

│18:39:46 rick_h_ | rvba: no, the sharee test code didn't test the navigation html at all relying on the underlying code to test/do it
│18:40:26 rick_h_ | rvba: thought about adding a test, but wasn't sure if it was worth the LoC to test what's tested already.

review: Approve

[Richard Harding (rharding)]

> Looks good.
>
> [0]
>
> 101 - '<a class="previous"
> href="http://example.org/">PreVious</span>' +
> 102 - '<div id="client-listing"></div>' +
> 103 + '<a class="previous"
> href="http://example.org/">PreVious</a>' +
> 104 + '<div id="client-listing"></div>' +
>
> I've spotted the fix (s/span/a/) but the indentation seems off.

The indentation was done just to indicate that there's a nested html structure there. The bug here is not limiting to the parent node scope and just tried to bring out that there's a parent there.

=== modified file 'lib/lp/app/javascript/listing_navigator.js'
--- lib/lp/app/javascript/listing_navigator.js	2012-07-16 12:15:49 +0000
+++ lib/lp/app/javascript/listing_navigator.js	2012-07-18 15:40:26 +0000
@@ -30,21 +30,6 @@
 
 
 /**
- * Generate selector helpers so that we only find nodes within our own
- * UI.
- */
-module.one = function (parent, selector) {
-    var updated_selector = parent + " " + selector;
-    return Y.one(updated_selector);
-};
-
-module.all = function (parent, selector) {
-    var updated_selector = parent + " " + selector;
-    return Y.all(updated_selector);
-};
-
-
-/**
  * Constructor.
  *
  * A simplistic model of the current batch.
@@ -199,9 +184,10 @@
      * Call the callback when a node matching the selector is clicked.
      *
      * The node is also marked up appropriately.
+     * Scoped at the parentNode of the target.
      */
     clickAction: function(selector, callback) {
-        var nodes = Y.all(selector);
+        var nodes = this.get('target').get('parentNode').all(selector);
         nodes.on('click', function(e) {
             e.preventDefault();
             callback.call(this);
@@ -536,9 +522,9 @@
  * Rewrite all nodes with navigation classes so that they are hyperlinks.
  * Content is retained.
  */
-module.linkify_navigation = function(parent_selector) {
+module.linkify_navigation = function(container_node) {
     Y.each(['previous', 'next', 'first', 'last'], function(class_name) {
-        module.all(parent_selector, '.' + class_name).each(function(node) {
+        container_node.all('.' + class_name).each(function(node) {
             new_node = Y.Node.create('<a href="#"></a>');
             new_node.addClass(class_name);
             new_node.setContent(node.getContent());

=== modified file 'lib/lp/app/javascript/tests/test_listing_navigator.js'
--- lib/lp/app/javascript/tests/test_listing_navigator.js	2012-07-16 11:50:02 +0000
+++ lib/lp/app/javascript/tests/test_listing_navigator.js	2012-07-18 15:40:26 +0000
@@ -188,7 +188,7 @@
                 '<span class="first">FiRST</span>' +
                 '<span class="last">lAst</span>' +
                 '</div>');
-            module.linkify_navigation('#bugs-table-listing');
+            module.linkify_navigation(Y.one('#bugs-table-listing'));
             function checkNav(selector, content) {
                 var nodelist = Y.all(selector);
                 nodelist.each(function (node) {

=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2012-07-16 11:50:02 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2012-07-18 15:40:26 +0000
@@ -173,10 +173,11 @@
         if (Y.Lang.isNull(target)){
             return null;
         }
+        var container = target.get('parentNode');
         var navigation_indices = Y.all('.batch-navigation-index');
         var pre_fetch = Y.lp.app.listing_navigator.get_feature_flag(
             'bugs.dynamic_bug_listings.pre_fetch');
-        Y.lp.app.listing_navigator.linkify_navigation('#bugs-table-listing');
+        Y.lp.app.listing_navigator.linkify_navigation(container);
         var navigator = new module.BugListingNavigator({
             current_url: window.location,
             cache: LP.cache,
@@ -185,8 +186,10 @@
             navigation_indices: navigation_indices,
             pre_fetch: Boolean(pre_fetch)
         });
-        navigator.set('backwards_navigation', Y.all('.first,.previous'));
-        navigator.set('forwards_navigation', Y.all('.last,.next'));
+        navigator.set('backwards_navigation',
+                      container.all('.first,.previous'));
+        navigator.set('forwards_navigation',
+                      container.all('.last,.next'));
         navigator.clickAction('.first', navigator.first_batch);
         navigator.clickAction('.next', navigator.next_batch);
         navigator.clickAction('.previous', navigator.prev_batch);

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js	2012-07-16 17:57:01 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js	2012-07-18 15:40:26 +0000
@@ -104,8 +104,8 @@
         test_from_page_with_client: function() {
             Y.one('#fixture').setContent(
                 '<div id="bugs-table-listing">' +
-                '<a class="previous" href="http://example.org/">PreVious</span>' +
-                '<div id="client-listing"></div>' +
+                    '<a class="previous" href="http://example.org/">PreVious</a>' +
+                    '<div id="client-listing"></div>' +
                 '</div>');
             Y.Assert.areSame('http://example.org/', this.getPreviousLink());
             module.BugListingNavigator.from_page();

=== modified file 'lib/lp/registry/javascript/sharing/shareetable.js'
--- lib/lp/registry/javascript/sharing/shareetable.js	2012-07-16 11:50:02 +0000
+++ lib/lp/registry/javascript/sharing/shareetable.js	2012-07-18 15:40:26 +0000
@@ -99,20 +99,24 @@
     },
 
     make_navigator: function() {
+        var target = Y.one('#sharee-table');
+        var container = target.get('parentNode');
         var navigation_indices = Y.all('.batch-navigation-index');
-        Y.lp.app.listing_navigator.linkify_navigation('#sharee-table-listing');
+        Y.lp.app.listing_navigator.linkify_navigation(target);
         var ns = Y.lp.registry.sharing.shareelisting_navigator;
         var cache = LP.cache;
         cache.total = this.get('sharees').length;
         var navigator = new ns.ShareeListingNavigator({
             current_url: window.location,
             cache: cache,
-            target: Y.one('#sharee-table'),
+            target: target,
             navigation_indices: navigation_indices,
             batch_info_template: '<div></div>'
         });
-        navigator.set('backwards_navigation', Y.all('.first,.previous'));
-        navigator.set('forwards_navigation', Y.all('.last,.next'));
+        navigator.set('backwards_navigation',
+                      container.all('.first,.previous'));
+        navigator.set('forwards_navigation',
+                      container.all('.last,.next'));
         navigator.clickAction('.first', navigator.first_batch);
         navigator.clickAction('.next', navigator.next_batch);
         navigator.clickAction('.previous', navigator.prev_batch);

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_shareetable.js'
--- lib/lp/registry/javascript/sharing/tests/test_shareetable.js	2012-07-07 14:00:30 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_shareetable.js	2012-07-18 15:40:26 +0000
@@ -41,7 +41,6 @@
             var sharee_table = Y.Node.create(
                     Y.one('#sharee-table-template').getContent());
             this.fixture.appendChild(sharee_table);
-
         },
 
         tearDown: function () {