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, <nma...@apache.org> 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"/> > > >
