You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by David E Jones <jo...@hotwaxmedia.com> on 2007/08/31 23:26:56 UTC
Re: svn commit: r571591 - /ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
Jacques,
I've commented on this a lot before, but this REALLY isn't a bug fix, at all. This is definitely in the improvement camp, and just an improvement in error messages and handling.
While maintaining the release branch is really great, just remember the rule for releases is: LESS IS MORE!
Every change to the release branch creates the possibility of introducing MORE bugs. So if it's not a bug fix, we don't want to do it.
Just so we're all on the same page: what we REALLY need for the release branch is use and testing. Just applying fixes from the trunk to the branch will never get it to a releasable state...
-David
jleroux@apache.org wrote:
> Author: jleroux
> Date: Fri Aug 31 14:22:44 2007
> New Revision: 571591
>
> URL: http://svn.apache.org/viewvc?rev=571591&view=rev
> Log:
> Applied fix from trunk for revision: 567520
>
> Modified:
> ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
>
> Modified: ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
> URL: http://svn.apache.org/viewvc/ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java?rev=571591&r1=571590&r2=571591&view=diff
> ==============================================================================
> --- ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java (original)
> +++ ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java Fri Aug 31 14:22:44 2007
> @@ -108,7 +108,7 @@
> }
> }
> if (this.orderHeader == null) {
> - throw new IllegalArgumentException("Order header is not valid");
> + throw new IllegalArgumentException("Order header passed in is not valid for orderId [" + orderHeader.getString("orderId") + "]");
> }
> }
>
> @@ -125,7 +125,12 @@
> try {
> this.orderHeader = delegator.findByPrimaryKey("OrderHeader", UtilMisc.toMap("orderId", orderId));
> } catch (GenericEntityException e) {
> - throw new IllegalArgumentException("Invalid orderId");
> + String errMsg = "Error finding order with ID [" + orderId + "]: " + e.toString();
> + Debug.logError(e, errMsg, module);
> + throw new IllegalArgumentException(errMsg);
> + }
> + if (this.orderHeader == null) {
> + throw new IllegalArgumentException("Order not found with orderId [" + orderId + "]");
> }
> }
>
> @@ -155,7 +160,7 @@
> GenericValue productStore = delegator.findByPrimaryKeyCache("ProductStore", UtilMisc.toMap("productStoreId", productStoreId));
> return productStore;
> } catch (GenericEntityException ex) {
> - Debug.logError("Failed to get product store for order header [" + orderHeader + "] due to exception "+ ex.getMessage(), module);
> + Debug.logError(ex, "Failed to get product store for order header [" + orderHeader + "] due to exception "+ ex.getMessage(), module);
> return null;
> }
> }
>
>
Re: svn commit: r571591 - /ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
Posted by David E Jones <jo...@hotwaxmedia.com>.
Thanks for your understanding Jacques. Like you said, this is just a matter of time people can contribute to the project, and it's great that you are doing what you can!
-David
Jacques Le Roux wrote:
> David,
>
> De : "David E Jones" <jo...@hotwaxmedia.com>
>> Jacques,
>>
>> I've commented on this a lot before, but this REALLY isn't a bug fix, at all. This is definitely in the improvement camp, and just
> an improvement in error messages and handling.
>
> OK, I reverted from previous file version, sorry for misunderstanding. Actually this relieves me from worries I had when commiting
> it in 1st place. Actually I was trying to commit Hans changes in rev. 571504 ("correction for an error when editing a orderitem,
> reported by sumit and tested by sumit and Bilgin") and because I got conflicts I thought this changes in trunk were needed (were the
> last previous). But I was already suspecting that it was not a good idea because I was not sure both fixed bugs. Though "correction
> for an error" suggest this I had vaguely followed the thread which ended by Hans's fix, and then was not sure what it was really
> intended for (hope you get the idea at that point, sorry for confusion)
>
>> While maintaining the release branch is really great, just remember the rule for releases is: LESS IS MORE!
>>
>> Every change to the release branch creates the possibility of introducing MORE bugs. So if it's not a bug fix, we don't want to do
> it.
>> Just so we're all on the same page: what we REALLY need for the release branch is use and testing. Just applying fixes from the
> trunk to the branch will never get it to a releasable state...
>
> Yes sure, I agree, all is a matter of free time...
>
> Thanks for advices
>
> Jacques
>
>> -David
>>
>>
>> jleroux@apache.org wrote:
>>> Author: jleroux
>>> Date: Fri Aug 31 14:22:44 2007
>>> New Revision: 571591
>>>
>>> URL: http://svn.apache.org/viewvc?rev=571591&view=rev
>>> Log:
>>> Applied fix from trunk for revision: 567520
>>>
>>> Modified:
>>> ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
>>>
>>> Modified: ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
>>> URL:
> http://svn.apache.org/viewvc/ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java?rev=571591&r1=571590&r2=571591&view=diff
>>> ==============================================================================
>>> --- ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java (original)
>>> +++ ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java Fri Aug 31 14:22:44 2007
>>> @@ -108,7 +108,7 @@
>>> }
>>> }
>>> if (this.orderHeader == null) {
>>> - throw new IllegalArgumentException("Order header is not valid");
>>> + throw new IllegalArgumentException("Order header passed in is not valid for orderId [" +
> orderHeader.getString("orderId") + "]");
>>> }
>>> }
>>>
>>> @@ -125,7 +125,12 @@
>>> try {
>>> this.orderHeader = delegator.findByPrimaryKey("OrderHeader", UtilMisc.toMap("orderId", orderId));
>>> } catch (GenericEntityException e) {
>>> - throw new IllegalArgumentException("Invalid orderId");
>>> + String errMsg = "Error finding order with ID [" + orderId + "]: " + e.toString();
>>> + Debug.logError(e, errMsg, module);
>>> + throw new IllegalArgumentException(errMsg);
>>> + }
>>> + if (this.orderHeader == null) {
>>> + throw new IllegalArgumentException("Order not found with orderId [" + orderId + "]");
>>> }
>>> }
>>>
>>> @@ -155,7 +160,7 @@
>>> GenericValue productStore = delegator.findByPrimaryKeyCache("ProductStore", UtilMisc.toMap("productStoreId",
> productStoreId));
>>> return productStore;
>>> } catch (GenericEntityException ex) {
>>> - Debug.logError("Failed to get product store for order header [" + orderHeader + "] due to exception "+
> ex.getMessage(), module);
>>> + Debug.logError(ex, "Failed to get product store for order header [" + orderHeader + "] due to exception "+
> ex.getMessage(), module);
>>> return null;
>>> }
>>> }
>>>
>>>
>
Re: svn commit: r571591 -
/ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
David,
De : "David E Jones" <jo...@hotwaxmedia.com>
>
> Jacques,
>
> I've commented on this a lot before, but this REALLY isn't a bug fix, at all. This is definitely in the improvement camp, and just
an improvement in error messages and handling.
OK, I reverted from previous file version, sorry for misunderstanding. Actually this relieves me from worries I had when commiting
it in 1st place. Actually I was trying to commit Hans changes in rev. 571504 ("correction for an error when editing a orderitem,
reported by sumit and tested by sumit and Bilgin") and because I got conflicts I thought this changes in trunk were needed (were the
last previous). But I was already suspecting that it was not a good idea because I was not sure both fixed bugs. Though "correction
for an error" suggest this I had vaguely followed the thread which ended by Hans's fix, and then was not sure what it was really
intended for (hope you get the idea at that point, sorry for confusion)
> While maintaining the release branch is really great, just remember the rule for releases is: LESS IS MORE!
>
> Every change to the release branch creates the possibility of introducing MORE bugs. So if it's not a bug fix, we don't want to do
it.
>
> Just so we're all on the same page: what we REALLY need for the release branch is use and testing. Just applying fixes from the
trunk to the branch will never get it to a releasable state...
Yes sure, I agree, all is a matter of free time...
Thanks for advices
Jacques
>
> -David
>
>
> jleroux@apache.org wrote:
> > Author: jleroux
> > Date: Fri Aug 31 14:22:44 2007
> > New Revision: 571591
> >
> > URL: http://svn.apache.org/viewvc?rev=571591&view=rev
> > Log:
> > Applied fix from trunk for revision: 567520
> >
> > Modified:
> > ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
> >
> > Modified: ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java
> > URL:
http://svn.apache.org/viewvc/ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java?rev=571591&r1=571590&r2=571591&view=diff
> > ==============================================================================
> > --- ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java (original)
> > +++ ofbiz/branches/release4.0/applications/order/src/org/ofbiz/order/order/OrderReadHelper.java Fri Aug 31 14:22:44 2007
> > @@ -108,7 +108,7 @@
> > }
> > }
> > if (this.orderHeader == null) {
> > - throw new IllegalArgumentException("Order header is not valid");
> > + throw new IllegalArgumentException("Order header passed in is not valid for orderId [" +
orderHeader.getString("orderId") + "]");
> > }
> > }
> >
> > @@ -125,7 +125,12 @@
> > try {
> > this.orderHeader = delegator.findByPrimaryKey("OrderHeader", UtilMisc.toMap("orderId", orderId));
> > } catch (GenericEntityException e) {
> > - throw new IllegalArgumentException("Invalid orderId");
> > + String errMsg = "Error finding order with ID [" + orderId + "]: " + e.toString();
> > + Debug.logError(e, errMsg, module);
> > + throw new IllegalArgumentException(errMsg);
> > + }
> > + if (this.orderHeader == null) {
> > + throw new IllegalArgumentException("Order not found with orderId [" + orderId + "]");
> > }
> > }
> >
> > @@ -155,7 +160,7 @@
> > GenericValue productStore = delegator.findByPrimaryKeyCache("ProductStore", UtilMisc.toMap("productStoreId",
productStoreId));
> > return productStore;
> > } catch (GenericEntityException ex) {
> > - Debug.logError("Failed to get product store for order header [" + orderHeader + "] due to exception "+
ex.getMessage(), module);
> > + Debug.logError(ex, "Failed to get product store for order header [" + orderHeader + "] due to exception "+
ex.getMessage(), module);
> > return null;
> > }
> > }
> >
> >