[PR] Reject party/role combinations the party does not hold (OFBIZ-12370, OFBIZ-12372, OFBIZ-12373) [ofbiz-framework]

["karthikchundi-commits (via GitHub)" <gi...@apache.org>]

karthikchundi-commits opened a new pull request, #1727:
URL: https://github.com/apache/ofbiz-framework/pull/1727

   ### Jira tickets
   - https://issues.apache.org/jira/browse/OFBIZ-12370 (InvoiceRole)
   - https://issues.apache.org/jira/browse/OFBIZ-12372 (BillingAccountRole)
   - https://issues.apache.org/jira/browse/OFBIZ-12373 (FinAccountRole)
   
   ### Purpose and Description
   
   Follow-up to #1692 (OFBIZ-12374), which fixed the same family of bug for TaxAuthority. These three tickets are a different flavor of it: unlike TaxAuthority, the roleTypeId on InvoiceRole/BillingAccountRole/FinAccountRole is a free-choice dropdown over every RoleType, not a single fixed role, so the "role-scoped LookupXxx form" pattern used for TaxAuthority/Supplier/GovernmentAgency doesn't directly generalize here - there's no single roleTypeId to hardcode into a lookup form.
   
   Instead:
   
   - **createInvoiceRole**: no existing safety net. Submitting a party/role combination where the party doesn't already hold that role hits the `InvoiceRole`/`PartyRole` foreign key constraint and surfaces a raw SQL error to the user.
   - **createBillingAccountRole** / **createFinAccountRole**: an unconditional `ensurePartyRole` eca on `invoke` silently *fabricates* a `PartyRole` record for whatever roleTypeId was submitted, regardless of whether it's a sensible role for that party. No FK error, but a spurious PartyRole gets created with no real-world basis - this matches the "PartyRole created" wording in both ticket titles.
   
   This turns out to be an existing, established OFBiz pattern for exactly this failure mode - `createWorkEffortAndPartyAssign` (`WorkEffortServicesScript.groovy`) already does a `PartyRole` existence check up front and returns the `PartyRoleAssociationRequired` error message ("Party id: X should be in role: Y") if it's missing. This PR generalizes that same check into a small reusable service rather than duplicating it three times inline:
   
   - `checkPartyRoleExists` (new service, `applications/party/servicedef/services.xml` + `PartySimpleMethods.xml`, mirroring the existing `ensureNaPartyRole` helper right next to it): looks up `PartyRole` by `(partyId, roleTypeId)`; if missing, fails with the existing `PartyRoleAssociationRequired` label (same one `createWorkEffortAndPartyAssign` uses), including the RoleType's description for a readable message.
   - Wired as an `in-validate` eca on `createInvoiceRole`, `createBillingAccountRole`, and `createFinAccountRole` (`applications/accounting/servicedef/secas.xml`). Since `in-validate` runs before `invoke`, an invalid combination is rejected before the existing `ensurePartyRole` invoke-event ecas ever run - so those ecas are left untouched and only become reachable for combinations that are already valid (where they're a harmless no-op, since the PartyRole already exists).
   
   ### Testing
   
   - `AutoAcctgInvoiceTests.testCreateInvoiceRoleRejectsPartyWithoutRole` and `AutoAcctgFinAccountTests.testCreateFinAccountRoleRejectsPartyWithoutRole`: new negative-case regression tests added alongside the existing positive-case tests in each file, asserting the CARRIER role (which DEMO_COMPANY does not hold, per `AccountingTestsData.xml`) is now rejected.
   - `AutoAcctgBillingAccountTests` (new file + new `auto-accounting-billingaccount-tests` entry in `accountingtests.xml`): BillingAccountRole had no existing test coverage at all, so this adds a small suite covering create-BillingAccount, a valid role assignment (INTERNAL_ORGANIZATIO, which DEMO_COMPANY does hold), and the same negative case - also asserting no spurious `PartyRole` gets created for the rejected combination.
   - Confirmed the existing `testCreateInvoiceRole` / `testCreateFinAccountRole` positive-case tests still pass under the new validation without changes, since their `DEMO_COMPANY` + `INTERNAL_ORGANIZATIO` combination is already seeded as a real `PartyRole` in `AccountingTestsData.xml`.
   - Verified `applications/accounting/servicedef/secas.xml`, `applications/party/servicedef/services.xml`, and `applications/party/minilang/party/PartySimpleMethods.xml` against their actual XSD schemas (`service-eca.xsd`, `services.xsd`, `simple-methods.xsd`) with `xmllint --schema` - all validate cleanly.
   - Ran `gradle compileTestGroovy` for the whole tree (JDK 21) - compiles clean, including the new/changed test files.
   - I don't have a running OFBiz instance with a live database in this environment, so I could not execute the Groovy test suite itself end-to-end (only compile it) - I'd appreciate a CI run / maintainer review of the actual test execution in addition to the fix.
   
   ### Notes / scope
   
   `createBudgetRole` has the identical unconditional `ensurePartyRole` invoke-eca pattern as BillingAccountRole/FinAccountRole (same silent-fabrication issue), but it isn't covered by any of these three tickets, so I left it untouched to keep this PR scoped to OFBIZ-12370/12372/12373. Happy to follow up there too if useful.
   
   ### Checklist
   - [x] My code follows the code style of this project
   - [ ] I have updated the documentation accordingly (n/a - no user-facing docs for this internal validation)
   - [x] I have read the CONTRIBUTING document
   - [x] I have added tests to cover my changes


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@ofbiz.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reject party/role combinations the party does not hold (OFBIZ-12370, OFBIZ-12372, OFBIZ-12373) [ofbiz-framework]

["karthikchundi-commits (via GitHub)" <gi...@apache.org>]

karthikchundi-commits commented on PR #1727:
URL: https://github.com/apache/ofbiz-framework/pull/1727#issuecomment-5548698247

   Following up here since this (and the dependent #1736) haven't had a first review yet. These are a follow-up to the TaxAuthority fix in #1692 — same underlying issue (party/role combinations created with no validation), just for InvoiceRole/BillingAccountRole/FinAccountRole/BudgetRole. Happy to address any feedback. Thanks!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@ofbiz.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org