launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/subscriber-portlet-spinner into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #820146 in Launchpad itself: "Subscription portlet displays subscribing spinner too late"
  https://bugs.launchpad.net/launchpad/+bug/820146

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/subscriber-portlet-spinner/+merge/70247

Small fix to display the Subscribing... spinner as soon as the "Subscribe Me" link is clicked.

== Implementation ==

The subscription portlet normally needs to first retrieve the subscribee details (via an xhr call) in order to indicate progress and do the next step in the subscribe process. However, in the case of "Subscribe Me" where the subscribers list is not to be updated, then the spinner can be displayed before this first xhr is made.

== Tests ==

Update a couple of test_subscribers_lists.js tests to reflect the new behaviour.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/subscribers/subscribers_list.js
  lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js

-- 
https://code.launchpad.net/~wallyworld/launchpad/subscriber-portlet-spinner/+merge/70247
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/subscriber-portlet-spinner into lp:launchpad.

=== modified file 'lib/lp/app/javascript/subscribers/subscribers_list.js'
--- lib/lp/app/javascript/subscribers/subscribers_list.js	2011-08-01 08:03:41 +0000
+++ lib/lp/app/javascript/subscribers/subscribers_list.js	2011-08-03 03:52:35 +0000
@@ -354,6 +354,9 @@
         if (is_subscribed) {
             loader._unsubscribe_me();
         } else {
+            if (!loader.display_me_in_list) {
+                loader.subscribers_list.startActivity("Subscribing...");
+            }
             loader._subscribePersonURI(
                 LP.links.me, loader.subscribe_me_level);
         }
@@ -459,8 +462,6 @@
     if (update_subscribers_list) {
         this.subscribers_list.addSubscriber(subscriber, level);
         this.subscribers_list.indicateSubscriberActivity(subscriber);
-    } else {
-        this.subscribers_list.startActivity("Subscribing...");
     }
     var loader = this;
 

=== modified file 'lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js'
--- lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js	2011-08-02 01:42:10 +0000
+++ lib/lp/app/javascript/subscribers/tests/test_subscribers_list.js	2011-08-03 03:52:35 +0000
@@ -2187,6 +2187,13 @@
                     person_uri.match("/~user$"));
                 subscribe_done = true;
             };
+        var activity_started = false;
+        var old_start = module.SubscribersList.prototype.startActivity;
+        module.SubscribersList.prototype.startActivity =
+            function(text) {
+                Y.Assert.areEqual(text, "Subscribing...");
+                activity_started = true;
+            };
 
         loader._setupSubscribeMe();
 
@@ -2194,13 +2201,14 @@
         var link = Y.one('#sub-me-link');
         link.simulate('click');
 
-        Y.Assert.isTrue(subscribe_done);
+        Y.Assert.isTrue(activity_started, 'activity was not started');
 
         // Restore original methods.
         module.SubscribersLoader.prototype._subscribePersonURI =
             old_method;
         module.SubscribersLoader.prototype._updateSubscribeMeLink =
             old_update_method;
+        module.SubscribersList.prototype.startActivity = old_start;
     },
 
     test_unsubscribeMe: function() {
@@ -2496,8 +2504,8 @@
 
     test_subscribe_success_without_list_update: function() {
         // When subscribing someone such that the subscribers list should not
-        // updated, check that the correct start/stop progress indication
-        // calls are used ie stopActivity is called indicating success.
+        // updated, check that the correct stop progress indication
+        // call is used ie stopActivity is called indicating success.
 
         var subscriber = { name: "user", self_link: "/~user" };
 
@@ -2507,14 +2515,7 @@
         window.LP.links.me = "/~user";
 
         // Mock-up stopActivity to ensure it's called.
-        var activity_started = false;
         var activity_stopped = false;
-        var old_start = module.SubscribersList.prototype.startActivity;
-        module.SubscribersList.prototype.startActivity =
-            function(text) {
-                Y.Assert.areEqual(text, "Subscribing...");
-                activity_started = true;
-            };
         var old_stop = module.SubscribersList.prototype.stopActivity;
         module.SubscribersList.prototype.stopActivity =
             function() {
@@ -2536,12 +2537,9 @@
         };
 
         loader._subscribe(person, 'Level1');
-
-        Y.Assert.isTrue(activity_started, 'activity was not started');
         Y.Assert.isTrue(activity_stopped, 'activity was not stopped');
 
         // Restore original methods.
-        module.SubscribersList.prototype.startActivity = old_start;
         module.SubscribersList.prototype.stopActivity = old_stop;
     },