launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/distro-structural-subscription into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #902252 in Launchpad itself: " sort buttons that are missing on the distribution dynamic bugs listing"
  https://bugs.launchpad.net/launchpad/+bug/902252

For more details, see:
https://code.launchpad.net/~abentley/launchpad/distro-structural-subscription/+merge/85356

= Summary =
Fix bug #902252: sort buttons that are missing on the distribution dynamic bugs listing

== Proposed fix ==
Change the structural subscription code so that it does not raise an exception if there is no structural subscription link.  There are cases, specifically related to distros, where it is reasonable for the link to be missing, even if a user is logged in.

== Pre-implementation notes ==
Discussed with bac

== Implementation details ==
Changed Y.error to Y.log and updated the tests accordingly.

Fixed some lint.

== Tests ==
xvfb-run bin/test -t test_structural_subscription.html

== Demo and Q/A ==
Ensure you are member of custom-buglisting-demo and not a member of charm-contributors on qastaging.  Go to https://bugs.qastaging.launchpad.net/charm

The order-by bar should be present, the next link should be green, and there should be no "Subscribe to bug mail" link.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/javascript/tests/test_structural_subscription.js
  lib/lp/registry/javascript/structural-subscription.js
-- 
https://code.launchpad.net/~abentley/launchpad/distro-structural-subscription/+merge/85356
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/distro-structural-subscription into lp:launchpad.

=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js	2011-11-26 04:03:29 +0000
+++ lib/lp/registry/javascript/structural-subscription.js	2011-12-12 16:15:30 +0000
@@ -575,7 +575,9 @@
                 .set('size', '50'))
             .append(Y.Node.create('<a/>')
                 .set('target', 'help')
-                .set('href', '/+help-registry/structural-subscription-tags.html')
+                .set(
+                    'href', '/+help-registry/structural-subscription-tags'
+                    + '.html')
                 .addClass('sprite')
                 .addClass('maybe')
                 .append(Y.Node.create('<span/>')
@@ -779,7 +781,8 @@
         '  <dd>' +
         '    <input type="text" name="name">' +
         '    <a target="help" class="sprite maybe"' +
-        '          href="/+help-registry/structural-subscription-name.html">&nbsp;' +
+        '          href="/+help-registry/structural-subscription-name' +
+        '.html">&nbsp;' +
         '      <span class="invisible-link">Structural subscription' +
         '        description help</span></a> ' +
         '  </dd>' +
@@ -1836,7 +1839,8 @@
     // Modify the menu-link-subscribe-to-bug-mail link to be visible.
     var link = Y.one(link_id);
     if (!Y.Lang.isValue(link)) {
-        Y.error('Link to set as the pop-up link not found.');
+        Y.log('No structural subscription link found.');
+        return;
     }
     link.removeClass('invisible-link');
     link.addClass('visible-link');

=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-07-08 05:12:39 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-12-12 16:15:30 +0000
@@ -1472,16 +1472,17 @@
             delete this.content_box;
         },
 
-        _should: {
-            error: {
-                test_setup_subscription_link_none: new Error(
-                    'Link to set as the pop-up link not found.')
-            }
-        },
-
         // Setting up a subscription link with no link in the DOM should fail.
         test_setup_subscription_link_none: function() {
+            var logged_missing_link = false;
+            Y.on('yui:log', function(e){
+                if (e.msg === "No structural subscription link found."){
+                    logged_missing_link = true;
+                }
+            });
             module.setup_subscription_link(this.config, "#link");
+            Y.Assert.isTrue(
+                logged_missing_link, "Missing link not reported.");
         },
 
         // Setting up a subscription link should unset the 'invisible-link',
@@ -1849,5 +1850,5 @@
     }));
 
     Y.lp.testing.Runner.run(suite);
-        
+
 });