Merge lp:~abentley/launchpad/pre-cache-batches into lp:launchpad

[Aaron Bentley]

Pre-fetch next batch of bug listings.

= Summary =
Implement limited pre-fetching of bug listings.

== Proposed fix ==
The "next" link is pre-fetched, but only if pre-fetching is activated.

== Pre-implementation notes ==
Discussed with deryck

== Implementation details ==
bugs.dynamic_bug_listings.pre_fetch must be enabled to see this in action.

bugs.dynamic_bug_listings.pre_fetch is declared to be related to the view, so that it is provided in the JSON cache.

The first_batch_config next_batch_config and prev_batch_config are extracted from first_batch, next_batch, and last_batch, because initially, all of those would be pre-fetched.

update ensures we do not pre-fetch batches we already have.

Several methods are updated to understand fetch_only, (i.e. don't render).

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

== Demo and Q/A ==
Ensure bugs.dynamic_bug_listings.pre_fetch is active for you. Go to a bugs listing page. Click next. It should update almost instantly, without a roundrip. (This will also cause it to pre-fetch the new next batch.)

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/beta-notification.js
  lib/lp/app/javascript/tests/test_beta_notification.js
  lib/canonical/launchpad/webapp/publisher.py
  lib/lp/bugs/javascript/buglisting.js
  lib/canonical/launchpad/webapp/tests/test_publisher.py
  lib/lp/app/javascript/tests/test_beta_notification.html
  lib/canonical/launchpad/webapp/tests/test_view_model.py
  lib/lp/services/features/flags.py
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/javascript/tests/test_buglisting.js

[Brad Crittenden (bac)]

Hi Aaron,

Thanks for clearing up my confusion on IRC wrt the Y.bind issue.

typo: ajacent -> adjacent

Your tests provide good coverage and are readable. Thanks.

--Brad

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-11-18 20:16:26 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-18 20:16:27 +0000
@@ -2455,7 +2455,10 @@
 
     implements(IBugTaskSearchListingMenu)
 
-    related_features = ['bugs.dynamic_bug_listings.enabled']
+    related_features = (
+        'bugs.dynamic_bug_listings.enabled',
+        'bugs.dynamic_bug_listings.pre_fetch',
+    )
 
     # Only include <link> tags for bug feeds when using this view.
     feed_types = (
 
=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2011-11-18 20:16:26 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2011-11-18 20:16:27 +0000
@@ -40,6 +40,7 @@
     backwards_navigation: {valueFn: empty_nodelist},
     forwards_navigation: {valueFn: empty_nodelist},
     io_provider: {value: null},
+    pre_fetch: {value: false},
     navigation_indices: {valueFn: empty_nodelist}
 };
 
@@ -64,6 +65,7 @@
         this.set('field_visibility', cache.field_visibility);
         batch_key = this.handle_new_batch(cache);
         this.set('current_batch', cache);
+        this.pre_fetch_batches();
         // Work around mustache.js bug 48 "Blank lines are not preserved."
         // https://github.com/janl/mustache.js/issues/48
         if (Y.Lang.isValue(template)) {
@@ -90,6 +92,7 @@
             var batch_key = e.newVal.batch_key;
             var batch = this.get('batches')[batch_key];
             this.set('current_batch', batch);
+            this.pre_fetch_batches();
             this.render();
         }
     },
@@ -167,8 +170,12 @@
             'inactive', !this.has_next());
     },
 
-    update_from_new_model: function(query, model){
-        this.update_from_cache(query, this.handle_new_batch(model));
+    update_from_new_model: function(query, fetch_only, model){
+        var batch_key = this.handle_new_batch(model);
+        if (fetch_only) {
+            return;
+        }
+        this.update_from_cache(query, batch_key);
     },
 
     /**
@@ -202,6 +209,22 @@
         return query;
     },
 
+
+    /**
+     * Pre-fetch adjacent batches.
+     */
+    pre_fetch_batches: function(){
+        var that=this;
+        if (!this.get('pre_fetch')){
+            return;
+        }
+        Y.each(this.get_pre_fetch_configs(), function(config){
+            config.fetch_only = true;
+            that.update(config);
+        });
+    },
+
+
     /**
      * Update the display to the specified batch.
      *
@@ -213,10 +236,13 @@
         var cached_batch = this.get('batches')[key];
         var query = this.get_batch_query(config);
         if (Y.Lang.isValue(cached_batch)) {
+            if (config.fetch_only){
+                return;
+            }
             this.update_from_cache(query, key);
         }
         else {
-            this.load_model(query);
+            this.load_model(query, config.fetch_only);
         }
     },
 
@@ -232,53 +258,70 @@
         });
     },
 
-    /**
-     * Update the navigator to display the first batch.
-     *
-     * The order_by defaults to the current ordering, but may be overridden.
-     */
-    first_batch: function(order_by) {
+    first_batch_config: function(order_by){
         if (order_by === undefined) {
             order_by = this.get('current_batch').order_by;
         }
-        this.update({
+        return {
             forwards: true,
             memo: null,
             start: 0,
             order_by: order_by
-        });
+        };
     },
 
     /**
-     * Update the navigator to display the next batch.
+     * Update the navigator to display the first batch.
+     *
+     * The order_by defaults to the current ordering, but may be overridden.
      */
-    next_batch: function() {
+    first_batch: function(order_by) {
+        this.update(this.first_batch_config(order_by));
+    },
+
+    next_batch_config: function(){
         var current_batch = this.get('current_batch');
         if (!this.has_next()){
-            return;
+            return null;
         }
-        this.update({
+        return {
             forwards: true,
             memo: current_batch.next.memo,
             start: current_batch.next.start,
             order_by: current_batch.order_by
-        });
+        };
     },
-
     /**
-     * Update the navigator to display the previous batch.
+     * Update the navigator to display the next batch.
      */
-    prev_batch: function() {
+    next_batch: function() {
+        var config = this.next_batch_config();
+        if (config === null){
+            return;
+        }
+        this.update(config);
+    },
+    prev_batch_config: function(){
         var current_batch = this.get('current_batch');
         if (!this.has_prev()){
-            return;
+            return null;
         }
-        this.update({
+        return {
             forwards: false,
             memo: current_batch.prev.memo,
             start: current_batch.prev.start,
             order_by: current_batch.order_by
-        });
+        };
+    },
+    /**
+     * Update the navigator to display the previous batch.
+     */
+    prev_batch: function() {
+        var config = this.prev_batch_config();
+        if (config === null){
+            return;
+        }
+        this.update(config);
     },
     /**
      * Change which fields are displayed in the batch.  Input is a config with
@@ -291,15 +334,28 @@
     },
 
     /**
+     * Generate a list of configs to pre-fetch.
+     */
+    get_pre_fetch_configs: function(){
+        var configs = [];
+        var next_batch_config = this.next_batch_config();
+        if (next_batch_config !== null){
+            configs.push(next_batch_config);
+        }
+        return configs;
+    },
+
+    /**
      * Load the specified batch via ajax.  Display & cache on load.
      *
      * query is the query string for the URL, as a mapping.  (See
      * get_batch_query).
      */
-    load_model: function(query) {
+    load_model: function(query, fetch_only) {
         var load_model_config = {
             on: {
-                success: Y.bind(this.update_from_new_model, this, query),
+                success: Y.bind(
+                    this.update_from_new_model, this, query, fetch_only),
                 failure: this.get('failure_handler')
             }
         };
@@ -328,18 +384,30 @@
     });
 };
 
+
+/**
+ * Return the value for a given feature flag in the current scope.
+ * Only flags declared as "related_features" on the view are available.
+ */
+var get_feature_flag = function(flag_name){
+    return LP.cache.related_features[flag_name].value;
+};
+
+
 /**
  * Factory to return a ListingNavigator for the given page.
  */
 namespace.ListingNavigator.from_page = function() {
     var target = Y.one('#client-listing');
     var navigation_indices = Y.all('.batch-navigation-index');
+    var pre_fetch = get_feature_flag('bugs.dynamic_bug_listings.pre_fetch');
     var navigator = new namespace.ListingNavigator({
         current_url: window.location,
         cache: LP.cache,
         template: LP.mustache_listings,
         target: target,
-        navigation_indices: navigation_indices
+        navigation_indices: navigation_indices,
+        pre_fetch: Boolean(pre_fetch)
     });
     namespace.linkify_navigation();
     navigator.set('backwards_navigation', Y.all('.first,.previous'));
 
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-18 20:16:26 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-18 20:16:27 +0000
@@ -59,7 +59,7 @@
             prev: null
         };
         var query = navigator.get_batch_query(batch);
-        navigator.update_from_new_model(query, batch);
+        navigator.update_from_new_model(query, false, batch);
         bugtask = navigator.get('current_batch').mustache_model.bugtasks[0];
         Y.Assert.isFalse(bugtask.hasOwnProperty('show_item'));
     }
@@ -330,7 +330,7 @@
                 bugtasks: ['a', 'b', 'c']
             }};
         var query = navigator.get_batch_query(batch);
-        navigator.update_from_new_model(query, batch);
+        navigator.update_from_new_model(query, true, batch);
         Y.lp.testing.assert.assert_equal_structure(
             batch, navigator.get('batches')[key]);
     },
@@ -470,6 +470,7 @@
         current_url: url,
         cache: lp_cache,
         io_provider: mock_io,
+        pre_fetch: config.pre_fetch,
         target: target,
         template: ''
     });
@@ -605,7 +606,10 @@
             cache: {
                 current_batch: {},
                 next: null,
-                prev: null
+                prev: null,
+                related_features: {
+                    'bugs.dynamic_bug_listings.pre_fetch': {value: 'on'}
+                }
             }
         };
     },
@@ -626,6 +630,120 @@
     }
 }));
 
+suite.add(new Y.Test.Case({
+    name: "pre-fetching batches",
+
+    /**
+     * get_pre_fetch_configs should return a config for the next batch.
+     */
+    test_get_pre_fetch_configs: function(){
+        var navigator = get_navigator();
+        var configs = navigator.get_pre_fetch_configs();
+        var batch_keys = [];
+        Y.each(configs, function(value){
+            batch_keys.push(navigator.constructor.get_batch_key(value));
+        });
+        Y.Assert.areSame('["foo",467,true,500]', batch_keys[0]);
+        Y.Assert.areSame(1, batch_keys.length);
+    },
+
+    /**
+     * get_pre_fetch_configs should return an empty list if no next batch.
+     */
+    test_get_pre_fetch_configs_no_next: function(){
+        var navigator = get_navigator('', {no_next: true});
+        var configs = navigator.get_pre_fetch_configs();
+        var batch_keys = [];
+        Y.each(configs, function(value){
+            batch_keys.push(navigator.constructor.get_batch_key(value));
+        });
+        Y.Assert.areSame(0, batch_keys.length);
+    },
+
+    get_pre_fetch_navigator: function(config){
+        var navigator = get_navigator('', config);
+        var lp_client = new Y.lp.client.Launchpad();
+        var batch = lp_client.wrap_resource(null, {
+            context: {
+                resource_type_link: 'http://foo_type',
+                web_link: 'http://foo/bar'
+            },
+            next: {memo: 57, start: 56}});
+        navigator.set('current_batch', batch);
+        return navigator;
+    },
+
+    /**
+     * Calling pre_fetch_batches should produce a request for the next batch.
+     */
+    test_pre_fetch_batches: function(){
+        var navigator = this.get_pre_fetch_navigator();
+        var io_provider = navigator.get('io_provider');
+        navigator.set('pre_fetch', true);
+        Y.Assert.isNull(io_provider.last_request);
+        navigator.pre_fetch_batches();
+        Y.Assert.areSame(
+            io_provider.last_request.url,
+            '/bar/+bugs/++model++?foo=bar&orderby=&memo=57&start=56');
+    },
+
+    /**
+     * Calling pre_fetch_batches should not produce a request for the next
+     * batch if Navigator.get('pre_fetch') is false.
+     */
+    test_pre_fetch_disabled: function(){
+        var last_url;
+        var navigator = this.get_pre_fetch_navigator();
+        navigator.pre_fetch_batches();
+        Y.Assert.areSame(null, navigator.get('io_provider').last_request);
+    },
+
+    /**
+     * Initialization does a pre-fetch.
+     */
+    test_pre_fetch_on_init: function(){
+        var navigator = get_navigator('', {pre_fetch: true});
+        var last_url = navigator.get('io_provider').last_request.url;
+        Y.Assert.areSame(
+            last_url,
+            '/bar/+bugs/++model++?foo=bar&orderby=foo&memo=467&start=500');
+    },
+    /**
+     * update_from_new_model does a pre-fetch.
+     */
+    test_pre_fetch_on_update_from_new_model: function(){
+        var navigator = get_navigator('');
+        var io_provider = navigator.get('io_provider');
+        var lp_client = new Y.lp.client.Launchpad();
+        var batch = lp_client.wrap_resource(null, {
+            context: {
+                resource_type_link: 'http://foo_type',
+                web_link: 'http://foo/bar'
+            },
+            order_by: 'baz',
+            memo: 'memo1',
+            next: {
+                memo: "pi",
+                start: 314
+            },
+            forwards: true,
+            start: 5,
+            mustache_model: {
+                item: [
+                    {name: 'first'},
+                    {name: 'second'}
+                ],
+                bugtasks: ['a', 'b', 'c']
+            }});
+        Y.Assert.isNull(io_provider.last_request);
+        navigator.set('pre_fetch', true);
+        navigator.update_from_new_model({}, false, batch);
+        Y.Assert.areSame(
+            io_provider.last_request.url,
+            '/bar/+bugs/++model++?foo=bar&orderby=baz&memo=pi&start=314');
+    }
+}));
+
 var handle_complete = function(data) {
     window.status = '::::' + JSON.stringify(data);
     };
 
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-11-16 23:46:01 +0000
+++ lib/lp/services/features/flags.py	2011-11-18 20:16:27 +0000
@@ -65,6 +65,12 @@
      '',
      'Dynamic bug listings',
      'https://dev.launchpad.net/LEP/CustomBugListings'),
+    ('bugs.dynamic_bug_listings.pre_fetch',
+     'boolean',
+     ('Enables pre-fetching bug listing results.'),
+     '',
+     'Listing pre-fetching',
+     'https://dev.launchpad.net/LEP/CustomBugListings'),
     ('code.ajax_revision_diffs.enabled',
      'boolean',
      ("Offer expandable inline diffs for branch revisions."),