launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/dds-req-packagediff-fix-flashing into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #746277 in Launchpad itself: "Retry (js) link to request package diff computation (again) is too wide."
  https://bugs.launchpad.net/launchpad/+bug/746277

For more details, see:
https://code.launchpad.net/~rvb/launchpad/dds-req-packagediff-fix-flashing/+merge/55775

This branch fixes the retry link to request package diffs so that removing the link removes all the js listeners.

== Summary ==
On the +localpackagediffs page, when a package diff fails, a retry link is displayed. The listener was not properly cleared so it seemed that the clickable zone of this retry link was far too wide. This branch refactors the way the initial link is added so that removing it clears all the js listeners.

== Tests ==
lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.html

== QA ==
On a failing package diff request (https://dogfood.launchpad.net/deribuntu/dangerous/+localpackagediffs), make sure than the Retry click listener is limited to the word "Retry".

-- 
https://code.launchpad.net/~rvb/launchpad/dds-req-packagediff-fix-flashing/+merge/55775
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/dds-req-packagediff-fix-flashing into lp:launchpad.

=== modified file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-03-28 16:33:48 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js	2011-03-31 15:43:56 +0000
@@ -457,7 +457,7 @@
 */
 namespace.setup_request_derived_diff = function(container, dsd_link) {
     // Setup handler for diff requests.
-    container.one('.package-diff-placeholder').on('click', function(e) {
+    container.one('.package-diff-compute-request').on('click', function(e) {
         e.preventDefault();
         compute_package_diff(container, dsd_link);
     });
@@ -511,7 +511,7 @@
     };
     var failure_cb = function(transaction_id, response, args) {
         container.one('p.update-in-progress-message').remove();
-        var retry_handler = function(e) {
+        var recompute = function(e) {
             e.preventDefault();
             compute_package_diff(container, dsd_link);
         };
@@ -519,20 +519,15 @@
         var msg = response.responseText;
 
         // If the error is not of the type raised by an error properly
-        // raisedby python then set a standard error message.
+        // raised by python then set a standard error message.
         if (response.status !== 400) {
             msg = 'Failed to compute package diff.';
         }
         var failure_msg = Y.lp.soyuz.base.makeFailureNode(
-            Y.Escape.html(msg), retry_handler);
-        var placeholder = container.one('.package-diff-placeholder')
+            Y.Escape.html(msg), recompute);
+        var placeholder = container.one('.package-diff-placeholder');
         placeholder.set('innerHTML','');
         placeholder.appendChild(failure_msg);
-
-        var anim = Y.lazr.anim.red_flash({
-            node: placeholder
-        });
-        anim.run();
     };
     var config = {
         on: {

=== modified file 'lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.html'
--- lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.html	2011-03-28 20:45:09 +0000
+++ lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.html	2011-03-31 15:43:56 +0000
@@ -42,9 +42,11 @@
             evolution</a>
           <span class="package-diff-button"></span>
           <dt class="package-diff-placeholder">
-          <a class="js-action sprite add" href="#">
-            Compute differences from last common version:
-          </a></dt>
+          <span class="package-diff-compute-request">
+            <a class="js-action sprite add" href="">
+              Compute differences from last common version:
+            </a>
+          </span></dt>
           <dd>
             <ul class="package-diff-status">
               <li>

=== modified file 'lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.js	2011-03-28 21:32:35 +0000
+++ lib/lp/registry/javascript/tests/test_distroseriesdifferences_details.js	2011-03-31 15:43:56 +0000
@@ -75,10 +75,37 @@
                 config.on.success(completed_voc);
             }
         };
-
         dsd_details.poll_interval = 2000;
-
-   },
+    },
+
+    test_request_wrong_click: function() {
+        // Click on the placeholder has no effect.
+        // The listeners are on the the link with class
+        // '.package-diff-compute-request'.
+        // bug=746277.
+        var placeholder = Y.one('#placeholder');
+        placeholder.set('innerHTML', placeholder_content);
+        placeholder
+            .one('#derived')
+                .removeClass('PENDING')
+                    .addClass('FAILED');
+        dsd_details.setup_packages_diff_states(
+            placeholder.one('.diff-extra-container'), dsd_uri);
+        var func_req = undefined;
+        var lp_prot = Y.lp.client.Launchpad.prototype;
+        lp_prot.named_post = function(url, func, config) {
+            func_req = func;
+            config.on.success();
+        };
+
+        var wrong_button = placeholder.one('.package-diff-placeholder');
+
+        wrong_button.simulate('click');
+        var package_diff = Y.one('#parent');
+
+        // The request has not been triggered.
+        Assert.isTrue(package_diff.hasClass('request-derived-diff'));
+    },
 
     test_request_package_diff_computation: function() {
         // A click on the button changes the package diff status and requests
@@ -98,7 +125,7 @@
             config.on.success();
         };
 
-        var button = Y.one('.package-diff-placeholder');
+        var button = placeholder.one('.package-diff-compute-request');
 
         button.simulate('click');
         var package_diff = Y.one('#parent');

=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2011-03-31 05:54:22 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt	2011-03-31 15:43:56 +0000
@@ -42,7 +42,10 @@
     <dt
       tal:condition="python: not(context.package_diff) or not(context.parent_package_diff)"
       class="package-diff-placeholder">
-        <a class="js-action sprite add" href="#">Compute differences from last common version</a>:
+        <span class="package-diff-compute-request">
+          <a class="js-action sprite add" href="">
+            Compute differences from last common version</a>:
+        </span>
     </dt>
     <dt
       tal:condition="python: context.package_diff and context.parent_package_diff">