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