ofbiz-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cimballi <cimballi.cimba...@gmail.com>
Subject Re: Bugs in OrderView.groovy
Date Mon, 01 Jun 2009 12:16:31 GMT
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
>>>>>>
>>>>>
>>>
>>>
>
>

Mime
View raw message