budaidev commented on PR #6370:
URL: https://github.com/apache/fineract/pull/6370#issuecomment-5599723832
> Scope note first: despite the title, this is product-definition only - the
adoc says so explicitly. Disbursement charges can be created/updated/offered
via the template API but aren't assessed during disbursement, inherited from
the product, or touched in the amortization schedule. Worth confirming that's
tracked as a follow-up JIRA, since the product becomes creatable in the
meantime while the platform silently ignores it.
>
> On the code itself, two real bugs on the update path:
>
> **1. `chargePaymentMode` is validated for WC charges but never
persisted.** The validation block checks
`ChargeAppliesTo.WORKING_CAPITAL_LOAN`, but the assignment block a few lines
down is gated on `isLoanCharge()` (`ChargeAppliesTo.LOAN` only) - so a WC
charge's `chargePaymentMode` is validated, accepted, and then silently
discarded. `PUT /charges/{id}` with `chargePaymentMode` set returns 200,
reports nothing in `changes`, and the column stays whatever it was (nullable,
defaults to NULL). The integration tests don't catch this because the only
assertion on this field reads a `NULL` column back as `0` via
`ResultSet.getInt`.
>
> **2. `PUT` with an explicit `null` `chargePaymentMode` is rejected, while
`POST` accepts it.** `command.hasParameter(...)` returns true for a JSON
`null`, and `isOneOfTheseValues` treats a null value as a validation failure -
so `PUT` with `{"chargePaymentMode": null}` 400s. The create path in
`ChargeDefinitionCommandFromApiJsonDeserializer` explicitly guards against this
(`if (chargePaymentMode != null) {...}`), and there's even a test pinning that
exact behavior for `POST` (`Order(23)`) - but no equivalent guard or test on
`PUT`.
>
> Please pick a fix for #1 (either extend the persist block to WC, or
explicitly reject the parameter on WC update) and add the same null-guard from
the create path to `Charge.java:524` for #2, plus a mirroring test.
>
> Non-blocking: the penalty/overdue guard relaxation in `update()` also
widens for savings/client/share charges, not just WC - unreachable via their
own allow-lists today, but worth calling out explicitly in the commit message
since it's broader than
[FINERACT-2455](https://issues.apache.org/jira/browse/FINERACT-2455) strictly
requires.
>
> Recommendation: CHANGES_REQUESTED
Thanks, both confirmed and fixed. chargePaymentMode is now persisted on the
WC update path with its own assignment and a changes entry, pinned by
ChargeTest and FeignWorkingCapitalChargeProductTest. The PUT null case cannot
actually reach Charge.update() because the Gson serializer drops nulls before
the command is built (verified with a raw PUT on this head), but I still moved
the not-null decision into the entity so a WC charge resolves null to Regular
like on create, a loan charge rejects it, and the other types ignore it. The
penalty/overdue guard relaxation is now scoped to working capital charges only,
so client and share charges keep the old behaviour, with a unit test and a note
in the commit message.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
