Re: Bugs in OrderView.groovy

[Scott Gray]

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]

Error in request handler:
Exception: org.ofbiz.widget.screen.ScreenRenderException
Message: Error rendering screen

java.lang.NullPointerException (null)

Exception: java.lang.NullPointerException
Message: null

java.lang.NullPointerException

OrderView.run(OrderView.groovy:380)

...

Cimballi


On Sun, May 31, 2009 at 5:28 AM, Divesh Dutta
<divesh.du...@hotwaxmedia.com> wrote:

---- exception report ---------------------------------------------------------- [component://order/widget/ordermgr/ OrderViewScreens.xml#OrderHeaderView]: ---- cause --------------------------------------------------------------------- ---- stack trace --------------------------------------------------------------- org .codehaus .groovy.runtime.InvokerHelper.getProperty(InvokerHelper.java:178) org .codehaus .groovy .runtime .ScriptBytecodeAdapter.getProperty(ScriptBytecodeAdapter.java:477) org.ofbiz.base.util.GroovyUtil.runScriptAtLocation(GroovyUtil.java: 117)

+1

Thanks
--
Divesh


Ray wrote:

noted there are also lots of types of bugs.



This post is very different and could be titled "Potential bugs in

post with out doing a full code review of the file.


before dealing with the "I didn't note all of them" others.

I think with the:

Lots of talk about different types of contributors etc and it should be 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. OrderView.groovy". I don't think anybody involved in OFBiz can answer your 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

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.


and carries it's own risks of introducing new problems.




jumping in and adding/removing a speculative 'if' wrapper as you are
indirectly asking for quite a lot.


triggers a null pointer exception on line 380....

Ray


Cimballi wrote:

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, 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 On the other hand if you read the code and can produce a test case that

Well, I don't know how to explain you, it seems evident to me...

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.

In the first case you check if "productStore" is null or not. In the

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

the category of "patch providers" ! :-)

Cimballi



wrote:

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 On Fri, May 29, 2009 at 7:16 PM, Scott Gray <scott.g...@hotwaxmedia.com >

Hi Cimballi


create


Thanks
Scott

On 30/05/2009, at 10:39 AM, Cimballi wrote:

Inconsistencies aren't necessarily bugs, but you are most welcome to a patch and jira issue for the corrections you think should be made.

Hi,

There are several inconsistancies in the file




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

"applications/order/webapp/ordermgr/WEB-INF/actions/order/ OrderView.groovy".

smime.p7s
Description: S/MIME cryptographic signature