Re: svn commit: r1800245 - in /ofbiz/ofbiz-framework/trunk/applications/accounting: groovyScripts/rate/RateServices.groovy minilang/rate/ servicedef/services_rate.xml

[Scott Gray-3]

Hi Nicolas,

My first code review in a while so I apologize if I'm wrong on any points,
or if they've been discussed already.

1. It would be good if the services were converted one per commit, with the
empty minilang file removed at the end with a separate commit.  That would
allow reviewers to compare the removed XML code with the new groovy code.
2. Defaults should (where possible) be moved to the service definition
3. I could be wrong about this but I don't think it is a good practice to
assign/change values in the parameters map, I think callers would not
expect their inputs to be changed by the service.  But I'm not sure if that
is actually happening or if groovy has copied the map beforehand, I'm also
unsure of how minilang used to handle the same.
4. Because groovy has "Groovy Truth", UtilValidate.isEmpty/isNotEmpty isn't
required, you can simply do "if (parameters.ratesList)".  An empty list or
null would return false.
5. I think it would be better to add the service documentation to the
service definition instead of a comment inline with the code.
6. I'm curious about the getRateAmount() "level" variable, I see it is only
defined within the local scope of the if/else blocks whereas "serviceName"
was defined at the method scope.  Does groovy allow "level" to take on the
method scope or is it an error?

Regards
Scott

On 29 June 2017 at 19:46, <[hidden email]> wrote:

> Author: nmalin
> Date: Thu Jun 29 07:46:02 2017
> New Revision: 1800245
>
> URL: http://svn.apache.org/viewvc?rev=1800245&view=rev
> Log:
> Improved: Convert RateServices.xml mini-lang to groovyDSL (OFBIZ-9381)
> finish the service conversion : getRateAmount,
> getRatesAmountsFromWorkEffortId, getRatesAmountsFromPartyId,
> getRatesAmountsFromEmplPositionTypeId and filterRateAmountList
> I create a generic groovy function getRateAmoutForm to remove duplicate
> code and refactoring the filterRateAmount for simplicity
>
> Removed:
>     ofbiz/ofbiz-framework/trunk/applications/accounting/minilang/rate/
> Modified:
>     ofbiz/ofbiz-framework/trunk/applications/accounting/
> groovyScripts/rate/RateServices.groovy
>     ofbiz/ofbiz-framework/trunk/applications/accounting/
> servicedef/services_rate.xml
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
> groovyScripts/rate/RateServices.groovy
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> applications/accounting/groovyScripts/rate/RateServices.groovy?rev=
> 1800245&r1=1800244&r2=1800245&view=diff
> ============================================================
> ==================
> --- ofbiz/ofbiz-framework/trunk/applications/accounting/
> groovyScripts/rate/RateServices.groovy (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/
> groovyScripts/rate/RateServices.groovy Thu Jun 29 07:46:02 2017
> @@ -128,3 +128,142 @@ def expirePartyRate() {
>      }
>      return success()
>  }
> +
> +// Get the applicable rate amount value
> +def getRateAmount() {
> +    /* Search for the applicable rate from most specific to most general
> in the RateAmount entity
> +    Defaults for periodTypeId is per hour and default currency is the
> currency in general.properties
> +    The order is:
> +    1. for specific rateTypeId, workEffortId (workEffort)
> +    2. for specific rateTypeId, partyId (party)
> +    3. for specific rateTypeId, emplPositionTypeId (emplPositionType)
> +    4. for specific rateTypeId (rateType)
> +
> +    Then, the results are filtered to improve the result. If you pass a
> workEffortId and a partyId,
> +    the service will first search the list of all the rateAmount with the
> specified workEffortId. Then, if
> +    there is at least one rateAmount with same partyId than the one in
> the parameter in the list, the list will
> +    be reduced to those entries.
> +    At the end, the first record of the list is chosen.
> +
> +    For a easier debugging time, there is a log triggered when no records
> are found for the input. This log
> +    shows up when there are rateAmounts corresponding to the input
> parameters without the rateCurrencyUomId and
> +    the periodTypeId.*/
> +    if (!parameters.rateCurrencyUomId) {
> +        parameters.rateCurrencyUomId = UtilProperties.
> getPropertyValue('general.properties', 'currency.uom.id.default', 'USD')
> +    }
> +    if (!parameters.periodTypeId) {
> +        parameters.periodTypeId = 'RATE_HOUR'
> +    }
> +    String serviceName = null;
> +    if (parameters.workEffortId && parameters.workEffortId != '_NA_') {
> +        // workeffort level
> +        level = 'workEffort'
> +        serviceName = 'getRatesAmountsFromWorkEffortId'
> +    } else if (parameters.partyId && parameters.partyId != '_NA_') {
> +        // party level
> +        level='partyId'
> +        serviceName = 'getRatesAmountsFromPartyId'
> +    } else if (parameters.emplPositionTypeId &&
> parameters.emplPositionTypeId != '_NA_') {
> +        // party level
> +        level = 'emplPositionTypeId'
> +        serviceName = 'getRatesAmountsFromEmplPositionTypeId'
> +    }
> +    if (serviceName) {
> +        logError(parameters.toString() + " " + serviceName.toString())
> +        Map result = run service: serviceName, with: parameters
> +        parameters.ratesList = result.ratesList
> +        logError(parameters.ratesList.toString())
> +        result = run service: 'filterRateAmountList', with: parameters
> +        parameters.ratesList = result.filteredRatesList
> +    }
> +
> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
> +        parameters.ratesList = from('RateAmount').where([rateTypeId:
> parameters.rateTypeId]).queryList();
> +        Map result = run service: 'filterRateAmountList', with: parameters
> +        parameters.ratesList = EntityUtil.filterByDate(
> result.filteredRatesList)
> +    }
> +
> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
> +        rateType = from('RateAmount').where([rateTypeId:
> parameters.rateTypeId]).queryOne()
> +        logError('A valid rate amount could not be found for rateType: '
> + rateType.description)
> +    }
> +
> +    // We narrowed as much as we could the result, now returning the
> first record of the list
> +    Map result = success()
> +    if (UtilValidate.isNotEmpty(parameters.ratesList)) {
> +        rateAmount = parameters.ratesList[0]
> +        if (! rateAmount.rateAmount) rateAmount.rateAmount =
> BigDecimal.ZERO
> +        result.rateAmount = rateAmount.rateAmount
> +        result.periodTypeId = rateAmount.periodTypeId
> +        result.rateCurrencyUomId = rateAmount.rateCurrencyUomId
> +        result.level = level
> +        result.fromDate = rateAmount.fromDate
> +    }
> +    return result
> +}
> +
> +//Generic fonction to resolve a rate amount from a pk field
> +def getRatesAmountsFrom(String field) {
> +    String entityName = null
> +    if (field == 'workEffortId') entityName = 'WorkEffort'
> +    if (field == 'partyId') entityName = 'Party'
> +    if (field == 'emplPositionTypeId') entityName = 'EmplPositionType'
> +
> +    Map condition = [rateTypeId: parameters.rateTypeId,
> +                     periodTypeId: parameters.periodTypeId,
> +                     rateCurrencyUomId: parameters.rateCurrencyUomId]
> +    condition.put(field, parameters.get(field))
> +    List ratesList = from('RateAmount').where(condition).filterByDate().
> queryList()
> +    if (UtilValidate.isEmpty(ratesList)) {
> +        GenericValue periodType = from('PeriodType').where([periodTypeId:
> parameters.periodTypeId]).queryOne()
> +        GenericValue rateType = from('RateType').where([rateTypeId:
> parameters.rateTypeId]).queryOne()
> +        GenericValue partyNameView = from('PartyNameView').where([partyId:
> parameters.partyId]).queryOne()
> +        logError('A valid rate entry could be found for rateType:' +
> rateType.description + ', ' + entityName + ':' + parameters.get(field)
> +                + ', party: ' + partyNameView.lastName +
> partyNameView.middleName + partyNameView.firstName + partyNameView.groupName
> +                + ' However.....not for the period:' +
> periodType.description + ' and currency:' + parameters.rateCurrencyUomId)
> +    }
> +    Map result = success()
> +    result.ratesList = ratesList
> +    return result
> +}
> +// Get all the rateAmount for a given workEffortId
> +def getRatesAmountsFromWorkEffortId() {
> +    return getRatesAmountsFrom('workEffortId')
> +}
> +// Get all the rateAmount for a given partyId
> +def getRatesAmountsFromPartyId() {
> +    return getRatesAmountsFrom('partyId')
> +}
> +// Get all the rateAmount for a given emplPositionTypeId
> +def getRatesAmountsFromEmplPositionTypeId() {
> +    return getRatesAmountsFrom('emplPositionTypeId')
> +}
> +
> +//Filter a list of rateAmount. The result is the most heavily-filtered
> non-empty list
> +def filterRateAmountList() {
> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
> +        logWarning('The list parameters.ratesList was empty, not
> processing any further')
> +        return success()
> +    }
> +    //Check if there is a more specific rate
> +    Map filterMap = [:]
> +    if (parameters.workEffortId) {
> +        filterMap.workEffortId = parameters.workEffortId
> +    }
> +    if (parameters.partyId) {
> +        filterMap.partyId = parameters.partyId
> +    }
> +    if (parameters.emplPositionTypeId) {
> +        filterMap.emplPositionTypeId = parameters.emplPositionTypeId
> +    }
> +    if (parameters.rateTypeId) {
> +        filterMap.rateTypeId = parameters.rateTypeId
> +    }
> +    List tempRatesFilteredList = EntityUtil.filterByAnd(parameters.ratesList,
> filterMap)
> +    if (UtilValidate.isNotEmpty(tempRatesFilteredList)) {
> +        parameters.ratesList = tempRatesFilteredList
> +    }
> +    Map result = success()
> +    result.filteredRatesList = parameters.ratesList
> +    return result
> +}
> \ No newline at end of file
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
> servicedef/services_rate.xml
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
> applications/accounting/servicedef/services_rate.xml?
> rev=1800245&r1=1800244&r2=1800245&view=diff
> ============================================================
> ==================
> --- ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
> (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
> Thu Jun 29 07:46:02 2017
> @@ -56,8 +56,8 @@ under the License.
>          <override name="rateTypeId" optional="false"/>
>          <override name="fromDate" optional="false"/>
>      </service>
> -    <service name="getRateAmount" default-entity-name="RateAmount"
> engine="simple" auth="true"
> -        location="component://accounting/minilang/rate/RateServices.xml"
> invoke="getRateAmount">
> +    <service name="getRateAmount" default-entity-name="RateAmount"
> engine="groovy" auth="true"
> +        location="component://accounting/groovyScripts/rate/RateServices.groovy"
> invoke="getRateAmount">
>          <description>Get Rate Amount</description>
>          <permission-service service-name="acctgBasePermissionCheck"
> main-action="VIEW"/>
>          <auto-attributes include="pk" mode="IN" optional="true"/>
> @@ -68,41 +68,38 @@ under the License.
>          <attribute name="fromDate" type="Timestamp" mode="OUT"
> optional="true"/>
>          <override name="rateTypeId" optional="false"/>
>      </service>
> -    <service name="getRatesAmountsFromWorkEffortId" default-entity-name="RateAmount"
> engine="simple" auth="true"
> -             location="component://accounting/minilang/rate/RateServices.xml"
> invoke="getRatesAmountsFromWorkEffortId">
> +    <service name="getRatesAmountsFromWorkEffortId" default-entity-name="RateAmount"
> engine="groovy" auth="true"
> +        location="component://accounting/groovyScripts/rate/RateServices.groovy"
> invoke="getRatesAmountsFromWorkEffortId">
>          <description>Get all Rates Amounts for a given
> workEffortId</description>
>          <permission-service service-name="acctgBasePermissionCheck"
> main-action="VIEW"/>
>          <auto-attributes include="pk" mode="IN" optional="true"/>
>          <attribute name="periodTypeId" type="String" mode="INOUT"
> optional="true"/>
>          <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
> optional="true"/>
> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
> optional="true"/>
>          <attribute name="ratesList" type="List" mode="OUT"
> optional="true"/>
>          <override name="workEffortId" optional="false"/>
>      </service>
> -    <service name="getRatesAmountsFromPartyId" default-entity-name="RateAmount"
> engine="simple" auth="true"
> -             location="component://accounting/minilang/rate/RateServices.xml"
> invoke="getRatesAmountsFromPartyId">
> +    <service name="getRatesAmountsFromPartyId" default-entity-name="RateAmount"
> engine="groovy" auth="true"
> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
> invoke="getRatesAmountsFromPartyId">
>          <description>Get all Rates Amounts for a given
> partyId</description>
>          <permission-service service-name="acctgBasePermissionCheck"
> main-action="VIEW"/>
>          <auto-attributes include="pk" mode="IN" optional="true"/>
>          <attribute name="periodTypeId" type="String" mode="INOUT"
> optional="true"/>
>          <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
> optional="true"/>
> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
> optional="true"/>
>          <attribute name="ratesList" type="List" mode="OUT"
> optional="true"/>
>          <override name="partyId" optional="false"/>
>      </service>
> -    <service name="getRatesAmountsFromEmplPositionTypeId"
> default-entity-name="RateAmount" engine="simple" auth="true"
> -             location="component://accounting/minilang/rate/RateServices.xml"
> invoke="getRatesAmountsFromEmplPositionTypeId">
> +    <service name="getRatesAmountsFromEmplPositionTypeId"
> default-entity-name="RateAmount" engine="groovy" auth="true"
> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
> invoke="getRatesAmountsFromEmplPositionTypeId">
>          <description>Get all Rates Amounts for a given
> emplPositionTypeId</description>
>          <permission-service service-name="acctgBasePermissionCheck"
> main-action="VIEW"/>
>          <auto-attributes include="pk" mode="IN" optional="true"/>
>          <attribute name="periodTypeId" type="String" mode="INOUT"
> optional="true"/>
>          <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
> optional="true"/>
> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
> optional="true"/>
>          <attribute name="ratesList" type="List" mode="OUT"
> optional="true"/>
>          <override name="emplPositionTypeId" optional="false"/>
>      </service>
> -    <service name="filterRateAmountList" default-entity-name="RateAmount"
> engine="simple" auth="true"
> -             location="component://accounting/minilang/rate/RateServices.xml"
> invoke="filterRateAmountList">
> +    <service name="filterRateAmountList" default-entity-name="RateAmount"
> engine="groovy" auth="true"
> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
> invoke="filterRateAmountList">
>          <description>Get the most specific non-empty Rate Amount list
> from a list of Rate Amount, given the input parameters :
>          workEffortId, partyId, emplPositionTypeId and
> rateTypeId</description>
>          <auto-attributes include="pk" mode="IN" optional="true"/>
>
>
>

[Nicolas Malin-2]

Whaoo thanks Scott for this, I appreciate. Please give me some days to
re read your remark before my return

Cheers,

Nicolas

Le 03/07/2017 à 21:19, Scott Gray a écrit :

> Hi Nicolas,
>
> My first code review in a while so I apologize if I'm wrong on any points,
> or if they've been discussed already.
>
> 1. It would be good if the services were converted one per commit, with the
> empty minilang file removed at the end with a separate commit.  That would
> allow reviewers to compare the removed XML code with the new groovy code.
> 2. Defaults should (where possible) be moved to the service definition
> 3. I could be wrong about this but I don't think it is a good practice to
> assign/change values in the parameters map, I think callers would not
> expect their inputs to be changed by the service.  But I'm not sure if that
> is actually happening or if groovy has copied the map beforehand, I'm also
> unsure of how minilang used to handle the same.
> 4. Because groovy has "Groovy Truth", UtilValidate.isEmpty/isNotEmpty isn't
> required, you can simply do "if (parameters.ratesList)".  An empty list or
> null would return false.
> 5. I think it would be better to add the service documentation to the
> service definition instead of a comment inline with the code.
> 6. I'm curious about the getRateAmount() "level" variable, I see it is only
> defined within the local scope of the if/else blocks whereas "serviceName"
> was defined at the method scope.  Does groovy allow "level" to take on the
> method scope or is it an error?
>
> Regards
> Scott
>
> On 29 June 2017 at 19:46, <[hidden email]> wrote:
>
>> Author: nmalin
>> Date: Thu Jun 29 07:46:02 2017
>> New Revision: 1800245
>>
>> URL: http://svn.apache.org/viewvc?rev=1800245&view=rev
>> Log:
>> Improved: Convert RateServices.xml mini-lang to groovyDSL (OFBIZ-9381)
>> finish the service conversion : getRateAmount,
>> getRatesAmountsFromWorkEffortId, getRatesAmountsFromPartyId,
>> getRatesAmountsFromEmplPositionTypeId and filterRateAmountList
>> I create a generic groovy function getRateAmoutForm to remove duplicate
>> code and refactoring the filterRateAmount for simplicity
>>
>> Removed:
>>      ofbiz/ofbiz-framework/trunk/applications/accounting/minilang/rate/
>> Modified:
>>      ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy
>>      ofbiz/ofbiz-framework/trunk/applications/accounting/
>> servicedef/services_rate.xml
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>> applications/accounting/groovyScripts/rate/RateServices.groovy?rev=
>> 1800245&r1=1800244&r2=1800245&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy Thu Jun 29 07:46:02 2017
>> @@ -128,3 +128,142 @@ def expirePartyRate() {
>>       }
>>       return success()
>>   }
>> +
>> +// Get the applicable rate amount value
>> +def getRateAmount() {
>> +    /* Search for the applicable rate from most specific to most general
>> in the RateAmount entity
>> +    Defaults for periodTypeId is per hour and default currency is the
>> currency in general.properties
>> +    The order is:
>> +    1. for specific rateTypeId, workEffortId (workEffort)
>> +    2. for specific rateTypeId, partyId (party)
>> +    3. for specific rateTypeId, emplPositionTypeId (emplPositionType)
>> +    4. for specific rateTypeId (rateType)
>> +
>> +    Then, the results are filtered to improve the result. If you pass a
>> workEffortId and a partyId,
>> +    the service will first search the list of all the rateAmount with the
>> specified workEffortId. Then, if
>> +    there is at least one rateAmount with same partyId than the one in
>> the parameter in the list, the list will
>> +    be reduced to those entries.
>> +    At the end, the first record of the list is chosen.
>> +
>> +    For a easier debugging time, there is a log triggered when no records
>> are found for the input. This log
>> +    shows up when there are rateAmounts corresponding to the input
>> parameters without the rateCurrencyUomId and
>> +    the periodTypeId.*/
>> +    if (!parameters.rateCurrencyUomId) {
>> +        parameters.rateCurrencyUomId = UtilProperties.
>> getPropertyValue('general.properties', 'currency.uom.id.default', 'USD')
>> +    }
>> +    if (!parameters.periodTypeId) {
>> +        parameters.periodTypeId = 'RATE_HOUR'
>> +    }
>> +    String serviceName = null;
>> +    if (parameters.workEffortId && parameters.workEffortId != '_NA_') {
>> +        // workeffort level
>> +        level = 'workEffort'
>> +        serviceName = 'getRatesAmountsFromWorkEffortId'
>> +    } else if (parameters.partyId && parameters.partyId != '_NA_') {
>> +        // party level
>> +        level='partyId'
>> +        serviceName = 'getRatesAmountsFromPartyId'
>> +    } else if (parameters.emplPositionTypeId &&
>> parameters.emplPositionTypeId != '_NA_') {
>> +        // party level
>> +        level = 'emplPositionTypeId'
>> +        serviceName = 'getRatesAmountsFromEmplPositionTypeId'
>> +    }
>> +    if (serviceName) {
>> +        logError(parameters.toString() + " " + serviceName.toString())
>> +        Map result = run service: serviceName, with: parameters
>> +        parameters.ratesList = result.ratesList
>> +        logError(parameters.ratesList.toString())
>> +        result = run service: 'filterRateAmountList', with: parameters
>> +        parameters.ratesList = result.filteredRatesList
>> +    }
>> +
>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>> +        parameters.ratesList = from('RateAmount').where([rateTypeId:
>> parameters.rateTypeId]).queryList();
>> +        Map result = run service: 'filterRateAmountList', with: parameters
>> +        parameters.ratesList = EntityUtil.filterByDate(
>> result.filteredRatesList)
>> +    }
>> +
>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>> +        rateType = from('RateAmount').where([rateTypeId:
>> parameters.rateTypeId]).queryOne()
>> +        logError('A valid rate amount could not be found for rateType: '
>> + rateType.description)
>> +    }
>> +
>> +    // We narrowed as much as we could the result, now returning the
>> first record of the list
>> +    Map result = success()
>> +    if (UtilValidate.isNotEmpty(parameters.ratesList)) {
>> +        rateAmount = parameters.ratesList[0]
>> +        if (! rateAmount.rateAmount) rateAmount.rateAmount =
>> BigDecimal.ZERO
>> +        result.rateAmount = rateAmount.rateAmount
>> +        result.periodTypeId = rateAmount.periodTypeId
>> +        result.rateCurrencyUomId = rateAmount.rateCurrencyUomId
>> +        result.level = level
>> +        result.fromDate = rateAmount.fromDate
>> +    }
>> +    return result
>> +}
>> +
>> +//Generic fonction to resolve a rate amount from a pk field
>> +def getRatesAmountsFrom(String field) {
>> +    String entityName = null
>> +    if (field == 'workEffortId') entityName = 'WorkEffort'
>> +    if (field == 'partyId') entityName = 'Party'
>> +    if (field == 'emplPositionTypeId') entityName = 'EmplPositionType'
>> +
>> +    Map condition = [rateTypeId: parameters.rateTypeId,
>> +                     periodTypeId: parameters.periodTypeId,
>> +                     rateCurrencyUomId: parameters.rateCurrencyUomId]
>> +    condition.put(field, parameters.get(field))
>> +    List ratesList = from('RateAmount').where(condition).filterByDate().
>> queryList()
>> +    if (UtilValidate.isEmpty(ratesList)) {
>> +        GenericValue periodType = from('PeriodType').where([periodTypeId:
>> parameters.periodTypeId]).queryOne()
>> +        GenericValue rateType = from('RateType').where([rateTypeId:
>> parameters.rateTypeId]).queryOne()
>> +        GenericValue partyNameView = from('PartyNameView').where([partyId:
>> parameters.partyId]).queryOne()
>> +        logError('A valid rate entry could be found for rateType:' +
>> rateType.description + ', ' + entityName + ':' + parameters.get(field)
>> +                + ', party: ' + partyNameView.lastName +
>> partyNameView.middleName + partyNameView.firstName + partyNameView.groupName
>> +                + ' However.....not for the period:' +
>> periodType.description + ' and currency:' + parameters.rateCurrencyUomId)
>> +    }
>> +    Map result = success()
>> +    result.ratesList = ratesList
>> +    return result
>> +}
>> +// Get all the rateAmount for a given workEffortId
>> +def getRatesAmountsFromWorkEffortId() {
>> +    return getRatesAmountsFrom('workEffortId')
>> +}
>> +// Get all the rateAmount for a given partyId
>> +def getRatesAmountsFromPartyId() {
>> +    return getRatesAmountsFrom('partyId')
>> +}
>> +// Get all the rateAmount for a given emplPositionTypeId
>> +def getRatesAmountsFromEmplPositionTypeId() {
>> +    return getRatesAmountsFrom('emplPositionTypeId')
>> +}
>> +
>> +//Filter a list of rateAmount. The result is the most heavily-filtered
>> non-empty list
>> +def filterRateAmountList() {
>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>> +        logWarning('The list parameters.ratesList was empty, not
>> processing any further')
>> +        return success()
>> +    }
>> +    //Check if there is a more specific rate
>> +    Map filterMap = [:]
>> +    if (parameters.workEffortId) {
>> +        filterMap.workEffortId = parameters.workEffortId
>> +    }
>> +    if (parameters.partyId) {
>> +        filterMap.partyId = parameters.partyId
>> +    }
>> +    if (parameters.emplPositionTypeId) {
>> +        filterMap.emplPositionTypeId = parameters.emplPositionTypeId
>> +    }
>> +    if (parameters.rateTypeId) {
>> +        filterMap.rateTypeId = parameters.rateTypeId
>> +    }
>> +    List tempRatesFilteredList = EntityUtil.filterByAnd(parameters.ratesList,
>> filterMap)
>> +    if (UtilValidate.isNotEmpty(tempRatesFilteredList)) {
>> +        parameters.ratesList = tempRatesFilteredList
>> +    }
>> +    Map result = success()
>> +    result.filteredRatesList = parameters.ratesList
>> +    return result
>> +}
>> \ No newline at end of file
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
>> servicedef/services_rate.xml
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>> applications/accounting/servicedef/services_rate.xml?
>> rev=1800245&r1=1800244&r2=1800245&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
>> (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
>> Thu Jun 29 07:46:02 2017
>> @@ -56,8 +56,8 @@ under the License.
>>           <override name="rateTypeId" optional="false"/>
>>           <override name="fromDate" optional="false"/>
>>       </service>
>> -    <service name="getRateAmount" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -        location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRateAmount">
>> +    <service name="getRateAmount" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +        location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRateAmount">
>>           <description>Get Rate Amount</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>> @@ -68,41 +68,38 @@ under the License.
>>           <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <override name="rateTypeId" optional="false"/>
>>       </service>
>> -    <service name="getRatesAmountsFromWorkEffortId" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRatesAmountsFromWorkEffortId">
>> +    <service name="getRatesAmountsFromWorkEffortId" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +        location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRatesAmountsFromWorkEffortId">
>>           <description>Get all Rates Amounts for a given
>> workEffortId</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>> optional="true"/>
>>           <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
>> optional="true"/>
>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <attribute name="ratesList" type="List" mode="OUT"
>> optional="true"/>
>>           <override name="workEffortId" optional="false"/>
>>       </service>
>> -    <service name="getRatesAmountsFromPartyId" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRatesAmountsFromPartyId">
>> +    <service name="getRatesAmountsFromPartyId" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRatesAmountsFromPartyId">
>>           <description>Get all Rates Amounts for a given
>> partyId</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>> optional="true"/>
>>           <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
>> optional="true"/>
>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <attribute name="ratesList" type="List" mode="OUT"
>> optional="true"/>
>>           <override name="partyId" optional="false"/>
>>       </service>
>> -    <service name="getRatesAmountsFromEmplPositionTypeId"
>> default-entity-name="RateAmount" engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRatesAmountsFromEmplPositionTypeId">
>> +    <service name="getRatesAmountsFromEmplPositionTypeId"
>> default-entity-name="RateAmount" engine="groovy" auth="true"
>> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRatesAmountsFromEmplPositionTypeId">
>>           <description>Get all Rates Amounts for a given
>> emplPositionTypeId</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>> optional="true"/>
>>           <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
>> optional="true"/>
>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <attribute name="ratesList" type="List" mode="OUT"
>> optional="true"/>
>>           <override name="emplPositionTypeId" optional="false"/>
>>       </service>
>> -    <service name="filterRateAmountList" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="filterRateAmountList">
>> +    <service name="filterRateAmountList" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="filterRateAmountList">
>>           <description>Get the most specific non-empty Rate Amount list
>> from a list of Rate Amount, given the input parameters :
>>           workEffortId, partyId, emplPositionTypeId and
>> rateTypeId</description>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>
>>
>>

[Nicolas Malin-2]

Come back,

Le 03/07/2017 à 21:19, Scott Gray a écrit :
> Hi Nicolas,
>
> My first code review in a while so I apologize if I'm wrong on any points,
> or if they've been discussed already.
>
> 1. It would be good if the services were converted one per commit, with the
> empty minilang file removed at the end with a separate commit.  That would
> allow reviewers to compare the removed XML code with the new groovy code.
No problem, I wished realize one commit per file but it's too
complicate, and you remark is good. Now I need to take time for repair
my broken git repository because it's more easier to do this with git
compare to svn (for me).
> 2. Defaults should (where possible) be moved to the service definition
Agree, I will check. If you found some move forget, it's a mistake.
Don't hesitate to spot them.
> 3. I could be wrong about this but I don't think it is a good practice to
> assign/change values in the parameters map, I think callers would not
> expect their inputs to be changed by the service.  But I'm not sure if that
> is actually happening or if groovy has copied the map beforehand, I'm also
> unsure of how minilang used to handle the same.
I see. The problem to convert directly a language :)
> 4. Because groovy has "Groovy Truth", UtilValidate.isEmpty/isNotEmpty isn't
> required, you can simply do "if (parameters.ratesList)".  An empty list or
> null would return false.
My bad, in my mind groovy tested only null, thanks for the correction
> 5. I think it would be better to add the service documentation to the
> service definition instead of a comment inline with the code.
:) Lazy I'm, just a copy paste the minilang to convert on the fly. I
will correct that
> 6. I'm curious about the getRateAmount() "level" variable, I see it is only
> defined within the local scope of the if/else blocks whereas "serviceName"
> was defined at the method scope.  Does groovy allow "level" to take on the
> method scope or is it an error?
heuuu je sais pas, I will look that. To ensure they are no regression I
use the integration test and some ui test, so maybe I didn't test the
area where this is used, or it was no impact on the code ^^

Thanks Scott I will do a second pass on the code with your remarks.
Cheers,
Nicolas

> Regards
> Scott
>
> On 29 June 2017 at 19:46, <[hidden email]> wrote:
>
>> Author: nmalin
>> Date: Thu Jun 29 07:46:02 2017
>> New Revision: 1800245
>>
>> URL: http://svn.apache.org/viewvc?rev=1800245&view=rev
>> Log:
>> Improved: Convert RateServices.xml mini-lang to groovyDSL (OFBIZ-9381)
>> finish the service conversion : getRateAmount,
>> getRatesAmountsFromWorkEffortId, getRatesAmountsFromPartyId,
>> getRatesAmountsFromEmplPositionTypeId and filterRateAmountList
>> I create a generic groovy function getRateAmoutForm to remove duplicate
>> code and refactoring the filterRateAmount for simplicity
>>
>> Removed:
>>      ofbiz/ofbiz-framework/trunk/applications/accounting/minilang/rate/
>> Modified:
>>      ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy
>>      ofbiz/ofbiz-framework/trunk/applications/accounting/
>> servicedef/services_rate.xml
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>> applications/accounting/groovyScripts/rate/RateServices.groovy?rev=
>> 1800245&r1=1800244&r2=1800245&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/
>> groovyScripts/rate/RateServices.groovy Thu Jun 29 07:46:02 2017
>> @@ -128,3 +128,142 @@ def expirePartyRate() {
>>       }
>>       return success()
>>   }
>> +
>> +// Get the applicable rate amount value
>> +def getRateAmount() {
>> +    /* Search for the applicable rate from most specific to most general
>> in the RateAmount entity
>> +    Defaults for periodTypeId is per hour and default currency is the
>> currency in general.properties
>> +    The order is:
>> +    1. for specific rateTypeId, workEffortId (workEffort)
>> +    2. for specific rateTypeId, partyId (party)
>> +    3. for specific rateTypeId, emplPositionTypeId (emplPositionType)
>> +    4. for specific rateTypeId (rateType)
>> +
>> +    Then, the results are filtered to improve the result. If you pass a
>> workEffortId and a partyId,
>> +    the service will first search the list of all the rateAmount with the
>> specified workEffortId. Then, if
>> +    there is at least one rateAmount with same partyId than the one in
>> the parameter in the list, the list will
>> +    be reduced to those entries.
>> +    At the end, the first record of the list is chosen.
>> +
>> +    For a easier debugging time, there is a log triggered when no records
>> are found for the input. This log
>> +    shows up when there are rateAmounts corresponding to the input
>> parameters without the rateCurrencyUomId and
>> +    the periodTypeId.*/
>> +    if (!parameters.rateCurrencyUomId) {
>> +        parameters.rateCurrencyUomId = UtilProperties.
>> getPropertyValue('general.properties', 'currency.uom.id.default', 'USD')
>> +    }
>> +    if (!parameters.periodTypeId) {
>> +        parameters.periodTypeId = 'RATE_HOUR'
>> +    }
>> +    String serviceName = null;
>> +    if (parameters.workEffortId && parameters.workEffortId != '_NA_') {
>> +        // workeffort level
>> +        level = 'workEffort'
>> +        serviceName = 'getRatesAmountsFromWorkEffortId'
>> +    } else if (parameters.partyId && parameters.partyId != '_NA_') {
>> +        // party level
>> +        level='partyId'
>> +        serviceName = 'getRatesAmountsFromPartyId'
>> +    } else if (parameters.emplPositionTypeId &&
>> parameters.emplPositionTypeId != '_NA_') {
>> +        // party level
>> +        level = 'emplPositionTypeId'
>> +        serviceName = 'getRatesAmountsFromEmplPositionTypeId'
>> +    }
>> +    if (serviceName) {
>> +        logError(parameters.toString() + " " + serviceName.toString())
>> +        Map result = run service: serviceName, with: parameters
>> +        parameters.ratesList = result.ratesList
>> +        logError(parameters.ratesList.toString())
>> +        result = run service: 'filterRateAmountList', with: parameters
>> +        parameters.ratesList = result.filteredRatesList
>> +    }
>> +
>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>> +        parameters.ratesList = from('RateAmount').where([rateTypeId:
>> parameters.rateTypeId]).queryList();
>> +        Map result = run service: 'filterRateAmountList', with: parameters
>> +        parameters.ratesList = EntityUtil.filterByDate(
>> result.filteredRatesList)
>> +    }
>> +
>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>> +        rateType = from('RateAmount').where([rateTypeId:
>> parameters.rateTypeId]).queryOne()
>> +        logError('A valid rate amount could not be found for rateType: '
>> + rateType.description)
>> +    }
>> +
>> +    // We narrowed as much as we could the result, now returning the
>> first record of the list
>> +    Map result = success()
>> +    if (UtilValidate.isNotEmpty(parameters.ratesList)) {
>> +        rateAmount = parameters.ratesList[0]
>> +        if (! rateAmount.rateAmount) rateAmount.rateAmount =
>> BigDecimal.ZERO
>> +        result.rateAmount = rateAmount.rateAmount
>> +        result.periodTypeId = rateAmount.periodTypeId
>> +        result.rateCurrencyUomId = rateAmount.rateCurrencyUomId
>> +        result.level = level
>> +        result.fromDate = rateAmount.fromDate
>> +    }
>> +    return result
>> +}
>> +
>> +//Generic fonction to resolve a rate amount from a pk field
>> +def getRatesAmountsFrom(String field) {
>> +    String entityName = null
>> +    if (field == 'workEffortId') entityName = 'WorkEffort'
>> +    if (field == 'partyId') entityName = 'Party'
>> +    if (field == 'emplPositionTypeId') entityName = 'EmplPositionType'
>> +
>> +    Map condition = [rateTypeId: parameters.rateTypeId,
>> +                     periodTypeId: parameters.periodTypeId,
>> +                     rateCurrencyUomId: parameters.rateCurrencyUomId]
>> +    condition.put(field, parameters.get(field))
>> +    List ratesList = from('RateAmount').where(condition).filterByDate().
>> queryList()
>> +    if (UtilValidate.isEmpty(ratesList)) {
>> +        GenericValue periodType = from('PeriodType').where([periodTypeId:
>> parameters.periodTypeId]).queryOne()
>> +        GenericValue rateType = from('RateType').where([rateTypeId:
>> parameters.rateTypeId]).queryOne()
>> +        GenericValue partyNameView = from('PartyNameView').where([partyId:
>> parameters.partyId]).queryOne()
>> +        logError('A valid rate entry could be found for rateType:' +
>> rateType.description + ', ' + entityName + ':' + parameters.get(field)
>> +                + ', party: ' + partyNameView.lastName +
>> partyNameView.middleName + partyNameView.firstName + partyNameView.groupName
>> +                + ' However.....not for the period:' +
>> periodType.description + ' and currency:' + parameters.rateCurrencyUomId)
>> +    }
>> +    Map result = success()
>> +    result.ratesList = ratesList
>> +    return result
>> +}
>> +// Get all the rateAmount for a given workEffortId
>> +def getRatesAmountsFromWorkEffortId() {
>> +    return getRatesAmountsFrom('workEffortId')
>> +}
>> +// Get all the rateAmount for a given partyId
>> +def getRatesAmountsFromPartyId() {
>> +    return getRatesAmountsFrom('partyId')
>> +}
>> +// Get all the rateAmount for a given emplPositionTypeId
>> +def getRatesAmountsFromEmplPositionTypeId() {
>> +    return getRatesAmountsFrom('emplPositionTypeId')
>> +}
>> +
>> +//Filter a list of rateAmount. The result is the most heavily-filtered
>> non-empty list
>> +def filterRateAmountList() {
>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>> +        logWarning('The list parameters.ratesList was empty, not
>> processing any further')
>> +        return success()
>> +    }
>> +    //Check if there is a more specific rate
>> +    Map filterMap = [:]
>> +    if (parameters.workEffortId) {
>> +        filterMap.workEffortId = parameters.workEffortId
>> +    }
>> +    if (parameters.partyId) {
>> +        filterMap.partyId = parameters.partyId
>> +    }
>> +    if (parameters.emplPositionTypeId) {
>> +        filterMap.emplPositionTypeId = parameters.emplPositionTypeId
>> +    }
>> +    if (parameters.rateTypeId) {
>> +        filterMap.rateTypeId = parameters.rateTypeId
>> +    }
>> +    List tempRatesFilteredList = EntityUtil.filterByAnd(parameters.ratesList,
>> filterMap)
>> +    if (UtilValidate.isNotEmpty(tempRatesFilteredList)) {
>> +        parameters.ratesList = tempRatesFilteredList
>> +    }
>> +    Map result = success()
>> +    result.filteredRatesList = parameters.ratesList
>> +    return result
>> +}
>> \ No newline at end of file
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
>> servicedef/services_rate.xml
>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>> applications/accounting/servicedef/services_rate.xml?
>> rev=1800245&r1=1800244&r2=1800245&view=diff
>> ============================================================
>> ==================
>> --- ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
>> (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
>> Thu Jun 29 07:46:02 2017
>> @@ -56,8 +56,8 @@ under the License.
>>           <override name="rateTypeId" optional="false"/>
>>           <override name="fromDate" optional="false"/>
>>       </service>
>> -    <service name="getRateAmount" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -        location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRateAmount">
>> +    <service name="getRateAmount" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +        location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRateAmount">
>>           <description>Get Rate Amount</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>> @@ -68,41 +68,38 @@ under the License.
>>           <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <override name="rateTypeId" optional="false"/>
>>       </service>
>> -    <service name="getRatesAmountsFromWorkEffortId" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRatesAmountsFromWorkEffortId">
>> +    <service name="getRatesAmountsFromWorkEffortId" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +        location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRatesAmountsFromWorkEffortId">
>>           <description>Get all Rates Amounts for a given
>> workEffortId</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>> optional="true"/>
>>           <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
>> optional="true"/>
>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <attribute name="ratesList" type="List" mode="OUT"
>> optional="true"/>
>>           <override name="workEffortId" optional="false"/>
>>       </service>
>> -    <service name="getRatesAmountsFromPartyId" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRatesAmountsFromPartyId">
>> +    <service name="getRatesAmountsFromPartyId" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRatesAmountsFromPartyId">
>>           <description>Get all Rates Amounts for a given
>> partyId</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>> optional="true"/>
>>           <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
>> optional="true"/>
>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <attribute name="ratesList" type="List" mode="OUT"
>> optional="true"/>
>>           <override name="partyId" optional="false"/>
>>       </service>
>> -    <service name="getRatesAmountsFromEmplPositionTypeId"
>> default-entity-name="RateAmount" engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="getRatesAmountsFromEmplPositionTypeId">
>> +    <service name="getRatesAmountsFromEmplPositionTypeId"
>> default-entity-name="RateAmount" engine="groovy" auth="true"
>> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="getRatesAmountsFromEmplPositionTypeId">
>>           <description>Get all Rates Amounts for a given
>> emplPositionTypeId</description>
>>           <permission-service service-name="acctgBasePermissionCheck"
>> main-action="VIEW"/>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>> optional="true"/>
>>           <attribute name="rateCurrencyUomId" type="String" mode="INOUT"
>> optional="true"/>
>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>> optional="true"/>
>>           <attribute name="ratesList" type="List" mode="OUT"
>> optional="true"/>
>>           <override name="emplPositionTypeId" optional="false"/>
>>       </service>
>> -    <service name="filterRateAmountList" default-entity-name="RateAmount"
>> engine="simple" auth="true"
>> -             location="component://accounting/minilang/rate/RateServices.xml"
>> invoke="filterRateAmountList">
>> +    <service name="filterRateAmountList" default-entity-name="RateAmount"
>> engine="groovy" auth="true"
>> +             location="component://accounting/groovyScripts/rate/RateServices.groovy"
>> invoke="filterRateAmountList">
>>           <description>Get the most specific non-empty Rate Amount list
>> from a list of Rate Amount, given the input parameters :
>>           workEffortId, partyId, emplPositionTypeId and
>> rateTypeId</description>
>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>
>>
>>

[Nicolas Malin-2]

Scott I committed differents correction related to your remarks. I hope
it's ok for you but in the other case, I'm open :)

Thanks

Nicolas


Le 04/07/2017 à 21:57, Nicolas Malin a écrit :

> Come back,
>
> Le 03/07/2017 à 21:19, Scott Gray a écrit :
>> Hi Nicolas,
>>
>> My first code review in a while so I apologize if I'm wrong on any
>> points,
>> or if they've been discussed already.
>>
>> 1. It would be good if the services were converted one per commit,
>> with the
>> empty minilang file removed at the end with a separate commit. That
>> would
>> allow reviewers to compare the removed XML code with the new groovy
>> code.
> No problem, I wished realize one commit per file but it's too
> complicate, and you remark is good. Now I need to take time for repair
> my broken git repository because it's more easier to do this with git
> compare to svn (for me).
>> 2. Defaults should (where possible) be moved to the service definition
> Agree, I will check. If you found some move forget, it's a mistake.
> Don't hesitate to spot them.
>> 3. I could be wrong about this but I don't think it is a good
>> practice to
>> assign/change values in the parameters map, I think callers would not
>> expect their inputs to be changed by the service.  But I'm not sure
>> if that
>> is actually happening or if groovy has copied the map beforehand, I'm
>> also
>> unsure of how minilang used to handle the same.
> I see. The problem to convert directly a language :)
>> 4. Because groovy has "Groovy Truth", UtilValidate.isEmpty/isNotEmpty
>> isn't
>> required, you can simply do "if (parameters.ratesList)".  An empty
>> list or
>> null would return false.
> My bad, in my mind groovy tested only null, thanks for the correction
>> 5. I think it would be better to add the service documentation to the
>> service definition instead of a comment inline with the code.
> :) Lazy I'm, just a copy paste the minilang to convert on the fly. I
> will correct that
>> 6. I'm curious about the getRateAmount() "level" variable, I see it
>> is only
>> defined within the local scope of the if/else blocks whereas
>> "serviceName"
>> was defined at the method scope.  Does groovy allow "level" to take
>> on the
>> method scope or is it an error?
> heuuu je sais pas, I will look that. To ensure they are no regression
> I use the integration test and some ui test, so maybe I didn't test
> the area where this is used, or it was no impact on the code ^^
>
> Thanks Scott I will do a second pass on the code with your remarks.
> Cheers,
> Nicolas
>> Regards
>> Scott
>>
>> On 29 June 2017 at 19:46, <[hidden email]> wrote:
>>
>>> Author: nmalin
>>> Date: Thu Jun 29 07:46:02 2017
>>> New Revision: 1800245
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1800245&view=rev
>>> Log:
>>> Improved: Convert RateServices.xml mini-lang to groovyDSL (OFBIZ-9381)
>>> finish the service conversion : getRateAmount,
>>> getRatesAmountsFromWorkEffortId, getRatesAmountsFromPartyId,
>>> getRatesAmountsFromEmplPositionTypeId and filterRateAmountList
>>> I create a generic groovy function getRateAmoutForm to remove duplicate
>>> code and refactoring the filterRateAmount for simplicity
>>>
>>> Removed:
>>> ofbiz/ofbiz-framework/trunk/applications/accounting/minilang/rate/
>>> Modified:
>>>      ofbiz/ofbiz-framework/trunk/applications/accounting/
>>> groovyScripts/rate/RateServices.groovy
>>>      ofbiz/ofbiz-framework/trunk/applications/accounting/
>>> servicedef/services_rate.xml
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
>>> groovyScripts/rate/RateServices.groovy
>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>> applications/accounting/groovyScripts/rate/RateServices.groovy?rev=
>>> 1800245&r1=1800244&r2=1800245&view=diff
>>> ============================================================
>>> ==================
>>> --- ofbiz/ofbiz-framework/trunk/applications/accounting/
>>> groovyScripts/rate/RateServices.groovy (original)
>>> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/
>>> groovyScripts/rate/RateServices.groovy Thu Jun 29 07:46:02 2017
>>> @@ -128,3 +128,142 @@ def expirePartyRate() {
>>>       }
>>>       return success()
>>>   }
>>> +
>>> +// Get the applicable rate amount value
>>> +def getRateAmount() {
>>> +    /* Search for the applicable rate from most specific to most
>>> general
>>> in the RateAmount entity
>>> +    Defaults for periodTypeId is per hour and default currency is the
>>> currency in general.properties
>>> +    The order is:
>>> +    1. for specific rateTypeId, workEffortId (workEffort)
>>> +    2. for specific rateTypeId, partyId (party)
>>> +    3. for specific rateTypeId, emplPositionTypeId (emplPositionType)
>>> +    4. for specific rateTypeId (rateType)
>>> +
>>> +    Then, the results are filtered to improve the result. If you
>>> pass a
>>> workEffortId and a partyId,
>>> +    the service will first search the list of all the rateAmount
>>> with the
>>> specified workEffortId. Then, if
>>> +    there is at least one rateAmount with same partyId than the one in
>>> the parameter in the list, the list will
>>> +    be reduced to those entries.
>>> +    At the end, the first record of the list is chosen.
>>> +
>>> +    For a easier debugging time, there is a log triggered when no
>>> records
>>> are found for the input. This log
>>> +    shows up when there are rateAmounts corresponding to the input
>>> parameters without the rateCurrencyUomId and
>>> +    the periodTypeId.*/
>>> +    if (!parameters.rateCurrencyUomId) {
>>> +        parameters.rateCurrencyUomId = UtilProperties.
>>> getPropertyValue('general.properties', 'currency.uom.id.default',
>>> 'USD')
>>> +    }
>>> +    if (!parameters.periodTypeId) {
>>> +        parameters.periodTypeId = 'RATE_HOUR'
>>> +    }
>>> +    String serviceName = null;
>>> +    if (parameters.workEffortId && parameters.workEffortId !=
>>> '_NA_') {
>>> +        // workeffort level
>>> +        level = 'workEffort'
>>> +        serviceName = 'getRatesAmountsFromWorkEffortId'
>>> +    } else if (parameters.partyId && parameters.partyId != '_NA_') {
>>> +        // party level
>>> +        level='partyId'
>>> +        serviceName = 'getRatesAmountsFromPartyId'
>>> +    } else if (parameters.emplPositionTypeId &&
>>> parameters.emplPositionTypeId != '_NA_') {
>>> +        // party level
>>> +        level = 'emplPositionTypeId'
>>> +        serviceName = 'getRatesAmountsFromEmplPositionTypeId'
>>> +    }
>>> +    if (serviceName) {
>>> +        logError(parameters.toString() + " " + serviceName.toString())
>>> +        Map result = run service: serviceName, with: parameters
>>> +        parameters.ratesList = result.ratesList
>>> +        logError(parameters.ratesList.toString())
>>> +        result = run service: 'filterRateAmountList', with: parameters
>>> +        parameters.ratesList = result.filteredRatesList
>>> +    }
>>> +
>>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>>> +        parameters.ratesList = from('RateAmount').where([rateTypeId:
>>> parameters.rateTypeId]).queryList();
>>> +        Map result = run service: 'filterRateAmountList', with:
>>> parameters
>>> +        parameters.ratesList = EntityUtil.filterByDate(
>>> result.filteredRatesList)
>>> +    }
>>> +
>>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>>> +        rateType = from('RateAmount').where([rateTypeId:
>>> parameters.rateTypeId]).queryOne()
>>> +        logError('A valid rate amount could not be found for
>>> rateType: '
>>> + rateType.description)
>>> +    }
>>> +
>>> +    // We narrowed as much as we could the result, now returning the
>>> first record of the list
>>> +    Map result = success()
>>> +    if (UtilValidate.isNotEmpty(parameters.ratesList)) {
>>> +        rateAmount = parameters.ratesList[0]
>>> +        if (! rateAmount.rateAmount) rateAmount.rateAmount =
>>> BigDecimal.ZERO
>>> +        result.rateAmount = rateAmount.rateAmount
>>> +        result.periodTypeId = rateAmount.periodTypeId
>>> +        result.rateCurrencyUomId = rateAmount.rateCurrencyUomId
>>> +        result.level = level
>>> +        result.fromDate = rateAmount.fromDate
>>> +    }
>>> +    return result
>>> +}
>>> +
>>> +//Generic fonction to resolve a rate amount from a pk field
>>> +def getRatesAmountsFrom(String field) {
>>> +    String entityName = null
>>> +    if (field == 'workEffortId') entityName = 'WorkEffort'
>>> +    if (field == 'partyId') entityName = 'Party'
>>> +    if (field == 'emplPositionTypeId') entityName = 'EmplPositionType'
>>> +
>>> +    Map condition = [rateTypeId: parameters.rateTypeId,
>>> +                     periodTypeId: parameters.periodTypeId,
>>> +                     rateCurrencyUomId: parameters.rateCurrencyUomId]
>>> +    condition.put(field, parameters.get(field))
>>> +    List ratesList =
>>> from('RateAmount').where(condition).filterByDate().
>>> queryList()
>>> +    if (UtilValidate.isEmpty(ratesList)) {
>>> +        GenericValue periodType =
>>> from('PeriodType').where([periodTypeId:
>>> parameters.periodTypeId]).queryOne()
>>> +        GenericValue rateType = from('RateType').where([rateTypeId:
>>> parameters.rateTypeId]).queryOne()
>>> +        GenericValue partyNameView =
>>> from('PartyNameView').where([partyId:
>>> parameters.partyId]).queryOne()
>>> +        logError('A valid rate entry could be found for rateType:' +
>>> rateType.description + ', ' + entityName + ':' + parameters.get(field)
>>> +                + ', party: ' + partyNameView.lastName +
>>> partyNameView.middleName + partyNameView.firstName +
>>> partyNameView.groupName
>>> +                + ' However.....not for the period:' +
>>> periodType.description + ' and currency:' +
>>> parameters.rateCurrencyUomId)
>>> +    }
>>> +    Map result = success()
>>> +    result.ratesList = ratesList
>>> +    return result
>>> +}
>>> +// Get all the rateAmount for a given workEffortId
>>> +def getRatesAmountsFromWorkEffortId() {
>>> +    return getRatesAmountsFrom('workEffortId')
>>> +}
>>> +// Get all the rateAmount for a given partyId
>>> +def getRatesAmountsFromPartyId() {
>>> +    return getRatesAmountsFrom('partyId')
>>> +}
>>> +// Get all the rateAmount for a given emplPositionTypeId
>>> +def getRatesAmountsFromEmplPositionTypeId() {
>>> +    return getRatesAmountsFrom('emplPositionTypeId')
>>> +}
>>> +
>>> +//Filter a list of rateAmount. The result is the most heavily-filtered
>>> non-empty list
>>> +def filterRateAmountList() {
>>> +    if (UtilValidate.isEmpty(parameters.ratesList)) {
>>> +        logWarning('The list parameters.ratesList was empty, not
>>> processing any further')
>>> +        return success()
>>> +    }
>>> +    //Check if there is a more specific rate
>>> +    Map filterMap = [:]
>>> +    if (parameters.workEffortId) {
>>> +        filterMap.workEffortId = parameters.workEffortId
>>> +    }
>>> +    if (parameters.partyId) {
>>> +        filterMap.partyId = parameters.partyId
>>> +    }
>>> +    if (parameters.emplPositionTypeId) {
>>> +        filterMap.emplPositionTypeId = parameters.emplPositionTypeId
>>> +    }
>>> +    if (parameters.rateTypeId) {
>>> +        filterMap.rateTypeId = parameters.rateTypeId
>>> +    }
>>> +    List tempRatesFilteredList =
>>> EntityUtil.filterByAnd(parameters.ratesList,
>>> filterMap)
>>> +    if (UtilValidate.isNotEmpty(tempRatesFilteredList)) {
>>> +        parameters.ratesList = tempRatesFilteredList
>>> +    }
>>> +    Map result = success()
>>> +    result.filteredRatesList = parameters.ratesList
>>> +    return result
>>> +}
>>> \ No newline at end of file
>>>
>>> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/
>>> servicedef/services_rate.xml
>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>> applications/accounting/servicedef/services_rate.xml?
>>> rev=1800245&r1=1800244&r2=1800245&view=diff
>>> ============================================================
>>> ==================
>>> ---
>>> ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
>>> (original)
>>> +++
>>> ofbiz/ofbiz-framework/trunk/applications/accounting/servicedef/services_rate.xml
>>> Thu Jun 29 07:46:02 2017
>>> @@ -56,8 +56,8 @@ under the License.
>>>           <override name="rateTypeId" optional="false"/>
>>>           <override name="fromDate" optional="false"/>
>>>       </service>
>>> -    <service name="getRateAmount" default-entity-name="RateAmount"
>>> engine="simple" auth="true"
>>> - location="component://accounting/minilang/rate/RateServices.xml"
>>> invoke="getRateAmount">
>>> +    <service name="getRateAmount" default-entity-name="RateAmount"
>>> engine="groovy" auth="true"
>>> +
>>> location="component://accounting/groovyScripts/rate/RateServices.groovy"
>>>
>>> invoke="getRateAmount">
>>>           <description>Get Rate Amount</description>
>>>           <permission-service service-name="acctgBasePermissionCheck"
>>> main-action="VIEW"/>
>>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>> @@ -68,41 +68,38 @@ under the License.
>>>           <attribute name="fromDate" type="Timestamp" mode="OUT"
>>> optional="true"/>
>>>           <override name="rateTypeId" optional="false"/>
>>>       </service>
>>> -    <service name="getRatesAmountsFromWorkEffortId"
>>> default-entity-name="RateAmount"
>>> engine="simple" auth="true"
>>> - location="component://accounting/minilang/rate/RateServices.xml"
>>> invoke="getRatesAmountsFromWorkEffortId">
>>> +    <service name="getRatesAmountsFromWorkEffortId"
>>> default-entity-name="RateAmount"
>>> engine="groovy" auth="true"
>>> +
>>> location="component://accounting/groovyScripts/rate/RateServices.groovy"
>>>
>>> invoke="getRatesAmountsFromWorkEffortId">
>>>           <description>Get all Rates Amounts for a given
>>> workEffortId</description>
>>>           <permission-service service-name="acctgBasePermissionCheck"
>>> main-action="VIEW"/>
>>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>>> optional="true"/>
>>>           <attribute name="rateCurrencyUomId" type="String"
>>> mode="INOUT"
>>> optional="true"/>
>>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>>> optional="true"/>
>>>           <attribute name="ratesList" type="List" mode="OUT"
>>> optional="true"/>
>>>           <override name="workEffortId" optional="false"/>
>>>       </service>
>>> -    <service name="getRatesAmountsFromPartyId"
>>> default-entity-name="RateAmount"
>>> engine="simple" auth="true"
>>> - location="component://accounting/minilang/rate/RateServices.xml"
>>> invoke="getRatesAmountsFromPartyId">
>>> +    <service name="getRatesAmountsFromPartyId"
>>> default-entity-name="RateAmount"
>>> engine="groovy" auth="true"
>>> +
>>> location="component://accounting/groovyScripts/rate/RateServices.groovy"
>>>
>>> invoke="getRatesAmountsFromPartyId">
>>>           <description>Get all Rates Amounts for a given
>>> partyId</description>
>>>           <permission-service service-name="acctgBasePermissionCheck"
>>> main-action="VIEW"/>
>>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>>> optional="true"/>
>>>           <attribute name="rateCurrencyUomId" type="String"
>>> mode="INOUT"
>>> optional="true"/>
>>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>>> optional="true"/>
>>>           <attribute name="ratesList" type="List" mode="OUT"
>>> optional="true"/>
>>>           <override name="partyId" optional="false"/>
>>>       </service>
>>> -    <service name="getRatesAmountsFromEmplPositionTypeId"
>>> default-entity-name="RateAmount" engine="simple" auth="true"
>>> - location="component://accounting/minilang/rate/RateServices.xml"
>>> invoke="getRatesAmountsFromEmplPositionTypeId">
>>> +    <service name="getRatesAmountsFromEmplPositionTypeId"
>>> default-entity-name="RateAmount" engine="groovy" auth="true"
>>> +
>>> location="component://accounting/groovyScripts/rate/RateServices.groovy"
>>>
>>> invoke="getRatesAmountsFromEmplPositionTypeId">
>>>           <description>Get all Rates Amounts for a given
>>> emplPositionTypeId</description>
>>>           <permission-service service-name="acctgBasePermissionCheck"
>>> main-action="VIEW"/>
>>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>>           <attribute name="periodTypeId" type="String" mode="INOUT"
>>> optional="true"/>
>>>           <attribute name="rateCurrencyUomId" type="String"
>>> mode="INOUT"
>>> optional="true"/>
>>> -        <attribute name="fromDate" type="Timestamp" mode="OUT"
>>> optional="true"/>
>>>           <attribute name="ratesList" type="List" mode="OUT"
>>> optional="true"/>
>>>           <override name="emplPositionTypeId" optional="false"/>
>>>       </service>
>>> -    <service name="filterRateAmountList"
>>> default-entity-name="RateAmount"
>>> engine="simple" auth="true"
>>> - location="component://accounting/minilang/rate/RateServices.xml"
>>> invoke="filterRateAmountList">
>>> +    <service name="filterRateAmountList"
>>> default-entity-name="RateAmount"
>>> engine="groovy" auth="true"
>>> +
>>> location="component://accounting/groovyScripts/rate/RateServices.groovy"
>>>
>>> invoke="filterRateAmountList">
>>>           <description>Get the most specific non-empty Rate Amount list
>>> from a list of Rate Amount, given the input parameters :
>>>           workEffortId, partyId, emplPositionTypeId and
>>> rateTypeId</description>
>>>           <auto-attributes include="pk" mode="IN" optional="true"/>
>>>
>>>
>>>
>
>