launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug771232 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #771232 in Launchpad itself: "Edit structural subscription overlay is headed "Add a mail subscription for...""
  https://bugs.launchpad.net/launchpad/+bug/771232

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug771232/+merge/59302

This branch fixes the linked bug by making sure that any old form overlay is destroyed before a new one is created.  The problem before was that we were never deleting the old overlays, so when we set the edit header the first time, it worked, but subsequently we were changing the header of old, trash form overlays and the new one was displayed with the wrong header.

Thank you
-- 
https://code.launchpad.net/~gary/launchpad/bug771232/+merge/59302
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug771232 into lp:launchpad.

=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js	2011-04-21 21:13:49 +0000
+++ lib/lp/registry/javascript/structural-subscription.js	2011-04-27 22:04:26 +0000
@@ -325,6 +325,13 @@
  */
 function create_overlay(content_box_id, overlay_id, submit_button,
                         submit_callback, success_callback) {
+    // Some of our code currently expects to create a new overlay every time
+    // we render. This is not a performance problem now, and simplifies some
+    // of the code.  Therefore, before we create an overlay, we need to make
+    // sure that there is not one already.  If there is, destroy it.
+    if (Y.Lang.isValue(namespace._add_subscription_overlay)) {
+        namespace._add_subscription_overlay.destroy();
+    }
     // Create the overlay.
     namespace._add_subscription_overlay = new Y.lazr.FormOverlay({
         headerContent:

=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-04-19 18:14:45 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js	2011-04-27 22:04:26 +0000
@@ -1075,6 +1075,33 @@
             // And the new value is reflected in the subscription listing.
             label = Y.one('.filter-name span').get('text');
             Assert.isTrue(label.indexOf('NEW VALUE') !== -1);
+        },
+
+        test_title: function() {
+            // Make sure that the overlay title is set correctly for editing.
+            module.setup_bug_subscriptions(this.configuration);
+            // Click the edit link.
+            Y.one('a.edit-subscription').simulate('click');
+            var overlays = Y.all('.yui3-lazr-formoverlay');
+            // There is only one overlay in the DOM.
+            Assert.areEqual(overlays.size(), 1);
+            // The title is what we expect.
+            var title = overlays.item(0).one('#subscription-overlay-title');
+            Assert.areEqual(
+                title.get('text'),
+                'Edit subscription for Example project bugs');
+            // Now we cancel, so we can try rendering again.
+            Y.one('button[name="field.actions.cancel"]').simulate('click');
+            // If we do it again, everything is the same (see bug 771232).
+            Y.one('a.edit-subscription').simulate('click');
+            overlays = Y.all('.yui3-lazr-formoverlay');
+            // There is still only one overlay in the DOM.
+            Assert.areEqual(overlays.size(), 1);
+            // The title is still what we expect.
+            title = overlays.item(0).one('#subscription-overlay-title');
+            Assert.areEqual(
+                title.get('text'),
+                'Edit subscription for Example project bugs');
         }
 
     }));