launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05656
[Merge] lp:~deryck/launchpad/field-visibility-persistence-891780 into lp:launchpad
Deryck Hodge has proposed merging lp:~deryck/launchpad/field-visibility-persistence-891780 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #891780 in Launchpad itself: "Need persistence for bug listing display widget"
https://bugs.launchpad.net/launchpad/+bug/891780
For more details, see:
https://code.launchpad.net/~deryck/launchpad/field-visibility-persistence-891780/+merge/83037
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.
--
https://code.launchpad.net/~deryck/launchpad/field-visibility-persistence-891780/+merge/83037
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/field-visibility-persistence-891780 into lp:launchpad.
=== 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 15:37:28 +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 = {
+ 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(request, user)
TableBatchNavigator.__init__(
self, tasks, request, columns_to_show=columns_to_show, size=size)
@@ -2290,6 +2292,37 @@
return getUtility(IBugTaskSet).getBugTaskBadgeProperties(
self.currentBatch())
+ def _setFieldVisibility(self, request, user):
+ """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_template = '%s-buglist-fields'
+ cookie_name = ''
+ if user is not None:
+ cookie_name = cookie_name_template % user.name
+ else:
+ cookie_name = cookie_name_template % 'anon'
+ cookie = 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 item in cookie.split('&'):
+ field, value = item.split('=')
+ if value == 'true':
+ value = True
+ else:
+ value = False
+ fields_from_cookie[field] = value
+ 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]
=== 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 15:37:28 +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,22 @@
field_visibility = cache.objects['field_visibility']
self.assertTrue(field_visibility['show_title'])
+ 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 15:37:28 +0000
@@ -183,6 +183,7 @@
link.on('click', function(e) {
var defaults = this.get('field_visibility_defaults');
this.updateFieldVisibilty(defaults, true);
+ this.setCookie();
}, this);
return link;
},
@@ -226,7 +227,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 +265,66 @@
}
}
this.updateFieldVisibilty(fields);
+ this.setCookie(fields);
+ },
+
+ /**
+ * Helper method to always get the same cookie name, based
+ * on user name.
+ *
+ * @method getCookieName
+ */
+ getCookieName: function() {
+ var user;
+ if (Y.Lang.isValue(window.LP) && Y.Lang.isValue(LP.links)) {
+ var me = LP.links.me;
+ user = me.replace('/~', '');
+ } else {
+ user = 'anon';
+ }
+ return user + '-buglist-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.getCookieName();
+ 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.getCookieName();
+ 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 +334,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 +376,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 15:37:28 +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,
@@ -60,12 +66,16 @@
}
}
};
+ this.cookie_name = 'foobar-buglist-fields';
},
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 +122,13 @@
this.list_util.render();
},
+ test_cookie_name_drops_leading_slash: function() {
+ // The cookie name should exclude the "/~" that we
+ // normally set in LP.links.me.
+ this.list_util = new Y.lp.buglisting_utils.BugListingConfigUtil();
+ Assert.areEqual(this.cookie_name, this.list_util.getCookieName());
+ },
+
test_field_visibility_defaults_readonly: function() {
// field_visibility_defaults is a readOnly attribute,
// so field_visibility_defaults will always return the same
@@ -156,6 +173,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 +363,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 +485,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 +511,4 @@
buglisting_utils.suite = suite;
}, '0.1', {'requires': [
- 'test', 'node-event-simulate', 'lp.buglisting_utils']});
+ 'test', 'node-event-simulate', 'cookie', 'lp.buglisting_utils']});
Follow ups
