diff --git a/plips/implementation_notes/.keep b/plips/implementation_notes/.keep new file mode 100644 index 0000000000..e69de29bb2 diff --git a/plips/implementation_notes/plip10804-workflowmanager.txt b/plips/implementation_notes/plip10804-workflowmanager.txt deleted file mode 100644 index 7a1b1f4a2a..0000000000 --- a/plips/implementation_notes/plip10804-workflowmanager.txt +++ /dev/null @@ -1,10 +0,0 @@ -================ -PLIP 10804 Notes -================ - -* it's ready for review; however, I see the possibility for of changes on the details -of the implementation which is fine. - -* Right now it's implemented as an optional product install. This wasn't discussed in -the plip; however, I think it might be the better way to go. - diff --git a/plips/implementation_notes/plip10809-improve-tablesorter.txt b/plips/implementation_notes/plip10809-improve-tablesorter.txt deleted file mode 100644 index 728a36d0dc..0000000000 --- a/plips/implementation_notes/plip10809-improve-tablesorter.txt +++ /dev/null @@ -1,4 +0,0 @@ -PLIP 10809: Make any kind of data sortable through table_sorter.js -================================================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10809 \ No newline at end of file diff --git a/plips/implementation_notes/plip10878-siteadmin-role.txt b/plips/implementation_notes/plip10878-siteadmin-role.txt deleted file mode 100644 index 9f62b0cee2..0000000000 --- a/plips/implementation_notes/plip10878-siteadmin-role.txt +++ /dev/null @@ -1,94 +0,0 @@ -PLIP #10878 implementation notes (Add SiteAdmin role) -===================================================== - -Implementer: David Glick -PLIP text: https://dev.plone.org/plone/ticket/10878 - -Reference buildout is at -https://svn.plone.org/svn/plone/buildouts/plone-coredev/branches/4.1; run with --c plips/plip10878-siteadmin-role.cfg. This buildout is based on the PLIP 10776 -buildout because the implementation depends on the role ZCML subdirective -introduced in Zope 2.13. The failing tests when running bin/alltests are known -issues documented in Hanno's notes. - -First I looked through the list of permissions that are included with Plone out -of the box, and determined which should be assigned to the SiteAdmin role in a -fresh Plone site. The list I determined is documented (and tested) in -CMFPlone/tests/testSiteAdminRole.py. My overall aim was to include permissions -related to managing content and exclude permissions related to management via -the ZMI. This involved some judgment calls, so I would anticipate a need for -slight adjustments to the set of SiteAdmin permissions. - -I used two approaches to make this set of permissions a reality. For permissions -defined in "upstream" packages in the Zope repository, I updated the Plone -rolemap.xml to add the SiteAdmin role for these permissions when the Plone -profile is imported. (For existing sites, an upgrade step adds the SiteAdmin -role to all rolemap.xml permissions that have the Manager role, minus Manage -portal, plus some additional permissions that get the Manager role as a -Zope-wide default role.) For permissions defined in packages in the Plone, -archetypes, and collective repositories, I updated their Zope-wide default roles -to add SiteAdmin where appropriate (either using -Products.CMFCore.permissions.setDefaultRoles, or the new-in-Zope-2.13 role -subdirective of the permission ZCML directive). - -I updated Plone's built-in workflows to assign the SiteAdmin role wherever the -Manager role is assigned. An upgrade step for existing sites updates ALL -workflows, doing the same. It also calls the workflow tool's recursive method to -update role mappings and reindex the allowedRolesAndUsers index. - -I grepped through the codebase to find things protected by the 'Manage portal' -permission (which SiteAdmins cannot have) that should be accessible to -SiteAdmins. I then updated the following to use more specific permissions: - - - most Plone control panels (a unique permission for each one, so that the - access given to SiteAdmins can easily be focused) - -The following are still only accessible with the 'Manage portal' permission. In -some cases this is because they are still skin layer items (thus harder to -secure all at once like browser views) and not worth rewriting into browser -views: - - - the error log control panel - - the kupu control panel (not worth updating; kupu is going away) - - the collections control panel (not worth rewriting if we're switching to - plone.app.collection) - - removal of CMFDefault discussion items (not worth changing if we're updating - to use plone.app.discussion) - - setting local workflow policy - - lots of ZMI stuff - -In order to protect the control panel overview page, I converted it into a -browser view (well, I used the existing one that Hanno had started). -plone_control_panel is kept as a skin layer template that pulls in the browser -view, for backwards-compatibility. The group details form got a similar -treatment. - -One particular concern was to make sure that SiteAdmins can have access to mange -users and groups without risk of them escalating themselves to have Manager -privileges. To achieve this, I made sure that SiteAdmins do not get the general -"Manage users" permission which protects a number of methods of -user-management-related tools. SiteAdmins *do* get access to the Plone users and -groups control panel and related forms, which I updated and tested to make sure -that: - - - non-Managers cannot add or remove the Manager role to users or groups - - non-Managers cannot add or remove users or groups with the Manager role (or - trigger a password reset for them) - - non-Managers cannot add users to or remove them from groups with the Manager - role - -I did not tackle the problem of how to provide for role delegation in a more -general manner within the Users & Groups control panels. This is because I could -think of several ways to approach the problem and was unclear how to proceed, -and also because it would have distracted from the main goal of adding the -SiteAdmin role. Someone should submit a PLIP for this in the future, but for now -I decided it would be most expedient to focus on preventing escalation to -Manager. - -I added a SiteAdmins group that is created OOTB (and an upgrade step to add it -for existing sites). - -I plan to add developer documentation of how to register permissions with -SiteAdmin as a default role, at both -http://plone.org/documentation/manual/plone-community-developer-documentation -and http://plone.org/documentation/manual/upgrade-guide diff --git a/plips/implementation_notes/plip10886-event-improvements.rst b/plips/implementation_notes/plip10886-event-improvements.rst deleted file mode 100644 index ecb769be73..0000000000 --- a/plips/implementation_notes/plip10886-event-improvements.rst +++ /dev/null @@ -1,68 +0,0 @@ -================ -PLIP 10886 Notes -================ - -# This PLIP is ready for review - nearly, now only completion of this doc is -# missiing. - -PLIP ticket: https://dev.plone.org/ticket/10886 - - -Documentation -------------- - -Overview --------- - - - -Known issues, weak points and TODOs ------------------------------------ - -- Icalendar export doesn't export timezones properly. It just converts - everything to UTC, with no timezone definitions in the ical export. This can - be fixed quite straight forward. - -- One test fails only from time to time (Microsecond comparison bug): - plone.app.event.tests.test_atevent.PAEventATTest.test_edit, 156-162 - Maybe we should remove the (partial) support of Microseconds alltogether and - use a more tolerant comparing function for datetime objects (which doesn't - compare below a minute or so). - -- We use Products.DateRecurringIndex instead of - Products.PluginIndexes.DateIndex. While it is as efficient for indexing - events without recurrence, events with recurrence take longer. For each - occurrence of an recurring event, an index entry is created. We have an - hardcoded recurrence limit set to 1000 in plone.event.recurrence, which - should be enough for most use cases. - -- For recurring events, plone.app.event.recurrence.RecurrenceSupport.occurrence - goes through the whole recurrence set, which is inefficient. This is, because - we turn the generator based recurrence set to a list. While this is mainly a - problem with recurring events, it should be changed and optimized. - One (soft) reason, which hinders us from doing so, is that in the event view, - we want to show only the first 7 occurrences and the very last occurrence. To - get the last occurrence, we have to walk through the whole recurrence set. - This problem is documented here: - https://github.com/plone/plone.app.event/issues/60 - -- We use plone.event.IEventAccessor as a wrapper for Archetypes and Dexterity - based events. While I think, this is a good and flexible concept, the - EventAccessor implementations always adapt the object itself. We use this - whereever possible, even when accessing a brain's information would be - sufficient. This might be inefficient too, in some cases. - -- There are some open TODO's at: - https://github.com/plone/plone.app.event/issues - -Dependencies ------------- - - -Tests and test coverage ------------------------ - - -Further ideas -------------- - diff --git a/plips/implementation_notes/plip10901-javascript-standards.txt b/plips/implementation_notes/plip10901-javascript-standards.txt deleted file mode 100644 index ae240257d8..0000000000 --- a/plips/implementation_notes/plip10901-javascript-standards.txt +++ /dev/null @@ -1,39 +0,0 @@ -PLIP #10901 implementation notes -================================ - -(Set and enforce base coding standards for our own JavaScript) - -Implementer: Steve McMahon -PLIP text: http://dev.plone.org/plone/ticket/10901 - -The "enforce" portion of this PLIP is not completed. - -Standards -========= - -A new section of the developer manual has been written to document coding and -deployment practices. It's available, in draft mode, at: - -http://plone.org/documentation/manual/developer-manual/client-side-functionality-javascript - -Readers may need to be logged in to read this. - -Like the rest of the developer manual, this is intended to be a work in -progress. - -Code Review and Update -====================== - -Branch of Products.CMFPlone at: - -https://svn.plone.org/svn/plone/Plone/branches/plip10901-jslintification - -A js-standards.js file in plone_ecmascripts sets jslint flags that are intended -to set baseline standards. - -All of the JavaScripts in plone_ecmascripts have been reviewed and revised to -pass jslint checks with those options (and occasional per-module switches to -avoid coding I thought unnecessary). - -The coverage of test_rendering.pt (in plone_templates) has been extended to -cover more of the code in plone_ecmascripts, and should be near complete. \ No newline at end of file diff --git a/plips/implementation_notes/plip10902-todo.txt b/plips/implementation_notes/plip10902-todo.txt deleted file mode 100644 index fa36086b50..0000000000 --- a/plips/implementation_notes/plip10902-todo.txt +++ /dev/null @@ -1,116 +0,0 @@ -eleddy -====== -- adding a new collection, what is allowed users and roles supposed to - filter on? it doesn't seem to filter on anything -TODO: This filter should allow the end user to create a collection that contains documents on which user 'x' as the View permission. -Portal_catalog checks the View permission for the current user. Maybe we can use unrestrictedSearch if this filter is used. -INFO: unrestrictedSearch breaks security, we will remove the filter for now. Fixed in r50436. - - -- Once I add a criteria, then try to add another one that is the same, the - results end up being a little weird. For example, I have description and - the word "tomato" with three results. then add another description field - with no words at all and I get 21 results. I would expect these to be - an intersection, although I could be wrong since there is no indication. - I think its just a bug though. -XXX: remove index when used, or merge criteria? -INFO: Looked at this, this needs to be fixed in the queryparser, we need - to 'remember' previous queryvalues and merge them in a 'intelligent' - dictionary update method - - -- It would be nice if the query did not submit and filter - until after the user enters something in the box. I often found myself adding - a filter and then wanting to see what I had in the live search but it was gone. - This also makes sense in cases where the filter does not apply to the objects being - displayed (e.g. event start and end date) -XXX: would be nice. - -- Also nice to have is not to delete the contents of a box if I switch to - another criteria and then decide to come back. This makes sense if I'm - trying to decide between using title, description, or searchable text for - example -XXX: would be nice. -INFO: This really depends on the type you select, we can't remember everything - in the whole widget. Out of scope for now - -- More nice to have is that for items where you have no other choices for the - type of query (e.g Description "is" versus Locations rel/abs choice) I would - like to see that be text instead of a drop down. This control makes me wonder - as the end user if I should check for more options each time and the majority - of the time I don't need to. -XXX: would be nice. -INFO: Agreed, nice to have, for next release. - -- when adding the related to field I immediately get an error: - -2010-12-17 18:19:42 ERROR Zope.SiteErrorLog 1292638782.460.265875466564 http://localhost:8080/Plone/@@querybuildernumberofresults -Traceback (innermost last): - Module ZPublisher.Publish, line 127, in publish - Module ZPublisher.mapply, line 77, in mapply - Module ZPublisher.Publish, line 47, in call_object - Module plone.app.querystring.querybuilder, line 52, in number_of_results - Module plone.app.querystring.querybuilder, line 33, in __call__ - Module plone.app.querystring.querybuilder, line 45, in _makequery - Module plone.app.querystring.queryparser, line 44, in parseFormquery -AttributeError: 'module' object has no attribute '_referenceIs' -TODO: add parser for referenceIs -INFO: Removed related criteria for now in r50227. - -- Plone.app.collection: Need to have README say something -TODO: make readme -INFO: Added README in r50434 - -- The query field widget could use some better documentation. -TODO: documentation. -INFO: Added documentation, docstrings, comments to - archetypes.querywidget - - -rossp -===== -- The GenericSetup profiles need better titles and descriptions so - that users can know what they're looking at when installing add-ons. -TODO: fix profile titles -INFO: Fixed in r50430 (also in dependency packages) - -- The criteria value dropdowns are styled differently than other - dropdowns. -TODO: fix styling for custom widgets -INFO: This because it's not a normal dropdown, it's a custom html widget. - The same styling is not possible because that is OS-dependent. - Faking it would be an option, but I think that will create a lot of - extra clutter/mess. Currently there is no widget that does 'single-line - multiselection', we need this to keep all the layout within one row. - -- plone.app.collection, plone.app.querystring and - archetypes.querywidget are in dire need of comments, docstrings and - more complete interfaces for developer/maintenance documentation - purposes. It's far to difficult to approach and understand how - things fit together as-is. -TODO: create docs + overview image etc -INFO: We've hadded an overview images how the packages depend/linked to - eachother.(see plone.app.collection > docs > p.a.collection.png) - Comments, docstrings and such are added to p.a.collection, - p.a.querystring and archetypes.querywidget. Clutter is removed, - style changes are applied. - - -- The javascriptDisabled validator seems distasteful to me, but I - might not be understanding it. Why is this necessary and is there - really no better way? -TODO: explain in docs why this works the way it does. -INFO: This is necessary to create a validation error. - the validation error is needed to stay in in the current form, - which keeps archetypes from creating a temp object in portal_factory and - losing the request. After discussing this with several (ex frameworkteam) - people, i've implemetented this. - I've updated the docstrings in the python file. - -- I love how small plone.app.collection is. -Me too! -XXX: convert to Dexterity? -INFO: In the future: definitely! - -- I don't see any unit tests for the JS. -INFO: There are unit tests in archetypes.querywidget > tests > javascripts diff --git a/plips/implementation_notes/plip11697-authorinfo-overlay.txt b/plips/implementation_notes/plip11697-authorinfo-overlay.txt deleted file mode 100644 index b8cc3c3591..0000000000 --- a/plips/implementation_notes/plip11697-authorinfo-overlay.txt +++ /dev/null @@ -1,4 +0,0 @@ -PLIP 11697: All authorinfo links opened in an overlay -===================================================== - -PLIP ticket : https://dev.plone.org/plone/ticket/11697 \ No newline at end of file diff --git a/plips/implementation_notes/plip12234-batching.txt b/plips/implementation_notes/plip12234-batching.txt deleted file mode 100644 index a09d597184..0000000000 --- a/plips/implementation_notes/plip12234-batching.txt +++ /dev/null @@ -1,38 +0,0 @@ -PLIP #12235 implementation notes (Unify batching) -================================================= - -Implementer: Tom Gross -PLIP text: http://dev.plone.org/plone/ticket/12235 - -Reference buildout is at git://github.com/plone/buildout.coredev.git; run with --c plips/plip12234-batching.cfg. - -I introduced a new package plone.batching. This package is supposed to do -*all* batching in Plone. It has 100% test coverage. - -I pointed all known batching implementations to plone.batching and removed -all custom batching code. The known batches are in - - - Products.CMFPlone.PloneBatch (e.g. search) - - plone.app.content (e.g. folder_contents) - - plone.z3cform (e.g. ??? -> singing and dancing). - -To generate the sphinx api documentation run bin/sphinx. - -The implementation aims to be 100% backwards compatible, there is one -decision I made. In the former PloneBatch *len* returned the length of a -batch and in the former plone.app.content *len* returned the full length -of the sequence passed to the batch. In plone.batching the behavior is the -later. The len-method returns the full length of the sequence which is used -for the batch. The former PloneBatch behavior still works if imported from -Products.CMFPlone.PloneBatch. A deprecation warning is emited to use the -*pagesize* attribute for getting the batch length. - -Test cases are: - - - Add a lot items to a folder and call `folder_contents` - - Add a lot items to the site with one term present and search for it - - Install c.dancing and add a lot subscribers to a newsletter - -I plan to add developer documentation of how to use batches in custom code -and templates. diff --git a/plips/implementation_notes/plip12253-users-z3cform.txt b/plips/implementation_notes/plip12253-users-z3cform.txt deleted file mode 100644 index f6d7576f66..0000000000 --- a/plips/implementation_notes/plip12253-users-z3cform.txt +++ /dev/null @@ -1,49 +0,0 @@ -PLIP #12253 implementation notes (Use z3c.form in plone.app.users) -================================================================== - -Implementers: Vitaliy Podoba and Jamie Lentin -PLIP text: http://dev.plone.org/ticket/12253 - -Reference buildout is at git://github.com/plone/buildout.coredev.git; run with --c plips/plip12253-users-z3cform.cfg. - - -How to test ------------ - -PLIP buildout configuration includes collective.examples.userdata package. But -as long as you do not install it in quick installer tool, it won't influence -plone.app.users functionality as it's form extenders are registered for it's -package layer interface. - -End user forms to check: - -* registration form -* user preferences form -* personal information form -* change password form - -Management forms to check: - -* Add new user form -* Member Registration control panel (accessible via tab inside Users and Groups - panel) - -Please, check README.txt in plone.app.users and -collective.examples.userdata packages to get an idea how it all works. - - -Notes ------ - -* we haven't added Deprecation Warnings yet, not sure where should we insert it, - into form classes, or at modules import time? - -* as stated in PLIP description, we do not support ISchemaProvider approach and - instead switched to using IFormExtender (from plone.z3cform) to allow - developers extend user related forms in a more generic way - -* also we have our own schema adapter class which overrides __setattr__ and - __getattr__ methods to avoid code duplication for every new field in user - schema; so we add extra setter and getter only if we need to handle some user - property in a special way, it'll still work diff --git a/plips/implementation_notes/plip12844-tinymce.txt b/plips/implementation_notes/plip12844-tinymce.txt deleted file mode 100644 index 33bc939930..0000000000 --- a/plips/implementation_notes/plip12844-tinymce.txt +++ /dev/null @@ -1,26 +0,0 @@ -PLIP #12844 implementation notes (TinyMCE upgrade) -================================================== - -Implementer: Domen Kožar (GSOC 2011), Tom Gross -PLIP text: http://dev.plone.org/plone/ticket/12844 - -Reference buildout is at git://github.com/plone/buildout.coredev.git; run with --c plips/plip12844-tinymce.cfg. - -To fully upgrade the editor you need to run an upgrade profile: - - - Go to ZMI - - Go to portal_setup - - Choose Upgrades tab - - Run upgrade steps for Products.TinyMCE - -Full documentation be found here: - -http://readthedocs.org/docs/productstinymce/en/latest/index.html - -Test cases are: - - - Add images and internal links to a richtext field - - Count load time and number of requests in edit mode - - Check IE 9 and above - diff --git a/plips/implementation_notes/plip8699-pubdate-byline.txt b/plips/implementation_notes/plip8699-pubdate-byline.txt deleted file mode 100644 index 795b88a559..0000000000 --- a/plips/implementation_notes/plip8699-pubdate-byline.txt +++ /dev/null @@ -1,119 +0,0 @@ -PLIP #8699 implementation notes (Make byline use the publication date) -====================================================================== - -Implementer: Vitaliy Podoba -PLIP text: http://dev.plone.org/ticket/8699 - -Reference buildout is at git://github.com/plone/buildout.coredev.git; run with --c plips/plip8699-pubdate-byline.cfg. - -I updated PLIP description a bit according to suggestions I received on plone -devel mailing list: - -* basically we display Effective Date if it's been set -* otherwise we fallback to publish transition date taken directly from workflow - history - -In order to complete the task I modified the next packages: - -* plone.app.layout: extended document byline viewlet with publication date, - added tests. All went into dedicated plip branch: - https://github.com/plone/plone.app.layout/commit/eac4d4596f794f6c3dd32de001be9f597f5e76e2 -* plone.app.controlpanel: added 'Display publication date' setting to Security - control panel, right after 'Show Byline' option. All went into dedicated plip branch: - https://github.com/plone/plone.app.controlpanel/commit/75aba70d6170901a4a60d9581110eb09ca958eae -* Products.CMFPlone: added new property to portal properties tool to control - publication date visibility, added tests. All went directly into 4.3 branch - (master): - https://github.com/plone/Products.CMFPlone/commit/da0da454c8dbcfbd38e298b09574ec261ccdf344 -* plone.app.upgrade: added migration profile 4.2->4.3alpha1 containing above - mentioned property addition. All went directly into master branch: - https://github.com/plone/plone.app.upgrade/commit/90d84e1ad17efa211aca172c08ea4ab59ee6fe89 - -What I haven't done yet: - -* should I update plone.app.locales with new i18n strings appeared on Security - control panel? Or is this a separate process performed by our plone i18n team? -* should I update any documentation pages in order to finish this PLIP? - - -Test cases are --------------- - -1. Switch on publication date on Security panel. - Add a new document. - Byline doesn't include publication date. - -2. Switch on publication date on Security panel. - Add a new document. - Publish document. - Byline includes publication date and not a modification date. - -3. Switch on publication date on Security panel. - Add a new document. - Publish document. - On edit form empty Published Date. - Publication date should still be displayed in byline, this time - taken from workflow history. - -4. Switch on 'Allow byline for anonymous' on Security panel. - Switch on publication date on Security panel. - Add a new document. - Publish document. - Anonymous user should see publication date in byline. - -4. Switch off publication date on Security panel. - Add a new document. - Publish document. - No publication date in byline. - - -RE: eleddy's review -=================== - -* RE: settings panel - -I also agree that publication date has no relation to security panel, placed it -initially there just because we already had "Display byline to anonymous" -setting in there. And because plone.app.workflowmanager's panel is about -managing content types workflows, I decided to put our new setting into Site -Setting panel right after "Expose Dublin Core metadata" checkbox. - -* RE: setting wording - -I corrected it. Please, check and suggest anything better in case I'm still -wrong with my English ;) - -* RE: "display modification date only if object was modified after publication" - feature - -I reviewed this issue and decided to completely remove this "feature". As far as -I can see it only adds confusion: Effective Date could only be set with 5 -minutes approximation via Plone UI, while publish transition date includes even -milliseconds. On the other side, we only display minutes (w/o seconds) in author -byline viewlet dates. Thus sometimes it's hard to understand what's going on -behind the scenes. - -I had 2 options. 1: before checking whether modification date is not older than -publication date, cut down seconds part from dates; or 2: completely remove that -feature. - -I picked latter so now modification date is visible all the time, regardless of -publication date. If you think this functionality is important for us then I'll -take another look into it. - - -* RE: translation - -I updated plone.pot inside plone.app.locales with new messages introduced by -this PLIP. Should I do any extra steps here? Like syncing other *.po files, -etc.. - - -* RE: documentation - -I also updated this page http://plone.org/documentation/manual/plone-4-user-manual/using-collections/using-and-understanding-dates with short Publication Date paragraph. Please, review. - - -All branches code is merged into masters. -Thank you for your time! diff --git a/plips/implementation_notes/plip9288-improved-commenting-infrastructure.txt b/plips/implementation_notes/plip9288-improved-commenting-infrastructure.txt deleted file mode 100644 index 6aa9eba7fd..0000000000 --- a/plips/implementation_notes/plip9288-improved-commenting-infrastructure.txt +++ /dev/null @@ -1,198 +0,0 @@ -============================================= -PLIP 9288: Improved commenting infrastructure -============================================= - -This plip is ready for review. - -Plip ticket: https://dev.plone.org/plone/ticket/9288 - -Known Issues ------------- - - - Comment items acquire UID. - - https://dev.plone.org/plone/ticket/10652 - - There is a branch that requires plone.uuid that fixes the issue: - - http://svn.plone.org/svn/plone/plone.app.discussion/branches/tisto-plone-uuid/ - - - - Delete comment requires 'Manage portal' permission. - - https://dev.plone.org/plone/ticket/11145 - - - - Bulk moderation actions don't work (fixed). - - https://dev.plone.org/plone/ticket/11156 - - Bug is already fixed and can be closed. A trac bug prevents closing it. - - -Summary of changes / Features ------------------------------ - -As described in PLIP 9288, a new commenting system was -implemented with the following features: - - - Basic commenting functionality: Users can post comments or reply to existing - comments. Same functionality as the old/current commenting system (e.g. - post/reply to comment, allow comments on content types, on single content - objects, and on folders). The UI slightly changed because the new comments - do not have a title field anymore. Titles on comments doesn't make sense. - - - Moderation: It is possible to enable moderation, so that every comment has - to be reviewed before publishing. To enable moderation, go to the types - control panel, choose "Comment" content type and set workflow to - "Comment Review Workflow". - - - Mass editing screen/batch moderation: Like in Wordpress, there is a - moderation view ("moderate comments" in site-actions or - your-site/@@moderate-comments) for batch editing comments. - - - Captcha support: Four different captcha implementations can be used with - plone.app.discussion: - - - plone.formwidget.captcha (for Captcha spam protection) - - plone.formwidget.recaptcha (for ReCaptcha spam protection) - - collective.akismet (for Akismet spam protection) - - collective.z3cform.norobots (provides a "human" captcha widget based on a - list of questions/answers) - - If one or more of these plugins are installed, anonymous comments can be spam - protected. To enable a captcha solution, go to the discussion control panel, - allow "Anonymous comments" and choose a captcha solution from the drop-down - list. - - - Configurable and extensible comment forms: Using z3c.form for the commenting - forms allows for components to extend the comment form with additional fields - or to hide/reorder existing fields. The CaptchaExtender - (see browser/captcha.py) is an example of a very simple form extension. - - - EMail notification: plone.app.discussion allows users and administrators to - be notified about new comments by email. There are two kinds of email - notification: - - 1. User notification: Tell users when a comment has been added. - This method composes and sends emails to all users that have added a - comment to this conversation and enabled user notification. - - This requires the user_notification setting to be enabled in the - discussion control panel. - - The user notification feature requires z3c.form >= 2.3.3 and is commented - out in the code right now. If Plone 4.1 will use z3c.form >= 2.3.3 we can - enable this feature. - - 2. Moderator notification: Tell the moderator when a comment needs attention. - This method sends an email to the site admin (mail control panel setting) - if comment moderation is enabled and a new comment has been added that - needs to be approved. - - This requires the moderator_notification to be enabled in the discussion - control panel and the comment_review_workflow enabled for the comment - content type. - - - plain and intelligenttext comments: Go to the plone.app.discussion control - panel and choose "plain text" or "intelligenttext" from the "Comment text - transform" drop-down list. - - - Migration: It is possible to migrate the old comment system - to the new one. Go to "your-site/@@comment-migration" to - migrate comments. - - -Dependencies ------------- - -- plone.registry/plone.app.registry - -- z3c.form/plone.z3cform/plone.app.z3cform - -- plone.uuid/plone.app.uuid - -- plone.testing/plone.app.testing - - -Tests and code checkers ------------------------ - -- 129 Test - -- Test coverage is currently: 92 % - -- qunit javascript tests for comments viewlet and bulk moderation view - -- jslint and zptlint pass without violations - -- pylint still reports (minor) violations - - -Discussion add-on products --------------------------- - -- plone.formwidget.captcha (z3c.form captcha widget) - http://svn.plone.org/svn/plone/plone.formwidget.captcha/ - -- plone.formwidget.recaptcha (z3c.form recaptcha widget) - http://svn.plone.org/svn/plone/plone.formwidget.recaptcha/ - -- collective.autoresize (jQuery auto-resize for comment-text) - http://svn.plone.org/svn/collective/collective.autoresizetextarea/ - -- collective.akismet (for Akismet spam protection) - http://pypi.python.org/pypi/collective.akismet/ - -- collective.z3cform.norobots (provides a "human" captcha widget based on a list - of questions/answers) - http://pypi.python.org/pypi/collective.z3cform.norobots/1.1/ - - -Further ideas -------------- - -- RichText comments - -- Auto-approve whitelist - -- IP blacklist - -- Comment pagination - -- Migration from other commenting add-ons (qPloneComments, - iqpp.plone.commenting) - -For a complete list of ideas see: -http://www.pivotaltracker.com/projects/15135 - - -Documentation -------------- - -There is a Sphinx-based documentation. See: -http://packages.python.org/plone.app.discussion/ - - -Needed documentation changes ----------------------------- - -To my knowledge, there is no documentation about the current -commenting system on plone.org. - - -Backwards compatibility ------------------------ - -There is a migration script to migrate comments -("your-site/@@comment-migration"). - - -Credits -------- - - - Timo Stollenwerk - - Martin Aspeli - - Jon Stahl - - David Glick - - and others diff --git a/plips/implementation_notes/plip9324-collective.amberjack.txt b/plips/implementation_notes/plip9324-collective.amberjack.txt deleted file mode 100644 index f32b980aca..0000000000 --- a/plips/implementation_notes/plip9324-collective.amberjack.txt +++ /dev/null @@ -1,121 +0,0 @@ -PLIP #9324 implementation notes (collective.amberjack) -===================================================== - -Implementer: see http://svn.plone.org/svn/collective/collective.amberjack.core/tags/1.1/docs/AUTHORS.txt -PLIP text: https://dev.plone.org/plone/ticket/9324 - -Reference buildout is at -https://svn.plone.org/svn/plone/buildouts/plone-coredev/branches/4.1; -run with --c plips/plip9324-collective.amberjack.cfg. - -What to review --------------- -Create a new Plone site. Add 'Amberjack' and 'Amberjack tour launcher' products. -It installs: - - the AJ core - - the launch portlet on the left column - - 12 tours -Now you can choose to run the tours in two different ways: - - standalone. It runs the tours in the root folder of your Plone site. - - in a sandbox. It runs the tours in the actual user's folder. -By default the 'standalone' option is enabled. You can use the 'admin' user to -run all the tours. -If you want to enable the 'sandbox' one you need to: - - set the users folders - - add a new user - - in his members folder, he needs to have these roles set: - - contributor - - editor - - reviewer - - log in with that user and run the tours - -Use the portlet to start the tours. The portlet also tells you what you need to -do to start a given tour. For example: - - if you misconfigure the sandbox option, it won't let you start any tour. - - if a tour needs a 'review' role and you don't give it to your user that tour - will be unavailable (shown, but without the link to let it start) - - to run the 'create a page' one, you need to have the 'MyFolder' already - created. - -The tours are made of several steps. Every step is divided into several -micro-step. During the tours you can both use the '>>' marker you'll find on -the right side of every microstep or do it directly inside the plone interface. -In some cases you won't find a '>>' marker, but just a description of what you -have to do. - -Some micro-step are mandatory, others not. We decided to not put any evidence on -the interface (eg. the usual plone tiny red square). Every micro-step is an -order ('enter this value in that field', 'write something in another one') so -it's already clear what you should do. - -The validations are needed just to make the output of every tour consistent -with all the other tours. For example, you need to call the initial folder -'MyFolder' because of the plone-ids and because is referred in all the other -tours. We planned to remove this limitation in a further release. - -If you miss a mandatory micro-step a popup will alert you that you need to -complete it before continuing. Before leaving the page, for example submitting -a form, it will check if you did every thing that was needed. - -Of course you are free to exit the tour in any moment clicking on the big X -in the left top corner of the tour box. - -Preconditions and validations ------------------------------ -There are preconditions and validations that can be set and have to be met. - -Preconditions are related to tours. -As stated before, you can add a page only if a folder that should contain the -page had already been created ('isCreated') and if that page hadn't already been -created ('isNotCreated'). In addition, you have to be a Contributor ('hasRole') -to add it and Reviewer ('hasRole' again) to publish it. -In the same way all the other tours has been configured - -Validation are related to micro-steps. -You have to name the page 'My Page' o/w it will break the further tours. You -need to complete all the plone mandatory fields. A given field must be not -empty. -So those conditions may be: - - checkstep: checks that the user actually did what the step asked for - - a dictionary: - {"selector":{"description":"id"},"operator":"co","value":"Plone"} - - doitmanually: if set, this means that the user has to complete the microstep - manually. - - or none - -The start portlet ------------------ -It is configurable. you can choose: - - the title - - the tours that you want to show - - their order - - the layout - -The tours ---------- -The tours are all .cfg files in the collective.amberjack.plonetour package. -They are stored inside the "tours" folder with their translations. -Through the configuration in buildout they are available for the user. - -The available right now are: - - Add and publish a folder - - Add and publish a Page - - Add and publish a News Item - - Add and publish an Event - - Format a page using the visual editor - - Create internal links - - Create external links - - Upload an image - - Insert image on a page - - Upload and link to a File - - Using the Contents tab - - Using the Display menu - - -blueprints and known bugs for 1.1 release ------------------------------------------ -See: -https://launchpad.net/collective.amberjack/+milestone/1.1 - - diff --git a/plips/implementation_notes/plip9327.contentlisting.txt b/plips/implementation_notes/plip9327.contentlisting.txt deleted file mode 100644 index 54e434cca5..0000000000 --- a/plips/implementation_notes/plip9327.contentlisting.txt +++ /dev/null @@ -1,57 +0,0 @@ -PLIP #9327 implementation notes (plone.app.contentlisting) -===================================================== -implementer: Geir Bækholt and friends - -Plip ticket: -http://dev.plone.org/plone/ticket/9327 - -Reference buildout is at -https://svn.plone.org/svn/plone/buildouts/plone-coredev/branches/4.1; -run with --c plips/plip9327-contentlisting.cfg. - -What to review: - -The main usecase for contentlisting is to make lists of content items -in Plone uniform, regardless what the source of the content items is: -the catalog, a folderlisting, an orm or something completely -different. - -On its own, the most useful way of reviewing it is to look at the -tests and the code. - -There is a very basic doctest that introduces the concept of the -contentlisting from pure python, and an integration test suite that -covers more of the real life usage. - - -There are plans to change several or all of the templates listing -content in Plone to use plone.app.contentlisting, but have not yet had -time. For this purpose Plone itself has been branched and added to -this plip config file. No files have yet been modified in that -branched Plone. - - - -Contentlisting is used as a base for the implementation of -#9352-search-results and plip10902-new-collections - - -TO DO -====== - -* convert existing templates in Plone and default portlets to use contentlistings - -* Write better documentation for non-developers / template-tinkerers. - -* improve test-coverage to 100% (we are close, but not all there yet) - -* some more coverage of Dubmin Core methods - -* Check if this works for Dexterity types or if they need special handling. - - - - - - diff --git a/plips/implementation_notes/plip9352-search-results.txt b/plips/implementation_notes/plip9352-search-results.txt deleted file mode 100644 index 6677d87d1f..0000000000 --- a/plips/implementation_notes/plip9352-search-results.txt +++ /dev/null @@ -1,88 +0,0 @@ -======================================= -PLIP 9352: Improved search results view -======================================= - -This plip is ready for review. - -Plip ticket: https://dev.plone.org/plone/ticket/9352 - - -Known Issues ------------- - -There is a TODO.txt file in docs/ folder of plone.app.search package that -contains things left to be fixed and to be implemented. Other than that, the -major issue with the current implementation is that the new search results page -is slower than the old one. To get exact numbers to compare run performance -tests with: - - bin/test -s plone.app.search --tests-pattern=performance - -By simple "remove from template" method was figured out that the fact of -merging advanced search form into the search results page is not a reason for -slower view. This might be related to the way methods from -plone.app.contentlisting work, since the new search is wrapping results into -IContentListing interface. Would appreciate if somebody could help in figuring -out the actual reason. - - -Summary -------- - -Improved search results view is a combination of regular results view and -advanced search form where we can use fields of the form as the additional -filters of our search results. Advantage of the new view is in it's dynamic -presentation of the results by serving updated results with ajax calls after -applying filters or changing the sorting. - -In addition locations of some elements have been re-thought in order to let -users get to the search results faster without minimum disturbance. - -Sorting options have been renamed to better match "normal humans" language. - -The new search results view also provides information about section a search item is located in. This functionality is using elementary caching so doesn't slow down the search results view unless there are a lot of different sections in the site's root and the search results are coming from different sections. - -New search results view is also very accessible, so if javascript is disabled in a browser the user will get all the information equivalent to those with JS enabled but without ajax calls. That means when-ever a user is changing a filter or a sorting option he needs to re-submit the form. - - -Dependencies ------------- - -Dependancies are available in the corresponding trunks or branches and are checked out from the PLIP's configuration: - -- Products.CMFPlone - -- plone.app.contentlisting - -- plone.app.upgrade - -- plonetheme.sunburst - -- plonetheme.classic - -- plone.app.layout - -- plone.testing/plone.app.testing - - -Tests and code checkers ------------------------ - -- Selenium tests for ajax integration and some default elements testing. Moved - out of the general testing flow to not slow down the testing of Plone. - -- Performance tests (failing at the moment) to compare how much the new search - is slower than the old one. Moved out of the general testing flow to not slow - down the testing of Plone. - -- jslint, pyflakes, PEP8 check ups - - -Credits -------- - - - Denys Mishunov - - Nejc Zupan - - Andi Zeidler - - Geir Bækholt - - Plone Community diff --git a/plips/implementation_notes/plip9938-output-filters.txt b/plips/implementation_notes/plip9938-output-filters.txt deleted file mode 100644 index 8a8af020fe..0000000000 --- a/plips/implementation_notes/plip9938-output-filters.txt +++ /dev/null @@ -1,58 +0,0 @@ -PLIP #9938 implementation notes -=============================== - -(factor custom output transformations out of the editors) - -Implementer: David Glick -PLIP text: https://dev.plone.org/plone/ticket/9938 - -Reference buildout is at -https://svn.plone.org/svn/plone/buildouts/plone-coredev/branches/4.1; run with --c plips/plip9938-output-filters.cfg. This buildout is also used for PLIP 10778 -for ease of testing, but PLIP 9938 is not dependent on 10778. The main packages -with changes relevant to PLIP 9938 are: - -* plone.outputfilters (new package) -* Products.kupu -* Products.TinyMCE -* (Plone and plone.app.upgrade have some trivial changes.) - -``plone.outputfilters`` contains an implementation of the "resolve UID links" -and "add image captions" features, based on both the old kupu implementation -and the old TinyMCE implementation. The editors have been updated to depend -on and install plone.outputfilters, and their tests for these features have -been ported to plone.outputfilters. - -The old transforms in the editor packages have been turned into stubs which -defer to the plone.outputfilters implementation. (They cannot be removed -entirely at this point, because of persistent references in the database.) There -is an upgrade step which replaces the old transforms with the new one when Plone -is upgraded to 4.1. plone.outputfilters can also be installed in older versions -of Plone. - -Each editor used to have its own resolveuid.py script in a skin layer; this has -been turned into one browser view in plone.outputfilters. This view is aware of -plone.app.uuid (from PLIP 10778) and will use it to resolve UIDs if present. -Otherwise (such as in Plone < 4.1) there is a fallback to the old approach -(querying the catalog directly). The editors' content browsers have also been -updated to use the plone.app.uuid API when available. - -See the plone.outputfilters README for more details. - -While implementing the new transform, I took the opportunity to resolve a -few outstanding bugs: - -* Rendering relative links (i.e. with link-by-UID off) out-of-context (such as - in the new "All content" listing view) could result in broken links or links - that only worked thanks to Acquisition. Relative links are now absolutized to - avoid this problem. -* The TinyMCE image dialog failed to highlight the current - image if it was created when link-by-UID was off but it is now on, or vice - versa. (See http://dev.plone.org/plone/ticket/10970). This is now fixed. -* TinyMCE allowed the user to edit an image's alt text, and used it as the - caption in preference to the image's description. This caused confusion - for users migrating from kupu who had carefully set up distinct descriptions - and alt text. It was also a misuse of the alt text concept (which is intended - as a fallback when an image can't be rendered, not an always-present caption). - I've changed TinyMCE to edit the image's description instead, and always - use the description as the caption (like kupu did). diff --git a/plips/reviews/plip10778-review-alecm.txt b/plips/reviews/plip10778-review-alecm.txt deleted file mode 100644 index 352ab53460..0000000000 --- a/plips/reviews/plip10778-review-alecm.txt +++ /dev/null @@ -1,101 +0,0 @@ -PLIP 10778: Standalone UUID implementation -========================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10778 - -Review #3 by Alec Mitchell (alecpm@gmail.com, alecm on irc) - -The PLIP was reviewed on Mac OS X 10.5.8 using python 2.6.5 and Google -Chrome 6.0.472.62. - - -Review steps ------------- - -- Ran buildout using the plip9938-output-filters.cfg file. - -- Ran tests for plone.uuid and plone.app.uuid. - -- Ran bin/alltests to check for additional failures in the Plone stack - caused by PLIP changes. - -- Reviewed the code in plone.uuid and plone.app.uuid. - -- Reviewed changes in Archetypes and TinyMCE packages. - -- Checked uuid assignment and indexing through the web in the ZMI and - through the web accessible API. - -- Checked uuid behavior on copy, move and delete. - -- Ran custom buildout config with - plips/plip9938-output-filters.cfg and experimental/dexterity.cfg - with DexterityContent set to implement IAttributeUUID. - -- Installed Dexterity in Plone site in order to test content type - independence of plone.app.uuid through the Plone interface. - - -Notes and observations ----------------------- - -- All plone.uuid and plone.app.uuid tests pass. - -- There is a failure in archetypes.referencebrowserwidget which - appears to be caused by some change in the PLIP #9938 buildout. - -- The plone.uuid code is straightforward and well tested. It seems - slightly problematic that the result of an adaptation to the IUUID - interface does not result in an object implementing the IUUID - interface, but this is not a major concern. - -- It is not clear to me what the purpose of the IUUIDAware interface - is. Why aren't IUUID and IAttributeUUID be sufficient? - -- The plone.app.uuid code is straightforward and well tested. I would - prefer if the utility functions defined in plone.app.uuid.utils - followed PEP8 naming conventions. - -- There is a test in plone.app.uuid for a browser view defined in - plone.uuid, this is unconventional. - -- The changes made in the Product.Archetypes plip10778-plone.uuid branch are - straightforward and reasonable. The code now uses IUUID(obj) in - preference to obj.UID(), however some tests still use obj.UID() - which could lead to confusion. This is a minor point as the APIs - are synonyms and the UID method is not deprecated, but it's use - should be discouraged for any packages which intend to be content - agnostic. - -- It would be nice to see the manage_afterAdd and manage_afterCopy - methods in AT's Referenceable class go away in favor of an event - based system for managing uuids. This would bring the AT IUUID - implementation a little closer to the default one. - -- plone.app.uuid includes a default profile with a catalog.xml that - creates the 'uuid' index and metadata. However, this profile is not - registered. As a result, newly created sites do not have these - indexes. If it is no longer required, this profile should be - removed. - -- I would like to see some tests in plone.app.uuid to demonstrate that - it works with (dummy) non-AT content. - -- Through the web testing with AT content indicates that the UID/IUUID - changes are working as expected. - -- Through the web testing with uuid-enabled dexterity content - indicates that uuids are properly handled on Add/Copy/Move and - indexed in the UID index as expected. - -- The PLIP itself could use some updates, since some of the goals and - deliverables have changed. It should not be necessary to read - through the comments on the PLIP to understand those changes. - -Conclusion ----------- - -The implementation is straightforward and the new API is clear and -well tested. Other than the test failure in -archetypes.referencebrowser, the only concerns I have are cosmetic. I -consider this PLIP ready for merge. diff --git a/plips/reviews/plip10778-review-cah190.txt b/plips/reviews/plip10778-review-cah190.txt deleted file mode 100644 index 6899d07222..0000000000 --- a/plips/reviews/plip10778-review-cah190.txt +++ /dev/null @@ -1,62 +0,0 @@ -PLIP 10778: Standalone UUID implementation -========================================== - -PLIP ticket: http://dev.plone.org/plone/ticket/10778 - -Review by Craig Haynal (cah190@psu.edu, cah190 on irc) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Firefox 3.6.10. - - -Review steps ------------- - -- Ran buildout using the plip9938-output-filters.cfg file. - -- Ran tests for plone.uuid and plone.app.uuid, and Products.Archetypes. - -- Inspected code in plone.uuid, plone.app.uuid, and Products.Archetypes. - -- Ran bin/alltests. - -- Created and examined a new Plone site to look for any obvious issues. - -- Tested the @@uuid and @@redirect-to-uuid views. - - -Notes and observations ----------------------- - -- The PLIP buildout also includes the implementation for PLIP 9938. - -- plone.uuid has an XXX marker in configure.zcml, though it will be - relevant if we go with Zope 2.13 for Plone 4.1. - -- The 5 tests in plone.uuid cover nearly all the code. - -- Code for plone.uuid looks reasonable. - -- The @uuid view works as expected. - -- The 7 tests in plone.app.uuid cover nearly all the code. - -- The @@redirect-to-uuid view in plone.app.uuid throws an exception if - no UUID is provided, perhaps it should return something like a 400 Bad - Request or a 404 Not Found instead of raising a KeyError? - -- The @@redirect-to-uuid view causes a redirection loop to - @@redirect-to-uuid/None if an invalid/missing UUID is provided. - I would expect a 404 Not Found if the UUID cannot be resolved. - -- Code for plone.app.uuid looks reasonable. - -- Changes in the Products.Archetypes branch look reasonable and tests - are passing. - - -Conclusion ----------- - -Other than the @@redirect-to-uuid issues, the implementation -is excellent. I am +1 to merge if the error handling is improved -for that view. diff --git a/plips/reviews/plip10778-review-robgietema.txt b/plips/reviews/plip10778-review-robgietema.txt deleted file mode 100644 index 7f41a0f3da..0000000000 --- a/plips/reviews/plip10778-review-robgietema.txt +++ /dev/null @@ -1,55 +0,0 @@ -PLIP 10778: Standalone UUID implementation -========================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10778 - -Review #1 by Rob Gietema (rob@fourdigits.nl, Rob|4D on irc) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Firefox 3.6.10. - - -Review steps ------------- - -- Run buildout using the plip9938-output-filters.cfg file. - -- Run tests for plone.uuid. - -- Visual review of the code in plone.uuid. - -- Run tests for plone.app.uuid. - -- Visual review of the code in plone.app.uuid. - -- Started instance, created site. - - -Notes and observations ----------------------- - -- All 5 plone.uuid tests pass. Test coverage is close to 100%. - -- Code in plone.uuid looks sane. - -- Although not a requirement it is best practice to use PEP8 and the ZCML style - guide (http://www.python.org/dev/peps/pep-0008/, - http://wiki.zope.org/zope3/ZCMLStyleGuide) - -- All 7 plone.app.uuid tests pass. Test coverage is above 90%. - -- The plone.app.uuid i18n domain is used, this should be just plone. - -- Code in plone.app.uuid looks sane. - -- If I understand the comments on the plip correctly no new catalog index - should have been created. I do see a catalog.xml file in plone.app.uuid, is - this needed? - -- The test code is in testing.py and tests.py, I prefer this being in a tests - folder to not mess up the package root. - - -Conclusion ----------- - -Looking very good, some minor fixes and I'm +1 for merging. diff --git a/plips/reviews/plip10809-review-eleddy.txt b/plips/reviews/plip10809-review-eleddy.txt deleted file mode 100644 index fb614db9d9..0000000000 --- a/plips/reviews/plip10809-review-eleddy.txt +++ /dev/null @@ -1,39 +0,0 @@ -PLIP 10809: Table sort internationalization -=========================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10809 - -Review #1 by Elizabeth Leddy (eleddy) - -The PLIP was reviewed on Mac OS X 10.6.5 using python 2.6.6 and Chrome 7.02. - - -Review steps ------------- - - Review code - - run through jslint - - Create fresh plone site, test sorting on different dates without the sortable name - code - - Test sorting with the sortable name and various values - -Notes and observations ----------------------- - - not sure that this is related to this plip but when looking at a sortable table I - get a few viewport argument not recognized errors from this line in the html: - - - JS doesn't pass through new jslint standard. It's not a requirement for merging - but definitely nice to have. - - Added a table with a column of dates and sorting works on the first two clicks - but stops working afterwords. The test data set is: '2010-12-22', '2010-09-14', - '2010-06-11', '2010-01-09'. Again, first two sorts work then unresponsive. - - Code looks sane - - Author said there were issues with unittests but maybe integrating with - test_ecmascripts.pt is the best option here? - -Conclusion ----------- -This is a small plip and nothing seems to have been broken. I would have actually -like to see this plip go further and make some sorting work by default (e.g. -dates formatted with a '/'). I would like to see the code in the plip ticket -integrated into test_ecmascripts.pt at minimum before merging but other than -that, +1 to merge. diff --git a/plips/reviews/plip10809-review-robgietema.txt b/plips/reviews/plip10809-review-robgietema.txt deleted file mode 100644 index 356dcb95a4..0000000000 --- a/plips/reviews/plip10809-review-robgietema.txt +++ /dev/null @@ -1,47 +0,0 @@ -PLIP 10809: Table sort internationalization -=========================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10809 - -Review #1 by Rob Gietema (rob@fourdigits.nl, Rob|4D on irc) - -The PLIP was reviewed on Mac OS X 10.6.5 using python 2.6.6 and Firefox 3.6.13. - - -Review steps ------------- - -- Run buildout using the plip10809-improve-tablesorter.cfg file. - -- Run tests for Plone. - -- Run the js files through jslint - -- Review the changes in table_sorter.js - -- Start instance, create site, test the table sorter - - -Notes and observations ----------------------- - -- 2 tests fail in the Plone package but these don't seem related to the changes - made for this plip. - -- The table_sorter.js file doesn't pass the jslint tests. Officially this is - not part of the plip but it would be nice to fix this before merging with - plip 10901. - -- The changes in table_sorter.js all look sane. I know the current - implementation doesn't have unittests but I would really like to see js - unittests for new features which are added to Plone. - -- I've tested the table sorting in several scenario's and it was working like - it should. - - -Conclusion ----------- - -Looking very good, if some unittests are added and the jslintification is -discussed with the author of plip 10901 I'm +1 for merging. diff --git a/plips/reviews/plip10846-review-cah190.txt b/plips/reviews/plip10846-review-cah190.txt deleted file mode 100644 index fe8ba15139..0000000000 --- a/plips/reviews/plip10846-review-cah190.txt +++ /dev/null @@ -1,51 +0,0 @@ -PLIP 10846: Include plone.testing and plone.app.testing in KGS -============================================================== - -PLIP ticket: http://dev.plone.org/plone/ticket/10846 - -Review by Craig Haynal (cah190@psu.edu, cah190 on irc) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Firefox 3.6.10. - - -Review steps ------------- - -- Ran buildout using the plip9472-9473-registry-z3cform.cfg file. - -- Ran tests for plone.testing and plone.app.testing. - -- Inspected code in plone.testing and plone.app.testing. - -- Ran bin/alltests. - -- Created and examined a new Plone site to look for any obvious issues. - - -Notes and observations ----------------------- - -- The PLIP buildout also includes the implementation for PLIP 9472 and 9473. - -- The 6 tests in plone.testing have good coverage. - -- There are 3 XXX markers in plone.testing which are relevant if - we move to Zope 2.13. - -- The code in plone.testing looks reasonable. - -- 2 of the 3 tests in plone.app.testing are failing as of 27-Sep-2010. - The test coverage is acceptible for the code. - -- There is one XXX marker in plone.app.testing that should be addressed, - if necessary. - -- Plone behaves normally. - - -Conclusion ----------- - -The failing tests and the XXX marker in plone.app.testing need to be -addressed. The code looks solid, so I will be +1 for inclusion when -the tests are passing. diff --git a/plips/reviews/plip10846-review-robgietema.txt b/plips/reviews/plip10846-review-robgietema.txt deleted file mode 100644 index c9a0f61144..0000000000 --- a/plips/reviews/plip10846-review-robgietema.txt +++ /dev/null @@ -1,46 +0,0 @@ -PLIP 10846: Include plone.testing and plone.app.testing in KGS -============================================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10846 - -Review #1 by Rob Gietema (rob@fourdigits.nl, Rob|4D on irc) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Firefox 3.6.10. - - -Review steps ------------- - -- Run buildout using the plip9472-9473-registry-z3cform.cfg file. - -- Run tests for plone.testing. - -- Visual review of the code in plone.testing. - -- Run tests for plone.app.testing. - -- Visual review of the code in plone.app.testing. - - -Notes and observations ----------------------- - -- All 6 plone.testing tests pass. Test coverage is above 90%. - -- Code in plone.testing looks sane. - -- Although not a requirement it is best practice to use PEP8 and the ZCML style - guide (http://www.python.org/dev/peps/pep-0008/, - http://wiki.zope.org/zope3/ZCMLStyleGuide) - -- 2 out of 3 tests fail, this needs to be fixed. - -- Code in plone.app.testing looks sane. - -- plone.app.testing contains one XXX statement not sure if this is still valid. - - -Conclusion ----------- - -Looking very good, if the tests will be fixed I'm +1 for merging. diff --git a/plips/reviews/plip10877-review-alecm.txt b/plips/reviews/plip10877-review-alecm.txt deleted file mode 100644 index 5392c76219..0000000000 --- a/plips/reviews/plip10877-review-alecm.txt +++ /dev/null @@ -1,49 +0,0 @@ -PLIP 10877: Separate Products.CMFPlone from the Plone egg and its optional dependencies -======================================================================================= - -PLIP ticket: https://dev.plone.org/plone/ticket/10877 - -Review #1 by Alec Mitchell (alecpm@gmail.com, alecm on irc) - -The PLIP was reviewed on Mac OS X 10.5.8 using python 2.6.5 and Google -Chrome 6.0.472.53. - - -Review steps ------------- - -- Ran buildout using the plip10877-separate-products-cmfplone.cfg file. - -- Reviewed the code changes in the Plone package and the new - Products.CMFPlone package - -- Checked that svn history was preserved on the new branches. - -- Ran bin/alltests - -- Created and browsed a new Plone site to look for any obvious issues. - - -Notes and observations ----------------------- - -* Changes are minimal beyond the svn structure changes. - -* Test results are no different than the Plone 4.1 base tests. - -* The new svn structure seems reasonable and the history has been - preserved as expected. - -* The PLIP buildout config has an explicit PIL egg dependency, which - doesn't appear to exist in the standard buildout. I find this - curious. - - -Conclusion ----------- - -These changes are quite simple, and provide some very useful -benefits for deployments of Plone-based applications. I recommend -merging this work as soon as possible to minimize additional merge -work for subsequent PLIPs which may require changes to the -Plone/Products.CMFPlone package. diff --git a/plips/reviews/plip10877-review-robgietema.txt b/plips/reviews/plip10877-review-robgietema.txt deleted file mode 100644 index f9d5ba03bd..0000000000 --- a/plips/reviews/plip10877-review-robgietema.txt +++ /dev/null @@ -1,40 +0,0 @@ -PLIP 10877: Separate Products.CMFPlone from the Plone egg and its optional dependencies -======================================================================================= - -PLIP ticket: https://dev.plone.org/plone/ticket/10877 - -Review #2 by Rob Gietema (rob@fourdigits.nl, Rob|4D on irc) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Firefox 3.6.10. - - -Review steps ------------- - -- Run buildout using the plip10877-separate-products-cmfplone.cfg file. - -- Run tests for Products.CMFPlone. - -- Visual review of the code in Products.CMFPlone and Plone. - -- Check svn history. - -- Started instance, created site. - - -Notes and observations ----------------------- - -- The same tests as before the split passed. - -- If I understand this plip correctly the following file - https://svn.plone.org/svn/plone/Plone/branches/plip10877-plone/setup.py - should contain the optional packages and it doesn't. - -- The code is branched correctly keeping history intact. - - -Conclusion ----------- - -It doesn't seem the desired functionality is implemented. diff --git a/plips/reviews/plip10878-review-alecm.txt b/plips/reviews/plip10878-review-alecm.txt deleted file mode 100644 index c6f9765e6c..0000000000 --- a/plips/reviews/plip10878-review-alecm.txt +++ /dev/null @@ -1,61 +0,0 @@ -PLIP 10776: Include Zope 2.13 -============================================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10878 - -Review #3 by Alec Mitchell (alecm) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Chrome 7.0.517.41 - - -Review steps ------------- - -- Ran buildout using the plip10878-siteadmin-role.cfg file - -- Reviewed code changes in all affected packages, including - GenericSetup config and migrations. - -- Ran all tests - -- Created new plone site, added site admin user. Tested various - administrative actions as site admin user. - -- Verified that site admin could not escalate privileges. - - -Notes and observations ----------------------- -- Some of the permissions that should be assigned to the SiteAdmin - role do not appear to be assigned in the ZMI. This seems to be - because add-ons regiser their permission/role mappings using - CMFCore's setDefaultRoles and the zcml permission/role declarations, - which only applies to the Zope root (where the role is not yet - defined). This is true for the permissions from Products.TinyMCE, - Products.CMFEditions, plone.app.workflow, plone.portlet.collection - and plone.portlet.static. It seems that this is a purely cosmetic - problem though, because the SiteAdmin can access the functionality - restricted by these permissions, even if they don't appear to be - assigned in the ZMI. -- The migration code looks appropriate, though like all significant - migrations it will probably need some extensive real world testing. -- I question the decision to exclude local workflow configuration - policy from the SiteAdmin role. It seems to me like a pretty - typical - if advanced - SiteAdmin task. -- I saw something similar to the issue eleddy saw with role - assignments not appearing. Specifically, if I log in as a SiteAdmin - and add a user, the group assignments made during user creation do - not seem to apply. Later group and role assignments work fine, as - do group assignments performed as Manager during user add. -- The code to prevent privilege escalation works and appears well - thought out. - - -Conclusion ----------- -This implementation would be worth including for the increase in -permission granularity alone. Everything seems to be solidly -implemented, aside from some relatively trivial issues. I suspect -we'll need to make sure there's some very clear documentation to -assist people in e.g. updating custom workflows to support the new -role. I strongly support including this PLIP in 4.1. diff --git a/plips/reviews/plip10878-review-cah190.txt b/plips/reviews/plip10878-review-cah190.txt deleted file mode 100644 index 7875685fa4..0000000000 --- a/plips/reviews/plip10878-review-cah190.txt +++ /dev/null @@ -1,71 +0,0 @@ -PLIP 10878: Add "SiteAdmin" role -================================ - -PLIP ticket: http://dev.plone.org/plone/ticket/10878 - -Review by Craig Haynal (cah190@psu.edu, cah190 on irc) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Firefox 3.6.10. - - -Review steps ------------- - -- Ran the plip10878-siteadmin-role.cfg buildout. - -- Ran bin/alltests. - -- Created and examined a new Plone site to look for any obvious issues. - -- Created a user with the SiteAdmin role and tested administrative activities. - - -Notes and observations ----------------------- - -- This PLIP uses Zope 2.13 as per PLIP 10776. - -- Running alltests yields many failures above and beyond those from the - Zope 2.13 PLIP. There seem to be a number of errors regarding the - Access Transient Object permission: - - "ValueError: The permission Access Transient Objects is invalid." - - Currently there are 23 failures testing this PLIP versus 3 failures - testing the Zope 2.13 PLIP. - -- Despite the test failures, Zope starts up and behaves normally. - -- The configlets available to the SiteAdmin user largely behaved as expected. - -- When trying to add a new user as a SiteAdmin, the group assignment-at- - creation functionality does not appear to work. Checking boxes to assign - other users the SiteAdmin group or the Reviewers group had no effect on - user creation, i.e. the users were created without the selected groups. - The groups could still be assigned by editing the user or the group. - Manager-level users could create users and assign groups in the user - creation dialog, as expected. - -- Managing portlets as a SiteAdmin worked as expected. - -- Could not use the ZMI as SiteAdmin, as expected. - -- The SiteAdmin could not manage viewlets or use the ownership_form. - Managing viewlets is somewhat dangerous so that seems to be a sensible - restriction, but is there any reason we shouldn't allow SiteAdmins to - use the ownership_form? - -- Code changes seem sensible, though there is currently a breakpoint - in a test of the checked-in branch of plone.app.upgrades: - - https://svn.plone.org/svn/plone/plone.app.upgrade/branches/plip10878-siteadmin-role/plone/app/upgrade/v41/tests.py - -- Permissions at the Plone Site level look reasonable. - - -Conclusion ----------- - -I am satisfied with the functionality and implementation of this PLIP. -The failing tests will need to be addressed and the issues with the -Zope 2.13 PLIP will need to be resolved before this is ready for merge. diff --git a/plips/reviews/plip10878-review-eleddy.txt b/plips/reviews/plip10878-review-eleddy.txt deleted file mode 100644 index cec1f43df9..0000000000 --- a/plips/reviews/plip10878-review-eleddy.txt +++ /dev/null @@ -1,66 +0,0 @@ -PLIP 10776: Include Zope 2.13 -============================================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10878 - -Review #2 by Elizabeth Leddy (eleddy) - -The PLIP was reviewed on Mac OS X 10.6.4 using python 2.6.6 and Chrome 6.0.4 - - -Review steps ------------- - -- Ran buildout using the plip10878-siteadmin-role.cfg file - -- Ran all tests - -- Ran upgrade on an existing site - -- Created a new site, added users for admin role and validated that every menu - in the control panel edited and saved without error. Did the same for each non- - site admin user to confirm they couldn't access. - -- Reviewed portal workflows - verified that each state that had a manager permission - also had a site admin permission - -- Added users to admin group and verified groups work the same as direct role assignment - -Notes and observations ----------------------- -- Upgrade failed on both existing sites from configuration 4100 to 4015. No - obvious errors - not sure what happened. I am happy to help debug this. All I did - was create a site in the 4.1 branch, zope 2.12, add a site, then run the plip - buildout and restart. None of my sites successfully upgraded. -- WRT the upgrade I don't agree with automatically reindexing the allowedRolesAndUsers - index. I am open to discuss this but this would kill large sites. I don't know how - other people manage this but I know I have custom lazy workflow upgrading hacks in - several large projects. Is there a way to make this optional or to make a decision - based on the number of objects for example? This is especially important for projects - that may not need or see the benefit of SiteAdmin and won't upgrade because of it. -- SiteAdmin can edit portlets, but cannot manage viewlets. Is this by design? I don't - want to get into a bikeshed argument - just confirming. -- The control panel at 'plone_control_panel' lists two "Users and Groups", one pointing - to @@usergroup-userprefs and the other to @@usergroup-groupprefs. I don't know if this - is a result of this plip or not but it is very confusing since they have the same icon - and title. -- In the Users Overview control panel, clicked 'add new user' and added another user to the - Site Adminstrators group. On reload, the user was added but their inherited role was not - displayed (it was not checked, and the plone symbol wasn't there either). So I investigated - and it looks like the groups are not getting assigned for reviewers or site admins. - Clicking on the user and adding to the group manually works as expected. -- Now this is a bikeshed argument but the version of this role that everyone sees is - "SiteAdmin" (no space), which is small but reminds me of when plone roles we equivalent to - going to your own beheading. Any way we can put a space in here, or, restrict it to one - word so it feels refined? This is for sure small and I assume there is something - ridiculous to make this small change so I won't argue too hard for it. -- With the new site (the non upgraded is fine) there is an error trying to register as a - new user (enable self registration on) that could be from redoing forms: ComponentLookupError: ((, ), , u'') - - -Conclusion ----------- -There are a few bugs but mostly superficial. I wouldn't release without the couple of -fixes (especially the registration and migration) but I think its good to incorporate -the changes for the rest of the plipping. In general everything looks nice and works -as expected. The role will be a welcome addition. \ No newline at end of file diff --git a/plips/reviews/plip10886-review-cah190.txt b/plips/reviews/plip10886-review-cah190.txt deleted file mode 100644 index baf42457ca..0000000000 --- a/plips/reviews/plip10886-review-cah190.txt +++ /dev/null @@ -1,118 +0,0 @@ -PLIP 10886: plone.app.event - new eventtype for plone -===================================================== - -PLIP ticket: http://dev.plone.org/plone/ticket/10886 - -Review by Craig Haynal (cah190@psu.edu, cah190 on irc) - -The PLIP was reviewed on Mac OS X 10.6.5 using python 2.6.6 and Camino -2.0.6. - - -Review steps ------------- - -- Followed the instructions in plip10886-event-improvements.txt to run -the plip10886-event-improvements.cfg buildout. - -- Ran tests for plone.app.event, plone.event, -plone.formwidget.recurrence, plone.app.jquerytools, plone.app.portlets, -Products.DateRecurringIndex, Products.ATContentTypes, and -archetypes.datetimewidget, Products.CMFPlone. - -- Created and examined a new Plone site to look for any obvious issues. - -- Examined code. - - -Notes and observations ----------------------- - -- A number of the products used in this PLIP require git; that will add -one more required tool to check out these products for development. -Also, it appears that a github account is required to commit changes, -which adds another place developers will need to request commit access. - -- plone.app.event has good test coverage. - -- plone.event has a test failure in test testEventExporterFeed -(plone.event.tests.test_ical.ICalExportTests). - -- plone.app.jquerytools has a failing test in test -/Volumes/Data/Users/cah190/src/plip10886/buildouts/src/plone.app. -jquerytools/plone/app/jquerytools/tests/ploneIntegration.txt - -- Products.DateRecurringIndex has a failure in test -/Volumes/Data/Users/cah190/src/plip10886/buildouts/src/Products. -DateRecurringIndex/src/Products/DateRecurringIndex/index_timedelta.txt - -- Products.CMFPlone has 2 failures and 2 errors, but they appear -unrelated. - -- When adding or editing an Event object, entering any invalid data in -the date fields results in a traceback. - -- The start/end date widget is awkward, requiring all fields other than -month to be entered numerically (month uses a drop-down listbox). I -would rather see either all numerical entry fields, which may be -confusing given the various local date component ordering standards, or -all drop-down listboxes as in older version of Plone. - -- Single-digit values in times appear in the edit form as single digits -rather than as the more typical zero-left-padded values, e.g. 8:08 -appears as 8:8 on the form. - -- Recurrences do work when entered in an appropriate form, but entering -an invalid recurrence results in a traceback. - -- There is no recurrence widget beyond a simple textarea at present. I -would at least like to see some directions and/or a link to directions -on how to define a recurrence if we are to even consider shipping -without the widget. - -- Events in progress do not display in the Upcoming Events portlet (as -Plone 4.0.2 events do) e.g. an event that started 5 minutes ago and runs -for an hour does not appear in the portlet. - -- vCal export looks reasonable. - -- iCal export includes a line with "None" which may lead to failures -importing the iCal file. Is some empty or non-existent field bleeding -through as "None"? - -- Only the initial occurance of the event shows on the Events and -Calendar portlets; I would have expected to see as many as will fit in -the portlets. - -- I wasn't clear on how the timezone support should operate, as events -don't indicate which timezone they are scheduled in and the edit form -does not appear to reflect the current timezone. - -- There are a number of TODOs in the code. - -- plone.app.event is redefining the start and end indices in the -portal_catalog to use DateRecurringIndex instead of DateIndex. I worry -about compatibility with other products which rely on the start and end -indices, though DateRecurringIndex does claim to be a drop-in -replacement. It seems like there should be more tests to ensure -compatibility with DateIndex is preserved. - -- Overall, the code looks reasonable and most of the new components have -good tests. - - -Conclusion ----------- - -The new Event edit form needs to do server-side validation, though it -would also be good to have client-side validation. The iCal export -should be examined in more detail for the source of None. The Calendar -portlet should be updated to show recurring events. Test failures -should be addressed. We should have the UI team look at the date/time -entry fields. I'm also a bit concerned about the changes to the start -and end indices. - -This looks promising, but there are a number of shortcomings that should -be addressed before this PLIP is merged. I am -1 for merging right now, -but if the aforementioned issues are worked out I will happily change my -vote. diff --git a/plips/reviews/plip10886-review-eleddy.txt b/plips/reviews/plip10886-review-eleddy.txt deleted file mode 100644 index 436e7bb283..0000000000 --- a/plips/reviews/plip10886-review-eleddy.txt +++ /dev/null @@ -1,120 +0,0 @@ -PLIP 10866:Event type impreovements -======================================================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10866 - -Review #1 by Elizabeth Leddy (eleddy) - -The PLIP was reviewed on Mac OS X 10.6.5 using python 2.6.6, Chrome 7.0.517.44, -iCal for mac - -Review steps ------------- -- Run all tests -- Review Code -- Test all sorts of things in browser - -Notes and observations ----------------------- - - In plone.event one test case is failing: testEventExporterFeed - - In Products.DateRecurringIndex one test case is failing: index_timedelta.txt - - Need to have a migration step. I am slightly concerned that the directions require a new - Plone site to even test. - -Note: I realize that some of these comments may not be directly related to this plip but -please forgive me that its hard to separate all of these packages out. - - - Testing adding an event: - Adding a date out of range, e.g. 30 Feb or 60 June for a start or end date throws - AttributeError: 'str' object has no attribute 'day'. This could be resolved with - javascript validation but I am curious the motivation to switch from a drop down to a - text based field. In my experience using web applications, the end user expects a - drop down in this case. The same issue for the time fields. Anyone can enter any data - in these text fields and it causes a stack trace. The already reported bug was for end - date but all text fields have this error. - - Same error as above if the end date is after the start date - - This is a small detail but in the time fields if something is 0 hours or 0 minutes I think - it would be nice to have 2 zeros - 00:00. Of course if it moves to a drop down then not - need. This is also nice for when create a new event at say 18:03. 18:3 looks weird. - - Also a small detail, but it would be nice if when editing an event, the calendar - automatically moves to the month and year that the existing date is instead of todays month - and year. This is a nice to have but makes a world of difference to the end user. - - When downloading the event in its vcalendar version, the extension is .vcs.ics . I am not - 100% sure but I think the extension should just be .vcs. I don't see this in the code and - wonder if it's something the browser is doing. - - There seems to be an issue with an empty field and exporting to ical. Not sure where this - is coming from in the export: - ... - SUMMARY:Scrib Scrab - DTSTART:20110627T070000Z - DTEND:20111107T075900Z -------> None - DESCRIPTION:dfgdfgdfg\n - LOCATION:HOME - CLASS:PUBLIC - ... - - - Javascript does not take into account the new javascript standards. Would be nice to - integrate with those in mind. Does not pass jslint basics test. - - Javascript calendar needs some way to be manually closed besides clicking outside of - the box (an X type thing) - - The Javascript calendar is wrong. I think its just the labels across the top. They should - start with SUNDAY in the current rendering instead of Monday. Mine right now, for example, - states that Nov 29th is on a Tuesday. - - If we are going to push without the recurrence stuff completed lets hide the recurrence - tab in the meanwhile. Since this is not complete I did not thoroughly test anything - here. - - For testing (and end user sanity) it would be nice to - display the timezone things are occurring in every time the event is listed. I tried - adjusting the localized time string in portal_properties > site_properties but it doesn't - seem to be using localized time strings. - - In the same properties sheet calendar_starting_year and calendar_future_years_available - seem to be ignored. I know that I have several products that use calendar_starting_year - (for birth dates for example) although I'm not so convinced on future years. I think it - also depends on if this widget will be the "NEW and IMPROVED" datewidget for archetypes. - If this is the case we need to honor these variables, or remove them. The additional case - for this is when looking at the modified time vs the actual time. When a server is - running in one tz and a users browser sets the widget for another (which is nice btw), - without a timestamp things could appear like they travelled through time to be created. - Additionally its whack confusing when people create things from different TZ's. - - When listing the future events by default, events for today are not included. This may - mean tweaking the default criteria but I'm not sure exactly what the implementation - is behind the scenes. I would even argue that events from earlier in the same day should - be included. I know this doesn't make programmatic sense but it makes sense to the end - user. Whole day events STARTING TODAY (even if the end in 10 days)are not listed either so - there is definitely something worth investigating here. - - Adding a whole day event that starts on the 28th and ends on the 30th of november does not - return when clicking on "30" in the calendar portlet. The generated url for debugging if it - helps: http://localhost:8080/Plone/search?review_state=published&start.query:record:list:date=2010-11-30+23:59:59&start.range:record=max&end.query:record:list:date=2010-11-30+00:00:00&end.range:record=min - - If I create an event in one timezone, and then edit it in another, it does not adjust for my - timezone. I don't know how edge case this is but I definitely have a site with a use case - for it (teams of schedulers). For example, if I create an event in 9:00 PM EST then reopen - it in a browser in California the time should be 6:00 PM PST. I know this might be on a long - list of nice to haves but since you are adjusting for end user time zone when creating it - would be nice to tackle this as well. Clever approach btw. - -Conclusion ----------- -I am on the edge for this plip. This overhaul is beyond overdue but there are a lot of bugs here -that I actually pulled most of from my experience manhandling tickets from the old events module. -Users do this type of shit all the time. - -I don't think there is any need to require recurrence for integrating this plip but I am nervous -to change indexes and get all funky with it when the final implementation isn't tested. I also -get nervous that the final implementation of the recurrence widget COULD affect the code in -the back end. - -Overall the architecture is well thought through and the core of the code looks good. I can't -wait to get some of this timezone stuff worked out and into some of my own code. - -I'm open to discussion on this plip. I would say +1 to merge if the following were fixed: - - Must have a migration path - - Calendar widget must display correctly - - Events searching must be fixed - - Ability to display timezone of events - - Address all failing test cases - - All validation situations where attribute error is thrown addressed - -This is a decent amount of work and I would be happy to wait until 4.2 for this as well if it -included the recurrence widget completion and some more testing. The old events module isn't -bad enough to force something that needs a little bit more time to complete. diff --git a/plips/reviews/plip10901-review-eleddy.txt b/plips/reviews/plip10901-review-eleddy.txt deleted file mode 100644 index 5c885c2551..0000000000 --- a/plips/reviews/plip10901-review-eleddy.txt +++ /dev/null @@ -1,77 +0,0 @@ -PLIP 10901: Set and enforce base coding standards for our own JavaScript -======================================================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10901 - -Review #2 by Elizabeth Leddy (eleddy) - -The PLIP was reviewed on Mac OS X 10.6.5 using python 2.6.6 and Chrome 7.0.517.44 - - -Review steps ------------- -- Review documentation -- Run tests for Plone. -- Run all the js files in ecmascript through jslint -- Validated test_ecmascripts.pt - -Notes and observations ----------------------- - - The following do not validate with the given standards in hs-standards.js: - - form_tabbing.js - - formsubmithelpers.js - - input-label.js doesn't validate to the js-standards.js and sets its own - validation items. - - jquery.highlightsearchterms.js - same issue as input-label.js. - - livesearch.js - same issue as input-label.js. - - table_sorter.js - - testBeforeUnload.js - - testNodeutilities.js - - toc.js - - unittestUtilities.js (not sure if this needs to validate) - - unlockOnFormUnload.js - same issue as input-label.js - - Several js files don't validate to the standards.js but set their own validation - schemes. It's worth considering setting nomen:false in the standard for now - with the anticipation of setting it to true at some point in the future. - - 3 test cases are failing in test_ecmascripts.pt - - PloneBeforeUnloadTestCase.testSelectOne, exception TypeError: Cannot call method 'indexOf' of undefined - - PloneBeforeUnloadTestCase.testIdOverride, exception TypeError: Cannot call method 'indexOf' of undefined - - PloneBeforeUnloadTestCase.testNoUnloadProtection, exception TypeError: Cannot call method 'indexOf' of undefined - -Developer Manual Notes ----------------------- -(http://plone.org/documentation/manual/developer-manual/client-side-functionality-javascript/referencemanual-all-pages) - - The section on including javascript seems incomplete. Not sure what is - happening there - - In section 2, jQuery Tools, there is mention that masking should be - avoided but no reference to why. I think the why part of this should be - elaborated. - - Since this is the developer manual, I'd like to see links to examples of - using tabs, tooltips, overlays, and masking in plone (or at least to docs - explaining how they are integrated) - - In section 4 on the standards, I would like to see the tone be a little less - wishy washy and a little more definitive. e.g. instead of "JavaScript should - nearly never be present" just saying "JavaScript should never". - - I really like the progressive enhancement section, however it is still unclear - how to handle #10794 for example. A section on whether or not plone - still cares about graceful degradation would be appreciated so that devs can - have a clear stance on bugs like that. - - On my wish list is also a section on delegates and/or performance considerations. - The more we start to chow down on javascript the more important these will become. - - The section on ajax and errors refers to reverse progressive enhancement to - handle errors, which I personally disagree with as the community recommendation. - There are many elegant ways to handle errors, and in this care I actually support - trying to define recipes for handling common error situations. For example, - validation errors should always do X, 404's on form submits should be handled - with Y, etc. I'd be happy to discuss this in further detail. - - It would be nice to see a section on developing javascript in third party products. - People are going to be unhappy if they install the latest and greatest product X - and then it makes basic functionality not work. - -Conclusion ----------- -This PLIP is long overdo and I expect it to be a work in progress for a while here. - - -While I don't consider this ticket finished, I think the current changes can be merged -as soon as the 3 failing js test cases are addressed. \ No newline at end of file diff --git a/plips/reviews/plip10901-review-robgietema.txt b/plips/reviews/plip10901-review-robgietema.txt deleted file mode 100644 index 7dadbb13d1..0000000000 --- a/plips/reviews/plip10901-review-robgietema.txt +++ /dev/null @@ -1,63 +0,0 @@ -PLIP 10901: Set and enforce base coding standards for our own JavaScript -======================================================================== - -PLIP ticket: https://dev.plone.org/plone/ticket/10901 - -Review #1 by Rob Gietema (rob@fourdigits.nl, Rob|4D on irc) - -The PLIP was reviewed on Mac OS X 10.6.5 using python 2.6.6 and Firefox 3.6.13. - - -Review steps ------------- - -- Run buildout using the plip10901-javascript-standards.cfg file. - -- Run tests for Plone. - -- Review js-standards.js - -- Run all the js files in ecmascript through jslint - -- Visual review of the code in the js files - -- Start instance, create site, browser/edit content - - -Notes and observations ----------------------- - -- 2 tests fail in the Plone package but these don't seem related to the changes - made for this plip. - -- Checked all the options which are enabled by default. Differences with the - settings I'm using for Deco are: - - strict: I have this enabled but this isn't needed for Plone - - white: I have this enabled and personally like to have strict whitespace - but I can imagine it is a lot of work to fix this. If this can be done - easily then please do, if it can't then we might handle this with a next - 4.x release. - - maxlen: I have this set to 80 to prevent long lines. This is the same as - the previous issue; nice to have but not strictly needed - - maxerr: I set this to a high number (9999) so you get all the errors when - debugging and not just 50, this can be added. - -- I ran all the js files in the ecmascript folder through jslint and the - following didn't pass, this needs to be fixed: - - form_tabbing.js - - fromsubmithelpers.js - - table_sorter.js - - testBeforeUnload.js - - testNodeutilities.js - - toc.js - - unittestUtilities.js - - unlockOnFormUnload.js - -- Editing and browsing content works as expected. - - -Conclusion ----------- - -Looking very good, if the tests will be fixed I'm +1 for merging. The "nice to -have" features can also be implemented if there is extra time. diff --git a/plips/reviews/plip10902-review-cah190.txt b/plips/reviews/plip10902-review-cah190.txt deleted file mode 100644 index 016836b281..0000000000 --- a/plips/reviews/plip10902-review-cah190.txt +++ /dev/null @@ -1,74 +0,0 @@ -PLIP 10902: New collections -=========================== - -PLIP ticket: http://dev.plone.org/plone/ticket/10902 - -Review by Craig Haynal (cah190@psu.edu, cah190 on irc) - -The PLIP was reviewed on Mac OS X 10.6.8 using python 2.6.6 and Safari -5.0.5. - - -Review steps ------------- - -- Ran the plip10902-new-collections.cfg - -- Ran tests for plone.app.contentlisting, plone.app.collection, -plone.app.querystring, and archetypes.querywidget. - -- Created and examined a new Plone site to look for any issues. - -- Examined code. - - -Notes and observations ----------------------- - -- All tests in plone.app.contentlisting pass. - -- 5 errors in the plone.app.collection tests: test_getFoldersAndImages, -test_limitNumber, test_searchResults, test_viewingCollection, -testPortlet. - -- 5 errors in the plone.app.querystring tests: testGettingConfiguration, -testQueryBuilderHTML, testQueryBuilderNumberOfResults, -testQueryBuilderNumberOfResultsView, testQueryBuilderQuery - -- archetypes.querywidget has 1 failure in doctest integration.txt - -- Attempting to install plone.app.collection results in: - - (innermost last): - - * Module ZPublisher.Publish, line 126, in publish - * Module ZPublisher.mapply, line 77, in mapply - * Module ZPublisher.Publish, line 46, in call_object - * Module Products.CMFQuickInstallerTool.QuickInstallerTool, line 575, in installProducts - * Module Products.CMFQuickInstallerTool.QuickInstallerTool, line 512, in installProduct - __traceback_info__: ('plone.app.collection',) - * Module Products.GenericSetup.tool, line 323, in runAllImportStepsFromProfile - __traceback_info__: profile-plone.app.collection:default - * Module Products.GenericSetup.tool, line 1084, in _runImportStepsFromContext - * Module Products.GenericSetup.tool, line 998, in _doRunImportStep - __traceback_info__: typeinfo - * Module Products.CMFCore.exportimport.typeinfo, line 224, in importTypesTool - * Module Products.GenericSetup.utils, line 831, in importObjects - __traceback_info__: portal_types - * Module Products.GenericSetup.utils, line 507, in _importBody - * Module Products.CMFCore.exportimport.typeinfo, line 209, in _importNode - * Module Products.GenericSetup.utils, line 565, in _initObjects - __traceback_info__: ('Collection', 'Factory-based Type Information with dynamic views') - * Module OFS.ObjectManager, line 323, in _setObject - * Module OFS.ObjectManager, line 119, in checkValidId - -BadRequest: The id "Collection" is reserved. - -- Unable to test new collections through the Plone. - - -Conclusion ----------- - -This PLIP seems to be broken at the moment. The test errors and failures will need to be addressed -and it must be installable (or installed by default) to be usable. Not ready for merging. diff --git a/plips/reviews/plip10902-review-eleddy-2nd-pass.txt b/plips/reviews/plip10902-review-eleddy-2nd-pass.txt deleted file mode 100644 index 467c83976b..0000000000 --- a/plips/reviews/plip10902-review-eleddy-2nd-pass.txt +++ /dev/null @@ -1,75 +0,0 @@ -PLIP 10902: New collections -=========================== -PLIP ticket: http://dev.plone.org/plone/ticket/10902 - -Review by Elizabeth Leddy (elizabeth.leddy@gmail.com, eleddy on irc) - -The PLIP was reviewed on Mac OSX using python 2.6.5 and Google Chrome - -Review steps ------------- -- New site, new collections and items. This is my second review so I'm - going to stick to the interaction this time. - -Notes and observations ----------------------- -- Update of search criteria is nice. Much better. -- Is there a migration strategy for current collections? I'm not sold that it - needs to be required but I can see why this may be a question for a lot of - people. - -Must have changes ------------------ -- Description of content in preview should have class="description" and/or - documentDescription so that it can be consistently formatted. Bonus if - the default theme is updated to grey this out. -- "X items matching your search terms" is a great idea. the wording is a bit - weird for me - maybe "X items matching this search criteria" is a little - better? I'm open for argument on this - it was weird for me. -- WOW. You guys killed the creation date validation. Almost passed all of my tests. - Almost :) Still failing on entering text, e.g. "June 23, 2011". This throws - an error in the backend. I would just make sure to catch those at minimum, - validate with javascript on the front end before sending the result for - bonus points and performance happiness. - -Borderline ----------- -- There is a TON of beautiful ajax in the search and then the edit portlet - interface is just bleh. I would roll love to see something smooth here. I know - that a lot of people don't currently use this necessarily now but they - could if it had a little wax and shine. -- Review state selection is MUCHO better. The only think that is missing here - is file/image because they have no workflow. Ugh I know. I would just add an - option to that says "Include All Files and Images" or something like that. -- Would be nice to have javascript which hides "Number of Items" until/unless - Limit Search Results is checked. Would be extra nice to move this field on - top of the preview and include this in the limited results. Other option: in - the portlet, it just says specify max number or leave blank for none - why - not adopt this same approach in the product itself. If anything, it just makes - the interfaces the same. Also, its weird that the collection has a limit and - so does the portlet. Maybe a brief note on what overrides what? - -Nice to have changes --------------------- -- There is something weird about listing the location after the result. - This isn't obvious on a default install but behind rewriting I think it's - quite awkward. I would either hide it, or better yet, allow the end user - to decide whether or not its displayed. -- IMHO, review state default should be "Published" so that things don't - disappear right away. -- Is there any way to only display "Table Columns" if the tabular view is - selected? -- Should the default portlet include the item description? my intuition - says yes. - - -Conclusion ----------- -I'm glad we waited on this update. Very nice. The 3 changes above should -be easily fixable and I don't see why we can't merge. I actually might even -use these things now :) - -I would REALLY like to see portlet get a makover, but the rest of the -changes are so good that I have no doubt that someone will be inspired to -join in and pretty up the interface. I know that if I get any cycles it's -at the top of my list. diff --git a/plips/reviews/plip10902-review-eleddy.txt b/plips/reviews/plip10902-review-eleddy.txt deleted file mode 100644 index 2a34600f8a..0000000000 --- a/plips/reviews/plip10902-review-eleddy.txt +++ /dev/null @@ -1,126 +0,0 @@ -PLIP 10902: New collections -=========================== -PLIP ticket: http://dev.plone.org/plone/ticket/10902 - -Review by Elizabeth Leddy (elizabeth.leddy@gmail.com, eleddy on irc) - -The PLIP was reviewed on Mac OSX using python 2.6.5 and Google Chrome - -Review steps ------------- -- Created a new Plone site with the plone.app.collection add-on - included. -- Worked the UI in the portal -- Reviewed code. -- run js through jslint - -Notes and observations ----------------------- -- adding a new collection, what is allowed users and roles supposed to - filter on? it doesn't seem to filter on anything -- It's not entirely clear how to close the drop down for review state. I - think that I expected to click outside of the drop down and have it - disappear (onblur). -- Once I add a criteria, then try to add another one that is the same, the - results end up being a little weird. For example, I have description and - the word "tomato" with three results. then add another description field - with no words at all and I get 21 results. I would expect these to be - an intersection, although I could be wrong since there is no indication. - I think its just a bug though. -- When using location, if I select relative path and put in a full path, I - get lots of errors in the logs: - 2010-12-17 17:54:03 ERROR Zope.SiteErrorLog 1292637243.950.54441013414 http://localhost:8080/Plone/@@querybuildernumberofresults -Traceback (innermost last): - Module ZPublisher.Publish, line 127, in publish - Module ZPublisher.mapply, line 77, in mapply - Module ZPublisher.Publish, line 47, in call_object - Module plone.app.querystring.querybuilder, line 52, in number_of_results - Module plone.app.querystring.querybuilder, line 33, in __call__ - Module plone.app.querystring.querybuilder, line 45, in _makequery - Module plone.app.querystring.queryparser, line 48, in parseFormquery - Module plone.app.querystring.queryparser, line 237, in _relativePath -AttributeError: 'NoneType' object has no attribute 'getPhysicalPath' -- For me the path wording is a little confusing. I think simply renaming - Path(URL) to absolute path would help, since its the "opposite" of relative - path -- It would be nice if the query did not submit and filter - until after the user enters something in the box. I often found myself adding - a filter and then wanting to see what I had in the live search but it was gone. - This also makes sense in cases where the filter does not apply to the objects being - displayed (e.g. event start and end date) -- Adding a modification date after date "201" triggered an error (was trying to get - to "2010"). This happened with a lot of dates: - 2010-12-17 17:59:08 ERROR Zope.SiteErrorLog 1292637548.470.755860448 http://localhost:8080/Plone/@@querybuilder_html_results -Traceback (innermost last): - Module ZPublisher.Publish, line 127, in publish - Module ZPublisher.mapply, line 77, in mapply - Module ZPublisher.Publish, line 47, in call_object - Module plone.app.querystring.querybuilder, line 38, in html_results - Module plone.app.querystring.querybuilder, line 33, in __call__ - Module plone.app.querystring.querybuilder, line 49, in _makequery - Module plone.app.contentlisting.browser, line 66, in __call__ - Module Products.CMFPlone.CatalogTool, line 325, in searchResults - Module Products.ZCatalog.ZCatalog, line 656, in searchResults - Module Products.ZCatalog.Catalog, line 737, in searchResults - Module Products.ZCatalog.Catalog, line 476, in search - Module Products.PluginIndexes.DateIndex.DateIndex, line 172, in _apply_index - Module Products.CMFPlone.patches.dateIndexPatch, line 11, in _convert - Module Products.PluginIndexes.DateIndex.DateIndex, line 247, in _convert - Module DateTime.DateTime, line 350, in __init__ - Module DateTime.DateTime, line 643, in _parse_args - Module DateTime.DateTime, line 1011, in _parse -TimeError: 201 -- Also nice to have is not to delete the contents of a box if I switch to - another criteria and then decide to come back. This makes sense if I'm - trying to decide between using title, description, or searchable text for - example -- I'm slightly concerned by the number of requests it takes to return these - results. -- More nice to have is that for items where you have no other choices for the - type of query (e.g Description "is" versus Locations rel/abs choice) I would - like to see that be text instead of a drop down. This control makes me wonder - as the end user if I should check for more options each time and the majority - of the time I don't need to. -- I don't think that sorting on title in reverse is a good default. most people - would expect it to not be reversed. If modification date were selected I would - agree but I think sortable title is the correct choice here -- What happened to display as table checkbox? If its gone then we need to remove - the table columns listing. I think this is referring to the display view but - kinda makes no sense. -- How about this "table of contents" checkbox? From what I can tell it doesn't - do anything. I'm not too familiar with collections so I could be way off. -- when adding the related to field I immediately get an error: -2010-12-17 18:19:42 ERROR Zope.SiteErrorLog 1292638782.460.265875466564 http://localhost:8080/Plone/@@querybuildernumberofresults -Traceback (innermost last): - Module ZPublisher.Publish, line 127, in publish - Module ZPublisher.mapply, line 77, in mapply - Module ZPublisher.Publish, line 47, in call_object - Module plone.app.querystring.querybuilder, line 52, in number_of_results - Module plone.app.querystring.querybuilder, line 33, in __call__ - Module plone.app.querystring.querybuilder, line 45, in _makequery - Module plone.app.querystring.queryparser, line 44, in parseFormquery -AttributeError: 'module' object has no attribute '_referenceIs' -- The same issues with * and some unicode as with the p.a.search plip. Must be able to - handle ParseError: Query contains only common words: '* . In addition, need a way to - pipe feedback to the user. -- When working with adding the collections portlet, instead of saying "search results" it - would be nice to say "available collections". In general, this portlet ui has me - wondering what I should actually be doing. what does the "parent" button mean? -- Plone.app.collection: Need to have README say something -- Plone.app.collection: TODO in collection.py -- It looks like that in order to add a collection you have to be a manager which I - think is unnecessarily limiting. At minimum let's integrate it with Site Administrator - or even Editor. I think add collection portlet sounds like something for the site - administrator role as well. -- seems to be an extra leading semicolon in querywidget.js. There are lots of errors with - that js that make me cross browser nervous. Addressing these would make me feel happy -- The query field widget could use some better documentation. - -Conclusion ----------- -This UI is really slick. This makes me actually want to use collections. A few things -that must get fixed before merge: - - must address all the bad user input issues - - work on the stack traces in all the reviews - - fix obvious js errors -Those seem all doable and I'm sure this will make a nice addition to the 4.1 release. \ No newline at end of file diff --git a/plips/reviews/plip10902-review-responses.txt b/plips/reviews/plip10902-review-responses.txt deleted file mode 100644 index 3b14d742f4..0000000000 --- a/plips/reviews/plip10902-review-responses.txt +++ /dev/null @@ -1,474 +0,0 @@ -Responses to reviews -==================== - -eleddy -====== -- adding a new collection, what is allowed users and roles supposed to - filter on? it doesn't seem to filter on anything -TODO: This filter should allow the end user to create a collection that contains documents on which user 'x' as the View permission. -Portal_catalog checks the View permission for the current user. Maybe we can use unrestrictedSearch if this filter is used. - -- It's not entirely clear how to close the drop down for review state. I - think that I expected to click outside of the drop down and have it - disappear (onblur). -Fixed in r13665 - -- Once I add a criteria, then try to add another one that is the same, the - results end up being a little weird. For example, I have description and - the word "tomato" with three results. then add another description field - with no words at all and I get 21 results. I would expect these to be - an intersection, although I could be wrong since there is no indication. - I think its just a bug though. -XXX: remove index when used, or merge criteria? - -- When using location, if I select relative path and put in a full path, I - get lots of errors in the logs: - 2010-12-17 17:54:03 ERROR Zope.SiteErrorLog 1292637243.950.54441013414 http://localhost:8080/Plone/@@querybuildernumberofresults -Traceback (innermost last): - Module ZPublisher.Publish, line 127, in publish - Module ZPublisher.mapply, line 77, in mapply - Module ZPublisher.Publish, line 47, in call_object - Module plone.app.querystring.querybuilder, line 52, in number_of_results - Module plone.app.querystring.querybuilder, line 33, in __call__ - Module plone.app.querystring.querybuilder, line 45, in _makequery - Module plone.app.querystring.queryparser, line 48, in parseFormquery - Module plone.app.querystring.queryparser, line 237, in _relativePath -AttributeError: 'NoneType' object has no attribute 'getPhysicalPath' -Don't use a full path in a relative path criteria. I've added a check to prevent that attributeerror in r50222. - -- For me the path wording is a little confusing. I think simply renaming - Path(URL) to absolute path would help, since its the "opposite" of relative - path -Fixed in r50223 - -- It would be nice if the query did not submit and filter - until after the user enters something in the box. I often found myself adding - a filter and then wanting to see what I had in the live search but it was gone. - This also makes sense in cases where the filter does not apply to the objects being - displayed (e.g. event start and end date) -TODO: would be nice. - -- Adding a modification date after date "201" triggered an error (was trying to get - to "2010"). This happened with a lot of dates: - 2010-12-17 17:59:08 ERROR Zope.SiteErrorLog 1292637548.470.755860448 http://localhost:8080/Plone/@@querybuilder_html_results -Traceback (innermost last): - Module ZPublisher.Publish, line 127, in publish - Module ZPublisher.mapply, line 77, in mapply - Module ZPublisher.Publish, line 47, in call_object - Module plone.app.querystring.querybuilder, line 38, in html_results - Module plone.app.querystring.querybuilder, line 33, in __call__ - Module plone.app.querystring.querybuilder, line 49, in _makequery - Module plone.app.contentlisting.browser, line 66, in __call__ - Module Products.CMFPlone.CatalogTool, line 325, in searchResults - Module Products.ZCatalog.ZCatalog, line 656, in searchResults - Module Products.ZCatalog.Catalog, line 737, in searchResults - Module Products.ZCatalog.Catalog, line 476, in search - Module Products.PluginIndexes.DateIndex.DateIndex, line 172, in _apply_index - Module Products.CMFPlone.patches.dateIndexPatch, line 11, in _convert - Module Products.PluginIndexes.DateIndex.DateIndex, line 247, in _convert - Module DateTime.DateTime, line 350, in __init__ - Module DateTime.DateTime, line 643, in _parse_args - Module DateTime.DateTime, line 1011, in _parse -TimeError: 201 -Added a check if preview should be updated in r13666. - -- Also nice to have is not to delete the contents of a box if I switch to - another criteria and then decide to come back. This makes sense if I'm - trying to decide between using title, description, or searchable text for - example -XXX: would be nice. - -- I'm slightly concerned by the number of requests it takes to return these - results. -That's not a real problem, right? - -- More nice to have is that for items where you have no other choices for the - type of query (e.g Description "is" versus Locations rel/abs choice) I would - like to see that be text instead of a drop down. This control makes me wonder - as the end user if I should check for more options each time and the majority - of the time I don't need to. -XXX: would be nice. - -- I don't think that sorting on title in reverse is a good default. most people - would expect it to not be reversed. If modification date were selected I would - agree but I think sortable title is the correct choice here -Fixed in r50225 - -- What happened to display as table checkbox? If its gone then we need to remove - the table columns listing. I think this is referring to the display view but - kinda makes no sense. -This functionality has moved to the 'Display' menu -> 'Tabular view' - -- How about this "table of contents" checkbox? From what I can tell it doesn't - do anything. I'm not too familiar with collections so I could be way off. -Options removed in r50226 - -- when adding the related to field I immediately get an error: - -2010-12-17 18:19:42 ERROR Zope.SiteErrorLog 1292638782.460.265875466564 http://localhost:8080/Plone/@@querybuildernumberofresults -Traceback (innermost last): - Module ZPublisher.Publish, line 127, in publish - Module ZPublisher.mapply, line 77, in mapply - Module ZPublisher.Publish, line 47, in call_object - Module plone.app.querystring.querybuilder, line 52, in number_of_results - Module plone.app.querystring.querybuilder, line 33, in __call__ - Module plone.app.querystring.querybuilder, line 45, in _makequery - Module plone.app.querystring.queryparser, line 44, in parseFormquery -AttributeError: 'module' object has no attribute '_referenceIs' -TODO: add parser for referenceIs -Removed related criteria for now in r50227. - -- The same issues with * and some unicode as with the p.a.search plip. Must be able to - handle ParseError: Query contains only common words: '* . In addition, need a way to - pipe feedback to the user. -I can search just fine for 'འབྲུག་ཡུལ།'. -'Query contains only common words' doesn't happen anymore after adding the checks in r13666. - -- When working with adding the collections portlet, instead of saying "search results" it - would be nice to say "available collections". In general, this portlet ui has me - wondering what I should actually be doing. what does the "parent" button mean? -That's the default implementation for SearchableTextSourceBinder: out of scope for us. - -- Plone.app.collection: Need to have README say something -TODO: make readme - -- Plone.app.collection: TODO in collection.py -Removed - -- It looks like that in order to add a collection you have to be a manager which I - think is unnecessarily limiting. At minimum let's integrate it with Site Administrator - or even Editor. I think add collection portlet sounds like something for the site - administrator role as well. -Added 'Site administrator' in r50229 - -- seems to be an extra leading semicolon in querywidget.js. There are lots of errors with - that js that make me cross browser nervous. Addressing these would make me feel happy -The leading semicolon prevents merging issues when other js files do not close correctly. This is done by default in new files afaik. -Other JSLint issues are addressed. - -- The query field widget could use some better documentation. -TODO: documentation. - - -Conclusion ----------- -This UI is really slick. This makes me actually want to use collections. A few things -that must get fixed before merge: - - must address all the bad user input issues - - work on the stack traces in all the reviews - - fix obvious js errors -Those seem all doable and I'm sure this will make a nice addition to the 4.1 release. - - - - - - -rossp -===== -- The README.txt and description in setup.py are empty for - plone.app.collection, plone.app.querystring, and - archetypes.querywidget. These should have some minimal content at - the very least, preferably they should explain the package for those - who might want to re-use it. -Duplicate, see above. - -- archetypes.querywidget/setup.py only requires plone.app.querystring - but archetypes/querywidget/configure.zcml includes - plone.app.vocabularies and plone.app.contentlisting as well. If - archetypes.querywidget actually does depend on the ZCML of those two - eggs, then they should be added as dependencies in setup.py, - otherwise those ZCML includes should be removed. -This has been resolved. - -- Cleaned up a bunch of testing dependency issues. -- Added alltests test-groups eggs. -Thanks! - -- plone.app.querystring/setup.py depends on plone.directives.form, but - I don't see anything that uses it. -This has been resolved. - -- The GenericSetup profiles need better titles and descriptions so - that users can know what they're looking at when installing add-ons. -TODO: fix profile titles - -- The results preview is damn cool! Everything works as it should. -- I love the per-criteria counts, *very* useful. -Thanks! - -- The results preview could be a little clearer by saying "3 of 22 - results" instead of just "22 results" and showing only 3. -This was fixed some time ago. - -- I think the "Remove line" link in the criteria editing should have - something to call it out, probably whatever icon we use in Plone for - deleting/removing, such as a red X. -We don't do icons a lot anymore since Sunburst came out. The link has a class 'removecriteria' which can be used to do extra styling. -IMHO this - -- The criteria value dropdowns don't seem to go away when you click - elsewhere on the page. -Fixed, see above. - -- The criteria value dropdowns are styled differently than other - dropdowns. -TODO: fix styling for custom widgets - -- The date criteria value isn't using a calendar widget nor is any - description given of how to enter a textual date. I think this - might be important enough to be a must-have. - -- Since this is meant to be the new canonical Collection - implementation, shouldn't the Plone default News, Events, and Past - Events collections use this new implantation when creating new - sites? - -- Fixed some logging issues, was getting *way* too many "... INFO - plone.app.querystring Generated query: ..." log messages. Switched - from INFO to DEBUG. -Thanks! - -- Plone handled the following error several times. This needs to be - addressed. - - User Name - admin (admin) - Request URL - http://localhost:8080/Plone/@@querybuilder_html_results - Exception Type - BadRequest - Exception Value -

Site Error

An error was encountered while publishing this resource.

Invalid request

The parameter, query, was omitted from the request.

Make sure to specify all required parameters, and try the request again.

Troubleshooting Suggestions

  • The URL may be incorrect.
  • The parameters passed to this resource may be incorrect.
  • A resource that this resource relies on may be encountering an error.

For more detailed information about the error, please refer to the error log.

If the error persists please contact the site maintainer. Thank you for your patience.

- Traceback (innermost last): - - Module ZPublisher.Publish, line 126, in publish - Module ZPublisher.mapply, line 72, in mapply - Module ZPublisher.Publish, line 51, in missing_name - Module ZPublisher.HTTPResponse, line 741, in badRequestError - BadRequest:

Site Error

An error was encountered while publishing this resource.

Invalid request

The parameter, query, was omitted from the request.

Make sure to specify all required parameters, and try the request again.

Troubleshooting Suggestions

  • The URL may be incorrect.
  • The parameters passed to this resource may be incorrect.
  • A resource that this resource relies on may be encountering an error.

For more detailed information about the error, please refer to the error log.

If the error persists please contact the site maintainer. Thank you for your patience.

- - Display traceback as text - - REQUEST - form - cookies - dtpref_rows '20' - zmi_top_frame '' - dtpref_cols '100%' - tree-s 'eJzT0MgpMOQKVneEA1dbda4CI67EkgJjLj0AeGcHew' - zmi_use_css '1' - lazy items - SESSION > - other - AUTHENTICATION_PATH '' - TraversalRequestNameStack [] - LANGUAGE 'en' - AUTHENTICATED_USER - URL 'http://localhost:8080/Plone/@@querybuilder_html_results' - SERVER_URL 'http://localhost:8080' - LANGUAGE_TOOL - PUBLISHED - method 'GET' - ACTUAL_URL 'http://localhost:8080/Plone/@@querybuilder_html_results' - URL0 http://localhost:8080/Plone/@@querybuilder_html_results - URL1 http://localhost:8080/Plone - URL2 http://localhost:8080 - BASE0 http://localhost:8080 - BASE1 http://localhost:8080 - BASE2 http://localhost:8080/Plone - BASE3 http://localhost:8080/Plone/@@querybuilder_html_results - environ - CONNECTION_TYPE 'keep-alive' - HTTP_USER_AGENT 'Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.0 Safari/534.13' - HTTP_COOKIE 'dtpref_rows="20"; zmi_top_frame=""; dtpref_cols="100%"; zmi_use_css="1"; tree-s="eJzT0MgpMOQKVneEA1dbda4CI67EkgJjLj0AeGcHew"' - SERVER_NAME 'localhost6.localdomain6' - GATEWAY_INTERFACE 'CGI/1.1' - HTTP_ACCEPT 'application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5' - SERVER_SOFTWARE 'Zope/(2.13.1dev, python 2.6.6, linux2) ZServer/1.1' - REMOTE_ADDR '127.0.0.1' - HTTP_ACCEPT_LANGUAGE 'en-US,en;q=0.8' - SCRIPT_NAME '' - REQUEST_METHOD 'GET' - HTTP_HOST 'localhost:8080' - PATH_INFO '/Plone/@@querybuilder_html_results' - SERVER_PORT '8080' - SERVER_PROTOCOL 'HTTP/1.1' - QUERY_STRING '' - HTTP_ACCEPT_CHARSET 'ISO-8859-1,utf-8;q=0.7,*;q=0.3' - channel.creation_time 1291695482 - HTTP_ACCEPT_ENCODING 'gzip,deflate,sdch' - PATH_TRANSLATED '/Plone/@@querybuilder_html_results' -This shouldn't be an issue anymore. - -- No obviously related errors from bin/alltests that weren't exposed - with bin/test below. - - -- ran "bin/test -s plone.app.contentlisting -s plone.app.collection -s - plone.app.querystring -s archetypes.querywidget" and got the - following. This should be addressed: - - Set up plone.app.testing.layers.PloneFixture Traceback (most recent call last): - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 367, in run_layer - setup_layer(options, layer, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 627, in setup_layer - setup_layer(options, base, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 627, in setup_layer - setup_layer(options, base, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 632, in setup_layer - layer.setUp() - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 110, in setUp - self.setUpZCML() - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 222, in setUpZCML - loadAll('configure.zcml') - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 217, in loadAll - xmlconfig.file(filename, package, context=context) - File "/usr/local/lib/python/site-packages/zope.configuration-3.7.2-py2.6.egg/zope/configuration/xmlconfig.py", line 653, in file - context.execute_actions() - File "/usr/local/lib/python/site-packages/zope.configuration-3.7.2-py2.6.egg/zope/configuration/config.py", line 606, in execute_actions - callable(*args, **kw) - File "/usr/local/lib/python/site-packages/Products.GenericSetup-1.6.1-py2.6.egg/Products/GenericSetup/registry.py", line 707, in registerProfile - raise KeyError, 'Duplicate profile ID: %s' % profile_id - ConfigurationExecutionError: : u'Duplicate profile ID: Products.CMFUid:default' - in: - File "/usr/local/lib/python/site-packages/Products.CMFUid-2.2.1-py2.6.egg/Products/CMFUid/configure.zcml", line 10.2-16.8 - - - - Running plone.app.collection.tests.base.PACollection:Functional tests: - Set up plone.app.testing.layers.PloneFixture Traceback (most recent call last): - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 367, in run_layer - setup_layer(options, layer, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 627, in setup_layer - setup_layer(options, base, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 627, in setup_layer - setup_layer(options, base, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 632, in setup_layer - layer.setUp() - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 110, in setUp - self.setUpZCML() - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 222, in setUpZCML - loadAll('configure.zcml') - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 217, in loadAll - xmlconfig.file(filename, package, context=context) - File "/usr/local/lib/python/site-packages/zope.configuration-3.7.2-py2.6.egg/zope/configuration/xmlconfig.py", line 653, in file - context.execute_actions() - File "/usr/local/lib/python/site-packages/zope.configuration-3.7.2-py2.6.egg/zope/configuration/config.py", line 606, in execute_actions - callable(*args, **kw) - File "/usr/local/lib/python/site-packages/Products.GenericSetup-1.6.1-py2.6.egg/Products/GenericSetup/registry.py", line 707, in registerProfile - raise KeyError, 'Duplicate profile ID: %s' % profile_id - ConfigurationExecutionError: : u'Duplicate profile ID: Products.CMFDefault:default' - in: - File "/usr/local/lib/python/site-packages/Products.CMFDefault-2.2.2-py2.6.egg/Products/CMFDefault/profiles.zcml", line 6.2-11.8 - - - - Running plone.app.contentlisting.tests.base.ContentListing:Functional tests: - Set up plone.app.testing.layers.PloneFixture Traceback (most recent call last): - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 367, in run_layer - setup_layer(options, layer, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 627, in setup_layer - setup_layer(options, base, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 627, in setup_layer - setup_layer(options, base, setup_layers) - File "/usr/local/lib/python/site-packages/zope.testing-3.9.4-py2.6.egg/zope/testing/testrunner/runner.py", line 632, in setup_layer - layer.setUp() - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 110, in setUp - self.setUpZCML() - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 222, in setUpZCML - loadAll('configure.zcml') - File "/home/xen/src/plone-coredev.svn/src/plone.app.testing/plone/app/testing/layers.py", line 217, in loadAll - xmlconfig.file(filename, package, context=context) - File "/usr/local/lib/python/site-packages/zope.configuration-3.7.2-py2.6.egg/zope/configuration/xmlconfig.py", line 653, in file - context.execute_actions() - File "/usr/local/lib/python/site-packages/zope.configuration-3.7.2-py2.6.egg/zope/configuration/config.py", line 606, in execute_actions - callable(*args, **kw) - File "/usr/local/lib/python/site-packages/Products.GenericSetup-1.6.1-py2.6.egg/Products/GenericSetup/registry.py", line 707, in registerProfile - raise KeyError, 'Duplicate profile ID: %s' % profile_id - ConfigurationExecutionError: : u'Duplicate profile ID: Products.PlonePAS:PlonePAS' - in: - File "/home/xen/src/plone-coredev.svn/src/Products.PlonePAS/Products/PlonePAS/profiles.zcml", line 6.2-12.8 - - -This is resolved - -- It seems like the tabular_view has been factored out of the - standard_view. They were combined in atct_topic_view and I think - it's great to factor them out this way. I see that the "Display as - Table" checkbox field has also been removed from the collection edit - form, also good. But this does leave some confusion in play. From - the edit form it's not clear what the "Table Columns" field does - especially since it's help text still refers to the "Display as - Table" field. Do we think it's sufficiently clear to just change - the "Table Columns" help text to refer to selecting the right layout - from the display menu back on the view screen? -Fixed in r50230 - -- plone.app.collection, plone.app.querystring and - archetypes.querywidget are in dire need of comments, docstrings and - more complete interfaces for developer/maintenance documentation - purposes. It's far to difficult to approach and understand how - things fit together as-is. -TODO: create docs + overview image etc - -- Why is a new/separate collection portlet implementation provided in - plone.app.collection? Isn't the one in plone.portlet.collection - already compatible? -p.a.collection is a new implementation with different idioms and a different portal_type. - -- The javascriptDisabled validator seems distasteful to me, but I - might not be understanding it. Why is this necessary and is there - really no better way? -TODO: explain in docs why this works the way it does. - -- I love how small plone.app.collection is. -Me too! -XXX: convert to Dexterity? - -- It sure looks like the archetypes.querywidget.field.QueryField set - and __init__ methods are entirely unnecessary. -Fixed in r13669 - -- I don't see any unit tests for the JS. -XXX: explain in docs - -- I note that the query builder JS lives in archetypes.querywidget. - It seems like it would be best to have it factored out into a non-AT - dependent place. Would plone.app.querystring be the right place for - that? -Nope, p.a.querystring should just do parsing the abstracted formquery into a catalog query. - -- The test coverage for archetypes.querywidget and - plone.app.collection looks poor. -Upped coverage to 100% - -Conclusion ----------- - -The implementation is slick and really nice to use. It's a vast -improvement over what we have currently. I'd love to see it included -in 4.1. - -Having said that I don't think I'm comfortable merging it as-is, but -the changes required seem to be pretty doable. If more complete -docstrings, comments, and interfaces are added, the test failures are -addressed, and the BadRequest errors are addressed, I'm a strong +1 -for merging. diff --git a/plips/reviews/plip10902-review-rossp.txt b/plips/reviews/plip10902-review-rossp.txt deleted file mode 100644 index d66327d53b..0000000000 --- a/plips/reviews/plip10902-review-rossp.txt +++ /dev/null @@ -1,175 +0,0 @@ -PLIP 10902: New collections -=========================== - -PLIP ticket: http://dev.plone.org/plone/ticket/10902 - -Review by Ross Patterson (me@rpatterson.net, zenwryly on irc) - -The PLIP was reviewed on Ubuntu 11.04 (Natty) using python 2.7.1 -and Google Chrome 14.0. - - -Review steps ------------- - -- Ran the plips/plip10902-new-collections.cfg buildout. - -- Ran "bin/test -v -s plone.app.contentlisting -s plone.app.collection - -s plone.app.querystring -s archetypes.querywidget - --coverage=var/coverage". - -- Ran bin/alltests. TODO - -- Created a new Plone site with the plone.app.collection add-on - included. - -- Added several collections using the new UI. - -- Examined code. - - -Notes and observations ----------------------- - -Items prefixed with "MUST FIX" must, IMO, be addressed before merging. - -- MUST FIX: Insufficient docs for plone.app.collection, - plone.app.querystring, and archetypes.querywidget: - - - The README.txt (and thus long_description) don't provide any - documentation for either end users or customizers. - - The setup.py description is *still* empty. - -- MUST FIX: archetypes.querywidget/setup.py dependencies are - incomplete and the other package dependencies may be as well. For - example, archetypes.querywidget obviously depends on - Products.Archetypes. - -- MUST FIX: The GenericSetup profiles for archetypes.querywidget and - plone.app.collection need better titles so that users can know what - they're looking at when installing add-ons. - -- All tests pass with "bin/test -v -s plone.app.contentlisting -s - plone.app.collection -s plone.app.querystring -s - archetypes.querywidget --coverage=var/coverage" and coverage looks - excellent! - - 27 100% archetypes.querywidget.field (/home/xen/src/plone-coredev.svn/src/archetypes.querywidget/archetypes/querywidget/field.py) - 3 100% archetypes.querywidget.interfaces (/home/xen/src/plone-coredev.svn/src/archetypes.querywidget/archetypes/querywidget/interfaces.py) - 40 100% archetypes.querywidget.tests.base (/home/xen/src/plone-coredev.svn/src/archetypes.querywidget/archetypes/querywidget/tests/base.py) - 11 100% archetypes.querywidget.tests.test_integration_doctest (/home/xen/src/plone-coredev.svn/src/archetypes.querywidget/archetypes/querywidget/tests/test_integration_doctest.py) - 49 100% archetypes.querywidget.widget (/home/xen/src/plone-coredev.svn/src/archetypes.querywidget/archetypes/querywidget/widget.py) - 18 50% plone.app.collection.__init__ (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/__init__.py) - 1 100% plone.app.collection.browser.__init__ (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/browser/__init__.py) - 116 100% plone.app.collection.collection (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/collection.py) - 12 100% plone.app.collection.integration (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/integration.py) - 3 100% plone.app.collection.interfaces (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/interfaces.py) - 1 100% plone.app.collection.portlets.__init__ (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/portlets/__init__.py) - 102 100% plone.app.collection.portlets.collectionportlet (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/portlets/collectionportlet.py) - 37 100% plone.app.collection.tests.base (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/tests/base.py) - 199 100% plone.app.collection.tests.testCollection (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/tests/testCollection.py) - 13 23% plone.app.collection.validators (/home/xen/src/plone-coredev.svn/src/plone.app.collection/plone/app/collection/validators.py) - 49 100% plone.app.contentlisting.browser (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/browser.py) - 121 74% plone.app.contentlisting.catalog (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/catalog.py) - 73 80% plone.app.contentlisting.contentlisting (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/contentlisting.py) - 14 100% plone.app.contentlisting.interfaces (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/interfaces.py) - 60 95% plone.app.contentlisting.realobject (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/realobject.py) - 36 100% plone.app.contentlisting.tests.base (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/tests/base.py) - 10 100% plone.app.contentlisting.tests.test_integration_doctest (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/tests/test_integration_doctest.py) - 219 100% plone.app.contentlisting.tests.test_integration_unit (/home/xen/src/plone-coredev.svn/src/plone.app.contentlisting/plone/app/contentlisting/tests/test_integration_unit.py) - 18 100% plone.app.querystring.interfaces (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/interfaces.py) - 55 100% plone.app.querystring.querybuilder (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/querybuilder.py) - 111 92% plone.app.querystring.queryparser (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/queryparser.py) - 71 100% plone.app.querystring.registryreader (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/registryreader.py) - 44 100% plone.app.querystring.tests.base (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/tests/base.py) - 4 100% plone.app.querystring.tests.registry_testdata (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/tests/registry_testdata.py) - 39 100% plone.app.querystring.tests.testQueryBuilder (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/tests/testQueryBuilder.py) - 195 99% plone.app.querystring.tests.testQueryParser (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/tests/testQueryParser.py) - 43 100% plone.app.querystring.tests.testRegistryIntegration (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/tests/testRegistryIntegration.py) - 93 100% plone.app.querystring.tests.testRegistryReader (/home/xen/src/plone-coredev.svn/src/plone.app.querystring/plone/app/querystring/tests/testRegistryReader.py) - -- MUST FIX: When I add a Plone site with the p.a.collection profile, I - see two "Collection" options in the add menu. IMO adding the - p.a.collection profile should make the previous collection type - un-addable but existing old collections should still work. - - But that still leaves two collection types in other parts of the UI, - the query builder and search forms, for example. How should we - address this? - -- The results preview could be a little clearer by saying "3 - of 22 results" instead of just "22 results" and showing only 3. - - Update: Contrary to the response, this is still happening. - -- I think the "Remove line" link in the criteria editing should have - something to call it out, probably whatever icon we use in Plone for - deleting/removing, such as a red X. - - Update: still think this would be better and I don't think an x or a - minus is really icon proliferation. - -- The criteria value dropdowns are styled differently than other - dropdowns. - - Update: still think this should be addressed. It's a visual - difference without any meaning. - -- MUST FIX: The date criteria value isn't using a calendar widget nor - is any description given of how to enter a textual date. - -- Since this is meant to be the new canonical Collection - implementation, shouldn't the Plone default News, Events, and Past - Events collections use this new implantation when creating new - sites? - - Update: still think this would be better. - -- There's no documentation for the re-usable and extensible bits. For - example, the IQueryOperation stuff is pretty cool and could be - really useful if made accessible. - -- Why is a new/separate collection portlet implementation provided in - plone.app.collection? Isn't the one in plone.portlet.collection - already compatible? - - > p.a.collection is a new implementation with different idioms and a - > different portal_type. - - I think p.portlet.collection should be generalized to be made - compatible. The is so little in p.portlet.collection that I don't - want to implementations running around. - -- The javascriptDisabled validator seems distasteful to me, but I - might not be understanding it. Why is this necessary and is there - really no better way? - - Update: Still not addressed. - -- I don't see any unit tests for the JS. - - Update: Still not addressed. - -- I note that the query builder JS lives in archetypes.querywidget. - It seems like it would be best to have it factored out into a non-AT - dependent place. Would plone.app.querystring be the right place for - that? - - > Nope, p.a.querystring should just do parsing the abstracted - > formquery into a catalog query. - - But the JS UI for dynamically assembling a query is not at all AT - specific. I still the builder UI JS should be factored out for - re-use outside AT content, dexterity for example. - -Conclusion ----------- - -This is still really great and slick stuff and I still love it. :-) - -For merge I would want to see the docs, package and GS metadata -fixed. I would also need the two collection type issues ironed out. -Finally, *some* solution to the date criteria needs to be worked out. - -If we can get all that and a few of the other issues worked out, I'll -be +1 for merge. diff --git a/plips/reviews/plip10902-review-uiteam.txt b/plips/reviews/plip10902-review-uiteam.txt deleted file mode 100644 index 8de8dfaa75..0000000000 --- a/plips/reviews/plip10902-review-uiteam.txt +++ /dev/null @@ -1,121 +0,0 @@ -PLIP 10902: New collections -=========================== - -PLIP ticket: http://dev.plone.org/plone/ticket/10902 - -Review by the Plone UI team (ui AT lists.plone.org) - - -Preparations and basic obesrvations. ------------------------------------- -Created a new Plone site with plone.app.collection profile. - - Shouldn't the profile be part of Plone installation by default? - -Added some dummy content that should be later used in the test collection. - - There are 2 'Collection' items in the 'Add New…' menu. One for the new - collections, another one for the old ones. Should be only one. - -Added the new object of the *new* Collection type. Reviewing the 'Search -terms', 'Sort on' and 'Preview' fields as those that are new to the collection -type. - - -'Search terms' field. ---------------------- - - - Can we rename it to something more meaningful like 'Search for' or - something like that? 'Search terms' might sound like an alien meta-field - for statistically normal editors. - - The description of the field is quite long and confusing. "Choose what - you want to search for" should be more than enough here in our opinion. - - Why do we separate different fields in the 'Text' section of the - ? Updated search - results page ('Filter the results' menu) provides the idea of how users - possibly want to interact with the dates and the dates that do really - matter for the end users. - - Probably, if we would use a graphical element instead of textual 'Remove - line' the action would be clearer in this case. - - It's better to position 'Remove line' action on the left of the line. - Usually, users decide to remove a line in case they chose the wrong - parameter. In this case it's better to have remove line close to the - 'add line' action to reduce the required mouse movements. - - When one adds a new search parameter, default 'Select' dropdown, should - stay above the newly added rows in order to keep the semantical relation - ot the field's title. Moreover it's confusing to have the same - actionable element moving down on the page after every action. - - After adding a parameter for the 'Search terms', information 'n items - matching your search terms' appears to be: - 1. Not related to the current line at all — it's too far from the - added parameter. - 2. Information in this line is quite confusing at first. Does it show - the information for this line as if it would be a standalone (since - each line has it's own information of this kind) or it shows the info - based on all previous lines? Yes, it becomes more clear when you - analyze the information and see how the lines are changed after you - add more parameters, but it's very not clear before you get used to - it. We suppose that information on each line doesn't add any useful - information with this confusion. We would suggest to have one 'n items - matching your search terms' information for all lines as a total below - the parameters. - - -'Sort on' field. ----------------- - - - The parameters in the dropdown don't make sense to me. Yes, that's what - we used to have in Plone, but why not to improve it? Those parameters - make no sense to the end users because they have no clue what do those - mean internally (ask anybody who has not been working with Plone what - does 'Short name' or 'Effective date' mean. Or how should the sorting on - 'Tag' be done?). We would propose to use the same sorting options as the - new search results page has - relevance, date (newest first), - alphabetically. Might even drop the 'relevance' option. - - -'Preview'. ----------- - - - If the search gives less than 10 items, the description for the field - still says 'Preview of at most 10 items.' even though the very closely - located line within the field says something like '8 items matching your - search terms.'. This is very confusing. One of these (probably the - descriptions) should be dropped. - - Would be great to replace the text in the preview itself from simple '22 - items matching your search terms' to something like 'Shown 10 out of 22 - items matching your search terms.' in case the search returns more than - 10 items. - -Bugs found during the review. ------------------------------ - -- In the 'Searhcable Text' note that 'Text' block is the first on in the - select. Choose 'Tags' in this block. The new criteria row is added. Click - the same select (that now says 'Tags') again and note how the 'Text' block - becomes the second now, after the 'Dates'. - -- There is a bug in the preview. Select 'Description' in the 'Search terms' - field and leave it empty. You will get some results. in the preview field. - In our case it gave me 11 results in total for this parameter. Then, click - 'Remove line'. The preview results stay the same and are not reverted to the - initial state with 0 results. - -- The style of the new widgets used in the fields (tags selector, preview - field) looks like it has been merged from another site and are not part of - the current Plone site. They have black border and feel weird. Is there any - way to make them look generic enough to pick up the styling of the visual - theme around? So that the 'Preview' field's border would look like the - textarea's border and the tags selector would look like a system's