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

[Florian Schulze]

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.

Hi!

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 2.0.0.12 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  
interaction.

> = 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-1.0.1.1-py2.4.egg/plone/ 
> 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)


Regards,
Florian Schulze