FINERACT-2455: WC - Support discount fee and discount fee adjustment by transaction external id

[oleksii-novikov-onix]

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
• I followed the AI Policy.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[adamsaghy]

@oleksii-novikov-onix To cover all possibilities:

• Let's add a new field relatedExternalResourceId which accepts the external id of the target loan transaction
  • Similar behaviour like relatedResourceId but for external id

[galovics]

Routing, permissions, transaction-type checks and the COB filter all hold up, and the e2e coverage is good. No new JAX-RS routes are added - the four transactions/{transactionId} and transactions/external-id/{transactionExternalId} POST endpoints already existed for undo and just gain two more command values, and the builders use the same DISCOUNTFEE/DISCOUNTFEEADJUSTMENT + WORKINGCAPITALLOAN pair as the loan-level ones, so the same permissions apply. The loan and transaction external ids aren't mixed up (the transaction is loaded with findByIdAndWcLoan_Id), a wrong-type transaction is rejected in both directions, the duplicate discount fee guard still applies, and the COB filter is unaffected: isExternal now uses the anchored EXTERNAL_ID_LOAN_PATH_PATTERN, so /{loanId}/transactions/external-id/{x} resolves the numeric loan id (the #6351 issue), covered by WorkingCapitalLoanCOBApiFilterTest.

Things I'd change:

• A missing/foreign transaction id in the path returns 400, while the external-id form and the GET return 404. resolveTransactionId returns a supplied transactionId without checking it belongs to the loan, so an unknown one reaches the service and fails with a 400 ...transaction.not.found; the docs and e2e tests lock the 400 in. 404 is the usual convention for a path resource - reuse findByIdAndWcLoan_Id in resolveTransactionId and throw the not-found exception.
• The handlers pick the flow by whether loanId happens to be set (WorkingCapitalLoanDiscountFeeCommandHandler / ...AdjustmentCommandHandler: Optional.ofNullable(command.getLoanId())). It works only because the loan-level builders don't set loanId; other WC builders do, so if someone adds it "for consistency" every body-based call silently switches to the path form and the loan id is treated as a disbursement transaction id. Use an explicit signal (a dedicated action, or the transaction id on the wrapper) and add a unit test per branch.
• Pending maker-checker rows record a transaction id as the loan's resource id (entityName = WORKINGCAPITALLOAN, entityId = transactionId in the new builders); updateForAudit corrects it afterwards, but pending rows are misleading.
• Docs say a non-positive relatedResourceId gets ...not.a.number, but longGreaterThanZero returns ...not.greater.than.zero for 0/-1; and the new "no relatedResourceId in the body of a path form" check has no test.
• The non-numeric relatedResourceId validation now also changes the error on the existing body-based endpoint (probably an improvement, but worth noting), and adding externalLoanId to the transaction data/response/Avro isn't needed for this ticket (and is inserted mid-record) - consider its own ticket.

Recommendation: COMMENT

[oleksii-novikov-onix]

@oleksii-novikov-onix To cover all possibilities:

• Let's add a new field relatedExternalResourceId which accepts the external id of the target loan transaction

  • Similar behaviour like relatedResourceId but for external id

Done.

1. relatedExternalResourceId added to the body of discount fee and discount fee adjustment (both loan id and loan external id endpoints).
2. Same behaviour as relatedResourceId - same checks, same order, same error codes, same response.
3. Sending both fields at once is rejected with 400.
4. Covered by e2e, including Batch API; docs updated.

[oleksii-novikov-onix]

Routing, permissions, transaction-type checks and the COB filter all hold up, and the e2e coverage is good. No new JAX-RS routes are added - the four transactions/{transactionId} and transactions/external-id/{transactionExternalId} POST endpoints already existed for undo and just gain two more command values, and the builders use the same DISCOUNTFEE/DISCOUNTFEEADJUSTMENT + WORKINGCAPITALLOAN pair as the loan-level ones, so the same permissions apply. The loan and transaction external ids aren't mixed up (the transaction is loaded with findByIdAndWcLoan_Id), a wrong-type transaction is rejected in both directions, the duplicate discount fee guard still applies, and the COB filter is unaffected: isExternal now uses the anchored EXTERNAL_ID_LOAN_PATH_PATTERN, so /{loanId}/transactions/external-id/{x} resolves the numeric loan id (the #6351 issue), covered by WorkingCapitalLoanCOBApiFilterTest.

Things I'd change:

• A missing/foreign transaction id in the path returns 400, while the external-id form and the GET return 404. resolveTransactionId returns a supplied transactionId without checking it belongs to the loan, so an unknown one reaches the service and fails with a 400 ...transaction.not.found; the docs and e2e tests lock the 400 in. 404 is the usual convention for a path resource - reuse findByIdAndWcLoan_Id in resolveTransactionId and throw the not-found exception.
• The handlers pick the flow by whether loanId happens to be set (WorkingCapitalLoanDiscountFeeCommandHandler / ...AdjustmentCommandHandler: Optional.ofNullable(command.getLoanId())). It works only because the loan-level builders don't set loanId; other WC builders do, so if someone adds it "for consistency" every body-based call silently switches to the path form and the loan id is treated as a disbursement transaction id. Use an explicit signal (a dedicated action, or the transaction id on the wrapper) and add a unit test per branch.
• Pending maker-checker rows record a transaction id as the loan's resource id (entityName = WORKINGCAPITALLOAN, entityId = transactionId in the new builders); updateForAudit corrects it afterwards, but pending rows are misleading.
• Docs say a non-positive relatedResourceId gets ...not.a.number, but longGreaterThanZero returns ...not.greater.than.zero for 0/-1; and the new "no relatedResourceId in the body of a path form" check has no test.
• The non-numeric relatedResourceId validation now also changes the error on the existing body-based endpoint (probably an improvement, but worth noting), and adding externalLoanId to the transaction data/response/Avro isn't needed for this ticket (and is inserted mid-record) - consider its own ticket.

Recommendation: COMMENT

1. Missing or foreign transaction id in the path returns 400 instead of 404 - fixed, resolveTransactionId now looks it up with findByIdAndWcLoan_Id, and undo gets the same treatment since it shares that method.
2. Handlers pick the flow by whether loanId happens to be set - fixed, the path builders now put the transaction id in subentityId and keep entityId = loanId, the handlers branch on that, and both branches have a unit test.
3. Pending maker-checker rows record a transaction id as the loan's resource id - fixed by the same change, entityId is the loan id now, which is what entityName = WORKINGCAPITALLOAN promises.
4. Docs claim not.a.number for a non-positive relatedResourceId, and the path-form body check has no test - the docs were already corrected, and the check is now covered in the existing validator unit test.
5. The non-numeric relatedResourceId error changed on the existing body endpoint - intentional, a UUID there used to end in a 500 and is now rejected as a validation error.
6. externalLoanId is out of scope here - deliberate: the transaction payload identified the loan only by the numeric wcLoanId, while the legacy loan transaction and the WC charge payloads already carry externalLoanId, so a consumer working with UUIDs could not resolve the loan from an event.

[adamsaghy]

LGTM