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. notyour 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. CimballiOn 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 errorsin 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 thenull 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.NullPointerExceptionorg .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 benoted there are also lots of types of bugs. Your other posts highlight specific: do this, do that, it crashes/doesn'tprovide the expected result. That's helpful and will tend to get aresponsefairly 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 answeryour post with out doing a full code review of the file.There are numerous possible reasons for including the first check andnotthe second, it depends on lots of things and the file is split in toseveral'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 highlightedbefore dealing with the "I didn't note all of them" others. I think with the:you might be over simplifying the problem as there are always otherIf yes, why the first test ? If no, there is missing a test in the second case.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 potentialissue.It's basically a retrospective code review that will take a lot ofeffort, 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 someonejumping in and adding/removing a speculative 'if' wrapper as you areindirectly asking for quite a lot.On the other hand if you read the code and can produce a test case thattriggers 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 thesecond case, you don't check.So, what is the correct behaviour ? Should an order be linked to aproductStore 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 oftype "service" don't need productStore. To Scott : you should consider there are different kind ofcontributors 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 inthe category of "patch providers" ! :-) Cimballi On Fri, May 29, 2009 at 7:16 PM, Scott Gray <[email protected]> wrote:Hi CimballiInconsistencies aren't necessarily bugs, but you are most welcome tocreatea 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.
smime.p7s
Description: S/MIME cryptographic signature
