Groovy DSL cleanup: WorkEffort helper, double-negation fixes, update() adoption

applications/accounting/config/AccountingUiLabels.xml

@@ -13838,6 +13838,9 @@
         <value xml:lang="zh">财务状态</value>
         <value xml:lang="zh-TW">財務狀態</value>
     </property>
+    <property key="AccountingStatusItemNotFound">
+        <value xml:lang="en">Status [${statusId}] not found</value>
+    </property>
     <property key="AccountingSubsidiaryLedger">
         <value xml:lang="ar">دفتر الشركة التابعة</value>
         <value xml:lang="de">Nebenbuch</value>

applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/invoice/InvoiceServicesScript.groovy

@@ -70,7 +70,8 @@ Map getNextInvoiceId() {
         if (invoiceIdTemp) {
             //check the provided ID
             String errorMsg = UtilValidate.checkValidDatabaseId(invoiceIdTemp)
-            require(!(errorMsg != null), "In getNextInvoiceId ${errorMsg}")
+            boolean isInvoiceIdValid = errorMsg == null
+            require(isInvoiceIdValid, "In getNextInvoiceId ${errorMsg}")
         } else {
             invoiceIdTemp = delegator.getNextSeqId('Invoice', 1)
         }
@@ -169,7 +170,7 @@ Map getInvoice() {
 Map updateInvoice() {
     GenericValue invoice = from('Invoice').where(parameters).queryOne()
     require(invoice as boolean, label('AccountingUiLabels', 'AccountingInvoiceNotFound', parameters))
-    require(!(invoice.statusId != 'INVOICE_IN_PROCESS'),
+    require(invoice.statusId == 'INVOICE_IN_PROCESS',
             label('AccountingUiLabels', 'AccountingInvoiceUpdateOnlyWithInProcessStatus', [statusId: invoice.statusId]))
 
     // only save if something has changed, do not update status here
@@ -240,14 +241,14 @@ Map setInvoiceStatus() {
         return success(returnResult)
     }
 
-    require(!(from('StatusValidChange')
+    require(from('StatusValidChange')
             .where(statusId: oldStatusId, statusIdTo: parameters.statusId)
-            .queryCount() == 0), label('AccountingUiLabels', 'AccountingPSInvalidStatusChange'))
+            .queryCount() > 0, label('AccountingUiLabels', 'AccountingPSInvalidStatusChange'))
 
     // if new status is paid check if the complete invoice is applied
     if (parameters.statusId == 'INVOICE_PAID') {
         BigDecimal notApplied = InvoiceWorker.getInvoiceNotApplied(invoice)
-        require(!(notApplied != 0), label('AccountingUiLabels', 'AccountingInvoiceCannotChangeStatusToPaid'))
+        require(notApplied == 0, label('AccountingUiLabels', 'AccountingInvoiceCannotChangeStatusToPaid'))
         // if it's OK to mark invoice paid, use parameters for paidDate
         invoice.paidDate = parameters.paidDate ?: UtilDateTime.nowTimestamp()
     }
@@ -362,7 +363,7 @@ Map createInvoiceItem() {
         }
     }
     // accept 0
-    require(!(invoiceItem.amount == null), label('AccountingUiLabels', 'AccountingInvoiceAmountIsMandatory'))
+    require(invoiceItem.amount != null, label('AccountingUiLabels', 'AccountingInvoiceAmountIsMandatory'))
     invoiceItem.create()
     return success([invoiceId: invoiceItem.invoiceId,
                     invoiceItemSeqId: invoiceItem.invoiceItemSeqId])
@@ -385,7 +386,7 @@ Map updateInvoiceItem() {
         Map serviceResult = run service: 'calculateProductPrice', with: [product: product]
         invoiceItem.amount = serviceResult.price
     }
-    require(!(invoiceItem.amount == null), label('AccountingUiLabels', 'AccountingInvoiceAmountIsMandatory'))
+    require(invoiceItem.amount != null, label('AccountingUiLabels', 'AccountingInvoiceAmountIsMandatory'))
     if (lookedInvoiceItem != invoiceItem) {
         invoiceItem.store()
     }

applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/payment/PaymentServices.groovy

@@ -32,6 +32,7 @@ import org.apache.ofbiz.entity.condition.EntityConditionBuilder
 import org.apache.ofbiz.entity.condition.EntityOperator
 import org.apache.ofbiz.entity.util.EntityTypeUtil
 import org.apache.ofbiz.entity.util.EntityUtilProperties
+import org.apache.ofbiz.service.ServiceErrorException
 import org.apache.ofbiz.service.ServiceUtil
 
 Map createPayment() {
@@ -144,7 +145,7 @@ Map updatePayment() {
         oldPayment.comments = newPayment.comments
         oldPayment.paymentRefNum = newPayment.paymentRefNum ?: null
         oldPayment.finAccountTransId = newPayment.finAccountTransId ?: null
-        require(!(oldPayment != newPayment), label('AccountingUiLabels', 'AccountingPSUpdateNotAllowedBecauseOfStatus'))
+        require(oldPayment == newPayment, label('AccountingUiLabels', 'AccountingPSUpdateNotAllowedBecauseOfStatus'))
     }
     String statusIdSave = payment.statusId  // do not allow status change here
     payment.setNonPKFields(parameters)
@@ -287,17 +288,16 @@ Map createPaymentContent() {
 
 //TODO: This can be converted into entity-auto with a seca rule for updateContent
 Map updatePaymentContent() {
-    GenericValue lookedUpValue = from('PaymentContent').where(parameters).queryOne()
-    if (lookedUpValue) {
-        lookedUpValue.setNonPKFields(parameters)
-        lookedUpValue.store()
-        Map result = run service: 'updateContent', with: parameters
-        if (ServiceUtil.isError(result)) {
-            return result
-        }
-        return success()
+    try {
+        update('PaymentContent').where(parameters).set(parameters)
+    } catch (ServiceErrorException e) {
+        return error('Error getting Payment Content')
     }
-    return error('Error getting Payment Content')
+    Map result = run service: 'updateContent', with: parameters
+    if (ServiceUtil.isError(result)) {
+        return result
+    }
+    return success()
 }
 
 Map massChangePaymentStatus() {
@@ -463,7 +463,7 @@ Map cancelPaymentBatch() {
             .queryList()
 
     if (paymentGroupMemberAndTransList) {
-        require(!(paymentGroupMemberAndTransList[0].finAccountTransStatusId == 'FINACT_TRNS_APPROVED'),
+        require(paymentGroupMemberAndTransList[0].finAccountTransStatusId != 'FINACT_TRNS_APPROVED',
                 label('AccountingErrorUiLabels', 'AccountingTransactionIsAlreadyReconciled'))
 
         for (GenericValue paymentGroupMember : paymentGroupMemberAndTransList) {
@@ -723,7 +723,7 @@ Map createPaymentFromOrder() {
 
 Map createPaymentApplication() {
     // Create a Payment Application
-    require(!(!parameters.invoiceId && !parameters.billingAccountId && !parameters.taxAuthGeoId && !parameters.toPaymentId),
+    require(parameters.invoiceId || parameters.billingAccountId || parameters.taxAuthGeoId || parameters.toPaymentId,
             label('AccountingUiLabels', 'AccountingPaymentApplicationParameterMissing'))
     GenericValue paymentAppl = makeValue('PaymentApplication', parameters)
 
@@ -736,7 +736,7 @@ Map createPaymentApplication() {
         // get the invoice and do some further validation against it
         GenericValue invoice = from('Invoice').where('invoiceId', parameters.invoiceId).queryOne()
         // check the currencies if they are compatible
-        require(!(invoice.currencyUomId != payment.currencyUomId && invoice.currencyUomId != payment.actualCurrencyUomId),
+        require(invoice.currencyUomId == payment.currencyUomId || invoice.currencyUomId == payment.actualCurrencyUomId,
                 label('AccountingUiLabels', 'AccountingCurrenciesOfInvoiceAndPaymentNotCompatible'))
         if (invoice.currencyUomId != payment.currencyUomId && invoice.currencyUomId == payment.actualCurrencyUomId) {
             // if required get the payment amount in foreign currency (local we already have)
@@ -778,22 +778,22 @@ Map createPaymentApplication() {
 
 Map setPaymentStatus() {
     GenericValue payment = from('Payment').where('paymentId', parameters.paymentId).queryOne()
-    require(payment as boolean, "No payment found with ID ${parameters.paymentId}")
+    require(payment as boolean, label('AccountingUiLabels', 'AccountingPaymentRecordNotFound', parameters))
     String oldStatusId = payment.statusId
     GenericValue statusItem = from('StatusItem').where('statusId', parameters.statusId).cache().queryOne()
-    require(statusItem as boolean, "No status found with status ID ${parameters.statusId}")
+    require(statusItem as boolean, label('AccountingUiLabels', 'AccountingStatusItemNotFound', parameters))
 
     if (oldStatusId != parameters.statusId) {
         GenericValue statusChange = from('StatusValidChange').where('statusId', oldStatusId, 'statusIdTo', parameters.statusId).cache().queryOne()
         require(statusChange as boolean, label('CommonUiLabels', 'CommonErrorNoStatusValidChange'))
 
         // payment method is mandatory when set to sent or received
-        require(!(['PMNT_RECEIVED', 'PMNT_SENT'].contains(parameters.statusId) && !payment.paymentMethodId),
+        require(!['PMNT_RECEIVED', 'PMNT_SENT'].contains(parameters.statusId) || payment.paymentMethodId,
                 label('AccountingUiLabels', 'AccountingMissingPaymentMethod', [statusItem: statusItem]))
 
         // check if the payment fully applied when set to confirmed
-        require(!(parameters.statusId == 'PMNT_CONFIRMED' &&
-                PaymentWorker.getPaymentNotApplied(payment) != 0), label('AccountingUiLabels', 'AccountingPSNotConfirmedNotFullyApplied'))
+        require(parameters.statusId != 'PMNT_CONFIRMED' ||
+                PaymentWorker.getPaymentNotApplied(payment) == 0, label('AccountingUiLabels', 'AccountingPSNotConfirmedNotFullyApplied'))
     }
 
     // if new status is cancelled delete existing payment applications
@@ -952,7 +952,7 @@ Map removePaymentApplication() {
     // check payment
     if (paymentApplication.paymentId) {
         GenericValue payment = from('Payment').where(paymentId: paymentApplication.paymentId).queryOne()
-        require(!(payment.statusId == 'PMNT_CONFIRMED'), label('AccountingUiLabels', 'AccountingPaymentApplicationCannotRemovedWithConfirmedStatus'))
+        require(payment.statusId != 'PMNT_CONFIRMED', label('AccountingUiLabels', 'AccountingPaymentApplicationCannotRemovedWithConfirmedStatus'))
     }
 
     // check invoice
@@ -974,7 +974,7 @@ Map removePaymentApplication() {
     // check toPayment
     if (paymentApplication.toPaymentId) {
         GenericValue toPayment = from('Payment').where(paymentId: paymentApplication.toPaymentId).queryOne()
-        require(!(toPayment.statusId == 'PMNT_CONFIRMED'),
+        require(toPayment.statusId != 'PMNT_CONFIRMED',
                 label('AccountingUiLabels', 'AccountingPaymentApplicationCannotRemovedWithConfirmedStatus'))
         toMessage = label('AccountingUiLabels', 'AccountingPaymentApplToPayment', paymentApplicationFields)
     }

applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/rate/RateServices.groovy

@@ -22,6 +22,7 @@ import org.apache.ofbiz.base.util.UtilDateTime
 import org.apache.ofbiz.base.util.UtilProperties
 import org.apache.ofbiz.entity.GenericValue
 import org.apache.ofbiz.entity.util.EntityUtil
+import org.apache.ofbiz.service.ServiceErrorException
 import org.apache.ofbiz.service.ServiceUtil
 
 import java.sql.Timestamp
@@ -62,15 +63,13 @@ Map updateRateAmount() {
  * Service to expire a rate amount value
  */
 Map expireRateAmount() {
-    GenericValue lookedUpValue = delegator.makeValidValue('RateAmount', parameters)
-    lookedUpValue.rateCurrencyUomId = lookedUpValue.rateCurrencyUomId ?: UtilProperties.getPropertyValue('general.properties',
+    GenericValue lookupValue = delegator.makeValidValue('RateAmount', parameters)
+    lookupValue.rateCurrencyUomId = lookupValue.rateCurrencyUomId ?: UtilProperties.getPropertyValue('general.properties',
             'currency.uom.id.default')
-    lookedUpValue = from('RateAmount').where(lookedUpValue).queryOne()
-    if (lookedUpValue) {
-        Timestamp previousDay = UtilDateTime.adjustTimestamp(UtilDateTime.nowTimestamp(), 5, -1)
-        lookedUpValue.thruDate = UtilDateTime.getDayEnd(previousDay)
-        lookedUpValue.store()
-    } else {
+    Timestamp previousDay = UtilDateTime.adjustTimestamp(UtilDateTime.nowTimestamp(), 5, -1)
+    try {
+        update('RateAmount').where(lookupValue).set([thruDate: UtilDateTime.getDayEnd(previousDay)])
+    } catch (ServiceErrorException e) {
         return error('AccountingErrorUiLabels', 'AccountingDeleteRateAmount')
     }
     return success()

applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/tax/TaxAuthorityServicesScript.groovy

@@ -31,7 +31,8 @@ Map createPartyTaxAuthInfo() {
     GenericValue taxAuthority = from('TaxAuthority').where(parameters).queryOne()
     require(taxAuthority as boolean, label('PartyUiLabels', 'PartyTaxAuthPartyAndGeoNotAvailable'))
     String errorMesg = validatePartyTaxIdInline()
-    require(!(errorMesg), errorMesg)
+    boolean isTaxIdValid = !errorMesg
+    require(isTaxIdValid, errorMesg)
     GenericValue partyAuthInfo = makeValue('PartyTaxAuthInfo', parameters)
     partyAuthInfo.fromDate = partyAuthInfo.fromDate ?: UtilDateTime.nowTimestamp()
     partyAuthInfo.create()
@@ -43,7 +44,8 @@ Map createPartyTaxAuthInfo() {
  */
 Map updatePartyTaxAuthInfo() {
     String errorMesg = validatePartyTaxIdInline()
-    require(!(errorMesg), errorMesg)
+    boolean isTaxIdValid = !errorMesg
+    require(isTaxIdValid, errorMesg)
     GenericValue partyAuthInfo = from('PartyTaxAuthInfo').where(parameters).queryOne()
     require(partyAuthInfo as boolean, 'PartyTaxAuthInfo not found for the given parameters')
     partyAuthInfo.setNonPKFields(parameters, false)

applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/PaymentContentTests.groovy

@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.ofbiz.accounting.accounting
+
+import java.sql.Timestamp
+import org.apache.ofbiz.base.util.UtilDateTime
+import org.apache.ofbiz.entity.GenericValue
+import org.apache.ofbiz.service.ServiceUtil
+import org.apache.ofbiz.testtools.JunitJupiterTest
+import org.apache.ofbiz.testtools.JupiterTestHelper
+import org.junit.jupiter.api.Order
+import org.junit.jupiter.api.Test
+
+// updatePaymentContent() has no other caller anywhere in the codebase (only reachable by direct
+// service invocation), so unlike the other update() DSL conversions it had no existing test file
+// to add coverage to - this class is its dedicated home.
+@JunitJupiterTest
+class PaymentContentTests implements JupiterTestHelper {
+
+    // Regression coverage for the update() DSL / ServiceErrorException catch site added in
+    // updatePaymentContent(): a PK with no matching PaymentContent record must come back as a
+    // service error carrying the original plain-string "Error getting Payment Content" message,
+    // not the generic EntityUpdateBuilder message and not a silently-successful result.
+    @Test
+    @Order(1)
+    void testUpdatePaymentContentNotFound() {
+        String paymentId = testParams.paymentId ?: 'TEST_NONEXISTENT_PAYMENT'
+        String paymentContentTypeId = testParams.paymentContentTypeId ?: 'COMMENTS'
+        String contentId = testParams.contentId ?: 'TEST_NONEXISTENT_CONTENT'
+        Timestamp fromDate = UtilDateTime.toTimestamp('01/01/2099 00:00:00')
+        Map serviceCtx = [
+                paymentId: paymentId,
+                paymentContentTypeId: paymentContentTypeId,
+                contentId: contentId,
+                fromDate: fromDate,
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('updatePaymentContent', serviceCtx)
+        assert ServiceUtil.isError(serviceResult)
+        assert ServiceUtil.getErrorMessage(serviceResult) == 'Error getting Payment Content'
+    }
+
+    // updatePaymentContent() had no coverage at all before this - not even a happy-path test.
+    // Builds its own Payment/PaymentContentType/Content/PaymentContent fixture chain directly
+    // (rather than depending on another testdef suite's data-load - each <test-suite> in
+    // accounting's ofbiz-component.xml runs in its own isolated, rolled-back transaction, so a
+    // fixture loaded by one suite's entity-xml is not visible to a different suite), then calls
+    // the service and verifies both the PaymentContent update and the follow-on updateContent
+    // call persisted.
+    @Test
+    @Order(2)
+    void testUpdatePaymentContent() {
+        String paymentContentTypeId = testParams.paymentContentTypeId ?: 'TEST_PMT_CNT_TYPE'
+        delegator.createOrStore(delegator.makeValue('PaymentContentType', [paymentContentTypeId: paymentContentTypeId]))
+
+        String paymentId = testParams.paymentId ?: 'TEST_PMT_CONTENT'
+        GenericValue payment = delegator.makeValue('Payment', [
+                paymentId: paymentId,
+                paymentTypeId: 'CUSTOMER_PAYMENT',
+                partyIdFrom: 'DemoCustomer',
+                partyIdTo: 'Company',
+                statusId: 'PMNT_NOT_PAID',
+                effectiveDate: UtilDateTime.nowTimestamp(),
+                amount: BigDecimal.valueOf(20),
+                currencyUomId: 'USD'
+        ])
+        delegator.createOrStore(payment)
+
+        Map createContentResult = dispatcher.runSync('createContent',
+                [contentName: 'Original Payment Content', userLogin: userLogin])
+        assert ServiceUtil.isSuccess(createContentResult)
+        String contentId = createContentResult.contentId
+        assert contentId
+
+        Timestamp fromDate = UtilDateTime.toTimestamp('01/01/2020 00:00:00')
+        GenericValue paymentContent = delegator.makeValue('PaymentContent', [
+                paymentId: paymentId,
+                paymentContentTypeId: paymentContentTypeId,
+                contentId: contentId,
+                fromDate: fromDate
+        ])
+        paymentContent.create()
+
+        Timestamp thruDate = UtilDateTime.toTimestamp('01/01/2030 00:00:00')
+        String newContentName = 'Updated Payment Content'
+        Map serviceCtx = [
+                paymentId: paymentId,
+                paymentContentTypeId: paymentContentTypeId,
+                contentId: contentId,
+                fromDate: fromDate,
+                thruDate: thruDate,
+                contentName: newContentName,
+                userLogin: userLogin
+        ]
+        Map serviceResult = dispatcher.runSync('updatePaymentContent', serviceCtx)
+        assert ServiceUtil.isSuccess(serviceResult)
+
+        GenericValue updatedPaymentContent = from('PaymentContent')
+                .where('paymentId', paymentId, 'paymentContentTypeId', paymentContentTypeId,
+                        'contentId', contentId, 'fromDate', fromDate).queryOne()
+        assert updatedPaymentContent
+        assert updatedPaymentContent.thruDate == thruDate
+
+        GenericValue updatedContent = from('Content').where('contentId', contentId).queryOne()
+        assert updatedContent
+        assert updatedContent.contentName == newContentName
+    }
+
+}