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.

[somasorosdpc]

@adamsaghy I fixed the following issues:

• isValid check is now consistent (1)
• I kept the original error code and message (2)
• fixed the Id of the api by making relation to the original Loan template enum (3, 5)
• added missing unit test scenarios (4)

[somasorosdpc]

API backward compatibility should fail, because API changed. We no longer accept few values on create/edit loan product - advanced payment allocation - transaction type field, and it is documented on swagger.

[galovics]

The design is good: the new WorkingCapitalPaymentAllocationTransactionType enum is the single source for the template, the parser/validator and the loan-level override path, so they can't drift, and the five allowed types (DEFAULT, REPAYMENT, PAYOUT_REFUND, GOODWILL_CREDIT, CHARGE_ADJUSTMENT) are exactly the ones the allocator and WorkingCapitalLoanWritePlatformServiceImpl handle (GOODWILL_CREDIT is included, so the #6349 problem doesn't come back). Keeping the core ordinals as ids is the right call. But the PR fails a required gate because it deliberately breaks the request contract.

API compatibility. run-api-backward-compatibility fails with R011 (request enum value deleted) for 9 values on POST /v1/working-capital-loan-products and both PUTs. The template change itself is additive, but this is a real break, not just a noisy check: WC shipped with the full list in allowableValues, so clients sending those values used to get 201/200 and now get 400. The break is presumably right (those types never did anything on WC), but it needs an explicit maintainer decision rather than merging around a red check, and the PR description is still the empty template with no justification.

Existing data. A product created earlier may already have, say, an INTEREST_REFUND rule saved. After this, a client that GETs, edits something, and PUTs the paymentAllocation back gets a 400 (wc-payment-allocation.with.not.valid.transaction.type), and the loan-level override goes through the same parser. Can we either add a Liquibase changeset removing unsupported rules from the product (and loan) allocation rule tables, or at least record it as a breaking change for the release notes?

Smaller:

• The Swagger allowableValues is a second hardcoded copy of the list (annotations can't reference values()), with nothing checking they match - a reflection unit test asserting equality with the enum would catch drift. The loan-level PostPaymentAllocationRule.transactionType now enforces the same five values but documents none.
• WorkingCapitalLoanBusinessEventSerializer.PAYMENT_TRANSACTION_TYPES is another pre-existing copy of the same set (enum minus DEFAULT) - worth deriving from the enum in a follow-up.
• validateAllocationRule's null-type check is now unreachable because the parser throws first.
• No test covers the loan application override path (POST /working-capital-loans with paymentAllocation), which this also restricts.
• Two commits under the same key - squash.

Recommendation: CHANGES_REQUESTED