Re: svn commit: r1824294 - in /ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility: facility/CountFacilityInventoryByProduct.groovy facility/ViewFacilityInventoryByProduct.groovy shipment/ReceiveInventoryAgainstPurchaseOrder.groovy

[Michael Brohl-3]

Hi Jacques,

do you think that it is appropriate to present the user an error message
to check the log? Normal users won't have access to the log in a
productive environment I guess. The message should contain a qualified
information what was wrong.

The message is also not translated by UILabels which should always be
the case for frontend messages.

Regards,

Michael


Am 15.02.18 um 12:01 schrieb [hidden email]:

> Author: jleroux
> Date: Thu Feb 15 11:01:41 2018
> New Revision: 1824294
>
> URL: http://svn.apache.org/viewvc?rev=1824294&view=rev
> Log:
> Fixed: Fix Default or Empty Catch block in Java and Groovy files
> (OFBIZ-8341)
>
> Fixes few remaining swallowed Exception in Groovy files.
>
> Not sure if always showing an error message in ui is a good idea.
> But this should not happen too often and could help users to fix wrong passed
> numbers...
>
> Modified:
>      ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy
>      ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy
>      ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy?rev=1824294&r1=1824293&r2=1824294&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy Thu Feb 15 11:01:41 2018
> @@ -181,7 +181,8 @@ if (action) {
>               checkTime = UtilDateTime.toTimestamp(cal.getTime())
>               searchParameterString += "&monthsInPastLimit=" + monthsInPastLimitStr
>           } catch (Exception e) {
> -            // Ignore
> +            Debug.logError(e, "Caught an exception : " + e.toString(), module)
> +            request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>           }
>       }
>  
> @@ -278,7 +279,8 @@ if (action) {
>                           try {
>                               salesUsageQuantity += salesUsageItem.getDouble("quantity").doubleValue()
>                           } catch (Exception e) {
> -                            // Ignore
> +                            Debug.logError(e, "Caught an exception : " + e.toString(), module)
> +                            request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>                           }
>                       }
>                   }
> @@ -299,7 +301,8 @@ if (action) {
>                           try {
>                               productionUsageQuantity += productionUsageItem.getDouble("quantity").doubleValue()
>                           } catch (Exception e) {
> -                            // Ignore
> +                            Debug.logError(e, "Caught an exception : " + e.toString(), module)
> +                            request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>                           }
>                       }
>                   }
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy?rev=1824294&r1=1824293&r2=1824294&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy Thu Feb 15 11:01:41 2018
> @@ -174,7 +174,8 @@ if (action) {
>               checkTime = UtilDateTime.toTimestamp(cal.getTime())
>               searchParameterString += "&monthsInPastLimit=" + monthsInPastLimitStr
>           } catch (Exception e) {
> -            // Ignore
> +            Debug.logError(e, "Caught an exception : " + e.toString(), module)
> +            request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>           }
>       }
>  
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy?rev=1824294&r1=1824293&r2=1824294&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy Thu Feb 15 11:01:41 2018
> @@ -216,8 +216,10 @@ if (productIdToReceive) {
>           if (productQtyToReceive) {
>               try {
>                   quantity = Double.parseDouble(productQtyToReceive)
> -            } catch (Exception e) {
> +            } catch (NumberFormatException nfe) {
>                   // Ignore the quantity update if there's a problem parsing it
> +                Debug.logError(nfe, "Caught an exception : " + nfe.toString(), module)
> +                request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>               }
>           }
>  
>
>

[Jacques Le Roux]

Thanks for the review Michael,

Yes I was wondering about that, so put the second part in the commit log.

I did something alike but simpler at r1824157. There, only NumberFormatException was clearly involved. So even if the message is only in English it
should be profitable and does not speak about the log.
Anyway, I agree all should be better translated. Actually I must say I did not think about it and simply followed the way it's mostly done currently
for the 40 same cases (look for <<request.setAttribute("_ERROR_MESSAGE>>in Groovy files)
I have creates OFBIZ-10225 for that.

About the log, I agree it's not a good idea to ask an user to look into the log, I was lazy :)

Why did I do that? I could not let only the log. Because we speak of Groovy files which are used in the context of UI. For Groovy, Java or Minilang
used in context of services, obviously the log and only the log fits. But it's not enough for UI. The user needs to know immediately that a problem
occurred. I was lazy because, unlike of r1824157, fpor these case in CountFacilityInventoryByProduct and View FacilityInventoryByProduct it needs more
care (is it only a NFE exception, which fields ar concerned, etc.)

My idea was that if ever a problem occurs in production the user would be able to look into the log (small config) or would ask for help. I agree it's
not good :) I'll look at it again

BTW, in r1824434  I have used the same pattern than in r1824157 for NumberFormatException in ReceiveInventoryAgainstPurchaseOrder (to be l10n)

I need to check if r1824157 needs to be improved as well (return on NumberFormatException)...

Jacques


Le 15/02/2018 à 13:51, Michael Brohl a écrit :

> Hi Jacques,
>
> do you think that it is appropriate to present the user an error message to check the log? Normal users won't have access to the log in a productive
> environment I guess. The message should contain a qualified information what was wrong.
>
> The message is also not translated by UILabels which should always be the case for frontend messages.
>
> Regards,
>
> Michael
>
>
> Am 15.02.18 um 12:01 schrieb [hidden email]:
>> Author: jleroux
>> Date: Thu Feb 15 11:01:41 2018
>> New Revision: 1824294
>>
>> URL: http://svn.apache.org/viewvc?rev=1824294&view=rev
>> Log:
>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>> (OFBIZ-8341)
>>
>> Fixes few remaining swallowed Exception in Groovy files.
>>
>> Not sure if always showing an error message in ui is a good idea.
>> But this should not happen too often and could help users to fix wrong passed
>> numbers...
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy
>> ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy
>> ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy?rev=1824294&r1=1824293&r2=1824294&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/CountFacilityInventoryByProduct.groovy Thu Feb 15 11:01:41 2018
>> @@ -181,7 +181,8 @@ if (action) {
>>               checkTime = UtilDateTime.toTimestamp(cal.getTime())
>>               searchParameterString += "&monthsInPastLimit=" + monthsInPastLimitStr
>>           } catch (Exception e) {
>> -            // Ignore
>> +            Debug.logError(e, "Caught an exception : " + e.toString(), module)
>> +            request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>>           }
>>       }
>>   @@ -278,7 +279,8 @@ if (action) {
>>                           try {
>>                               salesUsageQuantity += salesUsageItem.getDouble("quantity").doubleValue()
>>                           } catch (Exception e) {
>> -                            // Ignore
>> +                            Debug.logError(e, "Caught an exception : " + e.toString(), module)
>> + request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>>                           }
>>                       }
>>                   }
>> @@ -299,7 +301,8 @@ if (action) {
>>                           try {
>>                               productionUsageQuantity += productionUsageItem.getDouble("quantity").doubleValue()
>>                           } catch (Exception e) {
>> -                            // Ignore
>> +                            Debug.logError(e, "Caught an exception : " + e.toString(), module)
>> + request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>>                           }
>>                       }
>>                   }
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy?rev=1824294&r1=1824293&r2=1824294&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/facility/ViewFacilityInventoryByProduct.groovy Thu Feb 15 11:01:41 2018
>> @@ -174,7 +174,8 @@ if (action) {
>>               checkTime = UtilDateTime.toTimestamp(cal.getTime())
>>               searchParameterString += "&monthsInPastLimit=" + monthsInPastLimitStr
>>           } catch (Exception e) {
>> -            // Ignore
>> +            Debug.logError(e, "Caught an exception : " + e.toString(), module)
>> +            request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>>           }
>>       }
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy?rev=1824294&r1=1824293&r2=1824294&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/product/groovyScripts/facility/shipment/ReceiveInventoryAgainstPurchaseOrder.groovy Thu Feb 15
>> 11:01:41 2018
>> @@ -216,8 +216,10 @@ if (productIdToReceive) {
>>           if (productQtyToReceive) {
>>               try {
>>                   quantity = Double.parseDouble(productQtyToReceive)
>> -            } catch (Exception e) {
>> +            } catch (NumberFormatException nfe) {
>>                   // Ignore the quantity update if there's a problem parsing it
>> +                Debug.logError(nfe, "Caught an exception : " + nfe.toString(), module)
>> +                request.setAttribute("_ERROR_MESSAGE", "An exception occured please check the log")
>>               }
>>           }
>>
>>
>
>