Merge lp:~benji/launchpad/bug-768336 into lp:launchpad

[Benji York]

[r=bac][bug=768336][no-qa] fix subscriber name race condition

Bug 768336 describes a problem when subscribing to a bug: "on FF4, the
added row in the portlet only contains the person icon and the remove
icon, without my display name."

It turns out that there is a race condition when looking up a
subscriber's information via the web service. The request is dispatched
asynchronously, but the caller expects the results to be available when
the request-dispatching function returns; it often is not.

It looked like I was going to have to refactor the entire call chain to
be asynchronous, but there were two places where functions were called
without needing to return a result and they could be used as the points
to splice in asynchronous behavior without being hacky.

I can no longer reproduce the bug in FF4 and Chrome continues to work.

The existing tests continue to pass, they can be run by loading
lib/lp/bugs/javascript/tests/test_subscriber.html in a browser.

I made a small, related change by removing the unused displaynameload
event and associated test.

This branch is lint-free.

[Brad Crittenden (bac)]

Nice branch Benji. Thanks for removing 'displaynameload' ... that's the sort of thing that could've lived for a long time doing absolutely nothing.

The rest of the branch looks good.

review: Approve (code)

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-22 11:23:48 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-04-22 18:19:47 +0000
@@ -498,6 +498,14 @@
  * @method add_user_name_link
  */
 function add_user_name_link(subscription) {
+    // Be paranoid about display_name, since timeouts or other errors
+    // could mean display_name wasn't set on initialization.
+    subscription.get('person').set_display_name(function () {
+        _add_user_name_link(subscription);
+    });
+}
+
+function _add_user_name_link(subscription) {
     var person = subscription.get('person');
     var link_node = build_user_link_html(subscription);
     var subscribers = Y.one('#subscribers-links');
@@ -982,11 +990,6 @@
     var name = subscription.get('person').get('name');
     var css_name = subscription.get('person').get('css_name');
     var full_name = subscription.get('person').get('full_display_name');
-    // Be paranoid about display_name, since timeouts or other errors
-    // could mean display_name wasn't set on initialization.
-    if (subscription.get('person').get('display_name') === '') {
-        subscription.get('person').set_display_name();
-    }
     var display_name = subscription.get('person').get('display_name');
     var terms = {
         name: name,
@@ -1453,9 +1456,12 @@
 function add_temp_user_name(subscription) {
     // Be paranoid about display_name, since timeouts or other errors
     // could mean display_name wasn't set on initialization.
-    if (subscription.get('person').get('display_name') === '') {
-        subscription.get('person').set_display_name();
-    }
+    subscription.get('person').set_display_name(function () {
+        _add_temp_user_name(subscription);
+    });
+}
+
+function _add_temp_user_name(subscription) {
     var display_name = subscription.get('person').get('display_name');
     var img_src;
     if (subscription.is_team()) {

=== modified file 'lib/lp/bugs/javascript/subscriber.js'
--- lib/lp/bugs/javascript/subscriber.js	2011-04-19 18:03:50 +0000
+++ lib/lp/bugs/javascript/subscriber.js	2011-04-22 18:19:47 +0000
@@ -294,16 +294,20 @@
      *
      * @method get_display_name_from_api
      * @param client {Object} An LP API client.
+     * @param on_done {Object} A function to call when the display name has
+     * been loaded.
      */
-    get_display_name_from_api: function(client) {
+    get_display_name_from_api: function(client, on_done) {
         var self = this;
         var cfg = {
             on: {
                 success: function(person) {
-                    self.set(
-                        'display_name', person.lookup_value('display_name'));
+                    var display_name = person.lookup_value('display_name');
+                    self.set('display_name', display_name);
                     self.set_truncated_display_name();
-                    self.fire('displaynameload');
+                    if (Y.Lang.isFunction(on_done)) {
+                        on_done();
+                    }
                 }
             }
         };
@@ -343,18 +347,19 @@
      * correctly.
      *
      * @method set_display_name
+     * @param on_done {Object} A function to call when the display name has
+     * been loaded.
      */
-    set_display_name: function() {
+    set_display_name: function(on_done) {
         var display_name = this.get_display_name_from_node();
         if (display_name !== '') {
             this.set('display_name', display_name);
             this.set_truncated_display_name();
-            this.fire('displaynameload');
         } else {
             if (window.LP !== undefined &&
                 window.LP.links.me !== undefined) {
                 var client = new Y.lp.client.Launchpad();
-                this.get_display_name_from_api(client);
+                this.get_display_name_from_api(client, on_done);
             }
         }
     },

=== modified file 'lib/lp/bugs/javascript/tests/test_subscriber.js'
--- lib/lp/bugs/javascript/tests/test_subscriber.js	2011-04-19 18:03:50 +0000
+++ lib/lp/bugs/javascript/tests/test_subscriber.js	2011-04-22 18:19:47 +0000
@@ -157,7 +157,6 @@
     get_display_name_from_api: function(client) {
         this.set('display_name', 'From API');
         this.set_truncated_display_name();
-        this.fire('displaynameload');
     }
 });
 
@@ -202,24 +201,6 @@
 }));
 
 /*
- * Test that the displaynameload event fires.
- */
-suite.add(new Y.Test.Case({
-    name: 'Subscriber displaynameload',
-
-    test_display_name_load_event: function() {
-        var test = this;
-        var event_fired = false;
-        var subscriber = new Y.lp.bugs.subscriber.Subscriber();
-        subscriber.on('displaynameload', function(e) {
-            Y.assert(true);
-        });
-        subscriber.set('uri', '/~tester');
-        subscriber.initializer();
-    }
-}));
-
-/*
  * Test that a Subscription is properly initialized from
  * a simple config and that the basic methods work.
  */