Merge lp:~gmb/launchpad/subscribers-timeout-bug-471974 into lp:launchpad

This branch fixes bug 471974 by moving the loading of the JSON struct of
subscriber IDs out of the main bug page template. Instead, we'll now
lazily-load it when we actually need it - i.e. when we're loading the
subscribers portlet.

The lazy-loading involves requesting a JSON dump from the server, via a
view designed precisely for that task. This seems a bit hacky, but it's
the quickest way to solve the problem. We will investigate more elegant
solutions later.

The upshot of this is that we shouldn't see any more timeouts on the bug
page due to large numbers of subscribers (especially from dupes).

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/javascript/bugs/bugtask-index.js
  lib/lp/bugs/browser/bug.py
  lib/lp/bugs/browser/bugsubscription.py
  lib/lp/bugs/browser/configure.zcml
  lib/lp/bugs/templates/bug-portlet-subscribers-ids.pt
  lib/lp/bugs/templates/bug-portlet-subscribers.pt
  lib/lp/bugs/templates/bugtask-index.pt

== JSLint notices ==
jslint: No problem found in '/home/graham/canonical/lp-branches/subscribers-timeout-bug-471974/lib/canonical/launchpad/javascript/bugs/bugtask-index.js'.

jslint: 1 file to lint.

== Pylint notices ==

lib/lp/bugs/browser/bug.py
    28: [F0401] Unable to import 'email.MIMEMultipart' (No module named MIMEMultipart)
    29: [F0401] Unable to import 'email.MIMEText' (No module named MIMEText)
    43: [F0401] Unable to import 'lazr.enum' (No module named enum)
    44: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)
    45: [F0401] Unable to import 'lazr.lifecycle.snapshot' (No module named lifecycle)
    46: [F0401] Unable to import 'lazr.restful.interfaces' (No module named restful)

lib/lp/bugs/browser/bugsubscription.py
    16: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    17: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)

= Changes made =

== lib/canonical/launchpad/javascript/bugs/bugtask-index.js ==

 - I've added Javascript to load the subscriber_ids struct just before
   loading the contents of the subscribers portlet. This should stop the
   bug page from timing out.
 - Note that, should loading the subscriber ids from the JSON dump time
   out (which is theoretically possible), the portlet will still load.

== lib/lp/bugs/browser/bug.py ==

 - I've removed the subscriber_ids_json property from the BugViewMixin.
   It belongs specifically in our JSON-dumping view.

== lib/lp/bugs/browser/bugsubscription.py ==

 - I've added a BugPortletSubscribersIds view. This contains a
   subscriber_ids_json property, which returns a JSON dump of the
   subscriber IDs for the current bug.

== lib/lp/bugs/browser/configure.zcml ==

 - I've added the ZCML for the new view.

== lib/lp/bugs/templates/bug-portlet-subscribers-ids.pt ==

 - I've added this view to contain the JSON dump.

== lib/lp/bugs/templates/bug-portlet-subscribers.pt ==

 - I've added a link to the bug-portlet-subscribers-ids URL so that we
   can grab it using JS and load the JSON dump from it.

== lib...

Hi Graham,

Thanks for this fix. I note there are two issues. One, the icon and subscriber name now appear on different lines and for the life of me I cannot figure out why.

Also I'm bothered that a change of this size has no tests. You could do a view test to check the results of your new view. What about Windmill tests?

Perhaps it is just the current state of affairs but the fragility of this change bothers me.

I've removed the 'js' portion from this review as you may want to get an expert to do that portion.

On Tue, Nov 17, 2009 at 08:08:18PM -0000, Brad Crittenden wrote:
> Review: Needs Information code
> Hi Graham,

Hi Brad, thanks for the review.

> Thanks for this fix. I note there are two issues. One, the icon and
> subscriber name now appear on different lines and for the life of me I
> cannot figure out why.

That's not a result of this branch; you can see the same behaviour on
edge at the moment. I'll file a bug about it.

>
> Also I'm bothered that a change of this size has no tests. You could
> do a view test to check the results of your new view. What about
> Windmill tests?
>

You're quite right. I've added a view test for the JSON dump (See below
for diff). The changes should still pass the current windmill test
suite, though IIRC the inline subscribers part of the tests is currently
failing anyway, so it's difficult to tell. I'll run it through windmill
before landing.

> Perhaps it is just the current state of affairs but the fragility of
> this change bothers me.
>

I don't think the change is particularly fragile. I designed it
specifically so that if fetching the IDs fails we'll just fall back to
loading the portlet anyway. The only time someone will have a noticeable
problem in that case is if they try to subscribe and unsubscribe quickly
in succession (in which case the subscribe / unsubscribe link may not
update properly) or if they try to unsubscribe from a dupe, in which
case they'll have to do it via the old +subscribe form rather than via
AJAX.

Can you be more specific about the fragility that concerns you?

> I've removed the 'js' portion from this review as you may want to get
> an expert to do that portion.

I'll ask Gavin to take a look since he's OCR today.

Incremental diff of changes
---------------------------

=== added file 'lib/lp/bugs/browser/tests/bug-subscription-views.txt'
--- lib/lp/bugs/browser/tests/bug-subscription-views.txt 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/bug-subscription-views.txt 2009-11-18 11:50:28 +0000
@@ -0,0 +1,40 @@
+Bug subscription views
+======================
+
+Getting subscriber CSS IDs
+--------------------------
+
+It's possible to get a mapping of bug subscriber names to CSS IDs using
+the +bug-portlet-subscribers-ids view.
+
+ >>> from lp.bugs.interfaces.bug import IBugSet
+ >>> bug_15 = getUtility(IBugSet).get(15)
+
+ >>> subscriber_ids_view = create_initialized_view(
+ ... bug_15, '+bug-portlet-subscribers-ids')
+
+The view's subscriber_ids_js property returns a JSON struct of the
+person name to CSS ID mappings for the bug's subscribers.
+
+ >>> print subscriber_ids_view.subscriber_ids_js
+ {"name16": "subscriber-16"}
+
+If bug 15 is marked as a duplicate of another bug, its subscribers will
+be included in that bugs subscriber_ids_js mapping.
+
+ >>> bug_13 = getUtility(IBugSet).get(13)
+ >>> subscriber_ids_view = create_initialized_view(
+ ... bug_13, '+bug-portlet-subscribers-ids')
+
+ >>> print subscriber_ids_view.subscriber_ids_js
+ {"name12": "subscriber-12"}
+
+ >>> login('<email address hidden>')
+ >>> bug_15.duplicateof = bug_13
+ >>> bug_13 = getUtility(IBugSet).get(13)
+
+ >>> subscriber...

Here's an incremental diff of the changes that Gavin suggested on IRC:

=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-11-17 16:00:21 +0000
+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js 2009-11-18 15:20:12 +0000
@@ -54,8 +54,7 @@
 Y.augment(PortletTarget, Y.Event.Target);
 Y.bugs.portlet = new PortletTarget();
 Y.bugs.portlet.subscribe('bugs:portletloaded', function() {
- setup_subscription_link_handlers();
- load_subscribers_from_duplicates();
+ load_subscriber_ids();
 });
 Y.bugs.portlet.subscribe('bugs:dupeportletloaded', function() {
     setup_unsubscribe_icon_handlers();
@@ -78,6 +77,22 @@
     setup_unsubscribe_icon_handlers();
 });

+/* If loading the subscriber IDs JSON has succeeded, set up the
+ * subscription link handlers and load the subscribers from dupes.
+ */
+Y.bugs.portlet.subscribe('bugs:portletsubscriberidsloaded', function() {
+ setup_subscription_link_handlers();
+ load_subscribers_from_duplicates();
+});
+
+/* If loading the subscriber IDs JSON fails we still need to load the
+ * subscribers from duplicates but we don't set up the subscription link
+ * handlers.
+ */
+Y.bugs.portlet.subscribe('bugs:portletsubscriberidsfailed', function() {
+ load_subscribers_from_duplicates();
+});
+
 /*
  * Subscribing someone else requires loading a grayed out
  * username into the DOM until the subscribe action completes.
@@ -1806,35 +1821,44 @@
         Y.bugs.portlet.fire('bugs:portletloaded');
     }

- function load_portlet() {
- var config = {on: {success: setup_portlet,
- failure: hide_spinner}};
- var url = Y.get(
- '#subscribers-content-link').getAttribute('href').replace(
- 'bugs.', '');
- Y.io(url, config);
- }
+ var config = {on: {success: setup_portlet,
+ failure: hide_spinner}};
+ var url = Y.get(
+ '#subscribers-content-link').getAttribute('href').replace(
+ 'bugs.', '');
+ Y.io(url, config);
+};

- function load_subscriber_ids(transactionid, response, args) {
+function load_subscriber_ids() {
+ function on_success(transactionid, response, args) {
         try {
- var subscriber_ids_node = Y.Node.create(
- response.responseText);
- var subscriber_ids_json = subscriber_ids_node.get('innerHTML');
- subscriber_ids = Y.JSON.parse(subscriber_ids_json);
+ subscriber_ids = Y.JSON.parse(response.responseText);
+ Y.log("Loaded subscriber IDs.");
+
+ // Fire a custom event to trigger the setting-up of the
+ // subscription handlers.
+ Y.bugs.portlet.fire('bugs:portletsubscriberidsloaded');
         } catch (e) {
             Y.log("Unable to load subscriber ids.", "error");
+
+ // Fire an event to signal failure. This ensures that the
+ // subscribers-from-dupes still get loaded into the portlet.
+ Y.bugs.portlet.fire('bugs:portletsubscriberidsfailed');
         }
-
- load_portlet();
- }
-
- var config = {on: {success: lo...

Thanks for adding the view test Graham. I'm satisfied with the code portion and I know Gavin and you have a handle on the JS parts.