Reject party/role combinations the party does not hold for BudgetRole (OFBIZ-12371) - #1736
Open
karthikchundi-commits wants to merge 2 commits into
Open
Conversation
…OFBIZ-12372, OFBIZ-12373) InvoiceRole, BillingAccountRole, and FinAccountRole all let a user submit any partyId + roleTypeId combination with no validation that the party actually holds that role. For InvoiceRole this hits the InvoiceRole/ PartyRole foreign key constraint and surfaces a raw SQL error. For BillingAccountRole and FinAccountRole, an unconditional ensurePartyRole eca instead silently fabricates a PartyRole record for the submitted roleTypeId, regardless of whether it makes sense for that party. Adds a small, reusable checkPartyRoleExists service (party component, mirroring the existing ensureNaPartyRole helper) that looks up PartyRole by (partyId, roleTypeId) and, if missing, fails with the same PartyRoleAssociationRequired message already used for this exact class of bug in createWorkEffortAndPartyAssign. Wires it as an in-validate eca on createInvoiceRole, createBillingAccountRole, and createFinAccountRole, so invalid combinations are rejected before the existing invoke-event ensurePartyRole ecas ever run. Adds regression tests: a negative case per ticket proving the party/role combination is now rejected (and, for BillingAccountRole/ FinAccountRole, that no PartyRole is fabricated), plus a new AutoAcctgBillingAccountTests suite since BillingAccountRole had no existing test coverage. Signed-off-by: Karth <karthik.chundi@gmail.com>
… (OFBIZ-12371) Same bug shape already fixed for InvoiceRole/BillingAccountRole/ FinAccountRole: createBudgetRole's unconditional ensurePartyRole invoke eca silently fabricates a PartyRole record for whatever roleTypeId is submitted, with no validation that the party actually holds (or should hold) that role. Wires the existing checkPartyRoleExists service (added for the other three role types) as an in-validate eca on createBudgetRole, so invalid combinations are rejected before the invoke-event ensurePartyRole eca ever runs. Adds a positive-case and negative-case regression test to AutoAcctgBudgetTests, following the same pattern used for the other three role types. Depends on the checkPartyRoleExists service already added for OFBIZ-12370/12372/12373 (open as apache#1727) - this change is a no-op without it. Signed-off-by: Karth <karthik.chundi@gmail.com>
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.
Jira ticket
https://issues.apache.org/jira/browse/OFBIZ-12371
Depends on #1727
This PR depends on #1727 and should be reviewed/merged after it. It builds directly on top of that branch, which adds the
checkPartyRoleExistsservice this PR wires up forcreateBudgetRole. Without #1727, thecheckPartyRoleExistsservice referenced here doesn't exist yet.Purpose and Description
Same bug shape as OFBIZ-12370/12372/12373 (fixed in #1727), one ticket over in the same family Pierre Smits filed:
createBudgetRolehas an unconditionalensurePartyRoleinvokeeca that silently fabricates aPartyRolerecord for whateverroleTypeId/partyIdis submitted on the Budget Role page, with no validation that the party actually holds (or should hold) that role.This wires the
checkPartyRoleExistsservice added in #1727 as anin-validateeca oncreateBudgetRole, mirroring the exact same pattern already used there forcreateInvoiceRole/createBillingAccountRole/createFinAccountRole. Sincein-validateruns beforeinvoke, an invalid party/role combination is rejected before the existingensurePartyRoleeca ever fires - that eca is left untouched and becomes reachable only for combinations that are already valid.Changes 🏗️
applications/accounting/servicedef/secas.xml: adds anin-validateeca oncreateBudgetRolecallingcheckPartyRoleExists, right before the existinginvokeeca.applications/accounting/src/test/groovy/org/apache/ofbiz/accounting/accounting/AutoAcctgBudgetTests.groovy: adds a positive-case test (DEMO_COMPANY+INTERNAL_ORGANIZATIO, a role the test party already legitimately holds perAccountingTestsData.xml) and a negative-case test (DEMO_COMPANY+CARRIER, a role it does not hold - must now be rejected, and must not fabricate a spuriousPartyRole), following the exact pattern used for the other three role types in Reject party/role combinations the party does not hold (OFBIZ-12370, OFBIZ-12372, OFBIZ-12373) #1727.Testing
applications/accounting/servicedef/secas.xmlagainst the realservice-eca.xsdschema viaxmllint --schema- validates cleanly.gradle compileTestGroovy(JDK 21) - compiles clean, including the new test methods.checkstyleMain checkstyleTest codenarcMain codenarcTest) ran clean on push - no violations.Checklist