launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Review: Approve

Looks great, thanks! A few minor points:

1. test_view_date_model_adds_specification

Please add a check to the test above to ensure cache does not contain specification_sharing_policies when ff is not used.

2. Very minor nitpick

Please change "the new spec. sharing policy" to "the new specification sharing policy" as it looks funny with the "."

3. Do we need the garbo job?

A check on staging may be in order to measure timing, but I would have thought this could have been done with a quick bit of manual sql. I guess the code is done now so it's moot, but it will save a followup landing to remove the job. Your call.

A quick comment:

The sharing page started out doing just an XHR call and no reload when the sharing policies were changed. However, the underlying model changes turn out to be complex and the difficulty of retrieving the required data to re-sync the view is exacerbated by the result set batching. And the DOM changes are numerous and tricky. So it's way easier just to issue a reload, given the all XHR code was already written before the need for subsequent view model updates became apparent. It could be done using a form post but a new view class (lp form) would need to be written to handle the post. Doing an client reload actually works nicer anyway since there's no browser 'click' (for want of a better term) and associated page flicker and the page refresh does appear to be seamless to the user since it avoids a whole layer of browser mechanations.

-- 
https://code.launchpad.net/~deryck/launchpad/sharing-page-for-specifications/+merge/124314
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.