You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ofbiz.apache.org by "danwatford (via GitHub)" <gi...@apache.org> on 2023/02/07 16:00:08 UTC
[GitHub] [ofbiz-framework] danwatford commented on a diff in pull request #517: Codenarc integration
danwatford commented on code in PR #517:
URL: https://github.com/apache/ofbiz-framework/pull/517#discussion_r1089690105
##########
framework/webtools/groovyScripts/stats/StatsSinceStart.groovy:
##########
@@ -85,17 +60,23 @@ while (iterator.hasNext()) {
statsId = iterator.next()
Review Comment:
Can requestIdMap can be removed?
##########
framework/webtools/groovyScripts/service/JobDetails.groovy:
##########
@@ -22,15 +22,15 @@ import org.apache.ofbiz.entity.GenericValue
import org.apache.ofbiz.base.util.UtilGenerics
import org.apache.ofbiz.entity.serialize.XmlSerializer
-GenericValue job = ((Delegator)delegator).findOne("JobSandbox", [jobId:parameters.jobId], false)
+GenericValue job = ((Delegator) delegator).findOne('JobSandbox', [jobId: parameters.jobId], false)
context.job = job
if (job) {
- GenericValue runtimeData = job.getRelatedOne("RuntimeData", false)
+ GenericValue runtimeData = job.getRelatedOne('RuntimeData', false)
if (runtimeData) {
- runtimeInfoMap = UtilGenerics.checkMap(XmlSerializer.deserialize(runtimeData.getString("runtimeInfo"), delegator), String.class, Object.class)
+ runtimeInfoMap = UtilGenerics.checkMap(XmlSerializer.deserialize(runtimeData.getString('runtimeInfo'), delegator), String, Object)
Review Comment:
Is it okay to drop the .class field when referencing a type in Groovy? Anyone have a link to the relevant part of the language documentation?
##########
framework/webtools/groovyScripts/entity/XmlDsDump.groovy:
##########
@@ -17,151 +17,158 @@
* under the License.
*/
-import java.util.*
-import java.io.*
-import org.apache.ofbiz.base.util.*
-import org.apache.ofbiz.entity.model.*
-import org.apache.ofbiz.entity.util.*
-import org.apache.ofbiz.entity.transaction.*
-import org.apache.ofbiz.entity.condition.*
+import org.apache.ofbiz.base.util.Debug
+import org.apache.ofbiz.base.util.UtilFormatOut
+import org.apache.ofbiz.entity.condition.EntityComparisonOperator
+import org.apache.ofbiz.entity.condition.EntityCondition
+import org.apache.ofbiz.entity.condition.EntityJoinOperator
+import org.apache.ofbiz.entity.model.ModelViewEntity
+import org.apache.ofbiz.entity.transaction.TransactionUtil
+import org.apache.ofbiz.entity.util.EntityFindOptions
outpath = parameters.outpath
filename = parameters.filename
maxRecStr = parameters.maxrecords
entitySyncId = parameters.entitySyncId
passedEntityNames = null
-if (parameters.entityName) passedEntityNames = parameters.entityName instanceof Collection ? parameters.entityName as TreeSet : [parameters.entityName] as TreeSet
+if (parameters.entityName) {
+ passedEntityNames = parameters.entityName instanceof Collection ? parameters.entityName as TreeSet : [parameters.entityName] as TreeSet
+}
// get the max records per file setting and convert to a int
-maxRecordsPerFile = 0
-if (maxRecStr) {
- try {
- maxRecordsPerFile = Integer.parseInt(maxRecStr)
- }
- catch (Exception e) {
- }
-}
+int maxRecordsPerFile = maxRecStr ? (maxRecStr as int) : 0
Review Comment:
This will throw an exception if `maxRecStr` does not represent an integer.
```suggestion
int maxRecordsPerFile = maxRecStr && maxRecStr.isInteger() ? (maxRecStr as int) : 0
```
##########
applications/accounting/groovyScripts/invoice/EditInvoice.groovy:
##########
@@ -94,58 +91,57 @@ if (invoice) {
context.invoiceItems = invoiceItemsConv
- invoiceTotal = InvoiceWorker.getInvoiceTotal(invoice).multiply(conversionRate).setScale(decimals, rounding)
- invoiceNoTaxTotal = InvoiceWorker.getInvoiceNoTaxTotal(invoice).multiply(conversionRate).setScale(decimals, rounding)
+ invoiceTotal = InvoiceWorker.getInvoiceTotal(invoice) * conversionRate.setScale(decimals, rounding)
Review Comment:
This looks like a possible change in behaviour.
Before the change, _invoiceTotal_ and _conversionRate_ were multiplied together and then the result was scaled via setScale().
After the change, _conversionRate_ is scaled and then the result is multiplied with _invoiceTotal_.
A similar change of behaviour exists in the following line, too.
##########
applications/accounting/groovyScripts/invoice/PrintInvoices.groovy:
##########
@@ -26,39 +25,40 @@ import java.text.DateFormat
invoiceDetailList = []
invoiceIds.each { invoiceId ->
invoicesMap = [:]
- invoice = from("Invoice").where('invoiceId', invoiceId).queryOne()
+ invoice = from('Invoice').where('invoiceId', invoiceId).queryOne()
invoicesMap.invoice = invoice
-
- currency = parameters.currency // allow the display of the invoice in the original currency, the default is to display the invoice in the default currency
- BigDecimal conversionRate = new BigDecimal("1")
+
+ currency = parameters.currency // allow the display of the invoice in the original currency,
+ // the default is to display the invoice in the default currency
+ BigDecimal conversionRate = new BigDecimal('1')
ZERO = BigDecimal.ZERO
- decimals = UtilNumber.getBigDecimalScale("invoice.decimals")
- rounding = UtilNumber.getBigDecimalRoundingMode("invoice.rounding")
-
+ decimals = UtilNumber.getBigDecimalScale('invoice.decimals')
+ rounding = UtilNumber.getBigDecimalRoundingMode('invoice.rounding')
+
if (invoice) {
- if (currency && !invoice.getString("currencyUomId").equals(currency)) {
+ if (currency && invoice.getString('currencyUomId') != currency) {
conversionRate = InvoiceWorker.getInvoiceCurrencyConversionRate(invoice)
invoice.currencyUomId = currency
- invoice.invoiceMessage = " converted from original with a rate of: " + conversionRate.setScale(8, rounding)
+ invoice.invoiceMessage = ' converted from original with a rate of: ' + conversionRate.setScale(8, rounding)
}
-
- invoiceItems = invoice.getRelated("InvoiceItem", null, ["invoiceItemSeqId"], false)
+
+ invoiceItems = invoice.getRelated('InvoiceItem', null, ['invoiceItemSeqId'], false)
invoiceItemsConv = []
invoiceItems.each { invoiceItem ->
- if (invoiceItem.amount) {
- invoiceItem.amount = invoiceItem.getBigDecimal("amount").multiply(conversionRate).setScale(decimals, rounding)
- invoiceItemsConv.add(invoiceItem)
- }
+ if (invoiceItem.amount) {
+ invoiceItem.amount = invoiceItem.getBigDecimal('amount') * conversionRate.setScale(decimals, rounding)
+ invoiceItemsConv.add(invoiceItem)
+ }
}
-
+
invoicesMap.invoiceItems = invoiceItemsConv
-
- invoiceTotal = InvoiceWorker.getInvoiceTotal(invoice).multiply(conversionRate).setScale(decimals, rounding)
- invoiceNoTaxTotal = InvoiceWorker.getInvoiceNoTaxTotal(invoice).multiply(conversionRate).setScale(decimals, rounding)
+
+ invoiceTotal = InvoiceWorker.getInvoiceTotal(invoice) * conversionRate.setScale(decimals, rounding)
Review Comment:
Behaviour change: scaling should be applied to multiplication result
##########
applications/accounting/groovyScripts/invoice/PrintInvoices.groovy:
##########
@@ -26,39 +25,40 @@ import java.text.DateFormat
invoiceDetailList = []
invoiceIds.each { invoiceId ->
invoicesMap = [:]
- invoice = from("Invoice").where('invoiceId', invoiceId).queryOne()
+ invoice = from('Invoice').where('invoiceId', invoiceId).queryOne()
invoicesMap.invoice = invoice
-
- currency = parameters.currency // allow the display of the invoice in the original currency, the default is to display the invoice in the default currency
- BigDecimal conversionRate = new BigDecimal("1")
+
+ currency = parameters.currency // allow the display of the invoice in the original currency,
+ // the default is to display the invoice in the default currency
+ BigDecimal conversionRate = new BigDecimal('1')
ZERO = BigDecimal.ZERO
- decimals = UtilNumber.getBigDecimalScale("invoice.decimals")
- rounding = UtilNumber.getBigDecimalRoundingMode("invoice.rounding")
-
+ decimals = UtilNumber.getBigDecimalScale('invoice.decimals')
+ rounding = UtilNumber.getBigDecimalRoundingMode('invoice.rounding')
+
if (invoice) {
- if (currency && !invoice.getString("currencyUomId").equals(currency)) {
+ if (currency && invoice.getString('currencyUomId') != currency) {
conversionRate = InvoiceWorker.getInvoiceCurrencyConversionRate(invoice)
invoice.currencyUomId = currency
- invoice.invoiceMessage = " converted from original with a rate of: " + conversionRate.setScale(8, rounding)
+ invoice.invoiceMessage = ' converted from original with a rate of: ' + conversionRate.setScale(8, rounding)
}
-
- invoiceItems = invoice.getRelated("InvoiceItem", null, ["invoiceItemSeqId"], false)
+
+ invoiceItems = invoice.getRelated('InvoiceItem', null, ['invoiceItemSeqId'], false)
invoiceItemsConv = []
invoiceItems.each { invoiceItem ->
- if (invoiceItem.amount) {
- invoiceItem.amount = invoiceItem.getBigDecimal("amount").multiply(conversionRate).setScale(decimals, rounding)
- invoiceItemsConv.add(invoiceItem)
- }
+ if (invoiceItem.amount) {
+ invoiceItem.amount = invoiceItem.getBigDecimal('amount') * conversionRate.setScale(decimals, rounding)
Review Comment:
Behaviour change: conversionRate is being scaled before it is multipled by amount. Scaling should be applied after the multiplication.
##########
applications/accounting/groovyScripts/invoice/PrintInvoices.groovy:
##########
@@ -26,39 +25,40 @@ import java.text.DateFormat
invoiceDetailList = []
invoiceIds.each { invoiceId ->
invoicesMap = [:]
- invoice = from("Invoice").where('invoiceId', invoiceId).queryOne()
+ invoice = from('Invoice').where('invoiceId', invoiceId).queryOne()
invoicesMap.invoice = invoice
-
- currency = parameters.currency // allow the display of the invoice in the original currency, the default is to display the invoice in the default currency
- BigDecimal conversionRate = new BigDecimal("1")
+
+ currency = parameters.currency // allow the display of the invoice in the original currency,
+ // the default is to display the invoice in the default currency
+ BigDecimal conversionRate = new BigDecimal('1')
ZERO = BigDecimal.ZERO
- decimals = UtilNumber.getBigDecimalScale("invoice.decimals")
- rounding = UtilNumber.getBigDecimalRoundingMode("invoice.rounding")
-
+ decimals = UtilNumber.getBigDecimalScale('invoice.decimals')
+ rounding = UtilNumber.getBigDecimalRoundingMode('invoice.rounding')
+
if (invoice) {
- if (currency && !invoice.getString("currencyUomId").equals(currency)) {
+ if (currency && invoice.getString('currencyUomId') != currency) {
conversionRate = InvoiceWorker.getInvoiceCurrencyConversionRate(invoice)
invoice.currencyUomId = currency
- invoice.invoiceMessage = " converted from original with a rate of: " + conversionRate.setScale(8, rounding)
+ invoice.invoiceMessage = ' converted from original with a rate of: ' + conversionRate.setScale(8, rounding)
}
-
- invoiceItems = invoice.getRelated("InvoiceItem", null, ["invoiceItemSeqId"], false)
+
+ invoiceItems = invoice.getRelated('InvoiceItem', null, ['invoiceItemSeqId'], false)
invoiceItemsConv = []
invoiceItems.each { invoiceItem ->
- if (invoiceItem.amount) {
- invoiceItem.amount = invoiceItem.getBigDecimal("amount").multiply(conversionRate).setScale(decimals, rounding)
- invoiceItemsConv.add(invoiceItem)
- }
+ if (invoiceItem.amount) {
+ invoiceItem.amount = invoiceItem.getBigDecimal('amount') * conversionRate.setScale(decimals, rounding)
+ invoiceItemsConv.add(invoiceItem)
+ }
}
-
+
invoicesMap.invoiceItems = invoiceItemsConv
-
- invoiceTotal = InvoiceWorker.getInvoiceTotal(invoice).multiply(conversionRate).setScale(decimals, rounding)
- invoiceNoTaxTotal = InvoiceWorker.getInvoiceNoTaxTotal(invoice).multiply(conversionRate).setScale(decimals, rounding)
+
+ invoiceTotal = InvoiceWorker.getInvoiceTotal(invoice) * conversionRate.setScale(decimals, rounding)
+ invoiceNoTaxTotal = InvoiceWorker.getInvoiceNoTaxTotal(invoice) * conversionRate.setScale(decimals, rounding)
Review Comment:
Behaviour change: scaling should be applied to multiplication result
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ofbiz.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org