dev@javaserverfaces.java.net

Re: seeking review 1663

From: Roger Kitain <roger.kitain_at_oracle.com>
Date: Fri, 10 Sep 2010 06:28:11 -0400

  r=rogerk

Make sure you are adding the issue number and description at the top of
the generated change bundle.

On 9/10/2010 3:01 AM, Sheetal Vartak wrote:
> Another round of review.
> Attached is the changebundle to make sure that the error message gets printed only once for that request.
>
> Thanks
> Sheetal
>
>
>
> On Sep 1, 2010, at 11:08 PM, Sheetal Vartak wrote:
>
>> Hi Ed,
>>
>> Attached are the changes and the new text case. Please review.
>> I had a add a few more checks to ComponentSupport.java in order to avoid cases where the the component to add is the HtmlForm itself. Also in the case of composite components, when going up the hierarchy from the composite component to UINamingContainer, the parent of UINamingContainer is null. So I had to make sure my code worked in that case.
>> All the tests passed with my changes.
>>
>> Thanks
>> Sheetal
>> <changebundle.txt><newfiles.jar>
>> On Aug 31, 2010, at 7:35 AM, Ed Burns wrote:
>>
>>>>>>>> On Fri, 27 Aug 2010 10:34:06 -0700, Sheetal Vartak<sheetal.vartak_at_oracle.com> said:
>>> SV> Thanks for your comments. Attaching the changes incorporated with
>>> SV> the feedback. Please review.
>>>
>>> Summary: It's getting there, but not production ready yet. please read
>>> these comments and produce another iteration of the changebundle.
>>>
>>> SECTION: Review on the testcase portion of the change bundle
>>>
>>> I hate to do this, because I know you want to get it in, but I really
>>> need an automated test to be included as well.
>>>
>>> The good news is that it's not hard to add one in this case. I'll
>>> sketch it out for you here. I hope this proves useful to you in
>>> general. If so, please consider adding some content to the Mojarra FAQ.
>>>
>>> 1. Look for an existing test that does something similar
>>>
>>> I thought there might be an existing test that deals with other
>>> ProjectStage==Development features. I know that one of those features
>>> deals with messsages, so I ran a find command in jsf-ri/systest
>>> looking for .java files containing the string "messages". I found
>>> com/sun/faces/systest/NavigationTestCase.java.
>>>
>>> This one is in the right neighborhood, but it's not a perfect fit. If
>>> you wanted to be a "Done, and Gets Things Smart" [1] person, you could
>>> make a new java package for tests that deal with
>>> ProjectStage==Development features and put NavigationTestCase and your
>>> new test case in there. However, that's you're call. I'm not going
>>> to ask you to do that.
>>>
>>> In any case, I suggest using your IDE's refactor copy feature to copy
>>> NavigationTestCase to MissingFormTestCase.
>>>
>>> 2. make a new xhtml file. Looking at
>>> NavigationTestCase.testNullOutcomeNoMessages(), it points to an xhtml
>>> file. Again, it's not a perfect fit, and you could refactor and make
>>> a new diretory, etc. I suggest copying the xhtml file mentioned in
>>> that test method to a new place,
>>> jsf-ri/systest/web/facelets/missingForm.xhtml, and hacking away all
>>> the markup and replacing it with some markup that would cause your
>>> error message to be displayed.
>>>
>>> 3. Hack your new MissingFormTestCase and replace it with the necessary
>>> content to assert that the correct message is displayed.
>>>
>>> SECTION: Review on the production code portion of the change bundle
>>>
>>> M jsf-ri/src/main/resources/com/sun/faces/resources/Messages.properties
>>>
>>> +com.sun.faces.FORM_OMITTED_FOR_ACTIONSOURCE=The button/link component
>>> needs to have a Form in its ancestry. Please add<h:form>.
>>>
>>> - It looks like this error message *and* symbolic constant was not
>>> updated when your understanding of the bug-fix was updated to reflect
>>> the desired intent.
>>>
>>> - When you go about adding a message like this, you have to ensure it's
>>> localizable. Please take a look at the diffs of Andy's recent commits
>>> regarding issue 1800 to see how to do this. svn log and svn diff are
>>> your friend here. svn-book.pdf is definitely also your friend.
>>>
>>> M jsf-ri/src/main/java/com/sun/faces/facelets/tag/jsf/ComponentSupport.java
>>>
>>> - + private static boolean inspectInterfaces(Class[] interfaces) {
>>>
>>> The comment on the method is accurate, but the method name is not.
>>> Please rename to a more descriptive name.
>>>
>>> - + private static boolean inspectParentAncestry(UIComponent
>>> component) {
>>>
>>> Same here.
>>>
>>> M jsf-ri/src/main/java/com/sun/faces/util/MessageUtils.java
>>>
>>> - It looks like you missed this comment
>>>
>>> // IMPORTANT - ensure that any new message constant is properly
>>> // tested in test/com/sun/faces/util/TestUtil_messages (see comments
>>> // in test class for details on the test).
>>>
>>> No problem, it's easy to miss.
>>>
>>>
>>>
>>> [1] http://steve-yegge.blogspot.com/2008/06/done-and-gets-things-smart.html
>>>
>>> --
>>> | ed.burns_at_sun.com | office: +1 407 458 0017
>>> | homepage: | http://ridingthecrest.com/
>>> | 5 work days until JSF 2.1 Milestone 5
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
>>> For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
>> For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
> For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net