aditinikam commented on PR #6034:
URL: https://github.com/apache/fineract/pull/6034#issuecomment-4904444663
Thanks @dr-fuch — good points. Clarifications and what I changed:
**Validation wasn't dropped.** The `StandingInstructionDataValidator` checks
were ported into `StandingInstructionWriteServiceImpl` during the migration
(same `DataValidatorBuilder` rules). But your structural point stands — the
write service isn't the right home.
**So I extracted it** into a dedicated
`StandingInstructionRequestDataValidator` taking the typed DTOs, as you
suggested:
```java
public void validateForCreate(StandingInstructionCreationRequest request) {
... }
public void validateForUpdate(StandingInstructionUpdatesRequest request) {
... }
```
The write service now delegates to it.
**The four scenarios** — good catch, the `Dues` amount rule was genuinely
missing. `Fixed → amount required` already covered scenarios 1 & 2; I added the
inverse for 3 & 4 so `Dues` now rejects an `Amount`, with unit tests for all
four combinations.
**On `@Min`/`@Max` on the DTO:** kept `DataValidatorBuilder` because the
DTOs are intentionally all-`String` for locale-aware number/date parsing;
`@Min`/`@Max` need numeric types, and retyping risks breaking locale-formatted
binding.
**On `AccountTransferStandingInstruction.update(...)`:** agreed it's a
legacy smell — I'll encapsulate the params into an object as a follow-up,
keeping this PR focused on validation.
--
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]
