launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/fix-subscribe-form-preloading-bug-722450 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #722450 bug subscription form is loaded even if not needed
  https://bugs.launchpad.net/bugs/722450

For more details, see:
https://code.launchpad.net/~gmb/launchpad/fix-subscribe-form-preloading-bug-722450/+merge/51510

This branch is a minor JS refactoring to prevent the advanced subscription overlay from being loaded with every page load, which as lifeless pointed out increases server load unnecessarily.

The fix for this was to move the code that fetches the form content for the overlay into a function that is called in the onClick handler for the "Subscribe" link. This also means that we can do away with reloading the form content after the user has made a change to their subscription, saving us another unnecessary page load.
-- 
https://code.launchpad.net/~gmb/launchpad/fix-subscribe-form-preloading-bug-722450/+merge/51510
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/fix-subscribe-form-preloading-bug-722450 into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/bugtask_index_portlets.js'
--- lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-02-24 00:23:04 +0000
+++ lib/lp/bugs/javascript/bugtask_index_portlets.js	2011-02-28 10:39:42 +0000
@@ -287,7 +287,8 @@
             subscription.set('is_team', false);
             var parent = e.target.get('parentNode');
             if (namespace.use_advanced_subscriptions) {
-                subscription_overlay.show();
+                load_and_show_advanced_subscription_overlay(
+                    subscription, subscription_overlay);
             } else {
                 // Look for the false conditions of subscription, which
                 // is_direct_subscription, etc. don't report correctly,
@@ -495,7 +496,6 @@
  * @param subscription {Object} A Y.lp.bugs.subscriber.Subscription object.
  */
 function setup_advanced_subscription_overlay(subscription) {
-    subscription.enable_spinner("Loading...");
     var subscription_overlay = new Y.lazr.FormOverlay({
         headerContent: '<h2>Subscribe to bug</h2>',
         form_submit_button:
@@ -505,7 +505,22 @@
         centered: true,
         visible: false
     });
+    subscription_overlay.render('#privacy-form-container');
+    return subscription_overlay;
+}
 
+/*
+ * Load the content for and display the advanced subscription overlay.
+ * The call to show() the overlay happens in the success handler for
+ * loading the form content. That way the overlay won't appear empty.
+ *
+ * @method load_and_show_advanced_subscription_overlay
+ * @param subscription {Object} A Y.lp.bugs.subscriber.Subscription object.
+ * @param subscription_overlay {Object} A Y.lazr.FormOverlay to load
+ *                                      content into.
+ */
+function load_and_show_advanced_subscription_overlay(subscription,
+                                                     subscription_overlay) {
     var subscription_link_url = subscription.get(
         'link').get('href') + '/++form++';
     subscription_overlay.set(
@@ -524,22 +539,19 @@
             'form_content', response.responseText);
         subscription_overlay.renderUI();
         subscription_overlay.bindUI();
-        subscription.disable_spinner();
+        subscription_overlay.show();
     }
     function on_failure(id, response, subscription_overlay) {
         subscription_overlay.set(
             'form_content',
             "Sorry, an error occurred while loading the form.");
         subscription_overlay.renderUI();
-        subscription.disable_spinner();
+        subscription_overlay.show();
     }
     var config = {
         on: {success: on_success, failure: on_failure},
     }
     Y.io(subscription_link_url, config);
-    subscription_overlay.render('#privacy-form-container');
-
-    return subscription_overlay;
 }
 
 /*