FINERACT-2455: WC - Loan Product Template Update Advanced Payment Allocation Transaction Type List

[somasorosdpc]

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]

@somasorosdpc Please review the below findings / concerns:

Findings

• WorkingCapitalAdvancedPaymentAllocationsJsonParser.java:67 — CONFIRMED. The template now offers only 5 transaction types, but the write path still parses into the core PaymentAllocationTransactionType and the validator only rejects null. POSTing transactionType: "DOWN_PAYMENT" is accepted and persisted; WorkingCapitalLoanAllocationRequestFactory.getAllocationRule never matches it, so the product carries a dead rule the UI can no longer show or edit.

• WorkingCapitalLoanProductApiResourceSwagger.java:195 — CONFIRMED. PostPaymentAllocation.transactionType still documents all 14 core types as allowableValues, directly contradicting the restricted template this PR introduces.
  WorkingCapitalPaymentAllocationTransactionType.java:61 — PLAUSIBLE. ordinal() + 1 reassigns ids the WC template previously returned (PAYOUT_REFUND 5→3, GOODWILL_CREDIT 6→4, CHARGE_ADJUSTMENT 8→5) and collides with different types in /loanproducts/template (id 3 = DOWN_PAYMENT there). Server round-trips use code, so this only bites a client keying off id.

• WorkingCapitalPaymentAllocationTransactionType.java:20 — CONFIRMED. The Apache license header is duplicated (lines 1–18 and 20–37). Compiles and RAT passes, but the second copy should go.

Two notes that aren't findings: the new enum's loanTransactionType field and isDefault() are currently unused (all domain/persistence code still uses the core enum), and WorkingCapitalLoanProductCRUDTest only asserts the template list is non-empty — nothing pins the restricted set, so the change has no test coverage.

[somasorosdpc]

@adamsaghy I fixed all the issues.

• validation now in action for the field
• Swagger documentation updated
• Ordinal is replaced to the transaction id, default is 0.
• removed duplicated licence
• removed isDefault() from the new Enum
• Added verification for the supported fields in WorkingCapitalLoanProductCRUDTest

[adamsaghy]

Please review the below findings / concerns:

1. isValid is case-insensitive but the enum lookup is not → misleading error message

WorkingCapitalPaymentAllocationTransactionType.isValid() matches with strip().equalsIgnoreCase(), while the parser then resolves with Enums.getIfPresent(PaymentAllocationTransactionType.class, transactionType), which is an exact-name match:

if (WorkingCapitalPaymentAllocationTransactionType.isValid(transactionType)) {
rule.setTransactionType(Enums.getIfPresent(PaymentAllocationTransactionType.class, transactionType).orNull());
}

WorkingCapitalAdvancedPaymentAllocationsJsonParser.java:66

I traced this through: "default" or " REPAYMENT " passes the gate, getIfPresent returns absent, and the rule is stored with transactionType == null. Nothing is wrongly accepted — WorkingCapitalAdvancedPaymentAllocationsValidator.validate always rejects it — but the error the caller gets is the wrong one:

single rule with "default" → wc-payment-allocation-without-default, "At least one DEFAULT payment allocation must be provided"
two mis-cased rules alongside a valid DEFAULT → both collapse to null → wc-payment-allocation-with-duplicate-transaction-type, "The same transaction type must be provided only once"

Neither tells the user their casing is wrong. Simplest fix: make isValid an exact-name match (v.name().equals(transactionType)), so the gate and the lookup agree and the new wc-payment-allocation-transaction-type-not-supported error is what fires.

2. Error reporting duplicates the validator and changes an existing error code

The new else branch hand-rolls the exception the validator already has a helper for (validator.raiseValidationError(code, msg) — the parser holds a validator reference), and it introduces a second code for what is conceptually the same failure as the existing wc-payment-allocation.with.not.valid.transaction.type. Two smaller points on top:

The new code uses dashes throughout (wc-payment-allocation-transaction-type-not-supported) whereas the validator's transaction-type code is dot-separated; worth matching one convention.
A missing transactionType (previously left null and reported by the validator as wc-payment-allocation.with.not.valid.transaction.type) now produces the new code and the message "Invalid transaction type" instead. That's an API error-code change for existing clients. Also, the message names neither the parameter nor the offending value — including the rejected value and the supported list would make it actionable.
3. The template's id values change meaning, silently

Old: PaymentAllocationTransactionType.getValuesAsEnumOptionDataList() uses ordinal() + 1 → DEFAULT=1, REPAYMENT=2, PAYOUT_REFUND=5, GOODWILL_CREDIT=6, CHARGE_ADJUSTMENT=8.
New: WorkingCapitalPaymentAllocationTransactionType.java:42 uses LoanTransactionType.getValue() with 0L for DEFAULT → 0, 2, 22, 23, 26.

So the same field (advancedPaymentAllocationTransactionTypes) now returns different ids for WC products than for regular loan products, and 0 is used as a sentinel for DEFAULT. If intentional, fine — but it isn't mentioned anywhere and the added test only asserts on getCode(), so nothing pins the ids. Either assert them in the test or keep the core ordinal() + 1 convention.

4. Test coverage only exercises the template, not the new rejection

The added assertions in WorkingCapitalLoanProductCRUDTest.testRetrieveTemplate cover the listing. The behavioural change with actual impact — create/update of a WC product (and of a WC loan application, via the shared parser) with e.g. DOWN_PAYMENT, INTEREST_REFUND, MERCHANT_ISSUED_REFUND now being rejected where it previously succeeded — has no test. I checked: no existing WC test uses one of the dropped types, so nothing breaks, but that also means nothing guards the new rule. A negative test asserting the new error code on POST/PUT would be worth adding, and this API break deserves a line in the PR description.

5. Nit — the new enum has no link to the one it gates

WorkingCapitalPaymentAllocationTransactionType mirrors PaymentAllocationTransactionType but the parser still resolves by string against the core enum. A toPaymentAllocationTransactionType() on the new enum (returning the mapped core constant) would remove the double lookup and make a future WC-only constant with no core counterpart a compile error rather than a silent null.