launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/client-bug-fields into lp:launchpad with lp:~abentley/launchpad/bug-fields as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~abentley/launchpad/client-bug-fields/+merge/81157

= Summary =
Enable client-side control of bug listing field visibility.

== Proposed fix ==
Provide ListingNavigator.change_fields.

== Pre-implementation notes ==
Discussed with deryck

== Implementation details ==
Because of a bug in Pystache, the individual bug tasks must contain display control variables such as show_title.  But mustache.js does not have this limitation, so we strip those control variables on the client side.

The Navigator.field_visibility mapping becomes authoritative, and render merges it into each batch as part of the rendering process.

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

== Demo and Q/A ==
There should be no visible change to the new bug listings.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/javascript/buglisting.js
  lib/lp/bugs/javascript/tests/test_buglisting.js
-- 
https://code.launchpad.net/~abentley/launchpad/client-bug-fields/+merge/81157
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/client-bug-fields into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-11-03 14:55:40 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-03 14:55:40 +0000
@@ -2502,6 +2502,8 @@
             cache = IJSONRequestCache(self.request)
             batch_navigator = self.search()
             cache.objects['mustache_model'] = batch_navigator.model
+            cache.objects['field_visibility'] = (
+                batch_navigator.field_visibility)
 
             def _getBatchInfo(batch):
                 if batch is None:

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-11-03 14:55:40 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-11-03 14:55:40 +0000
@@ -1436,6 +1436,15 @@
         self.assertFalse(cache.objects['forwards'])
         self.assertEqual(0, cache.objects['last_start'])
 
+    def test_cache_field_visibility(self):
+        """Cache contains sane-looking field_visibility values."""
+        task = self.factory.makeBugTask()
+        with self.dynamic_listings():
+            view = self.makeView(task, memo=1, forwards=False, size=1)
+        cache = IJSONRequestCache(view.request)
+        field_visibility = cache.objects['field_visibility']
+        self.assertTrue(field_visibility['show_title'])
+
     def getBugtaskBrowser(self):
         """Return a browser for a new bugtask."""
         bugtask = self.factory.makeBugTask()

=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2011-11-03 14:55:40 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2011-11-03 14:55:40 +0000
@@ -33,6 +33,9 @@
     delete this.search_params.orderby;
     this.io_provider = io_provider;
     this.current_batch = lp_client.wrap_resource(null, cache);
+    this.field_visibility = this.current_batch.field_visibility;
+    this.batches = {};
+    this.handle_new_batch(this.current_batch);
     this.template = template;
     //Work around mustache.js bug 48 "Blank lines are not preserved."
     // https://github.com/janl/mustache.js/issues/48
@@ -40,7 +43,6 @@
         this.template = this.template.replace(/\n/g, '
');
     }
     this.target = target;
-    this.batches = {};
     this.backwards_navigation = new Y.NodeList([]);
     this.forwards_navigation = new Y.NodeList([]);
     if (!Y.Lang.isValue(navigation_indices)){
@@ -105,6 +107,22 @@
 
 
 /**
+ * Handle a previously-unseen batch by storing it in the cache and stripping
+ * out field_visibility values that would otherwise shadow the real values.
+ */
+namespace.ListingNavigator.prototype.handle_new_batch = function(batch){
+    var key, i;
+    var batch_key = namespace.ListingNavigator.get_batch_key(batch);
+    Y.each(this.field_visibility, function(value, key){
+        for (i = 0; i < batch.mustache_model.bugtasks.length; i++){
+            delete batch.mustache_model.bugtasks[i][key];
+        }
+    });
+    this.batches[batch_key] = batch;
+};
+
+
+/**
  * Render bug listings via Mustache.
  *
  * If model is supplied, it is used as the data for rendering the listings.
@@ -113,7 +131,8 @@
  * The template is always LP.mustache_listings.
  */
 namespace.ListingNavigator.prototype.render = function(){
-    var model = this.current_batch.mustache_model;
+    var model = Y.merge(
+        this.current_batch.mustache_model, this.field_visibility);
     var batch_info = Mustache.to_html(this.batch_info_template, {
         start: this.current_batch.start + 1,
         end: this.current_batch.start +
@@ -152,8 +171,7 @@
  * order_by is the ordering used by the model.
  */
 namespace.ListingNavigator.prototype.update_from_model = function(model){
-    var key = namespace.ListingNavigator.get_batch_key(model);
-    this.batches[key] = model;
+    this.handle_new_batch(model);
     this.current_batch = model;
     this.render();
 };
@@ -253,6 +271,12 @@
 };
 
 
+namespace.ListingNavigator.prototype.change_fields = function(config){
+    this.field_visibility = Y.merge(this.field_visibility, config);
+    this.render();
+};
+
+
 /**
  * Load the specified batch via ajax.  Display & cache on load.
  */

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-02 20:02:59 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-03 14:55:40 +0000
@@ -14,9 +14,33 @@
     test_sets_search_params: function(){
         // search_parms includes all query values that don't control batching
         var navigator = new module.ListingNavigator(
-            'http://yahoo.com?foo=bar&start=1&memo=2&direction=3&orderby=4');
+            'http://yahoo.com?foo=bar&start=1&memo=2&direction=3&orderby=4',
+            {});
         Y.lp.testing.assert.assert_equal_structure(
             {foo: 'bar'}, navigator.search_params);
+    },
+    test_cleans_visibility_from_current_batch: function(){
+        // When initial batch is handled, field visibility is stripped.
+        var navigator = new module.ListingNavigator('', {
+            field_visibility: {show_item: true},
+            mustache_model: {bugtasks: [{show_item: true} ] }
+        });
+        bugtask = navigator.current_batch.mustache_model.bugtasks[0];
+        Y.Assert.isFalse(bugtask.hasOwnProperty('show_item'));
+    },
+    test_cleans_visibility_from_new_batch: function(){
+        // When new batch is handled, field visibility is stripped.
+        var bugtask;
+        var target = Y.Node.create('<div id="client-listing"></div>');
+        var model = {
+            field_visibility: {show_item: true},
+            mustache_model: {bugtasks: []}
+        };
+        var navigator = new module.ListingNavigator('', model, '', target);
+        navigator.update_from_model(
+            {mustache_model: {bugtasks: [{show_item: true}]}});
+        bugtask = navigator.current_batch.mustache_model.bugtasks[0];
+        Y.Assert.isFalse(bugtask.hasOwnProperty('show_item'));
     }
 }));
 
@@ -31,15 +55,16 @@
         var target = Y.Node.create('<div id="client-listing"></div>');
         var lp_cache = {
             mustache_model: {
-                foo: 'bar',
-                bugtasks: ['a', 'b', 'c']
+                bugtasks: [{foo: 'bar', show_foo: true}]
             },
             next: null,
             prev: null,
             start: 5,
-            total: 256
+            total: 256,
+            field_visibility: {show_foo: true}
         };
-        var template = "{{foo}}";
+        var template = "{{#bugtasks}}{{#show_foo}}{{foo}}{{/show_foo}}" +
+            "{{/bugtasks}}";
         var navigator =  new module.ListingNavigator(
             null, lp_cache, template, target);
         var index = Y.Node.create(
@@ -140,8 +165,18 @@
             index.getContent());
         navigator.render();
         Y.Assert.areEqual(
-            '<strong>6</strong> \u2192 <strong>8</strong> of 256 results',
+            '<strong>6</strong> \u2192 <strong>6</strong> of 256 results',
             index.getContent());
+    },
+    test_change_fields: function(){
+        // change_fields updates field visibility state and re-renders.
+        var navigator = this.get_render_navigator();
+        navigator.render();
+        Y.Assert.areEqual('bar', navigator.target.getContent());
+        Y.Assert.isTrue(navigator.field_visibility.show_foo);
+        navigator.change_fields({show_foo: false});
+        Y.Assert.isFalse(navigator.field_visibility.show_foo);
+        Y.Assert.areEqual('', navigator.target.getContent());
     }
 }));
 
@@ -282,7 +317,7 @@
      * get_batch_query accepts the order_by param.
      */
     test_get_batch_query_orderby: function(){
-        var navigator = new module.ListingNavigator('?param=1');
+        var navigator = new module.ListingNavigator('?param=1', {});
         var query = navigator.get_batch_query({order_by: 'importance'});
         Y.Assert.areSame('importance', query.orderby);
         Y.Assert.areSame(1, query.param);
@@ -291,7 +326,7 @@
      * get_batch_query accepts the memo param.
      */
     test_get_batch_query_memo: function(){
-        var navigator = new module.ListingNavigator('?param=foo');
+        var navigator = new module.ListingNavigator('?param=foo', {});
         var query = navigator.get_batch_query({memo: 'pi'});
         Y.Assert.areSame('pi', query.memo);
         Y.Assert.areSame('foo', query.param);
@@ -300,7 +335,7 @@
      * When memo is null, query.memo is undefined.
      */
     test_get_batch_null_memo: function(){
-        var navigator = new module.ListingNavigator('?memo=foo');
+        var navigator = new module.ListingNavigator('?memo=foo', {});
         var query = navigator.get_batch_query({memo: null});
         Y.Assert.areSame(undefined, query.memo);
     },
@@ -309,7 +344,7 @@
      */
     test_get_batch_query_forwards: function(){
         var navigator = new module.ListingNavigator(
-            '?param=pi&direction=backwards');
+            '?param=pi&direction=backwards', {});
         var query = navigator.get_batch_query({forwards: true});
         Y.Assert.areSame('pi', query.param);
         Y.Assert.areSame(undefined, query.direction);
@@ -318,7 +353,7 @@
      * If 'forwards' is false, direction is set to backwards.
      */
     test_get_batch_query_backwards: function(){
-        var navigator = new module.ListingNavigator('?param=pi');
+        var navigator = new module.ListingNavigator('?param=pi', {});
         var query = navigator.get_batch_query({forwards: false});
         Y.Assert.areSame('pi', query.param);
         Y.Assert.areSame('backwards', query.direction);
@@ -327,7 +362,7 @@
      * If start is provided, it overrides existing values.
      */
     test_get_batch_query_start: function(){
-        var navigator = new module.ListingNavigator('?start=pi');
+        var navigator = new module.ListingNavigator('?start=pi', {});
         var query = navigator.get_batch_query({});
         Y.Assert.areSame(undefined, query.start);
         query = navigator.get_batch_query({start: 1});