ofbiz-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Scott Gray <scott.g...@hotwaxmedia.com>
Subject Re: Bugs in OrderView.groovy
Date Mon, 01 Jun 2009 12:49:11 GMT
I don't see any need to continue the conversation further, Cimballi  
thank you for pointing out the inconsistency and I'm sure someone will  
take a look at it if there is any interest.

Regards
Scott

On 2/06/2009, at 12:44 AM, BJ Freeman wrote:

> ok why don't you explain what the whole script is doing and how you  
> see
> this not being correct, in the context of an order created by ofbiz.  
> not
> your import.
>
> Cimballi sent the following on 6/1/2009 5:16 AM:
>> My email was not about if an order must have a productStore or not,
>> but about the fact that in the same file, you first accept a null
>> productStore, and then you don't.
>>
>> Cimballi
>>
>>
>> On Sun, May 31, 2009 at 11:20 PM, Scott Gray <scott.gray@hotwaxmedia.com 
>> > wrote:
>>> Just because the data model allows an order without a product  
>>> store doesn't
>>> mean that the code does.  There are a million ways that you can  
>>> cause errors
>>> in the system with incorrectly loaded data.
>>>
>>> Regards
>>> Scott
>>>
>>> On 1/06/2009, at 8:56 AM, Cimballi wrote:
>>>
>>>> Here is a data file which you can import and which will generate  
>>>> the
>>>> null pointer exception when trying to view the order :
>>>>
>>>> <?xml version="1.0" encoding="UTF-8"?>
>>>>
>>>> <entity-engine-xml>
>>>>
>>>> <OrderType description="Special Sales" hasTable="N"
>>>>  orderTypeId="SPECIAL_SALES_ORDER" parentTypeId="SALES_ORDER" />
>>>>
>>>> <OrderHeader orderId="OH0001" orderTypeId="SPECIAL_SALES_ORDER"
>>>>  orderDate="2009-01-01 12:00:00.0" entryDate="2009-01-01  
>>>> 12:00:00.0"
>>>>  statusId="ORDER_CREATED" />
>>>>
>>>> <OrderRole orderId="OH0001" partyId="DemoCustomer"
>>>>  roleTypeId="PLACING_CUSTOMER" />
>>>>
>>>> </entity-engine-xml>
>>>>
>>>> And the stack trace (the beginning) :
>>>>
>>>> 2009-05-31 15:54:22,417 (http-0.0.0.0-8443-1) [
>>>> ControlServlet.java:204:ERROR]
>>>> ---- exception report
>>>> ----------------------------------------------------------
>>>> Error in request handler:
>>>> Exception: org.ofbiz.widget.screen.ScreenRenderException
>>>> Message: Error rendering screen
>>>> [component://order/widget/ordermgr/ 
>>>> OrderViewScreens.xml#OrderHeaderView]:
>>>> java.lang.NullPointerException (null)
>>>> ---- cause
>>>> ---------------------------------------------------------------------
>>>> Exception: java.lang.NullPointerException
>>>> Message: null
>>>> ---- stack trace
>>>> ---------------------------------------------------------------
>>>> java.lang.NullPointerException
>>>>
>>>> org 
>>>> .codehaus 
>>>> .groovy.runtime.InvokerHelper.getProperty(InvokerHelper.java:178)
>>>>
>>>> org 
>>>> .codehaus 
>>>> .groovy 
>>>> .runtime 
>>>> .ScriptBytecodeAdapter.getProperty(ScriptBytecodeAdapter.java:477)
>>>> OrderView.run(OrderView.groovy:380)
>>>> org 
>>>> .ofbiz.base.util.GroovyUtil.runScriptAtLocation(GroovyUtil.java: 
>>>> 117)
>>>> ...
>>>>
>>>> Cimballi
>>>>
>>>>
>>>> On Sun, May 31, 2009 at 5:28 AM, Divesh Dutta
>>>> <divesh.dutta@hotwaxmedia.com> wrote:
>>>>> +1
>>>>>
>>>>> Thanks
>>>>> --
>>>>> Divesh
>>>>>
>>>>>
>>>>> Ray wrote:
>>>>>> Lots of talk about different types of contributors etc and it  
>>>>>> should be
>>>>>> noted there are also lots of types of bugs.
>>>>>>
>>>>>> Your other posts highlight specific: do this, do that, it
>>>>>> crashes/doesn't
>>>>>> provide the expected result. That's helpful and will tend to  
>>>>>> get a
>>>>>> response
>>>>>> fairly quickly as they may not require as much time to verify  
>>>>>> and fix.
>>>>>>
>>>>>> This post is very different and could be titled "Potential bugs 

>>>>>> in
>>>>>> OrderView.groovy". I don't think anybody involved in OFBiz can  
>>>>>> answer
>>>>>> your
>>>>>> post with out doing a full code review of the file.
>>>>>>
>>>>>> There are numerous possible reasons for including the first  
>>>>>> check and
>>>>>> not
>>>>>> the second, it depends on lots of things and the file is split  
>>>>>> in to
>>>>>> several
>>>>>> 'if' sections that may have a lot of impact on whether a  
>>>>>> productStore is
>>>>>> expected to be found. And in a file that is over 400 lines long 

>>>>>> it could
>>>>>> take some effort to assess and justify the one thing you've  
>>>>>> highlighted
>>>>>> before dealing with the "I didn't note all of them" others.
>>>>>>
>>>>>> I think with the:
>>>>>>> If yes, why the first test ?
>>>>>>> If no, there is missing a test in the second case.
>>>>>> you might be over simplifying the problem as there are always  
>>>>>> other
>>>>>> dependencies.
>>>>>>
>>>>>> So although it's a reasonable post to suggest there are  
>>>>>> problems in the
>>>>>> file the difficulty is you need someone else to volunteer a  
>>>>>> reasonable
>>>>>> amount of their own time to investigate, justify and fix a  
>>>>>> potential
>>>>>> issue.
>>>>>> It's basically a retrospective code review that will take a lot 

>>>>>> of
>>>>>> effort,
>>>>>> and carries it's own risks of introducing new problems.
>>>>>>
>>>>>> Generally code quality gets looked at when someone is working  
>>>>>> on a file.
>>>>>>
>>>>>> Don't take it personally on this post but I suspect you won't get
>>>>>> someone
>>>>>> jumping in and adding/removing a speculative 'if' wrapper as  
>>>>>> you are
>>>>>> indirectly asking for quite a lot.
>>>>>>
>>>>>> On the other hand if you read the code and can produce a test  
>>>>>> case that
>>>>>> triggers a null pointer exception on line 380....
>>>>>>
>>>>>> Ray
>>>>>>
>>>>>>
>>>>>> Cimballi wrote:
>>>>>>> Well, I don't know how to explain you, it seems evident to me...
>>>>>>> In the first case you check if "productStore" is null or not.
 
>>>>>>> In the
>>>>>>> second case, you don't check.
>>>>>>> So, what is the correct behaviour ? Should an order be linked
 
>>>>>>> to a
>>>>>>> productStore or not ?
>>>>>>> If yes, why the first test ?
>>>>>>> If no, there is missing a test in the second case.
>>>>>>>
>>>>>>>> From my point of view I would say no because an order with
 
>>>>>>>> products of
>>>>>>> type "service" don't need productStore.
>>>>>>>
>>>>>>> To Scott : you should consider there are different kind of
>>>>>>> contributors on open source projects, I'm the kind of  
>>>>>>> contributor who
>>>>>>> send emails when I find something I think is a bug, I'm still
 
>>>>>>> not in
>>>>>>> the category of "patch providers" ! :-)
>>>>>>>
>>>>>>> Cimballi
>>>>>>>
>>>>>>>
>>>>>>> On Fri, May 29, 2009 at 7:16 PM, Scott Gray
>>>>>>> <scott.gray@hotwaxmedia.com>
>>>>>>> wrote:
>>>>>>>> Hi Cimballi
>>>>>>>>
>>>>>>>> Inconsistencies aren't necessarily bugs, but you are most
 
>>>>>>>> welcome to
>>>>>>>> create
>>>>>>>> a patch and jira issue for the corrections you think should
 
>>>>>>>> be made.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Scott
>>>>>>>>
>>>>>>>> On 30/05/2009, at 10:39 AM, Cimballi wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> There are several inconsistancies in the file
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> "applications/order/webapp/ordermgr/WEB-INF/actions/order/

>>>>>>>>> OrderView.groovy".
>>>>>>>>>
>>>>>>>>> I didn't note all of them, but here is an example :
>>>>>>>>>
>>>>>>>>> Line 260, productStore can be null :
>>>>>>>>> productStore = orderHeader.getRelatedOne("ProductStore");
>>>>>>>>> if (productStore) {
>>>>>>>>>    facility = productStore.getRelatedOne("Facility");
>>>>>>>>>
>>>>>>>>> Line 380, here productStore cannot be null :
>>>>>>>>> productStoreId =
>>>>>>>>> orderHeader.getRelatedOne("ProductStore").productStoreId;
>>>>>>>>>
>>>>>>>>> Cimballi
>>>>>
>>>
>>
>
> -- 
> BJ Freeman
> http://www.businessesnetwork.com/automation
> http://bjfreeman.elance.com
> http://www.linkedin.com/profile?viewProfile=&key=1237480&locale=en_US&trk=tab_pro
> Systems Integrator.
>


Mime
View raw message