launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05606
[Merge] lp:~abentley/launchpad/pre-cache-batches into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/pre-cache-batches into lp:launchpad with lp:~abentley/launchpad/view-flags as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #888756 in Launchpad itself: "dynamic bug listings do round trips for new batches"
https://bugs.launchpad.net/launchpad/+bug/888756
For more details, see:
https://code.launchpad.net/~abentley/launchpad/pre-cache-batches/+merge/82716
= 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
--
https://code.launchpad.net/~abentley/launchpad/pre-cache-batches/+merge/82716
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/pre-cache-batches into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-11-18 17:35:31 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-11-18 17:35:33 +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 17:35:31 +0000
+++ lib/lp/bugs/javascript/buglisting.js 2011-11-18 17:35:33 +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)) {
@@ -86,12 +88,20 @@
* Event handler for history:change events.
*/
history_changed: function(e){
+<<<<<<< TREE
if (e.newVal.hasOwnProperty('batch_key')) {
var batch_key = e.newVal.batch_key;
var batch = this.get('batches')[batch_key];
this.set('current_batch', batch);
this.render();
}
+=======
+ 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();
+>>>>>>> MERGE-SOURCE
},
/**
@@ -167,8 +177,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 +216,22 @@
return query;
},
+
+ /**
+ * Pre-fetch ajacent 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 +243,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 +265,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 +341,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 +391,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 17:35:31 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js 2011-11-18 17:35:33 +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 17:35:33 +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."),
