launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

The proposal to merge lp:~deryck/launchpad/field-visibility-persistence-891780 into lp:launchpad has been updated.

Description changed to:

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.

For more details, see:
https://code.launchpad.net/~deryck/launchpad/field-visibility-persistence-891780/+merge/83037
-- 
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.