Re: svn commit: r1789711 - /ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy

[taher]

I'm not sure why you're using a fixed name instead of the field name
fetching mechanism, especially that this is a loop.

Also, I'm not sure the entire try / catch block is necessary. What test
made you believe you need to hard code the field in here?

On Fri, Mar 31, 2017 at 8:00 PM, <[hidden email]> wrote:

> Author: jleroux
> Date: Fri Mar 31 17:00:58 2017
> New Revision: 1789711
>
> URL: http://svn.apache.org/viewvc?rev=1789711&view=rev
> Log:
> No functional change, fixes a C/P with wrong value name in errMsgList
>
> Modified:
>     ofbiz/ofbiz-framework/trunk/applications/content/
> groovyScripts/content/GetContentLookupList.groovy
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/content/
> groovyScripts/content/GetContentLookupList.groovy
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> applications/content/groovyScripts/content/GetContentLookupList.groovy?
> rev=1789711&r1=1789710&r2=1789711&view=diff
> ============================================================
> ==================
> --- ofbiz/ofbiz-framework/trunk/applications/content/
> groovyScripts/content/GetContentLookupList.groovy (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/content/
> groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 17:00:58 2017
> @@ -70,7 +70,7 @@ try {
>      viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).
> intValue()
>  } catch (NumberFormatException nfe) {
>      Debug.logError(nfe, "Caught an exception : " + nfe.toString(),
> "GetContentLookupList.groovy")
> -    errMsgList.add("Entered value is non-numeric for numeric field: " +
> field.getName())
> +    errMsgList.add("Entered value is non-numeric for numeric field:
> VIEW_SIZE"))
>  }
>
>  context.viewSize = viewSize
>
>
>

[Jacques Le Roux]

Le 31/03/2017 à 19:16, Taher Alkhateeb a écrit :
> I'm not sure why you're using a fixed name instead of the field name
> fetching mechanism, especially that this is a loop.
VIEW_SIZE is a well known name for OFBiz developers and Michael is right we don't need to show a such error in UI. I see no loop :-o
> Also, I'm not sure the entire try / catch block is necessary.
Integer.valueOf throws a |NumberFormatException which we can't ignore. I don't think re-throwing the exception would be a good idea |
> What test made you believe you need to hard code the field in here?
You mean the VIEW_SIZE name :-o ?

Jacques

>
> On Fri, Mar 31, 2017 at 8:00 PM, <[hidden email]> wrote:
>
>> Author: jleroux
>> Date: Fri Mar 31 17:00:58 2017
>> New Revision: 1789711
>>
>> URL: http://svn.apache.org/viewvc?rev=1789711&view=rev
>> Log:
>> No functional change, fixes a C/P with wrong value name in errMsgList
>>
>> Modified:
>>      ofbiz/ofbiz-framework/trunk/applications/content/
>> groovyScripts/content/GetContentLookupList.groovy
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/
>> groovyScripts/content/GetContentLookupList.groovy
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>> applications/content/groovyScripts/content/GetContentLookupList.groovy?
>> rev=1789711&r1=1789710&r2=1789711&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/ofbiz-framework/trunk/applications/content/
>> groovyScripts/content/GetContentLookupList.groovy (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/content/
>> groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 17:00:58 2017
>> @@ -70,7 +70,7 @@ try {
>>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).
>> intValue()
>>   } catch (NumberFormatException nfe) {
>>       Debug.logError(nfe, "Caught an exception : " + nfe.toString(),
>> "GetContentLookupList.groovy")
>> -    errMsgList.add("Entered value is non-numeric for numeric field: " +
>> field.getName())
>> +    errMsgList.add("Entered value is non-numeric for numeric field:
>> VIEW_SIZE"))
>>   }
>>
>>   context.viewSize = viewSize
>>
>>
>>

[Jacques Le Roux]

BTW I forgot to thank you for your review. I think you mixed the new snippet with the one above from where I C/P.

Jacques


Le 31/03/2017 à 21:16, Jacques Le Roux a écrit :

> Le 31/03/2017 à 19:16, Taher Alkhateeb a écrit :
>> I'm not sure why you're using a fixed name instead of the field name
>> fetching mechanism, especially that this is a loop.
> VIEW_SIZE is a well known name for OFBiz developers and Michael is right we don't need to show a such error in UI. I see no loop :-o
>> Also, I'm not sure the entire try / catch block is necessary.
> Integer.valueOf throws a |NumberFormatException which we can't ignore. I don't think re-throwing the exception would be a good idea |
>> What test made you believe you need to hard code the field in here?
> You mean the VIEW_SIZE name :-o ?
>
> Jacques
>>
>> On Fri, Mar 31, 2017 at 8:00 PM, <[hidden email]> wrote:
>>
>>> Author: jleroux
>>> Date: Fri Mar 31 17:00:58 2017
>>> New Revision: 1789711
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1789711&view=rev
>>> Log:
>>> No functional change, fixes a C/P with wrong value name in errMsgList
>>>
>>> Modified:
>>>      ofbiz/ofbiz-framework/trunk/applications/content/
>>> groovyScripts/content/GetContentLookupList.groovy
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/
>>> groovyScripts/content/GetContentLookupList.groovy
>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>> applications/content/groovyScripts/content/GetContentLookupList.groovy?
>>> rev=1789711&r1=1789710&r2=1789711&view=diff
>>> ============================================================
>>> ==================
>>> --- ofbiz/ofbiz-framework/trunk/applications/content/
>>> groovyScripts/content/GetContentLookupList.groovy (original)
>>> +++ ofbiz/ofbiz-framework/trunk/applications/content/
>>> groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 17:00:58 2017
>>> @@ -70,7 +70,7 @@ try {
>>>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).
>>> intValue()
>>>   } catch (NumberFormatException nfe) {
>>>       Debug.logError(nfe, "Caught an exception : " + nfe.toString(),
>>> "GetContentLookupList.groovy")
>>> -    errMsgList.add("Entered value is non-numeric for numeric field: " +
>>> field.getName())
>>> +    errMsgList.add("Entered value is non-numeric for numeric field:
>>> VIEW_SIZE"))
>>>   }
>>>
>>>   context.viewSize = viewSize
>>>
>>>
>>>
>
>

[taher]

Indeed, copy and paste pattern is where I got mixed up. This whole block at
the top (the first try / catch) needs to be refactored. Either it was
copied from below or vice versa. Also I think the script needs to be
refactored, the logic is kind of all over the place for validation and
transaction management and whatnot.

On Fri, Mar 31, 2017 at 11:10 PM, Jacques Le Roux <
[hidden email]> wrote:

> BTW I forgot to thank you for your review. I think you mixed the new
> snippet with the one above from where I C/P.
>
> Jacques
>
>
>
> Le 31/03/2017 à 21:16, Jacques Le Roux a écrit :
>
>> Le 31/03/2017 à 19:16, Taher Alkhateeb a écrit :
>>
>>> I'm not sure why you're using a fixed name instead of the field name
>>> fetching mechanism, especially that this is a loop.
>>>
>> VIEW_SIZE is a well known name for OFBiz developers and Michael is right
>> we don't need to show a such error in UI. I see no loop :-o
>>
>>> Also, I'm not sure the entire try / catch block is necessary.
>>>
>> Integer.valueOf throws a |NumberFormatException which we can't ignore. I
>> don't think re-throwing the exception would be a good idea |
>>
>>> What test made you believe you need to hard code the field in here?
>>>
>> You mean the VIEW_SIZE name :-o ?
>>
>> Jacques
>>
>>>
>>> On Fri, Mar 31, 2017 at 8:00 PM, <[hidden email]> wrote:
>>>
>>> Author: jleroux
>>>> Date: Fri Mar 31 17:00:58 2017
>>>> New Revision: 1789711
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1789711&view=rev
>>>> Log:
>>>> No functional change, fixes a C/P with wrong value name in errMsgList
>>>>
>>>> Modified:
>>>>      ofbiz/ofbiz-framework/trunk/applications/content/
>>>> groovyScripts/content/GetContentLookupList.groovy
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/
>>>> groovyScripts/content/GetContentLookupList.groovy
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>> applications/content/groovyScripts/content/GetContentLookupList.groovy?
>>>> rev=1789711&r1=1789710&r2=1789711&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/applications/content/
>>>> groovyScripts/content/GetContentLookupList.groovy (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/applications/content/
>>>> groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 17:00:58
>>>> 2017
>>>> @@ -70,7 +70,7 @@ try {
>>>>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).
>>>> intValue()
>>>>   } catch (NumberFormatException nfe) {
>>>>       Debug.logError(nfe, "Caught an exception : " + nfe.toString(),
>>>> "GetContentLookupList.groovy")
>>>> -    errMsgList.add("Entered value is non-numeric for numeric field: " +
>>>> field.getName())
>>>> +    errMsgList.add("Entered value is non-numeric for numeric field:
>>>> VIEW_SIZE"))
>>>>   }
>>>>
>>>>   context.viewSize = viewSize
>>>>
>>>>
>>>>
>>>>
>>
>>
>

[Michael Brohl-3]

Taher,

I fully agree, I've already filed a Jira:
https://issues.apache.org/jira/browse/OFBIZ-9292

Cheers,

Michael


Am 31.03.17 um 22:23 schrieb Taher Alkhateeb:

> Indeed, copy and paste pattern is where I got mixed up. This whole block at
> the top (the first try / catch) needs to be refactored. Either it was
> copied from below or vice versa. Also I think the script needs to be
> refactored, the logic is kind of all over the place for validation and
> transaction management and whatnot.
>
> On Fri, Mar 31, 2017 at 11:10 PM, Jacques Le Roux <
> [hidden email]> wrote:
>
>> BTW I forgot to thank you for your review. I think you mixed the new
>> snippet with the one above from where I C/P.
>>
>> Jacques
>>
>>
>>
>> Le 31/03/2017 à 21:16, Jacques Le Roux a écrit :
>>
>>> Le 31/03/2017 à 19:16, Taher Alkhateeb a écrit :
>>>
>>>> I'm not sure why you're using a fixed name instead of the field name
>>>> fetching mechanism, especially that this is a loop.
>>>>
>>> VIEW_SIZE is a well known name for OFBiz developers and Michael is right
>>> we don't need to show a such error in UI. I see no loop :-o
>>>
>>>> Also, I'm not sure the entire try / catch block is necessary.
>>>>
>>> Integer.valueOf throws a |NumberFormatException which we can't ignore. I
>>> don't think re-throwing the exception would be a good idea |
>>>
>>>> What test made you believe you need to hard code the field in here?
>>>>
>>> You mean the VIEW_SIZE name :-o ?
>>>
>>> Jacques
>>>
>>>> On Fri, Mar 31, 2017 at 8:00 PM, <[hidden email]> wrote:
>>>>
>>>> Author: jleroux
>>>>> Date: Fri Mar 31 17:00:58 2017
>>>>> New Revision: 1789711
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1789711&view=rev
>>>>> Log:
>>>>> No functional change, fixes a C/P with wrong value name in errMsgList
>>>>>
>>>>> Modified:
>>>>>       ofbiz/ofbiz-framework/trunk/applications/content/
>>>>> groovyScripts/content/GetContentLookupList.groovy
>>>>>
>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/
>>>>> groovyScripts/content/GetContentLookupList.groovy
>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>> applications/content/groovyScripts/content/GetContentLookupList.groovy?
>>>>> rev=1789711&r1=1789710&r2=1789711&view=diff
>>>>> ============================================================
>>>>> ==================
>>>>> --- ofbiz/ofbiz-framework/trunk/applications/content/
>>>>> groovyScripts/content/GetContentLookupList.groovy (original)
>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/content/
>>>>> groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 17:00:58
>>>>> 2017
>>>>> @@ -70,7 +70,7 @@ try {
>>>>>        viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).
>>>>> intValue()
>>>>>    } catch (NumberFormatException nfe) {
>>>>>        Debug.logError(nfe, "Caught an exception : " + nfe.toString(),
>>>>> "GetContentLookupList.groovy")
>>>>> -    errMsgList.add("Entered value is non-numeric for numeric field: " +
>>>>> field.getName())
>>>>> +    errMsgList.add("Entered value is non-numeric for numeric field:
>>>>> VIEW_SIZE"))
>>>>>    }
>>>>>
>>>>>    context.viewSize = viewSize
>>>>>
>>>>>
>>>>>
>>>>>
>>>

[Jacques Le Roux]

+1

Jacques


Le 31/03/2017 à 22:26, Michael Brohl a écrit :

> Taher,
>
> I fully agree, I've already filed a Jira: https://issues.apache.org/jira/browse/OFBIZ-9292
>
> Cheers,
>
> Michael
>
>
> Am 31.03.17 um 22:23 schrieb Taher Alkhateeb:
>> Indeed, copy and paste pattern is where I got mixed up. This whole block at
>> the top (the first try / catch) needs to be refactored. Either it was
>> copied from below or vice versa. Also I think the script needs to be
>> refactored, the logic is kind of all over the place for validation and
>> transaction management and whatnot.
>>
>> On Fri, Mar 31, 2017 at 11:10 PM, Jacques Le Roux <
>> [hidden email]> wrote:
>>
>>> BTW I forgot to thank you for your review. I think you mixed the new
>>> snippet with the one above from where I C/P.
>>>
>>> Jacques
>>>
>>>
>>>
>>> Le 31/03/2017 à 21:16, Jacques Le Roux a écrit :
>>>
>>>> Le 31/03/2017 à 19:16, Taher Alkhateeb a écrit :
>>>>
>>>>> I'm not sure why you're using a fixed name instead of the field name
>>>>> fetching mechanism, especially that this is a loop.
>>>>>
>>>> VIEW_SIZE is a well known name for OFBiz developers and Michael is right
>>>> we don't need to show a such error in UI. I see no loop :-o
>>>>
>>>>> Also, I'm not sure the entire try / catch block is necessary.
>>>>>
>>>> Integer.valueOf throws a |NumberFormatException which we can't ignore. I
>>>> don't think re-throwing the exception would be a good idea |
>>>>
>>>>> What test made you believe you need to hard code the field in here?
>>>>>
>>>> You mean the VIEW_SIZE name :-o ?
>>>>
>>>> Jacques
>>>>
>>>>> On Fri, Mar 31, 2017 at 8:00 PM, <[hidden email]> wrote:
>>>>>
>>>>> Author: jleroux
>>>>>> Date: Fri Mar 31 17:00:58 2017
>>>>>> New Revision: 1789711
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1789711&view=rev
>>>>>> Log:
>>>>>> No functional change, fixes a C/P with wrong value name in errMsgList
>>>>>>
>>>>>> Modified:
>>>>>>       ofbiz/ofbiz-framework/trunk/applications/content/
>>>>>> groovyScripts/content/GetContentLookupList.groovy
>>>>>>
>>>>>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/
>>>>>> groovyScripts/content/GetContentLookupList.groovy
>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>>>> applications/content/groovyScripts/content/GetContentLookupList.groovy?
>>>>>> rev=1789711&r1=1789710&r2=1789711&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- ofbiz/ofbiz-framework/trunk/applications/content/
>>>>>> groovyScripts/content/GetContentLookupList.groovy (original)
>>>>>> +++ ofbiz/ofbiz-framework/trunk/applications/content/
>>>>>> groovyScripts/content/GetContentLookupList.groovy Fri Mar 31 17:00:58
>>>>>> 2017
>>>>>> @@ -70,7 +70,7 @@ try {
>>>>>>        viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).
>>>>>> intValue()
>>>>>>    } catch (NumberFormatException nfe) {
>>>>>>        Debug.logError(nfe, "Caught an exception : " + nfe.toString(),
>>>>>> "GetContentLookupList.groovy")
>>>>>> -    errMsgList.add("Entered value is non-numeric for numeric field: " +
>>>>>> field.getName())
>>>>>> +    errMsgList.add("Entered value is non-numeric for numeric field:
>>>>>> VIEW_SIZE"))
>>>>>>    }
>>>>>>
>>>>>>    context.viewSize = viewSize
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>
>