Code review comment for lp:~deryck/launchpad/field-visibility-persistence-891780

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.