Groovy DSL cleanup: WorkEffort helper, double-negation fixes, update() adoption - #2039
Merged
ashishvijaywargiya merged 13 commits intoSep 17, 2026
Merged
Conversation
…uards setPaymentStatus() used hardcoded English strings for its not-found checks instead of label()-resolved messages like the rest of the method; added AccountingPaymentRecordNotFound reuse and a new AccountingStatusItemNotFound label. Several require(!(X), msg) guards across PaymentServices, QuoteServicesScript, ShoppingListServicesScript, InvoiceServicesScript and TaxAuthorityServicesScript reused a negated expression right next to its own message (or nested a second negation inside), the same shape already responsible for one inverted-check bug on this branch. Rewrote each via De Morgan's laws or a plain equality flip into an equivalent guard with the negation-reuse removed, and gave the errorMesg/errorMessage/errorMsg validation checks an explicit named boolean. Also converted the two remaining old-style if/return error() not-found checks in RequirementServicesScript.groovy's deleteRequirementAndRelated() and autoAssignRequirementToSupplier() to require(), consistent with createTransferFromRequirement() in the same file. No behavior change.
Extended the earlier accounting/order fix to every remaining instance of the same shape across applications/ and framework/: a negated expression reused as its own error message, or a negation nested inside another negated compound condition. Same treatment as before - De Morgan's laws or a plain equality flip for compound/comparison conditions, an explicit named boolean for the errorMessage/errorMesg/messages-reused-as-message cases. Left the plain single-variable existence checks (require(!(quote), ...) and similar) untouched since there's no compound structure to invert and they aren't the footgun this is about. Most notable: WorkEffortServicesScript.groovy's deleteWorkEffort() and duplicateWorkEffort() are the exact methods whose double-negated permission check was already inverted once (commit 515fa7e). The logic was corrected there, but the risky nested-negation style was never cleaned up until now. No behavior change.
deleteWorkEffort() and duplicateWorkEffort() each carried their own independently-written WORKEFFORTMGR/_DELETE permission check with a different bypass condition. This is exactly the pair whose logic was accidentally inverted once before (commit 515fa7e) - duplication is why one copy could drift from the other undetected. Extracted the shared permission-check-and-error into requireWorkEffortDeletePermission(), parameterized by each method's own bypass condition, so the permission rule and its error label now live in one place. No behavior change.
Replaces the manual queryOne()+require()+store() with the update() DSL's built-in fail-fast query, wrapped in try/catch to preserve the original not-found message exactly. No behavior change.
This reverts commit 1c118a9. The original code called setNonPKFields(parameters, false) (setIfEmpty=false), which skips empty-string fields and preserves the record's existing value. EntityUpdateBuilder.set() always calls the single-arg setNonPKFields(fields) (setIfEmpty=true), which nulls out empty-string fields instead. PartyTaxAuthInfo's non-PK fields are all optional form inputs, so this DSL adoption would have silently cleared fields an edit form submits blank instead of leaving them untouched. Not a safe update() candidate given the DSL helper's current behavior; reverting rather than accepting the behavior change.
Replaces the manual queryOne()+if/else+store() with the update() DSL's built-in fail-fast query, wrapped in try/catch to preserve the original error message exactly. No behavior change.
Replaces the manual queryOne()+if/else+store() with the update() DSL's built-in fail-fast query, wrapped in try/catch to preserve the original i18n error message exactly. No behavior change.
The variable isn't actually ignored - it's the signal that update()'s built-in query found no matching record, which is exactly why each catch block returns the original not-found error. notFound names that reason instead of implying indifference. No behavior change.
Settling on this codebase's dominant convention for a caught exception variable rather than a bespoke name. No behavior change.
checkUpdateQuoteStatus(), updateQuoteItem(), autoUpdateQuotePrice() each replace a manual queryOne()+require()+store() with the update() DSL's built-in fail-fast query, wrapped in try/catch to preserve each original error message exactly. No behavior change.
Replaces the manual queryOne()+if/else+store() with the update() DSL's built-in fail-fast query, wrapped in try/catch to preserve the original error message exactly. The toName default is computed into a separate fields-to-set map rather than mutated onto parameters, so the where() lookup still matches the record's existing values. No behavior change.
Replaces the manual queryOne()+if/else+store() with the update() DSL's built-in fail-fast query, wrapped in try/catch to preserve the original error message exactly. The otherValue override is computed into a separate fields-to-set map rather than mutated onto parameters, so the where() lookup still matches the record's existing values. No behavior change.
Adds not-found regression tests for the 7 update() DSL conversions on this branch (expireRateAmount, checkUpdateQuoteStatus, updateQuoteItem, autoUpdateQuotePrice, updatePartyInvitation, updateProductPromoCond, updatePaymentContent), asserting the original localized error message still surfaces through the ServiceErrorException catch block. Also adds happy-path coverage for updatePaymentContent and updateProductPromoCond, which had none. No production code change - test-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I continued the Groovy DSL cleanup work on top of the earlier require()/fail() rollout, fixing a double-negation footgun across applications and framework and extracting the duplicated WorkEffort delete-permission check into one shared helper.
It also picks up the update() DSL adoption from where the last pass left off, converting 7 more call sites that were previously skipped in my last pass.
I re-checked every other converted site for the same issue and none of them are affected, so that risk is isolated to the one site I backed out.
Since the update() DSL had never been caught by a script-level try/catch before, I added negative tests for all seven conversions plus a couple of missing happy-path tests, and all four affected test suites pass clean.
Nothing here changes behavior on purpose, every conversion preserves the exact original error message a caller would have seen before.