launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05477
[Merge] lp:~abentley/launchpad/back-button-support into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/back-button-support into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #888616 in Launchpad itself: "dynamic bug listings break the back button"
https://bugs.launchpad.net/launchpad/+bug/888616
For more details, see:
https://code.launchpad.net/~abentley/launchpad/back-button-support/+merge/81875
= Summary =
Support back, next, reload in browser
== Proposed fix ==
Use YUI History module
== Pre-implementation notes ==
Discussed with deryck
== Implementation details ==
Rendering is now triggered by YUI's history:change event. The event contains the batch_key, which is used to retrieve the batch from the cache.
Various methods had their parameters changed, to reduce the number of sites where we generate a batch_key or batch query. update_from_model is split into update_from new_model and update_from_cache, the latter triggering the render using Y.history.
I also reduced the number of places where namespace.ListingNavigator is used, preferring this.constructor where applicable.
== Tests ==
bin/test -t test_buglistings.html
== Demo and Q/A ==
Go to a bug listing page. Click Next, Next. Press the browser back button. You should be on the second batch. Click the browser forward button. You should be on the third batch. Click reload. You should still be on the third batch.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/javascript/buglisting.js
lib/lp/bugs/javascript/tests/test_buglisting.js
--
https://code.launchpad.net/~abentley/launchpad/back-button-support/+merge/81875
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/back-button-support 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 16:54:24 +0000
@@ -48,6 +48,7 @@
initializer: function(config) {
var lp_client = new Y.lp.client.Launchpad();
var cache = lp_client.wrap_resource(null, config.cache);
+ var batch_key;
var template = config.template;
this.set('search_params', namespace.get_query(config.current_url));
delete this.get('search_params').start;
@@ -56,7 +57,7 @@
delete this.get('search_params').orderby;
this.set('io_provider', config.io_provider);
this.set('field_visibility', cache.field_visibility);
- this.handle_new_batch(cache);
+ batch_key = this.handle_new_batch(cache);
this.set('current_batch', cache);
// Work around mustache.js bug 48 "Blank lines are not preserved."
// https://github.com/janl/mustache.js/issues/48
@@ -68,6 +69,22 @@
if (Y.Lang.isValue(config.navigation_indices)) {
this.set('navigation_indices', config.navigation_indices);
}
+ this.set('history', new Y.History({
+ initialState: {
+ batch_key: batch_key
+ }
+ }));
+ this.get('history').on('change', this.history_changed, this);
+ },
+
+ /**
+ * Event handler for history:change events.
+ */
+ history_changed: function(e){
+ var batch_key = e.newVal.batch_key;
+ var batch = this.get('batches')[batch_key];
+ this.set('current_batch', batch);
+ this.render();
},
/**
@@ -76,12 +93,11 @@
* The node is also marked up appropriately.
*/
clickAction: function(selector, callback) {
- that = this;
var nodes = Y.all(selector);
nodes.on('click', function(e) {
e.preventDefault();
- callback.call(that);
- });
+ callback.call(this);
+ }, this);
nodes.addClass('js-action');
},
@@ -92,13 +108,14 @@
*/
handle_new_batch: function(batch) {
var key, i;
- var batch_key = namespace.ListingNavigator.get_batch_key(batch);
+ var batch_key = this.constructor.get_batch_key(batch);
Y.each(this.get('field_visibility'), function(value, key) {
for (i = 0; i < batch.mustache_model.bugtasks.length; i++) {
delete batch.mustache_model.bugtasks[i][key];
}
});
this.get('batches')[batch_key] = batch;
+ return batch_key;
},
/**
@@ -135,16 +152,20 @@
'inactive', this.get('current_batch').next === null);
},
+ update_from_new_model: function(query, model){
+ this.update_from_cache(query, this.handle_new_batch(model));
+ },
+
/**
* A shim to use the data of an LP.cache to render the bug listings and
* cache their data.
*
- * order_by is the ordering used by the model.
+ * query is a mapping of query variables generated by get_batch_query.
+ * batch_key is the key generated by get_batch_key for the model.
*/
- update_from_model: function(model) {
- this.handle_new_batch(model);
- this.set('current_batch', model);
- this.render();
+ update_from_cache: function(query, batch_key) {
+ var url = '?' + Y.QueryString.stringify(query);
+ this.get('history').addValue('batch_key', batch_key, {url: url});
},
/**
@@ -173,14 +194,14 @@
* will be retrieved and cached upon retrieval.
*/
update: function(config) {
- var key = namespace.ListingNavigator.get_batch_key(config);
+ var key = this.constructor.get_batch_key(config);
var cached_batch = this.get('batches')[key];
+ var query = this.get_batch_query(config);
if (Y.Lang.isValue(cached_batch)) {
- this.set('current_batch', cached_batch);
- this.render();
+ this.update_from_cache(query, key);
}
else {
- this.load_model(config);
+ this.load_model(query);
}
},
@@ -248,12 +269,14 @@
/**
* 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(config) {
- var query = this.get_batch_query(config);
+ load_model: function(query) {
var load_model_config = {
on: {
- success: Y.bind(this.update_from_model, this)
+ success: Y.bind(this.update_from_new_model, this, query)
}
};
var context = this.get('current_batch').context;
@@ -324,4 +347,4 @@
};
-}, "0.1", {"requires": ["node", 'lp.client']});
+}, "0.1", {"requires": ["history", "node", 'lp.client']});
=== 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 16:54:24 +0000
@@ -38,7 +38,8 @@
var target = Y.Node.create('<div id="client-listing"></div>');
var model = {
field_visibility: {show_item: true},
- mustache_model: {bugtasks: []}
+ mustache_model: {bugtasks: []},
+ memo: 1
};
var navigator = new module.ListingNavigator({
current_url: '',
@@ -46,8 +47,13 @@
template: '',
target: target
});
- navigator.update_from_model(
- {mustache_model: {bugtasks: [{show_item: true}]}});
+ var batch = {
+ mustache_model: {
+ bugtasks: [{show_item: true}]},
+ memo: 2
+ };
+ var query = navigator.get_batch_query(batch);
+ navigator.update_from_new_model(query, batch);
bugtask = navigator.get('current_batch').mustache_model.bugtasks[0];
Y.Assert.isFalse(bugtask.hasOwnProperty('show_item'));
}
@@ -265,8 +271,8 @@
suite.add(new Y.Test.Case({
name: 'Batch caching',
- test_update_from_model_caches: function() {
- /* update_from_model caches the settings in the module.batches. */
+ test_update_from_new_model_caches: function() {
+ /* update_from_new_model caches the settings in the module.batches. */
var lp_cache = {
context: {
resource_type_link: 'http://foo_type',
@@ -303,7 +309,8 @@
],
bugtasks: ['a', 'b', 'c']
}};
- navigator.update_from_model(batch);
+ var query = navigator.get_batch_query(batch);
+ navigator.update_from_new_model(query, batch);
Y.lp.testing.assert.assert_equal_structure(
batch, navigator.get('batches')[key]);
},
@@ -429,10 +436,13 @@
order_by: 'foo',
last_start: 23
};
+ var target = Y.Node.create('<div id="client-listing"></div>');
return new module.ListingNavigator({
current_url: url,
cache: lp_cache,
- io_provider: mock_io
+ io_provider: mock_io,
+ target: target,
+ template: ''
});
};
@@ -488,6 +498,50 @@
}));
suite.add(new Y.Test.Case({
+ name: 'browser history',
+
+ /**
+ * Update from cache generates a change event for the specified batch.
+ */
+ test_update_from_cache_generates_event: function(){
+ var navigator = get_navigator('');
+ var e = null;
+ navigator.get('history').on('change', function(inner_e){
+ e = inner_e;
+ });
+ navigator.get('batches')['some-batch-key'] = {
+ mustache_model: {
+ bugtasks: []
+ }
+ };
+ navigator.update_from_cache({foo: 'bar'}, 'some-batch-key');
+ Y.Assert.areEqual('some-batch-key', e.newVal.batch_key);
+ Y.Assert.areEqual('?foo=bar', e._options.url);
+ },
+
+ /**
+ * When a change event is emitted, the relevant batch becomes the current
+ * batch and is rendered.
+ */
+ test_change_event_renders_cache: function(){
+ var navigator = get_navigator('');
+ var batch = {
+ mustache_model: {
+ bugtasks: [],
+ foo: 'bar'
+ }
+ };
+ navigator.set('template', '{{foo}}');
+ navigator.get('batches')['some-batch-key'] = batch;
+ navigator.get('history').addValue('batch_key', 'some-batch-key');
+ Y.Assert.areEqual(batch, navigator.get('current_batch'));
+ Y.Assert.areEqual('bar', navigator.get('target').getContent());
+ }
+}));
+
+suite.add(new Y.Test.Case({
+ name: 'from_page tests',
+
setUp: function() {
window.LP = {
cache: {
