[Plone-cvs] r19142 - review/plip218-addable-portlets-restrictions-and-changes

[Andreas Z. <svn...@pl...>]

Author: witsch
Date: Thu Jan 31 16:35:22 2008
New Revision: 19142

Modified:
   review/plip218-addable-portlets-restrictions-and-changes/REVIEW-NOTES.txt
Log:
added notes from first bundle review for PLIP #218 (see #7752)

Modified: review/plip218-addable-portlets-restrictions-and-changes/REVIEW-NOTES.txt
==============================================================================
--- review/plip218-addable-portlets-restrictions-and-changes/REVIEW-NOTES.txt	(original)
+++ review/plip218-addable-portlets-restrictions-and-changes/REVIEW-NOTES.txt	Thu Jan 31 16:35:22 2008
@@ -137,3 +137,86 @@
 
   <portlet addview="portlets.PurgeMe" title="Foo" description="Foo"/>
   <portlet addview="portlets.PurgeMe" remove="" />
+
+
+-----
+
+
+first framework review by andi (witsch), 2008-01-31
+---------------------------------------------------
+
+i've reviewed the bundle on OSX 10.5.1 doing the following:
+
+  * bundle checkout, buildout etc
+
+  * visual review of code changes in `CMFPlone`__, `plone.portlets`__ and
+    `plone.app.portlets`__ (as diffs relative to the branches for PLIP 205)
+
+  * test runs for Products.CMFPlone, plone.portlets, plone.app.portlets
+
+  * test coverage checks for plone.portlets, plone.app.portlets
+
+  * some manual function tests including migration from a 3.0 site, gs export
+    of the portlet assignments, imports with erroneous xml, checks for the
+    various warnings etc
+
+.. __: http://dev.plone.org/plone/log/CMFPlone/branches/plip218-addable-portlets-restrictions-and-changes
+.. __: http://dev.plone.org/plone/log/plone.portlets/branches/plip218-addable-portlets-restrictions-and-changes
+.. __: http://dev.plone.org/plone/log/plone.app.portlets/branches/plip218-addable-portlets-restrictions-and-changes
+
+
+some notes and observations:
+
+  * wouldn't it make more sense to register with `IPortletManager` instead
+    of `zope.interface.Interface` in case no interfaces are given explicitly?
+    or are there cases where it's desirable for a portlet manager to not
+    implement `IPortletManager`?
+
+  * as with PLIP 205 the migration code is slightly outdated.
+    however, setting up the migration infrastructure for 3.1 is not really
+    within the scope of this PLIP and needs to be handled during merging
+    anyway
+
+  * there are only very minor differences between the respective branches of
+    plone.portlets for PLIPs 205 and 218, so i won't repeat any of the issues
+    reported for that package here.  please refer to the `REVIEW-NOTES.txt`
+    of PLIP 205
+
+  * like with PLIP 205 running the tests for the plone.* packages works, but::
+
+      bin/instance test -s Products.CMFPlone 
+
+    produces a failure in `testLocalizedCalendarWithEvents`, which should be 
+    fixed before the changes can be merged
+
+  * duplicate definitions of the same interface _do_ get checked now (as
+    opposed to PLIP 205), so i suppose this issue can be ignored, provided
+    both PLIPs are accepted for merging
+
+  * yet another edge-case:  is there anything we could do to help the user in
+    case of "conflicting" profiles?  say, products A and B both register
+    portlets to be addable to the dashboard, but product B also purges the
+    previous registrations doing so.  in that situation it's quite likely
+    users will ask the developer of product A why their portlets don't appear,
+    probably causing them quite some difficulties to figure this out (without
+    knowing about product B initially, for example).  is there any way we can
+    make this potentially less confusing to the user, perhaps by issuing a
+    warning when non-standard portlets are purged?
+
+  * otherwise the test coverage is quite good, and like with PLIP 205 the
+    newly introduced tests (described above) make a reasonable supplement to
+    the existing test base.  only minor things like removing a portlet via xml
+    or changing the title/description while extending a registration are not
+    tested (the `_removePortlet` method is, though), but all other potential
+    problems regarding errors in the xml i could think of were already nicely
+    covered by the new tests... :)
+
+  * also like with PLIP 205 there's not a lot of new code (which is good :)),
+    and what's there looks clean and readable.  addtional comments in the
+    right places help to understand the code quickly...
+
+
+conclusion:
+
+  overall, i'm +1 on merging this provided the newly introduced test failure
+  in CMFPlone will be fixed.