Re: svn commit: r893656 - /ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java

[Scott Gray-2]

Hi Jacques,

I don't think this commit is going to behave the way you expect it to,  
comments inline.

Regards
Scott

HotWax Media
http://www.hotwaxmedia.com

On 24/12/2009, at 12:26 PM, [hidden email] wrote:

> Author: jleroux
> Date: Wed Dec 23 23:26:33 2009
> New Revision: 893656
>
> URL: http://svn.apache.org/viewvc?rev=893656&view=rev
> Log:
> Fix a possible NPE (in POS but I guess not only) when a Product have  
> been created without a ProductType associated (from import data).
> The reason of this informative log is to facilitate the search then...
> Can't hurt anyway
>
> Modified:
>    ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/
> ShoppingCartItem.java
>
> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/
> shoppingcart/ShoppingCartItem.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java?rev=893656&r1=893655&r2=893656&view=diff
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/
> ShoppingCartItem.java (original)
> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/
> ShoppingCartItem.java Wed Dec 23 23:26:33 2009
> @@ -716,10 +716,16 @@
>         if (parentProduct != null)
>             this.parentProductId =  
> _parentProduct.getString("productId");
>         if (UtilValidate.isEmpty(itemType)) {
> -            if  
> (_product.getString("productTypeId").equals("ASSET_USAGE")) {
> -                this.itemType = "RENTAL_ORDER_ITEM";  // will  
> create additional workeffort/asset usage records
> +            if (UtilValidate.isNotEmpty(_product)) {

_product can't be null here because earlier code would have thrown an  
NPE already.  Don't you actually want to check if  
_product.getString("productTypeId") is null?
also the actual NPE fix is to do this:
if ("ASSET_USAGE".equals(_product.getString("productTypeId))) {

> +                if  
> (_product.getString("productTypeId").equals("ASSET_USAGE")) {
> +                    this.itemType = "RENTAL_ORDER_ITEM";  // will  
> create additional workeffort/asset usage records
> +                } else {
> +                    this.itemType = "PRODUCT_ORDER_ITEM";
> +                }
>             } else {
> -                this.itemType = "PRODUCT_ORDER_ITEM";
> +         Debug.logError("Error calling ShoppingCartItem (trying to  
> creates new ShoppingCartItem object)." +
> +         " Check that there is a type for this product ", module);
> +         return;
>             }
>         } else {
>             this.itemType = itemType;
>
>

[Jacques Le Roux]

You are totally right Scott, too hastily done while doing another stuff with another Eclipse instance while this Eclipse instance
was '"building workspace" (I'm sure you experienced that if you use Eclipse)
I will fix it as soon as Eclipse after loading will have '"building [it's] workspace" ;)

Thanks for spotting this

While I'm at it, a question to Eclipse user: I use 3.2.4 and for some days know I should update Freemarker IDE to 1.0.2 but it seems
that Eclipse 3.5.1 is needed. I see Eclipse SDK 3.5.1 in the update windows (M2) but update fails, any experiences on that? Else I
will wait some time, 3.2.4 is doing a good job for now...

Jacques

From: "Scott Gray" <[hidden email]>

> Hi Jacques,
>
> I don't think this commit is going to behave the way you expect it to,  comments inline.
>
> Regards
> Scott
>
> HotWax Media
> http://www.hotwaxmedia.com
>
> On 24/12/2009, at 12:26 PM, [hidden email] wrote:
>
>> Author: jleroux
>> Date: Wed Dec 23 23:26:33 2009
>> New Revision: 893656
>>
>> URL: http://svn.apache.org/viewvc?rev=893656&view=rev
>> Log:
>> Fix a possible NPE (in POS but I guess not only) when a Product have  been created without a ProductType associated (from import
>> data).
>> The reason of this informative log is to facilitate the search then...
>> Can't hurt anyway
>>
>> Modified:
>>    ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ ShoppingCartItem.java
>>
>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/ shoppingcart/ShoppingCartItem.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ShoppingCartItem.java?rev=893656&r1=893655&r2=893656&view=diff
>> = = = = = = = = ======================================================================
>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ ShoppingCartItem.java (original)
>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/ ShoppingCartItem.java Wed Dec 23 23:26:33 2009
>> @@ -716,10 +716,16 @@
>>         if (parentProduct != null)
>>             this.parentProductId =  _parentProduct.getString("productId");
>>         if (UtilValidate.isEmpty(itemType)) {
>> -            if  (_product.getString("productTypeId").equals("ASSET_USAGE")) {
>> -                this.itemType = "RENTAL_ORDER_ITEM";  // will  create additional workeffort/asset usage records
>> +            if (UtilValidate.isNotEmpty(_product)) {
>
> _product can't be null here because earlier code would have thrown an  NPE already.  Don't you actually want to check if
> _product.getString("productTypeId") is null?
> also the actual NPE fix is to do this:
> if ("ASSET_USAGE".equals(_product.getString("productTypeId))) {
>
>> +                if  (_product.getString("productTypeId").equals("ASSET_USAGE")) {
>> +                    this.itemType = "RENTAL_ORDER_ITEM";  // will  create additional workeffort/asset usage records
>> +                } else {
>> +                    this.itemType = "PRODUCT_ORDER_ITEM";
>> +                }
>>             } else {
>> -                this.itemType = "PRODUCT_ORDER_ITEM";
>> +        Debug.logError("Error calling ShoppingCartItem (trying to  creates new ShoppingCartItem object)." +
>> +        " Check that there is a type for this product ", module);
>> +        return;
>>             }
>>         } else {
>>             this.itemType = itemType;
>>
>>
>
>