launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/pickers-pickers-everywhere-take-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/pickers-pickers-everywhere-take-2/+merge/65390

Summary
=======
A piece of javascript for setting up structural subscriptions was failing, preventing execution of further js on the page, which killed pickers. The failure was caused by the setup expecting links that don't exist on the advanced bug search page, but do exist on other pages using the template.

Previously we thought the cause of the bug was redundant js on the page from the pickers; while redundant js is certainly not ideal, it's not actually the cause of the bug.

Proposed fix
============
Since the view says whether or not it's showing the advanced search form, we can just use that as a guard to block the javascript setup.

Pre-implementation notes 
=========================
Spoke with Curtis Hovey

Implementation details
======================
As in proposed.

Additionally, I updated the template to use not reassign the namespace for the setup function to "module". It looks like this was done for wrapping issues, but there are other ways to wrap it, and something like "module.setup" seems like it's a a collision waiting to happen when someone does the same trick for wrapping on a template.

Tests
=====
bin/test -vvcm lib/lp/registry/browser/tests/test_subscription_links

Demo and Q/A
============
Open https://qastaging.launchpad.net/launchpad/+bugs?advanced=1
The picker links should be active and work.

Go to any buglisting page; the bug subscription link should exist and work normally.

Launchpad lint
==============

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/templates/buglisting-default.pt
  lib/lp/registry/browser/tests/test_subscription_links.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/pickers-pickers-everywhere-take-2/+merge/65390
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/pickers-pickers-everywhere-take-2 into lp:launchpad.

=== modified file 'lib/lp/bugs/templates/buglisting-default.pt'
--- lib/lp/bugs/templates/buglisting-default.pt	2011-06-16 13:50:58 +0000
+++ lib/lp/bugs/templates/buglisting-default.pt	2011-06-21 19:11:37 +0000
@@ -10,11 +10,13 @@
 <metal:block fill-slot="head_epilogue">
   <meta condition="not: view/should_show_bug_information"
         name="robots" content="noindex,nofollow" />
-  <script type="text/javascript">
+
+    <script type="text/javascript"
+      tal:condition="not: view/shouldShowAdvancedForm">
     LPS.use('lp.registry.structural_subscription', function(Y) {
-        var module = Y.lp.registry.structural_subscription;
         Y.on('domready', function() {
-          module.setup({content_box: "#structural-subscription-content-box"});
+            Y.lp.registry.structural_subscription.setup(
+                {content_box: "#structural-subscription-content-box"});
         });
     });
   </script>

=== modified file 'lib/lp/registry/browser/tests/test_subscription_links.py'
--- lib/lp/registry/browser/tests/test_subscription_links.py	2011-06-16 13:50:58 +0000
+++ lib/lp/registry/browser/tests/test_subscription_links.py	2011-06-21 19:11:37 +0000
@@ -70,11 +70,12 @@
             "Expected edit_bug_mail link missing")
         # Ensure the LP.cache has been populated.
         self.assertTrue("LP.cache['administratedTeams']" in self.contents)
-        # And that the call to setup the subscription is in the HTML.  A
-        # windmill test is required to ensure that the call actually
+        # And that the call to setup the subscription is in the HTML.
+        # The presence of the configuration args are used alone, as anything
+        # else is needlessly brittle.
+        # A windmill test is required to ensure that the call actually
         # succeeded, by checking the link class for 'js-action'.
-        setup = ("""module.setup({content_box: """
-                 """"#structural-subscription-content-box"});""")
+        setup = ('{content_box: "#structural-subscription-content-box"});')
         self.assertTrue(setup in self.contents)