launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/dynamic-listing-robustness into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #888679 in Launchpad itself: "dynamic bug listings don't handle IO errors well"
  https://bugs.launchpad.net/launchpad/+bug/888679

For more details, see:
https://code.launchpad.net/~abentley/launchpad/dynamic-listing-robustness/+merge/81904

= Summary =
Make bug listings more robust by handling error cases better.

== Proposed fix ==
Pop up an error message if IO fails.  If the user attempts to go to the "next" or "previous" link when there is none, do nothing.

== Pre-implementation notes ==
Discussed IO handling with deryck

== Implementation details ==
Can't think of anything

== Tests ==
bin/test -t test_buglisting.html

== Demo and Q/A ==
Enable firebug.  On the first set of listings, clicking back should not produce an error message.  On the last set of listings, clicking next should not produce an error message.

Disconnect from the Internet.  Change the sort ordering.  An error message should appear.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/tests/test_buglisting.html
  lib/lp/bugs/javascript/buglisting.js
  lib/lp/bugs/javascript/tests/test_buglisting.js
-- 
https://code.launchpad.net/~abentley/launchpad/dynamic-listing-robustness/+merge/81904
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/dynamic-listing-robustness into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2011-11-07 18:48:20 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2011-11-10 20:47:24 +0000
@@ -55,6 +55,11 @@
         delete this.get('search_params').direction;
         delete this.get('search_params').orderby;
         this.set('io_provider', config.io_provider);
+        this.set('error_handler', new Y.lp.client.ErrorHandler());
+        this.get('error_handler').showError = Y.bind(
+            Y.lp.app.errors.display_error, window, null);
+        this.set(
+            'failure_handler', this.get('error_handler').getFailureHandler());
         this.set('field_visibility', cache.field_visibility);
         this.handle_new_batch(cache);
         this.set('current_batch', cache);
@@ -217,11 +222,15 @@
      * Update the navigator to display the next batch.
      */
     next_batch: function() {
+        var current_batch = this.get('current_batch');
+        if (current_batch.next === undefined){
+            return;
+        }
         this.update({
             forwards: true,
-            memo: this.get('current_batch').next.memo,
-            start:this.get('current_batch').next.start,
-            order_by: this.get('current_batch').order_by
+            memo: current_batch.next.memo,
+            start: current_batch.next.start,
+            order_by: current_batch.order_by
         });
     },
 
@@ -229,11 +238,15 @@
      * Update the navigator to display the previous batch.
      */
     prev_batch: function() {
+        var current_batch = this.get('current_batch');
+        if (current_batch.prev === undefined){
+            return;
+        }
         this.update({
             forwards: false,
-            memo: this.get('current_batch').prev.memo,
-            start:this.get('current_batch').prev.start,
-            order_by: this.get('current_batch').order_by
+            memo: current_batch.prev.memo,
+            start: current_batch.prev.start,
+            order_by: current_batch.order_by
         });
     },
     /**
@@ -251,9 +264,11 @@
      */
     load_model: function(config) {
         var query = this.get_batch_query(config);
-        var load_model_config = {
+        var load_model_config;
+        load_model_config = {
             on: {
-                success: Y.bind(this.update_from_model, this)
+                success: Y.bind(this.update_from_model, this),
+                failure: this.get('failure_handler')
             }
         };
         var context = this.get('current_batch').context;
@@ -324,4 +339,4 @@
 };
 
 
-}, "0.1", {"requires": ["node", 'lp.client']});
+}, "0.1", {"requires": ["node", 'lp.client', 'lp.app.errors']});

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.html'
--- lib/lp/bugs/javascript/tests/test_buglisting.html	2011-10-18 15:36:34 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.html	2011-11-10 20:47:24 +0000
@@ -14,16 +14,22 @@
   <script type="text/javascript"
           src="../../../app/javascript/testing/mockio.js"></script>
   <script type="text/javascript"
+          src="../../../app/javascript/client.js"></script>
+  <script type="text/javascript"
           src="../../../app/javascript/effects/effects.js"></script>
   <script type="text/javascript"
+          src="../../../app/javascript/errors.js"></script>
+  <script type="text/javascript"
           src="../../../app/javascript/expander.js"></script>
   <script type="text/javascript"
+          src="../../../app/javascript/formoverlay/formoverlay.js"></script>
+  <script type="text/javascript"
           src="../../../app/javascript/lp.js"></script>
-  <script type="text/javascript"
-          src="../../../app/javascript/client.js"></script>
 
     <script type="text/javascript"
       src="../../../contrib/javascript/mustache.js"></script>
+  <script type="text/javascript"
+      src="../../../app/javascript/overlay/overlay.js"></script>
 
     <!-- The module under test -->
     <script type="text/javascript"

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-07 18:48:20 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-10 20:47:24 +0000
@@ -259,6 +259,14 @@
         Y.Assert.areEqual(1, navigator.get('io_provider').requests.length);
         navigator.first_batch('intensity');
         Y.Assert.areEqual(1, navigator.get('io_provider').requests.length);
+    },
+    test_io_error: function() {
+        var overlay_node;
+        var navigator = this.get_intensity_listing();
+        navigator.first_batch('failure');
+        navigator.get('io_provider').failure();
+        overlay_node = Y.one('.yui3-lazr-formoverlay-errors');
+        Y.Assert.isTrue(Y.Lang.isValue(overlay_node));
     }
 }));
 
@@ -438,6 +446,7 @@
 
 suite.add(new Y.Test.Case({
     name: 'navigation',
+
     /**
      * last_batch uses memo="", start=navigator.current_batch.last_start,
      * direction=backwards, orderby=navigator.current_batch.order_by.
@@ -451,6 +460,7 @@
             'direction=backwards',
             navigator.get('io_provider').last_request.url);
     },
+
     /**
      * first_batch omits memo and direction, start=0,
      * orderby=navigator.current_batch.order_by.
@@ -462,6 +472,7 @@
             '/bar/+bugs/++model++?orderby=foo&start=0',
             navigator.get('io_provider').last_request.url);
     },
+
     /**
      * next_batch uses values from current_batch.next +
      * current_batch.ordering.
@@ -473,6 +484,18 @@
             '/bar/+bugs/++model++?orderby=foo&memo=467&start=500',
             navigator.get('io_provider').last_request.url);
     },
+
+    /**
+     * Calling next_batch when there is none is a no-op.
+     */
+    test_next_batch_missing: function() {
+        var navigator = get_navigator('?memo=pi&start=26');
+        delete navigator.get('current_batch').next;
+        navigator.next_batch();
+        Y.Assert.areSame(
+            null, navigator.get('io_provider').last_request);
+    },
+
     /**
      * prev_batch uses values from current_batch.prev + direction=backwards
      * and ordering=current_batch.ordering.
@@ -484,6 +507,17 @@
             '/bar/+bugs/++model++?orderby=foo&memo=457&start=400&' +
             'direction=backwards',
             navigator.get('io_provider').last_request.url);
+    },
+
+    /**
+     * Calling prev_batch when there is none is a no-op.
+     */
+    test_prev_batch_missing: function() {
+        var navigator = get_navigator('?memo=pi&start=26');
+        delete navigator.get('current_batch').prev;
+        navigator.prev_batch();
+        Y.Assert.areSame(
+            null, navigator.get('io_provider').last_request);
     }
 }));