launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/pystache-scoping-fix into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #911840 in Launchpad itself: "Launchpad works around pystache scoping bug"
  https://bugs.launchpad.net/launchpad/+bug/911840

For more details, see:
https://code.launchpad.net/~abentley/launchpad/pystache-scoping-fix/+merge/116508

= Summary =
Fix bug #911840: Launchpad works around pystache scoping bug

== Proposed fix ==
Update to latest pystache release, 0.5.2, and remove work-arounds.

== Pre-implementation notes ==
None

== LOC Rationale ==
Removes 63 lines.

== Implementation details ==
Because of the scoping bug, field visibility control variables had to be inserted into the individual item data.  Now that scoping works correctly, the control variables are inserted as part of rendering.

== Tests ==
bin/test -t test_bugtask -t test_buglisting

== Demo and Q/A ==
Go to a bugs page.  It should look normal.  Adjust the list of fields.  It should work.  Disable javascript and reload.  It should look the same, except that the Order by: bar is missing.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/tests/test_buglisting.js
  versions.cfg
  lib/lp/bugs/javascript/buglisting.js
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/browser/bugtask.py
-- 
https://code.launchpad.net/~abentley/launchpad/pystache-scoping-fix/+merge/116508
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/pystache-scoping-fix into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-07-19 00:09:34 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-07-24 16:01:23 +0000
@@ -2378,14 +2378,13 @@
         objects = IJSONRequestCache(self.request).objects
         if IUnauthenticatedPrincipal.providedBy(self.request.principal):
             objects = obfuscate_structure(objects)
-        return pystache.render(self.mustache_template,
-                               objects['mustache_model'])
+        model = dict(objects['mustache_model'])
+        model.update(self.field_visibility)
+        return pystache.render(self.mustache_template, model)
 
     @property
     def model(self):
         items = [bugtask.model for bugtask in self.getBugListingItems()]
-        for item in items:
-            item.update(self.field_visibility)
         return {'items': items}
 
 

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-07-19 00:09:34 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-07-24 16:01:23 +0000
@@ -2059,9 +2059,7 @@
             cache = IJSONRequestCache(view.request)
             items = cache.objects['mustache_model']['items']
             self.assertEqual(1, len(items))
-            combined = dict(item.model)
-            combined.update(view.search().field_visibility)
-        self.assertEqual(combined, items[0])
+            self.assertEqual(item.model, items[0])
 
     def test_no_next_prev_for_single_batch(self):
         """The IJSONRequestCache should contain data about ajacent batches.

=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2012-07-18 15:37:42 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2012-07-24 16:01:23 +0000
@@ -139,22 +139,6 @@
                 this.get('model').get_field_visibility());
         },
 
-        /**
-         * Handle a previously-unseen batch by storing it in the cache and
-         * stripping out field_visibility values that would otherwise shadow the
-         * real values.
-         */
-        handle_new_batch: function(batch) {
-            var key, i;
-            Y.each(batch.field_visibility, function(value, key) {
-                for (i = 0; i < batch.mustache_model.items.length; i++) {
-                    delete batch.mustache_model.items[i][key];
-                }
-            });
-            return this.constructor.superclass.handle_new_batch.call(this,
-                                                                     batch);
-        },
-
         make_model: function(batch_key, cache) {
             return new module.BugListingModel({
                 batch_key: batch_key,

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js	2012-07-18 15:37:42 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js	2012-07-24 16:01:23 +0000
@@ -36,51 +36,6 @@
             });
             Y.lp.testing.assert.assert_equal_structure(
                 {foo: 'bar'}, navigator.get('search_params'));
-        },
-        test_cleans_visibility_from_current_batch: function() {
-            // When initial batch is handled, field visibility is stripped.
-            var navigator = new module.BugListingNavigator({
-                current_url: '',
-                cache: {
-                    field_visibility: {show_item: true},
-                    mustache_model: {items: [{show_item: true} ] },
-                    next: null,
-                    prev: null
-                },
-                target: this.target
-            });
-            bugtask = navigator.get_current_batch().mustache_model.items[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 model = {
-                mustache_model: {items: []},
-                memo: 1,
-                next: null,
-                prev: null,
-                field_visibility: {},
-                field_visibility_defaults: {show_item: true}
-            };
-            var navigator = new module.BugListingNavigator({
-                current_url: '',
-                cache: model,
-                template: '',
-                target: this.target
-            });
-            var batch = {
-                mustache_model: {
-                    items: [{show_item: true}]},
-                memo: 2,
-                next: null,
-                prev: null,
-                field_visibility: {show_item: true}
-            };
-            var query = navigator.get_batch_query(batch);
-            navigator.update_from_new_model(query, false, batch);
-            bugtask = navigator.get_current_batch().mustache_model.items[0];
-            Y.Assert.isFalse(bugtask.hasOwnProperty('show_item'));
         }
     }));
 
@@ -104,8 +59,14 @@
         test_from_page_with_client: function() {
             Y.one('#fixture').setContent(
                 '<div id="bugs-table-listing">' +
+<<<<<<< TREE
                     '<a class="previous" href="http://example.org/";>PreVious</a>' +
                     '<div id="client-listing"></div>' +
+=======
+                '<a class="previous" href="http://example.org/";>' +
+                'PreVious</span>' +
+                '<div id="client-listing"></div>' +
+>>>>>>> MERGE-SOURCE
                 '</div>');
             Y.Assert.areSame('http://example.org/', this.getPreviousLink());
             module.BugListingNavigator.from_page();

=== modified file 'versions.cfg'
--- versions.cfg	2012-07-13 01:55:27 +0000
+++ versions.cfg	2012-07-24 16:01:23 +0000
@@ -89,7 +89,7 @@
 pygpgme = 0.2
 pymongo = 2.1.1
 pyOpenSSL = 0.10
-pystache = 0.3.1
+pystache = 0.5.2
 python-dateutil = 1.5
 # lp:python-memcached
 # r57 (includes a fix for bug 974632)