Bugs in OrderView.groovy

[Cimballi]

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

[Adrian Crum-2]

What is the bug? What was the expected behavior and what was the actual result?

-Adrian

--- On Fri, 5/29/09, Cimballi <[hidden email]> wrote:

> From: Cimballi <[hidden email]>
> Subject: Bugs in OrderView.groovy
> To: "user" <[hidden email]>
> Date: Friday, May 29, 2009, 3:39 PM
> 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
>

[Scott Gray-2]

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

[Cimballi-2]

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 <[hidden email]> 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]

I will give you a general answer to all you statements.
this is a community effort.
the people that answer on the mailing list do so as a volunteers, not as
a paid support person.
You may have a valid point. Don't expect a response if you don't want to
follow the guidelines that have been setup.
even if you do follow the guidelines don't expect a response.
they ones that might try to work with you, will require you describe in
a way that will take the least amount of time for them.

when you get to the place where you can open Jiras and provide patches,
then become familiar with the contribution.
http://docs.ofbiz.org/display/OFBADMIN/OFBiz+Contributors+Best+Practices


Cimballi sent the following on 5/29/2009 8:39 PM:

> 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 <[hidden email]> 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.

[Ray-91]

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 <[hidden email]> 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
>>
>

[Divesh Dutta]

+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
>> <[hidden email]> 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
>>>
>>

[Cimballi-2]

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
<[hidden email]> 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 <[hidden email]>
>>> 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]

the import tools was not meant for a novice to use but someone familiar
with the system.
that is the reason it it only allowed under admin.
it is up to the importer to know what is required and supply it.
the same goes for the datafile importing.



Cimballi sent the following on 5/31/2009 1:56 PM:

> 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
> <[hidden email]> 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 <[hidden email]>
>>>> 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.

[Scott Gray-2]

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
> <[hidden email]> 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 <[hidden email]
>>>> >
>>>> 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
>>>>>
>>>>
>>
>>

[Cimballi-2]

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 <[hidden email]> 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
>> <[hidden email]> 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
>>>>> <[hidden email]>
>>>>> 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]

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 <[hidden email]> 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
>>> <[hidden email]> 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
>>>>>> <[hidden email]>
>>>>>> 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.

[Scott Gray-2]

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 <[hidden email]
>> > 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
>>>> <[hidden email]> 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
>>>>>>> <[hidden email]>
>>>>>>> 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.
>