PLIP 234 framework review #1 by andi (witsch), 2009-01-26 ========================================================= review steps ------------ the bundle was reviewed on OSX 10.5.6 doing the following: * bundle checkout, buildout etc * visual review of code changes in `Plone`__, `plone.app.layout`__ and `plone.app.portlets`__ * test runs for `Products.CMFPlone`, `plone.app.layout` and `plone.app.portlets` * manual function tests to verify things work as intended .. __: http://dev.plone.org/plone/log/Plone/branches/calvinhp-inavigationroot-fixes .. __: http://dev.plone.org/plone/log/plone.app.layout/branches/calvinhp-inavigationroot-fixes .. __: http://dev.plone.org/plone/log/plone.app.portlets/branches/calvinhp-inavigationroot-fixes notes and observations ---------------------- * clicking the user name leads to the user's dashboard as expected. however, the link seems to be wrong as it takes me to the site-root, i.e. `http://.../plone/dashboard` instead of `.../plone/foo/dashboard` (with `foo` being the new navigation root). the latter link does work (the zcml for the dashboard view was adjusted after all), so i suppose only the action needs tweaking. * the same applies to the "site setup" link, but this might be intentional. * otherwise the manually set up navigation root seems to be reflected everywhere, i.e. things work as advertised. * code-wise the view variable `navigation_url` set up in the viewlets base class (in `p.a.layout/viewlets/common.py`) should perhaps be renamed to `navigation_root_url` for consistency. it is set to the value of `navigation_root_url()` from the "portal state" after all. * some of the values set up in the `__init__` method of some portlet's renderers are only ever used in one place ("news" and "recent", for example). in these cases i think a better pattern would be to actually determine the values were they are needed, especially when the method is memoized anyway. the adapter lookup itself (for `plone_portal_state`) could still be cached, but separating initialization and (single) usage of values doesn't make sense with regard to readability and maintenance. * almost none of the changes are actually tested. of course, some already existing tests will cover the functionality, but my guess would be that those only cover the case where the site root is also the navigation root. imho, the PLIP needs to see make sure things work in both cases, though, i.e. also for when the navigation root had been set to something else. * adding to that, 6 of the tests for `Products.CMFPlone` yield errors with the current version of the bundle. this doesn't happen with the three original branches under `src/` checked out at the latest merge point, i.e. r24100. * also a few notes for the reviewers would have been helpful. for example how to actually turn a folder into a navigation root. google spat out https://weblion.psu.edu/trac/weblion/wiki/MakeAsubfolderAnavigationRoot pretty quickly, of course, but imho helpful notes should be part of the PLIP submission nevertheless. conclusion ---------- while it's hard to tell if the changes to consider `INavigationRoot` throughout all of plone are complete, the PLIP implementation is certainly a huge step in the right direction, and once it has been accepted and merged, more occurrences can be regarded as simple bug fixes anyway. however, at the current state, that is with test failures and a general lack of tests for all the changes made, i cannot recommend the PLIP for merging into Plone 3.3, so i'm -1 for now. i do hope that calvin with address these shortcoming during the revision period, though. PLIP 234 framework review #2 by Tom Lazar (tomster), 2009-01-30 ============================================================== I have repeated the test runs and manual test steps that andi detailed above and came to the identical findings (not surprising, really, as there haven't been any code changes since and Andi and I run the same platform...) I have also reviewed the code changes for `Plone`__, `plone.app.layout`__ and `plone.app.portlets`__ and found nothing amiss (except the `__init__` issue for portlets that andi mentions above). .. __: http://dev.plone.org/plone/changeset?new=Plone%2Fbranches%2Fcalvinhp-inavigationroot-fixes%4024100&old=Plone%2Fbranches%2F3.2%4024100 .. __: http://dev.plone.org/plone/changeset?new=plone.app.layout%2Fbranches%2Fcalvinhp-inavigationroot-fixes%4024100&old=plone.app.layout%2Fbranches%2F1.1%4024100 .. __: http://dev.plone.org/plone/changeset?new=plone.app.portlets%2Fbranches%2Fcalvinhp-inavigationroot-fixes%4024100&old=plone.app.portlets%2Ftrunk%4024100 I confirm resp. agree with andi's notes and observations, as well as with his conclusion, particularly with the assessment that all of this is still fixable during the second revision period. Second Review (2009-02-12) ========================== I have reviewed the changes in the documentation and in the three affected packages since december and have conducted another local TTW test. The test showed that the application of INavigationRoot worked as advertised. While I personally still feel that the switch of the root in the navigation tree once you reach a new root is kind of 'creepy' I realize that a) this is intended and b) that in actual deployment the sub-roots are usually served under different domains from the front end webserver, so the effect won't be observable to users. While I agree (with andi), that in a perfect world, this PLIP would come with more tests, I also agree (with calvin, optilude etc.) that this PLIP is by nature more a bug fix than a new feature. I don't think we can reasonably expect that all fixes to Plone must come with 100% test coverage, or else they will not be accepted. Seeing that most changes are actually in template markup and not so much in code and given the test of the base portlet and that no existing tests break I vote +1 on this. Can I guarantee that it absolutely won't break anything? No. Am I convinced that Plone is better with this PLIP than without it? Yes :-)