launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05875
[Merge] lp:~abentley/launchpad/batch-dealising into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/batch-dealising into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #901843 in Launchpad itself: "Next/previous links on batches may retrieve already-seen batches in bug listings"
https://bugs.launchpad.net/launchpad/+bug/901843
For more details, see:
https://code.launchpad.net/~abentley/launchpad/batch-dealising/+merge/85023
= Summary =
Fix bug #901843: Next/previous links on batches may retrieve already-seen batches in bug listings
== Proposed fix ==
Determine relationship of ajacent batches from common values. Use this information to determine the alias of one batch. Store that batch under its alias.
== Pre-implementation notes ==
None
== Implementation details ==
Bug listing batches are identified by memo, direction, ordering and start position. Any batch can be described in forward or backward direction, and both are used in navigation. This means that each batch has two identifies. This code de-aliases those identities, to ensure that the batch can be loaded under its alias without further AJAX requests.
== Tests ==
bin/test -t test_buglisting.html
== Demo and Q/A ==
Ensure you are a member of custom-buglisting-demo on qastaging.
Go to https://bugs.qastaging.launchpad.net/nova
Click Next. Click Previous. The batch should appear without being loaded (no spinner should appear).
= 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/batch-dealising/+merge/85023
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/batch-dealising into lp:launchpad.
=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js 2011-12-06 16:36:16 +0000
+++ lib/lp/bugs/javascript/buglisting.js 2011-12-08 20:17:37 +0000
@@ -205,11 +205,17 @@
},
/**
+ * Return the batch key of the current batch.
+ */
+ get_current_batch_key: function(){
+ return this.get('model').get('history').get('batch_key');
+ },
+
+ /**
* Retrieve the current batch for rendering purposes.
*/
get_current_batch: function(){
- var batch_key = this.get('model').get('history').get('batch_key');
- return this.get('batches')[batch_key];
+ return this.get('batches')[this.get_current_batch_key()];
},
/**
@@ -230,6 +236,33 @@
},
/**
+ * If the supplied batch is adjacent to the current batch, find an alias
+ * of one of the batches and store it.
+ *
+ * A batch has two major aliases because "forwards" may be true or false.
+ * Either the ajacent batch will have an alias for the current batch, or
+ * the current batch will have an alias for the adjacent batch.
+ */
+ dealias_batches: function(batch){
+ var batch_a_keys = namespace.get_batch_key_list(batch);
+ var batch_b_keys = namespace.get_batch_key_list(
+ this.get_current_batch());
+ var aliases = namespace.find_batch_alias(batch_a_keys, batch_b_keys);
+ if (Y.Lang.isNull(aliases)){
+ return;
+ }
+ var alias_batch = this.get('batches')[aliases[0]];
+ if (Y.Lang.isValue(alias_batch)){
+ this.get('batches')[aliases[1]] = alias_batch;
+ } else {
+ alias_batch = this.get('batches')[aliases[1]];
+ if (Y.Lang.isValue(alias_batch)){
+ this.get('batches')[aliases[0]] = alias_batch;
+ }
+ }
+ },
+
+ /**
* Render bug listings via Mustache.
*
* If model is supplied, it is used as the data for rendering the
@@ -275,6 +308,7 @@
update_from_new_model: function(query, fetch_only, model){
var batch_key = this.handle_new_batch(model);
+ this.dealias_batches(model);
if (fetch_only) {
return;
}
@@ -390,45 +424,21 @@
this.update(this.first_batch_config(order_by));
},
- next_batch_config: function(){
- var current_batch = this.get_current_batch();
- if (!this.has_next()){
- return null;
- }
- 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 next batch.
*/
next_batch: function() {
- var config = this.next_batch_config();
+ var config = namespace.next_batch_config(this.get_current_batch());
if (config === null){
return;
}
this.update(config);
},
- prev_batch_config: function(){
- var current_batch = this.get_current_batch();
- if (!this.has_prev()){
- return null;
- }
- 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();
+ var config = namespace.prev_batch_config(this.get_current_batch());
if (config === null){
return;
}
@@ -439,7 +449,8 @@
*/
get_pre_fetch_configs: function(){
var configs = [];
- var next_batch_config = this.next_batch_config();
+ var next_batch_config = namespace.next_batch_config(
+ this.get_current_batch());
if (next_batch_config !== null){
configs.push(next_batch_config);
}
@@ -545,6 +556,85 @@
};
+/**
+ * Return a mapping describing the batch previous to the current batch.
+ */
+namespace.prev_batch_config = function(batch){
+ if (Y.Lang.isNull(batch.prev)){
+ return null;
+ }
+ return {
+ forwards: false,
+ memo: batch.prev.memo,
+ start: batch.prev.start,
+ order_by: batch.order_by
+ };
+};
+
+
+/**
+ * Return a mapping describing the batch after the current batch.
+ */
+namespace.next_batch_config = function(batch){
+ if (Y.Lang.isNull(batch.next)) {
+ return null;
+ }
+ return {
+ forwards: true,
+ memo: batch.next.memo,
+ start: batch.next.start,
+ order_by: batch.order_by
+ };
+};
+
+/**
+ * Return a list of the batch keys described in this batch: prev, current and
+ * next. If next or prev is null, the corresponding batch key will be null.
+ */
+namespace.get_batch_key_list = function(batch){
+ var prev_config = namespace.prev_batch_config(batch);
+ var next_config = namespace.next_batch_config(batch);
+ var keys = [];
+ if (Y.Lang.isNull(prev_config)){
+ keys.push(null);
+ } else {
+ keys.push(namespace.ListingNavigator.get_batch_key(prev_config));
+ }
+ keys.push(namespace.ListingNavigator.get_batch_key(batch));
+ if (Y.Lang.isNull(next_config)){
+ keys.push(null);
+ } else {
+ keys.push(namespace.ListingNavigator.get_batch_key(next_config));
+ }
+ return keys;
+};
+
+
+/**
+ * Find an alias between the two supplied batches, if they are adjacent.
+ * Returns a list of two batch keys that should be considered equivalent.
+ * If the supplied batches are not adjacent, returns null.
+ */
+namespace.find_batch_alias = function(batch_a, batch_b) {
+ var prev_batch;
+ var next_batch;
+ if (batch_a[2] === batch_b[1] || batch_a[1] === batch_b[0]){
+ prev_batch = batch_a;
+ next_batch = batch_b;
+ } else if (batch_b[2] === batch_a[1] || batch_b[1] === batch_a[0]){
+ prev_batch = batch_b;
+ next_batch = batch_a;
+ }
+ else {
+ return null;
+ }
+ if (prev_batch[1] !== next_batch[0]){
+ return [prev_batch[1], next_batch[0]];
+ } else {
+ return [prev_batch[2], next_batch[1]];
+ }
+};
+
}, "0.1", {
"requires": [
"history", "node", 'lp.client', 'lp.app.errors', 'lp.indicator'
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js 2011-12-06 16:36:16 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js 2011-12-08 20:17:37 +0000
@@ -359,6 +359,8 @@
memo: 'memo1',
forwards: true,
start: 5,
+ next: null,
+ prev: null,
mustache_model: {
item: [
{name: 'first'},
@@ -521,7 +523,10 @@
memo: 457,
start: 400
},
+ forwards: true,
order_by: 'foo',
+ memo: 457,
+ start: 450,
last_start: 23,
field_visibility: {},
field_visibility_defaults: {}
@@ -840,6 +845,7 @@
memo: "pi",
start: 314
},
+ prev: null,
forwards: true,
start: 5,
mustache_model: {
@@ -858,6 +864,7 @@
}
}));
+
suite.add(new Y.Test.Case({
name: "Test indicators",
@@ -884,7 +891,9 @@
var navigator = get_navigator();
navigator.update({});
navigator.get('io_provider').last_request.successJSON({
- mustache_model: {bugtasks: []}
+ mustache_model: {bugtasks: []},
+ next: null,
+ prev: null
});
Y.Assert.isFalse(navigator.indicator.get('visible'));
},
@@ -896,7 +905,9 @@
navigator.indicator.setBusy();
navigator.update({fetch_only: true});
navigator.get('io_provider').last_request.successJSON({
- mustache_model: {bugtasks: []}
+ mustache_model: {bugtasks: []},
+ next: null,
+ prev: null
});
Y.Assert.isTrue(navigator.indicator.get('visible'));
},
@@ -921,6 +932,132 @@
}
}));
+
+suite.add(new Y.Test.Case({
+ name: "Find batch aliases",
+
+ test_get_batch_key_list: function(){
+ var keys = module.get_batch_key_list({
+ prev: null,
+ next:null,
+ memo: 'pi',
+ start: -1,
+ forwards: true,
+ order_by: 'ordering'
+ });
+ Y.ArrayAssert.itemsAreSame(
+ [null, '["ordering","pi",true,-1]', null], keys);
+ keys = module.get_batch_key_list({
+ prev: {
+ memo: "pi",
+ start: -2
+ },
+ next: {
+ memo: "e",
+ start: 0
+ },
+ memo: 'pi',
+ start: -1,
+ forwards: true,
+ order_by: 'ordering'
+ });
+ Y.ArrayAssert.itemsAreSame([
+ '["ordering","pi",false,-2]',
+ '["ordering","pi",true,-1]',
+ '["ordering","e",true,0]'], keys);
+ },
+
+ /* Detect batch aliases for forward movement (next). */
+ test_find_batch_alias_moving_forward: function(){
+ var prev_batch = ['a', 'b', 'c'];
+ var next_batch = ["b'", 'c', 'd'];
+ var result = module.find_batch_alias(prev_batch, next_batch);
+ Y.Assert.areSame(result[0], 'b');
+ Y.Assert.areSame(result[1], "b'");
+ result = module.find_batch_alias(next_batch, prev_batch);
+ Y.Assert.areSame(result[0], 'b');
+ Y.Assert.areSame(result[1], "b'");
+ },
+
+ /* Detect batch aliases for backward movement (prev). */
+ test_find_batch_alias_moving_backward: function(){
+ var prev_batch = ['a', 'b', 'c'];
+ var next_batch = ['b', "c'", 'd'];
+ var result = module.find_batch_alias(prev_batch, next_batch);
+ Y.Assert.areSame(result[0], 'c');
+ Y.Assert.areSame(result[1], "c'");
+ result = module.find_batch_alias(next_batch, prev_batch);
+ Y.Assert.areSame(result[0], 'c');
+ Y.Assert.areSame(result[1], "c'");
+ },
+
+ /* Do not detect aliases if batches are unrelated */
+ test_find_batch_alias_unrelated: function(){
+ var prev_batch = ['a', 'b', 'c'];
+ var next_batch = ['d', 'e', 'f'];
+ var result = module.find_batch_alias(next_batch, prev_batch);
+ Y.Assert.isNull(result);
+ },
+
+ /**
+ * When dealias_batches is called on the next batch, the current batch is
+ * re-added to the batches mapping, under its alias from the next batch.
+ */
+ test_dealias_batches_next: function(){
+ var navigator = get_navigator();
+ var next_batch = {
+ memo: 467,
+ start: 500,
+ order_by: 'foo',
+ forwards: true,
+ prev: {
+ memo: 467,
+ start: 450
+ },
+ next: null
+ };
+ var prev_batch_config = module.prev_batch_config(next_batch);
+ var prev_batch_key = navigator.constructor.get_batch_key(
+ prev_batch_config);
+ navigator.dealias_batches(next_batch);
+ Y.Assert.areSame(
+ navigator.get('batches')[prev_batch_key],
+ navigator.get_current_batch()
+ );
+ Y.Assert.areNotSame(
+ prev_batch_key, navigator.get_current_batch_key());
+ },
+ /**
+ * When dealias_batches is called on the previous batch, the current batch
+ * is re-added to the batches mapping, under its alias from the previous
+ * batch.
+ */
+ test_dealias_batches_prev: function(){
+ var navigator = get_navigator();
+ var prev_batch = {
+ memo: 457,
+ start: 400,
+ order_by: 'foo',
+ forwards: false,
+ next: {
+ memo: 467,
+ start: 450
+ },
+ prev: null
+ };
+ var next_batch_config = module.next_batch_config(prev_batch);
+ var next_batch_key = navigator.constructor.get_batch_key(
+ next_batch_config);
+ navigator.dealias_batches(prev_batch);
+ Y.Assert.areSame(
+ navigator.get('batches')[next_batch_key],
+ navigator.get_current_batch()
+ );
+ Y.Assert.areNotSame(
+ next_batch_key, navigator.get_current_batch_key());
+ }
+}));
+
var handle_complete = function(data) {
window.status = '::::' + JSON.stringify(data);
};
