[Framework-Team] Re: PLIP 212 ready for review

Florian Schulze florian.schulze at gmx.net
Tue Feb 12 22:41:57 UTC 2008

On Tue, 12 Feb 2008 22:36:11 +0100, Tom Lazar  
<lists at tomster.org> wrote:

> hi florian, hello fellow framework team members,
> i started reviewing plip 212 on the plane back to berlin and got most  
> covered. i committed my initial review and post a copy of it here, for  
> your convenience. i still need to repeat the click tests i've done with  
> IE 6 and IE 7, as i haven't got Parallels installed on my macbook (yet).  
> also, i intend to look at the js code changes and migration stuff, but  
> judging from my previous experiences with florian's code i don't expect  
> any snags there. however, i did run into a few problems with this  
> buildout and currently don't think it's ready for merging, see my notes  
> below. i will continue the review tomorrow evening, along with my other  
> outstanding reviews.


Thanks for the review. I just wanted to make clear that I didn't write any  
of this code, I only wrote the PLIP, did the review buildout and foxed  
some small things. All the heavy lifting was done by Martijn Pieters. I  
will give some comments below.

> --snip--
> Note, all tests have been conducted with jquery 1.2.2 which is part of  
> the bundle, as well as with version 1.2.3 which is currently the latest  
> revision of jquery.
> = manual click tests
> performed with fresh checkout, new instance with Plone Default skin.  
> Using Firefox and Safari Version 3.0.4 (5523.15), Win IE 6 + 7  
> tbd. Features and areas covered:
> * livesearch, portal calendar pagination, external links, inline  
> editing, folder_contents reordering:
> no problems encountered, everything worked as expected.
> * highlighting of search results
> This didn't work on any browser. Calling a url such as  
> http://localhost:8080/jquery/front-page?searchterm=comfortable  would  
> not actually highlight the word (eventhough it exists in the default  
> front page)
> instead i got the following error in jquery.js (via firebug)
>      a is not a function
>      [Break on this error] eval(function(p,a,c,k,e,r){e=function(c) 
> {return(c<a?'':e(parseInt(c/a)))+((c=c%a...
> Note: this error remained, even after replacing the shipping jquery  
> version 1.2.2 with the meanwhile current version 1.2.3

This looks like a syntax error in that file. I will look into it.

> Additional errors:
> the display menu wasn't working as expected, i.e. the click action on  
> the display menu was not caught but instead resulted in a page load of  
> the `select_default_view`, however, this only occurred with jquery  
> 1.2.3, 1.2.2 worked fine.
> workflow actions however didn't work in neither 1.2.2 nor 1.2.3, i.e.  
> publishing resulted in a page load instead of just a viewlet refresh.

This is either another syntax error, or some issue with the KSS  

> = running unit tests =
> running all tests (./bin/instance/test -v -v -s plone) produced the  
> following result:
> Tests with failures:
>     test_published_news_items  
> (plone.app.portlets.tests.test_events_portlet.TestRenderer)
>     test_published_news_items  
> (plone.app.portlets.tests.test_news_portlet.TestRenderer)
>     /opt/zope/buildout/eggs/plone.app.workflow- 
> app/workflow/tests/onestateworkflow.txt
> Total: 1079 tests, 3 failures, 0 errors

I didn't look into this at all, because we didn't change anything besides  
JS code and some registrations in JS registry. I will look into upgrading  
the buildout to the latest Plone version if it isn't already.

> = code review =
> to be done (check migration, look into the js code changes)

Florian Schulze

More information about the Framework-Team mailing list