Merge lp:~deryck/launchpad/field-visibility-persistence-891780 into lp:launchpad

[Deryck Hodge]

[r=gmb][bug=891780] Set a cookie that CustomBugListings can use to update field_visibility of a bug list.

Our new feature CustomBugListings needs to honor changes to field visibility in the listings across page loads. A simple way to achieve this is to set a cookie that stores those prefs. This branch makes a few changes to make that happen:

 * js code is updated to set/delete cookies
   based on submitting a form overlay
 * new tests are written for js to confirm this
 * view is updated to check for existence of cookie
   and update field_visibility accordingly
 * view also has a test added

Robert had some concerns in the bug about using cookies, which we discussed more offline. His concerns were that:

 * another cookie might cause request size to be too large
 * session cookie might be better than adding a new cookie

I've checked on request size and we're way safe. Nothing to worry about there.

I rejected the session cookie idea initially because I was only reading and writing from js, and js code shouldn't be able to read the session cookie as plain text. Or be able to reverse the hash. I didn't think we needed to understand the cookie on the server side because I thought we hid the first batch we render behind a noscript tag and rendered everything else client side. We do indeed use the first batch rendered on page load, so now the view is involved. I'm happy to revisit this before we're done-done and work out an API that the client can use to submit stuff to the session cookie if this is really required.

I'm still not sure I like this idea, though. Paranoia makes me fear anything other than the server adding something to the session cookie, and I'm also not sure what we gain for that beyond consolidating on a single cookie. There is clearly value in keeping cookie count low, so I can be persuaded, but generally, I think what I have here works fine. I recognize I could be tending toward stubborn old-man refusal here, so I welcome correction if so.

If people feel strongly about going through our session cookie, I'd prefer to land this as it is, assuming the code is okay, and file an XXX to fix this to use our session cookie before we're off the feature.

[Graham Binns (gmb)]

Hi Deryck,

Nice branch; good work! I've got a few comments but nothing earth
shattering.

[1]

9 + user = getUtility(ILaunchBag).user

Without looking at the context in more detail I can't be certain, but
typically we should be preferring self.request.user over using the
LaunchBag.

[2]

29 + """Set field_visibility for the page load.
30 +
31 +
32 + If a cookie of the form $USER-buglist-fields is found,

Looks like you have an extra line of blankness here.

[3]

47 + if cookie is not None:
48 + for item in cookie.split('&'):
49 + field, value = item.split('=')
50 + if value == 'true':
51 + value = True
52 + else:
53 + value = False
54 + fields_from_cookie[field] = value
55 + self.field_visibility = fields_from_cookie

You could save yourself some lines here by using urlparse.parse_qsl,
which returns (key, value) tuples, thus:

    for field, value in urlparse.parse_qsl(cookie):
        # We only record True or False for field values, so we only care
        # about whether the value from the cookie is 'true'.
        fields_from_cookie[field] = (value == 'true')
    self.field_visibility = fields_from_cookie

[4]

138 + /**
139 + * Helper method to always get the same cookie name, based
140 + * on user name.
141 + *
142 + * @method getCookieName
143 + */
144 + getCookieName: function() {
145 + var user;
146 + if (Y.Lang.isValue(window.LP) && Y.Lang.isValue(LP.links)) {
147 + var me = LP.links.me;
148 + user = me.replace('/~', '');
149 + } else {
150 + user = 'anon';
151 + }
152 + return user + '-buglist-fields';
153 + },

Rather than doing this twice - once in the View and once in the JS - it
would make more sense to do it in the view and then bung it in the page
JS cache so that the JS can just use the value that's already been
calculated.

review: Approve (code)

[Robert Collins (lifeless)]

@Deryck - using the session cookie to control this, the *server* would
still be setting it, always. So I'm not sure what you are worried
about.

[Deryck Hodge (deryck)]

The server would set whatever the js code passed in, though. And yeah, maybe it's not worth worrying about. It just feels wrong to me.

I'm not opposed to having a single cookie that client code can write to via an API; just not sure if it should be the same one as the session cookie. And if it's not the session one, then it feels the long way around to have an API to set a cookie. :)

Again, I'll concede I may be overly paranoid here.

[Deryck Hodge (deryck)]

Also, thanks for the review, gmb! I'll make the changes you suggested. All good ones there.

[Robert Collins (lifeless)]

On Wed, Nov 23, 2011 at 8:09 AM, Deryck Hodge
<email address hidden> wrote:
> The server would set whatever the js code passed in, though.  And yeah, maybe it's not worth worrying about.  It just feels wrong to me.

It doesn't need to do that - you can validate the information really
easily. Or you can say that things like 'client.*' are opaque to the
server (but this defeats the whole reason here, so you will want it
validated).

[Deryck Hodge (deryck)]

Sure, we can validate. I didn't mean to suggest we couldn't. I still feel hesitation about sticking some-what arbitrary data into the same object we use for authentication, but I won't be so stubborn to not do this if everyone really wants it.

And really, I'm warming to the idea the more I think about it. I might even come all the way around in another day or two, after I get all this work pending from me landed.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-11-22 11:26:38 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-11-22 20:20:30 +0000
@@ -2267,7 +2267,8 @@
         # rules to a mixin so that MilestoneView and others can use it.
         self.request = request
         self.target_context = target_context
-        self.field_visibility = {
+        self.user = getUtility(ILaunchBag).user
+        self.field_visibility_defaults = {
             'show_age': False,
             'show_assignee': False,
             'show_bugtarget': True,
@@ -2281,7 +2282,8 @@
             'show_tags': False,
             'show_title': True,
         }
-        self.field_visibility_defaults = self.field_visibility
+        self.field_visibility = None
+        self._setFieldVisibility()
         TableBatchNavigator.__init__(
             self, tasks, request, columns_to_show=columns_to_show, size=size)
 
@@ -2290,6 +2292,37 @@
         return getUtility(IBugTaskSet).getBugTaskBadgeProperties(
             self.currentBatch())
 
+    def getCookieName(self):
+        """Return the cookie name used in bug listings js code."""
+        cookie_name_template = '%s-buglist-fields'
+        cookie_name = ''
+        if self.user is not None:
+            cookie_name = cookie_name_template % self.user.name
+        else:
+            cookie_name = cookie_name_template % 'anon'
+        return cookie_name
+
+    def _setFieldVisibility(self):
+        """Set field_visibility for the page load.
+
+        If a cookie of the form $USER-buglist-fields is found,
+        we set field_visibility from this cookie; otherwise,
+        field_visibility will match the defaults.
+        """
+        cookie_name = self.getCookieName()
+        cookie = self.request.cookies.get(cookie_name)
+        fields_from_cookie = {}
+        # "cookie" looks like a URL query string, so we split
+        # on '&' to get items, and then split on '=' to get
+        # field/value pairs.
+        if cookie is not None:
+            for field, value in urlparse.parse_qsl(cookie):
+                # We only record True or False for field values.
+                fields_from_cookie[field] = (value == 'true')
+            self.field_visibility = fields_from_cookie
+        else:
+            self.field_visibility = self.field_visibility_defaults
+
     def _getListingItem(self, bugtask):
         """Return a decorated bugtask for the bug listing."""
         badge_property = self.bug_badge_properties[bugtask]
@@ -2612,6 +2645,7 @@
                 batch_navigator.field_visibility)
             cache.objects['field_visibility_defaults'] = (
                 batch_navigator.field_visibility_defaults)
+            cache.objects['cbl_cookie_name'] = batch_navigator.getCookieName()
 
             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-21 00:31:41 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-11-22 20:20:30 +0000
@@ -1710,7 +1710,7 @@
         'client-listing', True, attrs={'id': 'client-listing'})
 
     def makeView(self, bugtask=None, size=None, memo=None, orderby=None,
-                 forwards=True):
+                 forwards=True, cookie=None):
         """Make a BugTaskSearchListingView.
 
         :param bugtask: The task to use for searching.
@@ -1734,7 +1734,7 @@
             query_vars['direction'] = 'backwards'
         query_string = urllib.urlencode(query_vars)
         request = LaunchpadTestRequest(
-            QUERY_STRING=query_string, orderby=orderby)
+            QUERY_STRING=query_string, orderby=orderby, HTTP_COOKIE=cookie)
         if bugtask is None:
             bugtask = self.factory.makeBugTask()
         view = BugTaskSearchListingView(bugtask.target, request)
@@ -1865,6 +1865,31 @@
         field_visibility = cache.objects['field_visibility']
         self.assertTrue(field_visibility['show_title'])
 
+    def test_cache_cookie_name(self):
+        """The cookie name should be in cache for js code access."""
+        task = self.factory.makeBugTask()
+        with self.dynamic_listings():
+            view = self.makeView(task, memo=1, forwards=False, size=1)
+        cache = IJSONRequestCache(view.request)
+        cookie_name = cache.objects['cbl_cookie_name']
+        self.assertEqual('anon-buglist-fields', cookie_name)
+
+    def test_cache_field_visibility_matches_cookie(self):
+        """Cache contains cookie-matching values for field_visibiliy."""
+        task = self.factory.makeBugTask()
+        cookie = (
+            'anon-buglist-fields=show_age=true&show_reporter=true'
+            '&show_id=true&show_bugtarget=true&show_title=true'
+            '&show_milestone_name=true&show_last_updated=true'
+            '&show_assignee=true&show_bug_heat=true&show_tags=true'
+            '&show_importance=true&show_status=true')
+        with self.dynamic_listings():
+            view = self.makeView(
+                task, memo=1, forwards=False, size=1, cookie=cookie)
+        cache = IJSONRequestCache(view.request)
+        field_visibility = cache.objects['field_visibility']
+        self.assertTrue(field_visibility['show_tags'])
+
     def test_cache_field_visibility_defaults(self):
         """Cache contains sane-looking field_visibility_defaults values."""
         task = self.factory.makeBugTask()

=== modified file 'lib/lp/bugs/javascript/buglisting_utils.js'
--- lib/lp/bugs/javascript/buglisting_utils.js	2011-11-16 17:10:36 +0000
+++ lib/lp/bugs/javascript/buglisting_utils.js	2011-11-22 20:20:30 +0000
@@ -102,6 +102,26 @@
         },
 
         /**
+         * The cookie name as set by the view.
+         *
+         * We get this value from the LP cache.
+         *
+         * @attribute cookie_name
+         * @type String
+         */
+        cookie_name: {
+            valueFn: function() {
+                if (
+                    Y.Lang.isValue(window.LP) &&
+                    Y.Lang.isValue(LP.cache.cbl_cookie_name)) {
+                    return LP.cache.cbl_cookie_name;
+                } else {
+                    return '';
+                }
+            }
+        },
+
+        /**
          * A reference to the form overlay used in the overlay.
          *
          * @attribute form
@@ -183,6 +203,7 @@
             link.on('click', function(e) {
                 var defaults = this.get('field_visibility_defaults');
                 this.updateFieldVisibilty(defaults, true);
+                this.setCookie();
             }, this);
             return link;
         },
@@ -226,7 +247,10 @@
          */
         updateFieldVisibilty: function(fields, destroy_form) {
             this.set(FIELD_VISIBILITY, fields);
-            this.get(FORM).hide();
+            var form = this.get(FORM);
+            if (Y.Lang.isValue(form)) {
+                form.hide();
+            }
             // Destroy the form and rebuild it.
             if (destroy_form === true) {
                 this.get(FORM).hide();
@@ -261,6 +285,49 @@
                 }
             }
             this.updateFieldVisibilty(fields);
+            this.setCookie(fields);
+        },
+
+        /**
+         * Update field_visibility based on fields stored
+         * in cookies.  This is used as a light-weight
+         * page to page persistence mechanism.
+         *
+         * @method updateFromCookie
+         */
+        updateFromCookie: function() {
+            var cookie_name = this.get('cookie_name');
+            var cookie_fields = Y.Cookie.getSubs(cookie_name);
+            if (Y.Lang.isValue(cookie_fields)) {
+                // We get true/false back as strings from Y.Cookie,
+                // so we have to convert them to booleans.
+                Y.each(cookie_fields, function(val, key, obj) {
+                    if (val === 'true') {
+                        val = true;
+                    } else {
+                        val = false;
+                    }
+                    obj[key] = val;
+                });
+                this.updateFieldVisibilty(cookie_fields);
+            }
+        },
+
+        /**
+         * Set the given value for the buglisting config cookie.
+         * If config is not specified, the cookie will be cleared.
+         *
+         * @method setCookie
+         */
+        setCookie: function(config) {
+            var cookie_name = this.get('cookie_name');
+            if (Y.Lang.isValue(config)) {
+                Y.Cookie.setSubs(cookie_name, config, {
+                    path: '/',
+                    expires: new Date('January 19, 2038')});
+            } else {
+                Y.Cookie.remove(cookie_name);
+            }
         },
 
         /**
@@ -270,6 +337,7 @@
          * @method _extraRenderUI
          */
         _extraRenderUI: function() {
+            this.updateFromCookie();
             var form_content = this.buildFormContent();
             var on_submit_callback = Y.bind(this.handleOverlaySubmit, this);
             util_overlay = new Y.lazr.FormOverlay({
@@ -311,4 +379,4 @@
     var buglisting_utils = Y.namespace('lp.buglisting_utils');
     buglisting_utils.BugListingConfigUtil = BugListingConfigUtil;
 
-}, '0.1', {'requires': ['lp.configutils', 'lazr.formoverlay']});
+}, '0.1', {'requires': ['cookie', 'lp.configutils', 'lazr.formoverlay']});

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting_utils.js'
--- lib/lp/bugs/javascript/tests/test_buglisting_utils.js	2011-11-16 16:59:30 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting_utils.js	2011-11-22 20:20:30 +0000
@@ -25,9 +25,15 @@
     },
 
     setUp: function() {
+        // _setDoc is required for tests using cookies to pass.
+        Y.Cookie._setDoc({cookie: ""});
         // Simulate LP.cache.field_visibility which will be
         // present in the actual page.
         window.LP = {
+            links: {
+                me: '/~foobar'
+            },
+
             cache: {
                 field_visibility: {
                     show_title: true,
@@ -57,15 +63,21 @@
                     show_reporter: false,
                     show_milestone_name: false,
                     show_tags: false
-                }
+                },
+
+                cbl_cookie_name: 'foobar-buglist-fields'
             }
         };
+        this.cookie_name = LP.cache.cbl_cookie_name;
     },
 
     tearDown: function() {
         if (Y.Lang.isValue(this.list_util)) {
             this.list_util.destroy();
         }
+        // Cleanup cookies.
+        Y.Cookie.remove(this.cookie_name);
+        Y.Cookie._setDoc(Y.config.doc);
     },
 
     /**
@@ -112,6 +124,12 @@
         this.list_util.render();
     },
 
+    test_cookie_name_attribute: function() {
+        // cookie_name is taken from the cache.
+        this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+        Assert.areEqual(this.cookie_name, this.list_util.get('cookie_name'));
+    },
+
     test_field_visibility_defaults_readonly: function() {
         // field_visibility_defaults is a readOnly attribute,
         // so field_visibility_defaults will always return the same
@@ -156,6 +174,30 @@
             expected_config, this.list_util.get('field_visibility'));
     },
 
+    test_cookie_updates_field_visibility_config: function() {
+        // If the $USER-buglist-fields cookie is present,
+        // the widget will update field_visibility to these values.
+        var expected_config = {
+            show_title: true,
+            show_id: true,
+            show_importance: true,
+            show_status: true,
+            show_bug_heat: false,
+            show_bugtarget: false,
+            show_age: true,
+            show_last_updated: true,
+            show_assignee: false,
+            show_reporter: false,
+            show_milestone_name: false,
+            show_tags: false
+        };
+        Y.Cookie.setSubs(this.cookie_name, expected_config);
+        this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+        this.list_util.render();
+        var actual_config = this.list_util.get('field_visibility');
+        ObjectAssert.areEqual(expected_config, actual_config);
+    },
+
     test_field_visibility_form_reference: function() {
         // The form created from field_visibility defaults is referenced
         // via BugListingConfigUtil.get('form')
@@ -322,6 +364,38 @@
         Assert.isTrue(event_fired);
     },
 
+    test_update_from_form_updates_cookie: function() {
+        // When the form is submitted, a cookie is set to match
+        // your preferred field_visibility.
+        this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+        this.list_util.render();
+        // Now poke at the page to set the cookie.
+        var config = Y.one('.config');
+        config.simulate('click');
+        var show_bugtarget = Y.one('.show_bugtarget');
+        show_bugtarget.simulate('click');
+        var update = Y.one('.update-buglisting');
+        update.simulate('click');
+        var expected_config = {
+            show_title: true,
+            show_id: true,
+            show_importance: true,
+            show_status: true,
+            show_bug_heat: true,
+            show_bugtarget: false,
+            show_age: false,
+            show_last_updated: false,
+            show_assignee: false,
+            show_reporter: false,
+            show_milestone_name: false,
+            show_tags: false
+        };
+        var expected_cookie = Y.Cookie._createCookieHashString(
+            expected_config);
+        var actual_cookie = Y.Cookie.get(this.cookie_name);
+        Assert.areEqual(expected_cookie, actual_cookie);
+    },
+
     test_fields_visibility_form_reset: function() {
         // Clicking "reset to defaults" on the form returns
         // field_visibility to its default values.
@@ -412,6 +486,25 @@
         var actual_inputs = this.getActualInputData();
         ArrayAssert.itemsAreSame(expected_names, actual_inputs[0]);
         ArrayAssert.itemsAreSame(expected_checked, actual_inputs[1]);
+    },
+
+    test_form_reset_removes_cookie: function() {
+        // Clicking "reset to defaults" on the overlay will
+        // remove any cookie added.
+        this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+        this.list_util.render();
+        // Now poke at the page to set the cookie.
+        var config = Y.one('.config');
+        config.simulate('click');
+        var show_bugtarget = Y.one('.show_bugtarget');
+        show_bugtarget.simulate('click');
+        var update = Y.one('.update-buglisting');
+        update.simulate('click');
+        // Now reset from the form.
+        config.simulate('click');
+        Y.one('.reset-buglisting').simulate('click');
+        var cookie = Y.Cookie.get(this.cookie_name);
+        Assert.areSame('', cookie);
     }
 
 }));
@@ -419,4 +512,4 @@
 buglisting_utils.suite = suite;
 
 }, '0.1', {'requires': [
-    'test', 'node-event-simulate', 'lp.buglisting_utils']});
+    'test', 'node-event-simulate', 'cookie', 'lp.buglisting_utils']});