diff --git a/applications/accounting/config/AccountingUiLabels.xml b/applications/accounting/config/AccountingUiLabels.xml index a5e12458736..9fcd5ecfbce 100644 --- a/applications/accounting/config/AccountingUiLabels.xml +++ b/applications/accounting/config/AccountingUiLabels.xml @@ -13838,6 +13838,9 @@ 财务状态 財務狀態 + + Status [${statusId}] not found + دفتر الشركة التابعة Nebenbuch diff --git a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/invoice/InvoiceServicesScript.groovy b/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/invoice/InvoiceServicesScript.groovy index 14e28955bd1..05c45d63894 100644 --- a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/invoice/InvoiceServicesScript.groovy +++ b/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() } diff --git a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/payment/PaymentServices.groovy b/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/payment/PaymentServices.groovy index ab4c66d7f03..0d8b75600fd 100644 --- a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/payment/PaymentServices.groovy +++ b/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) } diff --git a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/rate/RateServices.groovy b/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/rate/RateServices.groovy index 7fcc4a393d9..dba46e33ab2 100644 --- a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/rate/RateServices.groovy +++ b/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() diff --git a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/tax/TaxAuthorityServicesScript.groovy b/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/tax/TaxAuthorityServicesScript.groovy index 706d7ed2974..a961b711b48 100644 --- a/applications/accounting/src/main/groovy/org/apache/ofbiz/accounting/tax/TaxAuthorityServicesScript.groovy +++ b/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) diff --git a/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/PaymentContentTests.groovy b/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/PaymentContentTests.groovy new file mode 100644 index 00000000000..c969b254c58 --- /dev/null +++ b/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 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 + } + +} diff --git a/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/RateTests.groovy b/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/RateTests.groovy index d30ede3e15b..d7dea0018bf 100644 --- a/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/RateTests.groovy +++ b/applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/RateTests.groovy @@ -23,6 +23,7 @@ import org.apache.ofbiz.testtools.JupiterTestHelper import org.apache.ofbiz.service.ServiceUtil import org.apache.ofbiz.entity.GenericValue import org.apache.ofbiz.base.util.UtilDateTime +import org.apache.ofbiz.base.util.UtilProperties import java.sql.Timestamp import org.junit.jupiter.api.Order @@ -225,4 +226,25 @@ class RateTests implements JupiterTestHelper { assert rateAmount.thruDate } + // Regression coverage for the update() DSL / ServiceErrorException catch site added in + // expireRateAmount(): a rateTypeId with no matching RateAmount record must come back as a + // service error carrying the original localized AccountingDeleteRateAmount message, not the + // generic EntityUpdateBuilder message and not a silently-successful result. + @Test + @Order(10) + void testExpireRateAmountNotFound() { + Timestamp fromDate = UtilDateTime.toTimestamp('01/01/2099 00:00:00') + String rateTypeId = testParams.rateTypeId ?: 'TEST_NONEXISTENT_RATE_TYPE' + Map serviceCtx = [ + rateTypeId: rateTypeId, + fromDate: fromDate, + userLogin: userLogin + + ] + Map serviceResult = dispatcher.runSync('expireRateAmount', serviceCtx) + assert ServiceUtil.isError(serviceResult) + assert ServiceUtil.getErrorMessage(serviceResult) == + UtilProperties.getMessage('AccountingErrorUiLabels', 'AccountingDeleteRateAmount', Locale.US) + } + } diff --git a/applications/accounting/testdef/paymenttests.xml b/applications/accounting/testdef/paymenttests.xml index 7b93729e90c..245a042a25c 100644 --- a/applications/accounting/testdef/paymenttests.xml +++ b/applications/accounting/testdef/paymenttests.xml @@ -25,6 +25,9 @@ - + + + + diff --git a/applications/order/src/main/groovy/org/apache/ofbiz/order/order/CheckoutServices.groovy b/applications/order/src/main/groovy/org/apache/ofbiz/order/order/CheckoutServices.groovy index 4ae132d07d4..d058cb207c3 100644 --- a/applications/order/src/main/groovy/org/apache/ofbiz/order/order/CheckoutServices.groovy +++ b/applications/order/src/main/groovy/org/apache/ofbiz/order/order/CheckoutServices.groovy @@ -41,7 +41,8 @@ Map createUpdateCustomerAndShippingAddress() { SimpleMapProcessor.runSimpleMapProcessor('component://party/minilang/contact/PartyContactMechMapProcs.xml', 'emailAddress', parameters, emailAddressCtx, messages, context.locale) // Check errors - require(!(messages), StringUtil.join(messages, ',')) + boolean hasNoValidationErrors = !messages + require(hasNoValidationErrors, StringUtil.join(messages, ',')) ShoppingCart shoppingCart = parameters.shoppingCart String partyId = parameters.partyId @@ -117,7 +118,8 @@ Map createUpdateBillingAddressAndPaymentMethod() { SimpleMapProcessor.runSimpleMapProcessor('component://order/minilang/customer/CheckoutMapProcs.xml', 'billToPhone', parameters, billToPhoneContext, messages, context.locale) // Check Errors - require(!(messages), StringUtil.join(messages, ',')) + boolean hasNoValidationErrors = !messages + require(hasNoValidationErrors, StringUtil.join(messages, ',')) ShoppingCart shoppingCart = parameters.shoppingCart GenericValue userLogin = shoppingCart.getUserLogin() diff --git a/applications/order/src/main/groovy/org/apache/ofbiz/order/quote/QuoteServicesScript.groovy b/applications/order/src/main/groovy/org/apache/ofbiz/order/quote/QuoteServicesScript.groovy index 33faacedc33..10a86cef60b 100644 --- a/applications/order/src/main/groovy/org/apache/ofbiz/order/quote/QuoteServicesScript.groovy +++ b/applications/order/src/main/groovy/org/apache/ofbiz/order/quote/QuoteServicesScript.groovy @@ -30,6 +30,7 @@ import org.apache.ofbiz.product.config.ProductConfigWorker import org.apache.ofbiz.product.config.ProductConfigWrapper import org.apache.ofbiz.service.ExecutionServiceException import org.apache.ofbiz.service.ModelService +import org.apache.ofbiz.service.ServiceErrorException import org.apache.ofbiz.service.ServiceUtil /** @@ -38,10 +39,11 @@ import org.apache.ofbiz.service.ServiceUtil Map checkUpdateQuoteStatus() { require(security.hasEntityPermission('ORDERMGR', '_UPDATE', userLogin) as boolean, 'OrderErrorUiLabels', 'OrderSecurityErrorToRunCheckUpdateQuoteStatus') - GenericValue quote = from('Quote').where(parameters).queryOne() - require(quote as boolean, 'OrderErrorUiLabels', 'OrderQuoteDoesNotExists') - quote.statusId = 'QUO_ORDERED' - quote.store() + try { + update('Quote').where(parameters).set([statusId: 'QUO_ORDERED']) + } catch (ServiceErrorException e) { + fail('OrderErrorUiLabels', 'OrderQuoteDoesNotExists') + } return success() } @@ -81,7 +83,8 @@ Map getNextQuoteId() { require(!(quote), 'OrderErrorUiLabels', 'OrderQuoteIdAlreadyExists', [quoteId: quoteId]) // Check the provided ID String errorMessage = UtilValidate.checkValidDatabaseId(quoteId) - require(!(errorMessage), label('OrderErrorUiLabels', 'OrderQuoteGetNextIdError') + errorMessage) + boolean isQuoteIdValid = !errorMessage + require(isQuoteIdValid, label('OrderErrorUiLabels', 'OrderQuoteGetNextIdError') + errorMessage) } else { quoteId = delegator.getNextSeqId('Quote') } @@ -111,9 +114,9 @@ Map quoteSequenceEnforced() { * Create a new Quote. */ Map createQuote() { - require(!(parameters.partyId - && parameters.partyId != userLogin.partyId - && !security.hasEntityPermission('ORDERMGR', '_CREATE', userLogin)), + require(!parameters.partyId + || parameters.partyId == userLogin.partyId + || security.hasEntityPermission('ORDERMGR', '_CREATE', userLogin), 'OrderErrorUiLabels', 'OrderSecurityErrorToRunCreateQuote') // Create new entity and create all the fields. @@ -316,9 +319,9 @@ Map ensureWorkEffortAndCreateQuoteWorkEffort() { Map createQuoteItem() { GenericValue quote = from('Quote').where(parameters).queryOne() require(quote as boolean, 'OrderErrorUiLabels', 'OrderQuoteDoesNotExists') - require(!(quote.partyId - && quote.partyId != userLogin.partyId - && !security.hasEntityPermission('ORDERMGR', '_CREATE', userLogin)), + require(!quote.partyId + || quote.partyId == userLogin.partyId + || security.hasEntityPermission('ORDERMGR', '_CREATE', userLogin), 'OrderErrorUiLabels', 'OrderSecurityErrorToRunCreateQuoteItem') GenericValue quoteItem = delegator.makeValidValue('QuoteItem', parameters) if (!quoteItem.quoteItemSeqId) { @@ -327,7 +330,7 @@ Map createQuoteItem() { if (!parameters.quoteUnitPrice && parameters.productId) { GenericValue product = from('Product').where('productId', parameters.productId).cache().queryOne() - require(!(product?.isVirtual == 'Y'), 'OrderErrorUiLabels', 'OrderCannotAddVirtualProductToQuote') + require(product?.isVirtual != 'Y', 'OrderErrorUiLabels', 'OrderCannotAddVirtualProductToQuote') if (product?.productTypeId?.startsWith('AGGREGATED') && parameters.configId) { ProductConfigWrapper configWrapper = ProductConfigWorker.loadProductConfigWrapper(delegator, dispatcher, parameters.configId, @@ -354,10 +357,11 @@ Map updateQuoteItem() { 'OrderErrorUiLabels', 'OrderSecurityErrorToRunUpdateQuoteItem') Map pksQuoteItem = [quoteId: parameters.quoteId, quoteItemSeqId: parameters.quoteItemSeqId] - GenericValue quoteItem = from('QuoteItem').where(pksQuoteItem).queryOne() - require(quoteItem as boolean, 'OrderErrorUiLabels', 'OrderQuoteItemDoesNotExists') - quoteItem.setNonPKFields(parameters) - quoteItem.store() + try { + update('QuoteItem').where(pksQuoteItem).set(parameters) + } catch (ServiceErrorException e) { + fail('OrderErrorUiLabels', 'OrderQuoteItemDoesNotExists') + } return success() } @@ -463,9 +467,9 @@ Map createQuoteFromCart() { Map createQuoteInMap = parameters createQuoteInMap.partyId = cart.getPartyId() - require(!(createQuoteInMap.partyId - && createQuoteInMap.partyId != userLogin.partyId - && !security.hasEntityPermission('ORDERMGR', '_CREATE', userLogin)), + require(!createQuoteInMap.partyId + || createQuoteInMap.partyId == userLogin.partyId + || security.hasEntityPermission('ORDERMGR', '_CREATE', userLogin), 'OrderErrorUiLabels', 'OrderSecurityErrorToRunCreateQuoteFromCart') createQuoteInMap.currencyUomId = cart.getCurrency() @@ -540,14 +544,17 @@ Map createQuoteFromShoppingList() { Map autoUpdateQuotePrice() { require(security.hasEntityPermission('ORDERMGR', '_UPDATE', userLogin) as boolean, 'OrderErrorUiLabels', 'OrderSecurityErrorToRunAutoUpdateQuotePrice') - GenericValue quoteItem = from('QuoteItem').where(parameters).queryOne() - require(quoteItem as boolean, 'OrderErrorUiLabels', 'OrderQuoteItemDoesNotExists') + Map fieldsToSet = [:] if (parameters.manualQuoteUnitPrice) { - quoteItem.quoteUnitPrice = parameters.manualQuoteUnitPrice + fieldsToSet.quoteUnitPrice = parameters.manualQuoteUnitPrice } else if (parameters.defaultQuoteUnitPrice) { - quoteItem.quoteUnitPrice = parameters.defaultQuoteUnitPrice + fieldsToSet.quoteUnitPrice = parameters.defaultQuoteUnitPrice + } + try { + update('QuoteItem').where(parameters).set(fieldsToSet) + } catch (ServiceErrorException e) { + fail('OrderErrorUiLabels', 'OrderQuoteItemDoesNotExists') } - quoteItem.store() return success() } @@ -563,7 +570,7 @@ Map createQuoteFromCustRequest() { [custRequestId: parameters.custRequestId]) // Error if request type not equals to RF_QUOTE or RF_PUR_QUOTE - require(!(custRequest.custRequestTypeId != 'RF_QUOTE' && custRequest.custRequestTypeId != 'RF_PUR_QUOTE'), + require(custRequest.custRequestTypeId == 'RF_QUOTE' || custRequest.custRequestTypeId == 'RF_PUR_QUOTE', 'OrderErrorUiLabels', 'OrderQuoteNotARequest') Map createQuoteInMap = [ diff --git a/applications/order/src/main/groovy/org/apache/ofbiz/order/requirement/RequirementServicesScript.groovy b/applications/order/src/main/groovy/org/apache/ofbiz/order/requirement/RequirementServicesScript.groovy index 303c2212e71..7e11e89519a 100644 --- a/applications/order/src/main/groovy/org/apache/ofbiz/order/requirement/RequirementServicesScript.groovy +++ b/applications/order/src/main/groovy/org/apache/ofbiz/order/requirement/RequirementServicesScript.groovy @@ -31,15 +31,13 @@ import java.sql.Timestamp */ Map deleteRequirementAndRelated() { GenericValue requirement = from('Requirement').where(parameters).queryOne() - if (requirement) { - requirement.removeRelated('RequirementAttribute') - requirement.removeRelated('RequirementRole') - requirement.removeRelated('RequirementStatus') - requirement.removeRelated('RequirementCustRequest') - requirement.remove() - return success() - } - return error('Entity value not found with name: requirement Method = deleteRequirementAndRelated') + require(requirement as boolean, 'Entity value not found with name: requirement Method = deleteRequirementAndRelated') + requirement.removeRelated('RequirementAttribute') + requirement.removeRelated('RequirementRole') + requirement.removeRelated('RequirementStatus') + requirement.removeRelated('RequirementCustRequest') + requirement.remove() + return success() } /** @@ -47,30 +45,28 @@ Map deleteRequirementAndRelated() { */ Map autoAssignRequirementToSupplier() { GenericValue requirement = from('Requirement').where(parameters).queryOne() - if (requirement) { - if (requirement.requirementTypeId == 'PRODUCT_REQUIREMENT' - && requirement.productId - && requirement.quantity) { - EntityCondition condition = new EntityConditionBuilder().AND { - EQUALS(productId: requirement.productId) - LESS_THAN_EQUAL_TO(minimumOrderQuantity: requirement.quantity) - } - EntityQuery supplierProductsQuery = from('SupplierProduct').where(condition).orderBy('lastPrice', 'supplierPrefOrderId') - if (requirement.requiredByDate) { - supplierProductsQuery.filterByDate((Timestamp) requirement.requiredByDate, 'availableFromDate', 'availableThruDate') - } - GenericValue supplierProduct = supplierProductsQuery.queryFirst() - if (supplierProduct?.partyId) { - GenericValue requirementRole = delegator.makeValue('RequirementRole', [requirementId: requirement.requirementId, - partyId: supplierProduct.partyId, - roleTypeId: 'SUPPLIER', - fromDate: UtilDateTime.nowTimestamp()]) - delegator.createOrStore(requirementRole) - } + require(requirement as boolean, 'Entity value not found with name: requirement Method = autoAssignRequirementToSupplier') + if (requirement.requirementTypeId == 'PRODUCT_REQUIREMENT' + && requirement.productId + && requirement.quantity) { + EntityCondition condition = new EntityConditionBuilder().AND { + EQUALS(productId: requirement.productId) + LESS_THAN_EQUAL_TO(minimumOrderQuantity: requirement.quantity) + } + EntityQuery supplierProductsQuery = from('SupplierProduct').where(condition).orderBy('lastPrice', 'supplierPrefOrderId') + if (requirement.requiredByDate) { + supplierProductsQuery.filterByDate((Timestamp) requirement.requiredByDate, 'availableFromDate', 'availableThruDate') + } + GenericValue supplierProduct = supplierProductsQuery.queryFirst() + if (supplierProduct?.partyId) { + GenericValue requirementRole = delegator.makeValue('RequirementRole', [requirementId: requirement.requirementId, + partyId: supplierProduct.partyId, + roleTypeId: 'SUPPLIER', + fromDate: UtilDateTime.nowTimestamp()]) + delegator.createOrStore(requirementRole) } - return success() } - return error('Entity value not found with name: requirement Method = autoAssignRequirementToSupplier') + return success() } /** diff --git a/applications/order/src/main/groovy/org/apache/ofbiz/order/shoppinglist/ShoppingListServicesScript.groovy b/applications/order/src/main/groovy/org/apache/ofbiz/order/shoppinglist/ShoppingListServicesScript.groovy index 74f51ca49ce..8384fe26cae 100644 --- a/applications/order/src/main/groovy/org/apache/ofbiz/order/shoppinglist/ShoppingListServicesScript.groovy +++ b/applications/order/src/main/groovy/org/apache/ofbiz/order/shoppinglist/ShoppingListServicesScript.groovy @@ -213,9 +213,9 @@ Map calculateShoppingListDeepTotalPrice() { * Checks security on a ShoppingList */ Map checkShoppingListSecurity() { - require(!(userLogin && (userLogin.userLoginId != 'anonymous') && - parameters.partyId && (userLogin.partyId != parameters.partyId) - && !security.hasEntityPermission('PARTYMGR', "_${parameters.permissionAction}", parameters.userLogin)), + require(!userLogin || (userLogin.userLoginId == 'anonymous') || + !parameters.partyId || (userLogin.partyId == parameters.partyId) + || security.hasEntityPermission('PARTYMGR', "_${parameters.permissionAction}", parameters.userLogin), 'OrderErrorUiLabels', 'OrderSecurityErrorToRunForAnotherParty') Map result = success() @@ -228,8 +228,8 @@ Map checkShoppingListSecurity() { */ Map checkShoppingListItemSecurity() { GenericValue shoppingList = from('ShoppingList').where(parameters).queryOne() - require(!(shoppingList?.partyId && userLogin.partyId != shoppingList.partyId && - !security.hasEntityPermission('PARTYMGR', "_${parameters.permissionAction}", parameters.userLogin)), + require(!shoppingList?.partyId || userLogin.partyId == shoppingList.partyId || + security.hasEntityPermission('PARTYMGR', "_${parameters.permissionAction}", parameters.userLogin), 'OrderErrorUiLabels', 'OrderSecurityErrorToRunForAnotherParty', [parentMethodName: parameters.parentMethodName, diff --git a/applications/order/src/test/groovy/org/apache/ofbiz/order/order/test/QuoteTests.groovy b/applications/order/src/test/groovy/org/apache/ofbiz/order/order/test/QuoteTests.groovy index e05505070fb..40222512a14 100644 --- a/applications/order/src/test/groovy/org/apache/ofbiz/order/order/test/QuoteTests.groovy +++ b/applications/order/src/test/groovy/org/apache/ofbiz/order/order/test/QuoteTests.groovy @@ -18,6 +18,7 @@ */ package org.apache.ofbiz.order.order.test +import org.apache.ofbiz.base.util.UtilProperties import org.apache.ofbiz.entity.GenericValue import org.apache.ofbiz.order.shoppingcart.ShoppingCart import org.apache.ofbiz.service.ServiceUtil @@ -550,4 +551,63 @@ class QuoteTests implements JupiterTestHelper { assert ServiceUtil.isSuccess(serviceResult) } + // Regression coverage for the update() DSL / ServiceErrorException catch site added in + // checkUpdateQuoteStatus(): a quoteId with no matching Quote record must come back as a + // service error carrying the original localized OrderQuoteDoesNotExists message, not the + // generic EntityUpdateBuilder message and not a silently-successful result. + @Test + @Order(26) + void testCheckUpdateQuoteStatusNotFound() { + String quoteId = testParams.quoteId ?: 'TEST_NONEXISTENT_QUOTE' + Map serviceCtx = [ + userLogin: userLogin, + quoteId: quoteId, + ] + Map serviceResult = dispatcher.runSync('checkUpdateQuoteStatus', serviceCtx) + assert ServiceUtil.isError(serviceResult) + assert ServiceUtil.getErrorMessage(serviceResult) == + UtilProperties.getMessage('OrderErrorUiLabels', 'OrderQuoteDoesNotExists', Locale.US) + } + + // Regression coverage for the update() DSL / ServiceErrorException catch site added in + // updateQuoteItem(): a quoteId/quoteItemSeqId with no matching QuoteItem record must come back + // as a service error carrying the original localized OrderQuoteItemDoesNotExists message, not + // the generic EntityUpdateBuilder message and not a silently-successful result. + @Test + @Order(27) + void testUpdateQuoteItemNotFound() { + String quoteId = testParams.quoteId ?: 'TEST_NONEXISTENT_QUOTE' + String quoteItemSeqId = testParams.quoteItemSeqId ?: '00001' + Map serviceCtx = [ + userLogin: userLogin, + quoteId: quoteId, + quoteItemSeqId: quoteItemSeqId, + ] + Map serviceResult = dispatcher.runSync('updateQuoteItem', serviceCtx) + assert ServiceUtil.isError(serviceResult) + assert ServiceUtil.getErrorMessage(serviceResult) == + UtilProperties.getMessage('OrderErrorUiLabels', 'OrderQuoteItemDoesNotExists', Locale.US) + } + + // Regression coverage for the update() DSL / ServiceErrorException catch site added in + // autoUpdateQuotePrice(): a quoteId/quoteItemSeqId with no matching QuoteItem record must come + // back as a service error carrying the original localized OrderQuoteItemDoesNotExists message, + // not the generic EntityUpdateBuilder message and not a silently-successful result. + @Test + @Order(28) + void testAutoUpdateQuotePriceNotFound() { + String quoteId = testParams.quoteId ?: 'TEST_NONEXISTENT_QUOTE' + String quoteItemSeqId = testParams.quoteItemSeqId ?: '00001' + Map serviceCtx = [ + userLogin: userLogin, + quoteId: quoteId, + quoteItemSeqId: quoteItemSeqId, + defaultQuoteUnitPrice: BigDecimal.valueOf(12) + ] + Map serviceResult = dispatcher.runSync('autoUpdateQuotePrice', serviceCtx) + assert ServiceUtil.isError(serviceResult) + assert ServiceUtil.getErrorMessage(serviceResult) == + UtilProperties.getMessage('OrderErrorUiLabels', 'OrderQuoteItemDoesNotExists', Locale.US) + } + } diff --git a/applications/party/src/main/groovy/org/apache/ofbiz/party/contact/ContactMechServicesScript.groovy b/applications/party/src/main/groovy/org/apache/ofbiz/party/contact/ContactMechServicesScript.groovy index af681b16bd0..8cd054624e7 100644 --- a/applications/party/src/main/groovy/org/apache/ofbiz/party/contact/ContactMechServicesScript.groovy +++ b/applications/party/src/main/groovy/org/apache/ofbiz/party/contact/ContactMechServicesScript.groovy @@ -75,7 +75,8 @@ String hasValidStateProvince(String countryGeoId, String stateProvinceGeoId) { */ Map createPostalAddress() { String errorMessage = hasValidStateProvince(parameters.countryGeoId, parameters.stateProvinceGeoId) - require(!(errorMessage), 'PartyUiLabels', errorMessage) + boolean isStateProvinceValid = !errorMessage + require(isStateProvinceValid, 'PartyUiLabels', errorMessage) GenericValue newValue = makeValue('PostalAddress', parameters) Map createContactMechMap = [contactMechTypeId: 'POSTAL_ADDRESS', contactMechId: parameters.contactMechId] Map serviceResult = run service: 'createContactMech', with: createContactMechMap @@ -91,7 +92,8 @@ Map createPostalAddress() { */ Map updatePostalAddress() { String errorMessage = hasValidStateProvince(parameters.countryGeoId, parameters.stateProvinceGeoId) - require(!(errorMessage), 'PartyUiLabels', errorMessage) + boolean isStateProvinceValid = !errorMessage + require(isStateProvinceValid, 'PartyUiLabels', errorMessage) GenericValue lookedValue = from('PostalAddress').where('contactMechId', parameters.contactMechId).queryOne() require(lookedValue as boolean, 'ServiceErrorUiLabels', 'ServiceValueNotFound') GenericValue newValue = (GenericValue) lookedValue.clone() diff --git a/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyInvitationServices.groovy b/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyInvitationServices.groovy index ba9ca1e6670..148dfdc4352 100644 --- a/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyInvitationServices.groovy +++ b/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyInvitationServices.groovy @@ -20,6 +20,7 @@ package org.apache.ofbiz.party.party import org.apache.ofbiz.base.util.UtilDateTime import org.apache.ofbiz.entity.GenericValue +import org.apache.ofbiz.service.ServiceErrorException // Party Invitation Services Map createPartyInvitation() { @@ -36,15 +37,15 @@ Map createPartyInvitation() { } Map updatePartyInvitation() { - GenericValue lookedUpValue = from('PartyInvitation').where(parameters).queryOne() - if (!lookedUpValue) { - return error('PartyUiLabels', 'PartyInvitationNotValidError') - } + Map fieldsToSet = new HashMap(parameters) if (! parameters.toName && parameters.partyId) { - parameters.toName = PartyHelper.getPartyName(delegator, parameters.partyId, false) + fieldsToSet.toName = PartyHelper.getPartyName(delegator, parameters.partyId, false) + } + try { + update('PartyInvitation').where(parameters).set(fieldsToSet) + } catch (ServiceErrorException e) { + return error('PartyUiLabels', 'PartyInvitationNotValidError') } - lookedUpValue.setNonPKFields(parameters) - lookedUpValue.store() return success() } diff --git a/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyServicesScript.groovy b/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyServicesScript.groovy index c364777e1a6..0786627a757 100644 --- a/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyServicesScript.groovy +++ b/applications/party/src/main/groovy/org/apache/ofbiz/party/party/PartyServicesScript.groovy @@ -684,7 +684,8 @@ Map createUpdatePerson() { 'person', parameters, personContext, messages, context.locale) // Check errors - require(!(messages), StringUtil.join(messages, ',')) + boolean hasNoValidationErrors = !messages + require(hasNoValidationErrors, StringUtil.join(messages, ',')) GenericValue party = from('Party') .where(partyId: partyId) @@ -714,7 +715,8 @@ Map quickCreateCustomer() { 'emailAddress', parameters, emailContext, messages, context.locale) // Check errors - require(!(messages), StringUtil.join(messages, ',')) + boolean hasNoValidationErrors = !messages + require(hasNoValidationErrors, StringUtil.join(messages, ',')) // Create person Map serviceResult = run service: 'createPerson', with: personContext diff --git a/applications/party/src/test/groovy/org/apache/ofbiz/party/party/test/PartyMiscTests.groovy b/applications/party/src/test/groovy/org/apache/ofbiz/party/party/test/PartyMiscTests.groovy index c47c3ccdf81..ec826322ff8 100644 --- a/applications/party/src/test/groovy/org/apache/ofbiz/party/party/test/PartyMiscTests.groovy +++ b/applications/party/src/test/groovy/org/apache/ofbiz/party/party/test/PartyMiscTests.groovy @@ -18,6 +18,7 @@ */ package org.apache.ofbiz.party.party.test +import org.apache.ofbiz.base.util.UtilProperties import org.apache.ofbiz.entity.GenericValue import org.apache.ofbiz.service.ServiceUtil import org.apache.ofbiz.testtools.JunitJupiterTest @@ -321,4 +322,22 @@ class PartyMiscTests implements JupiterTestHelper { assert partyInvitation.emailAddress == emailAddress } + // Regression coverage for the update() DSL / ServiceErrorException catch site added in + // updatePartyInvitation(): a partyInvitationId with no matching PartyInvitation record must + // come back as a service error carrying the original localized PartyInvitationNotValidError + // message, not the generic EntityUpdateBuilder message and not a silently-successful result. + @Test + @Order(16) + void testUpdatePartyInvitationNotFound() { + String partyInvitationId = testParams.partyInvitationId ?: 'TEST_NONEXISTENT_INVITE' + Map serviceCtx = [ + partyInvitationId: partyInvitationId, + userLogin: userLogin + ] + Map serviceResult = dispatcher.runSync('updatePartyInvitation', serviceCtx) + assert ServiceUtil.isError(serviceResult) + assert ServiceUtil.getErrorMessage(serviceResult) == + UtilProperties.getMessage('PartyUiLabels', 'PartyInvitationNotValidError', Locale.US) + } + } diff --git a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/category/CategoryServicesScript.groovy b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/category/CategoryServicesScript.groovy index 2568d387afe..b7720f8fee4 100644 --- a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/category/CategoryServicesScript.groovy +++ b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/category/CategoryServicesScript.groovy @@ -56,7 +56,8 @@ Map createProductCategory() { if (parameters.productCategoryId) { newEntity.productCategoryId = parameters.productCategoryId String errorMessage = UtilValidate.checkValidDatabaseId(newEntity.productCategoryId) - require(!(errorMessage != null), errorMessage) + boolean isCategoryIdValid = errorMessage == null + require(isCategoryIdValid, errorMessage) } else { newEntity.productCategoryId = delegator.getNextSeqId('ProductCategory') } diff --git a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/inventory/InventoryServicesScript.groovy b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/inventory/InventoryServicesScript.groovy index 4c18d8e6119..35d7b1558e1 100644 --- a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/inventory/InventoryServicesScript.groovy +++ b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/inventory/InventoryServicesScript.groovy @@ -152,12 +152,12 @@ Map createInventoryItem() { // if inventoryItem's unitCost is still empty, or negative return an error message // TODO/WARNING: getProductCost returns 0 even if no std costs are found - require(!(!inventoryItem.unitCost && inventoryItem.unitCost != (BigDecimal) 0), + require(inventoryItem.unitCost || inventoryItem.unitCost == (BigDecimal) 0, label('ProductUiLabels', 'FacilityInventoryItemsMissingUnitCost')) // if you don't want inventory item with unitCost = 0, change the operator // attribute from "less" to "less-equals". - require(!(inventoryItem.unitCost < (BigDecimal) 0), label('ProductUiLabels', 'FacilityInventoryItemsNegativeUnitCost')) + require(inventoryItem.unitCost >= (BigDecimal) 0, label('ProductUiLabels', 'FacilityInventoryItemsNegativeUnitCost')) inventoryItem.inventoryItemId = delegator.getNextSeqId('InventoryItem') inventoryItem.create() diff --git a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/product/ProductServicesScript.groovy b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/product/ProductServicesScript.groovy index 23199d6256f..d9f6702719c 100644 --- a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/product/ProductServicesScript.groovy +++ b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/product/ProductServicesScript.groovy @@ -42,7 +42,8 @@ Map createProduct() { GenericValue newEntity = makeValue('Product', parameters) if (newEntity.productId) { String errorMessage = UtilValidate.checkValidDatabaseId(newEntity.productId) - require(!(errorMessage), errorMessage) + boolean isProductIdValid = !errorMessage + require(isProductIdValid, errorMessage) GenericValue dummyProduct = from('Product').where(parameters).queryOne() require(!(dummyProduct), 'CommonErrorUiLabels', 'CommonErrorDuplicateKey') } else { diff --git a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/promo/PromoServicesScript.groovy b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/promo/PromoServicesScript.groovy index 7cbc2a68b30..706f64a8f45 100644 --- a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/promo/PromoServicesScript.groovy +++ b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/promo/PromoServicesScript.groovy @@ -19,6 +19,7 @@ package org.apache.ofbiz.product.product.promo import org.apache.ofbiz.entity.GenericValue +import org.apache.ofbiz.service.ServiceErrorException Map createProductPromoCond() { if (parameters.carrierShipmentMethod) { @@ -31,14 +32,14 @@ Map createProductPromoCond() { } Map updateProductPromoCond() { - GenericValue lookedUpValue = from('ProductPromoCond').where(parameters).queryOne() - if (lookedUpValue) { - if (parameters.carrierShipmentMethod) { - parameters.otherValue = parameters.carrierShipmentMethod - } - lookedUpValue.setNonPKFields(parameters) - lookedUpValue.store() - return success() + Map fieldsToSet = new HashMap(parameters) + if (parameters.carrierShipmentMethod) { + fieldsToSet.otherValue = parameters.carrierShipmentMethod + } + try { + update('ProductPromoCond').where(parameters).set(fieldsToSet) + } catch (ServiceErrorException e) { + fail(label('ServiceErrorUiLabels', 'ServiceValueNotFound')) } - fail(label('ServiceErrorUiLabels', 'ServiceValueNotFound')) + return success() } diff --git a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/store/ProductStoreServices.groovy b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/store/ProductStoreServices.groovy index 3ea288f9e6b..9c5ae0b5538 100644 --- a/applications/product/src/main/groovy/org/apache/ofbiz/product/product/store/ProductStoreServices.groovy +++ b/applications/product/src/main/groovy/org/apache/ofbiz/product/product/store/ProductStoreServices.groovy @@ -37,8 +37,8 @@ Map createProductStore() { Map result = success() require(security.hasEntityPermission('CATALOG', '_CREATE', parameters.userLogin) as boolean, 'ProductUiLabels', 'ProductCatalogCreatePermissionError') - require(!(parameters.oneInventoryFacility == 'Y' - && !parameters.inventoryFacilityId), 'ProductUiLabels', 'InventoryFacilityIdRequired') + require(parameters.oneInventoryFacility != 'Y' + || parameters.inventoryFacilityId, 'ProductUiLabels', 'InventoryFacilityIdRequired') if (parameters.showPricesWithVatTax == 'Y') { require(parameters.vatTaxAuthGeoId as boolean, 'ProductUiLabels', 'ProductVatTaxAuthGeoNotSet') require(parameters.vatTaxAuthPartyId as boolean, 'ProductUiLabels', 'ProductVatTaxAuthPartyNotSet') @@ -67,7 +67,7 @@ Map createProductStore() { Map updateProductStore() { require(security.hasEntityPermission('CATALOG', '_UPDATE', parameters.userLogin) as boolean, 'ProductUiLabels', 'ProductCatalogUpdatePermissionError') - require(!(parameters.oneInventoryFacility == 'Y' && !parameters.inventoryFacilityId), + require(parameters.oneInventoryFacility != 'Y' || parameters.inventoryFacilityId, 'ProductUiLabels', 'InventoryFacilityIdRequired') GenericValue store = from('ProductStore').where(productStoreId: parameters.productStoreId).queryOne() String oldFacilityId = store.inventoryFacilityId diff --git a/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentReceiptServices.groovy b/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentReceiptServices.groovy index 33897d57638..cafb44cb7c8 100644 --- a/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentReceiptServices.groovy +++ b/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentReceiptServices.groovy @@ -74,8 +74,8 @@ Map receiveInventoryProduct () { // Return an error if both quantityAccepted and quantityRejected are zero or less than zero BigDecimal quantityRejected = parameters.quantityRejected ?: BigDecimal.ZERO - require(!((quantityRejected == BigDecimal.ZERO && parameters.quantityAccepted == BigDecimal.ZERO) - || (quantityRejected < BigDecimal.ZERO || parameters.quantityAccepted < BigDecimal.ZERO)), + require((quantityRejected != BigDecimal.ZERO || parameters.quantityAccepted != BigDecimal.ZERO) + && (quantityRejected >= BigDecimal.ZERO && parameters.quantityAccepted >= BigDecimal.ZERO), 'ProductUiLabels', 'ProductNoItemsToAcceptOrReject') Map result = success() diff --git a/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentServices.groovy b/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentServices.groovy index 00b6646369f..2b2d30f2fc2 100644 --- a/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentServices.groovy +++ b/applications/product/src/main/groovy/org/apache/ofbiz/product/shipment/ShipmentServices.groovy @@ -60,7 +60,8 @@ Map updateShipment() { } } // now finally check for errors - require(!(errorList), errorList.toString()) + boolean hasNoValidationErrors = !errorList + require(hasNoValidationErrors, errorList.toString()) Map serviceResult = run service: 'checkAndUpdateWorkEffort', with: parameters require(ServiceUtil.isSuccess(serviceResult) as boolean, serviceResult.errorMessage) @@ -756,15 +757,15 @@ Map quickShipEntireOrder() { List shipmentShipGroupFacilityList // first get the order header; make sure we have a product store GenericValue orderHeader = from('OrderHeader').where(parameters).queryOne() - require(!(!orderHeader || !orderHeader.productStoreId), + require(orderHeader && orderHeader.productStoreId, 'ProductUiLabels', 'FacilityShipmentMissingProductStore') // get the product store entity GenericValue productStore = from('ProductStore').where(productStoreId: orderHeader.productStoreId).queryOne() // no reservations; no shipment; cannot use quick ship - require(!('Y' != productStore?.reserveInventory), 'ProductUiLabels', + require(productStore?.reserveInventory == 'Y', 'ProductUiLabels', 'FacilityShipmentNotCreatedForNotReserveInventory', [productStore: productStore]) // can't insert duplicate rows in shipmentPackageContent - require(!(productStore.explodeOrderItems == 'Y'), 'ProductUiLabels', + require(productStore.explodeOrderItems != 'Y', 'ProductUiLabels', 'FacilityShipmentNotCreatedForExplodesOrderItems', [productStore: productStore]) // locate shipping facilities associated with order item rez's List orderItemShipGrpInvResFacilityIds = from('OrderItemAndShipGrpInvResAndItem') @@ -1313,12 +1314,12 @@ Map addOrderShipmentToShipment() { // get orderItem GenericValue orderItem = from('OrderItem').where(parameters).queryOne() // make sure the orderItem is not already present in this shipment - require(!(from('OrderShipment') + require(from('OrderShipment') .where(orderId: parameters.orderId, orderItemSeqId: parameters.orderItemSeqId, shipGroupSeqId: parameters.shipGroupSeqId, shipmentId: parameters.shipmentId) - .queryCount() != 0), + .queryCount() == 0, "Not adding Order Item to plan for shipment [${parameters.shipmentId}] because" + " the order item is already in the shipment (order [${parameters.orderId}]," + " order item [${parameters.orderItemSeqId}])") @@ -1329,7 +1330,7 @@ Map addOrderShipmentToShipment() { } BigDecimal remainingQuantity = serviceResult.remainingQuantity - require(!(parameters.quantity > remainingQuantity), + require(parameters.quantity <= remainingQuantity, "Not adding Order Item to plan for shipment [${parameters.shipmentId}] because" + ' the quantity is greater than the remaining quantity' + " (order [${parameters.orderId}], order item [${parameters.orderItemSeqId}])") diff --git a/applications/product/src/test/groovy/org/apache/ofbiz/product/product/test/ProductPromoCondTests.groovy b/applications/product/src/test/groovy/org/apache/ofbiz/product/product/test/ProductPromoCondTests.groovy index 6ecb5e5f08d..8f17a06be88 100644 --- a/applications/product/src/test/groovy/org/apache/ofbiz/product/product/test/ProductPromoCondTests.groovy +++ b/applications/product/src/test/groovy/org/apache/ofbiz/product/product/test/ProductPromoCondTests.groovy @@ -20,6 +20,7 @@ package org.apache.ofbiz.product.product.test import java.sql.Timestamp import org.apache.ofbiz.base.util.UtilDateTime +import org.apache.ofbiz.base.util.UtilProperties import org.apache.ofbiz.entity.GenericValue import org.apache.ofbiz.order.shoppingcart.ShoppingCart import org.apache.ofbiz.testtools.JunitJupiterTest @@ -393,6 +394,66 @@ class ProductPromoCondTests implements JupiterTestHelper { assert serviceResult.compareBase < 0 } + // Regression coverage for the update() DSL / ServiceErrorException catch site added in + // updateProductPromoCond(): a PK with no matching ProductPromoCond record must come back as a + // service error carrying the original localized ServiceValueNotFound message, not the generic + // EntityUpdateBuilder message and not a silently-successful result. + @Test + @Order(12) + void testUpdateProductPromoCondNotFound() { + String productPromoId = testParams.productPromoId ?: 'TEST_NONEXISTENT_PROMO' + String productPromoRuleId = testParams.productPromoRuleId ?: '01' + String productPromoCondSeqId = testParams.productPromoCondSeqId ?: '01' + Map serviceCtx = [ + productPromoId: productPromoId, + productPromoRuleId: productPromoRuleId, + productPromoCondSeqId: productPromoCondSeqId, + userLogin: userLogin + ] + Map serviceResult = dispatcher.runSync('updateProductPromoCond', serviceCtx) + assert ServiceUtil.isError(serviceResult) + assert ServiceUtil.getErrorMessage(serviceResult) == + UtilProperties.getMessage('ServiceErrorUiLabels', 'ServiceValueNotFound', Locale.US) + } + + // updateProductPromoCond() had no coverage at all before this - not even a happy-path test. + // Creates a fresh ProductPromoCond fixture directly, then calls the service and verifies the + // change actually persisted (this also exercises the update() DSL's success path, which the + // not-found test above cannot). + @Test + @Order(13) + void testUpdateProductPromoCond() { + String productPromoId = testParams.productPromoId ?: 'TEST_PROMO_UPD' + String productPromoRuleId = testParams.productPromoRuleId ?: '01' + String productPromoCondSeqId = testParams.productPromoCondSeqId ?: '01' + GenericValue productPromo = delegator.makeValue('ProductPromo', [productPromoId: productPromoId]) + delegator.createOrStore(productPromo) + GenericValue productPromoRule = delegator.makeValue('ProductPromoRule', + [productPromoId: productPromoId, productPromoRuleId: productPromoRuleId]) + delegator.createOrStore(productPromoRule) + GenericValue productPromoCond = delegator.makeValue('ProductPromoCond', + [productPromoId: productPromoId, productPromoRuleId: productPromoRuleId, + productPromoCondSeqId: productPromoCondSeqId, condValue: 'OLD_VALUE']) + delegator.createOrStore(productPromoCond) + + String newCondValue = testParams.condValue ?: 'NEW_VALUE' + Map serviceCtx = [ + productPromoId: productPromoId, + productPromoRuleId: productPromoRuleId, + productPromoCondSeqId: productPromoCondSeqId, + condValue: newCondValue, + userLogin: userLogin + ] + Map serviceResult = dispatcher.runSync('updateProductPromoCond', serviceCtx) + assert ServiceUtil.isSuccess(serviceResult) + + GenericValue updated = from('ProductPromoCond') + .where('productPromoId', productPromoId, 'productPromoRuleId', productPromoRuleId, + 'productPromoCondSeqId', productPromoCondSeqId).queryOne() + assert updated + assert updated.condValue == newCondValue + } + private Map prepareConditionMap(ShoppingCart cart, String condValue) { return prepareConditionMap(cart, condValue, false) } diff --git a/applications/workeffort/src/main/groovy/org/apache/ofbiz/workeffort/workeffort/workeffort/WorkEffortServicesScript.groovy b/applications/workeffort/src/main/groovy/org/apache/ofbiz/workeffort/workeffort/workeffort/WorkEffortServicesScript.groovy index 9325d79dee3..efed7623906 100644 --- a/applications/workeffort/src/main/groovy/org/apache/ofbiz/workeffort/workeffort/workeffort/WorkEffortServicesScript.groovy +++ b/applications/workeffort/src/main/groovy/org/apache/ofbiz/workeffort/workeffort/workeffort/WorkEffortServicesScript.groovy @@ -198,7 +198,8 @@ Map createWorkEffort() { GenericValue workEffort = makeValue('WorkEffort', parameters) workEffort.workEffortId = workEffort.workEffortId ?: delegator.getNextSeqId('WorkEffort') String errMsg = UtilValidate.checkValidDatabaseId(workEffort.workEffortId) - require(!(errMsg), errMsg) + boolean isWorkEffortIdValid = !errMsg + require(isWorkEffortIdValid, errMsg) Timestamp now = UtilDateTime.nowTimestamp() workEffort.setFields([lastStatusUpdate: now, @@ -230,7 +231,7 @@ Map updateWorkEffort() { if (parameters.currentStatusId && workEffort.currentStatusId && parameters.currentStatusId != workEffort.currentStatusId) { Map statusValidChange = [statusId: workEffort.currentStatusId, statusIdTo: parameters.currentStatusId] - require(!(from('StatusValidChange').where(statusValidChange).queryCount() == 0), + require(from('StatusValidChange').where(statusValidChange).queryCount() > 0, label('WorkEffortUiLabels', 'WorkEffortStatusChangeNotValid', statusValidChange)) run service: 'createWorkEffortStatus', with: [*: parameters, statusId: parameters.currentStatusId, @@ -256,12 +257,10 @@ Map updateWorkEffort() { */ Map deleteWorkEffort() { // check permissions before moving on: if update or delete logged in user must be associated OR have corresponding UPDATE or DELETE permissions - require(!(from('WorkEffortPartyAssignment') - .where(workEffortId: parameters.workEffortId, - partyId: userLogin.partyId) - .queryCount() == 0 && - !security.hasEntityPermission('WORKEFFORTMGR', '_DELETE', userLogin)), - label('WorkEffortUiLabels', 'WorkEffortDeletePermissionError')) + boolean isAssignedToWorkEffort = from('WorkEffortPartyAssignment') + .where(workEffortId: parameters.workEffortId, partyId: userLogin.partyId) + .queryCount() > 0 + requireWorkEffortDeletePermission(isAssignedToWorkEffort) GenericValue workEffort = from('WorkEffort').where(parameters).queryOne() @@ -324,12 +323,11 @@ Map copyWorkEffort() { * @return Success response containing the workEffortId, error response otherwise. */ Map duplicateWorkEffort() { - require(!((parameters.removeWorkEffortAssocs == 'Y' || + boolean removesWorkEffortData = parameters.removeWorkEffortAssocs == 'Y' || parameters.removeWorkEffortContents == 'Y' || parameters.removeWorkEffortNotes == 'Y' || - parameters.removeWorkEffortAssignmentRates == 'Y') && - !security.hasEntityPermission('WORKEFFORTMGR', '_DELETE', userLogin)), - label('WorkEffortUiLabels', 'WorkEffortDeletePermissionError')) + parameters.removeWorkEffortAssignmentRates == 'Y' + requireWorkEffortDeletePermission(!removesWorkEffortData) String workEffortId = parameters.workEffortId ?: delegator.getNextSeqId('WorkEffort') GenericValue oldWorkEffort = from('WorkEffort').where(workEffortId: parameters.oldWorkEffortId).queryOne() GenericValue duplicateWorkEffort = oldWorkEffort.clone() @@ -407,7 +405,7 @@ void duplicateWorkEffortAssoc(String relationEntityName, String oldWorkEffortId, Map assocAcceptedCustRequestToWorkEffort() { // check status of customer request if valid GenericValue custRequet = from('CustRequest').where(parameters).cache().queryOne() - require(!(custRequet.statusId != 'CRQ_ACCEPTED'), label('CommonUiLabels', 'CommonErrorStatusNotValid')) + require(custRequet.statusId == 'CRQ_ACCEPTED', label('CommonUiLabels', 'CommonErrorStatusNotValid')) // create customer request / work effort relation run service: 'createWorkEffortRequest', with: parameters @@ -547,3 +545,8 @@ Map updateWorkEffortContactMech() { } return success([contactMechId: newContactMechId, oldContactMechId: workEffortContactMech.contactMechId]) } + +private void requireWorkEffortDeletePermission(boolean permissionBypassed) { + require(permissionBypassed || security.hasEntityPermission('WORKEFFORTMGR', '_DELETE', userLogin), + label('WorkEffortUiLabels', 'WorkEffortDeletePermissionError')) +} diff --git a/framework/common/src/main/groovy/org/apache/ofbiz/common/PortalPageServices.groovy b/framework/common/src/main/groovy/org/apache/ofbiz/common/PortalPageServices.groovy index 456ed3f3aa9..626f2b37cff 100644 --- a/framework/common/src/main/groovy/org/apache/ofbiz/common/PortalPageServices.groovy +++ b/framework/common/src/main/groovy/org/apache/ofbiz/common/PortalPageServices.groovy @@ -127,7 +127,7 @@ Map deletePortalPagePortlet() { * @return Success response with all attributes */ Map getPortletAttributes() { - require(!(!parameters.ownerUserLoginId && !parameters.portalPageId), + require(parameters.ownerUserLoginId || parameters.portalPageId, 'Service getPortletAttributes did not receive either ownerUserLoginId OR portalPageId') if (parameters.ownerUserLoginId) { GenericValue portalPagePortlet = from('PortalPageAndPortlet')