mariiaKraievska commented on PR #6662:
URL: https://github.com/apache/fineract/pull/6662#issuecomment-6274493607
> ## Review summary
> Hi @mariiaKraievska, thanks for the PR. I'm not convinced the new rule
plays well with what the server itself stores. On product create we always
write `breachGraceDays = 0` and `breachStartType = DISBURSEMENT`, even when
there's no breach. So any client that does GET -> edit -> PUT on a product (or
loan) without a breach now gets `cannot.enable.breach.*.without.breach` for
values it never set. Look at the IT changes:
`testUpdateWorkingCapitalLoanProductBreachGraceDays` and
`testUpdateBreachStartType` had to switch from the full builder payload to a
minimal PUT to keep passing. That's the compat break showing up in our own
tests. Also the check only runs one way: you can't send grace days without a
bucket, but you can still drop the bucket (`delinquencyBucketId: null` /
`breachId: null`) and keep the grace days, so we still end up in the state this
PR is trying to prevent. Plus this touches the same lines in
`WorkingCapitalLoanApplicationDataValidator` as #6636 (same ticket), so
one of them will need a rebase. Thoughts?
>
> Recommendation: REQUEST_CHANGES
>
> ## Inline comments
> ###
`fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloanproduct/serialization/WorkingCapitalLoanProductDataValidator.java:412-415`
(also :441-445 and the matching spots in
`WorkingCapitalLoanApplicationDataValidator`)
> ```java
> + rejectIfSpecifiedWithoutParent(baseDataValidator,
WorkingCapitalLoanProductConstants.breachGraceDaysParamName,
> + breachGraceDays != null, effectiveBreachId,
> +
WorkingCapitalLoanProductConstants.cannotEnableBreachGraceDaysWithoutBreach);
> ```
>
> > The write service defaults these to `0` / `DISBURSEMENT` when they're
not provided (`WorkingCapitalLoanProductWritePlatformServiceImpl`,
`WorkingCapitalLoanProductUpdateUtil`), and they come back that way on GET. So
sending the default back now fails. Either reject only non-default values
(grace days > 0, or a start type different from what's stored), or stop
persisting the defaults when there's no breach/bucket. Right now the server
stores a state it would reject on the next write. Same for delinquency start
type on the loan side.
>
> ###
`fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloanproduct/serialization/WorkingCapitalLoanProductDataValidator.java:388`
(and `resolveEffectiveDelinquencyBucketId` / `resolveEffectiveBreachId` in the
loan validator)
> ```java
> + final Long effectiveDelinquencyBucketId =
resolveEffectiveDelinquencyBucketId(element, product);
> ```
>
> > This only fires when grace/start type is in the request. Removing the
bucket or breach (`delinquencyBucketId: null`) while the product/loan still has
grace days set goes through fine, so we get the inconsistent state anyway. Can
we either reject that or clear the grace days/start type when the parent is
removed? Also existing products/loans that already carry grace days without a
bucket aren't handled at all. Are we fine with them staying like that?
>
> ###
`integration-tests/src/test/java/org/apache/fineract/integrationtests/WorkingCapitalLoanProductCRUDTest.java:684`
> ```java
> - final PutWorkingCapitalLoanProductsProductIdRequest updateRequest
= new WorkingCapitalLoanProductTestBuilder().withName(uniqueName)
> -
.withShortName(uniqueShortName).withBreachGraceDays(7).buildUpdateRequest();
> + final PutWorkingCapitalLoanProductsProductIdRequest updateRequest
= new PutWorkingCapitalLoanProductsProductIdRequest()
> + .breachGraceDays(7).locale("en");
> ```
>
> > Why change the request shape here? The test already got a breach above.
If the full payload doesn't work anymore that's the regression from the first
comment, and the test shouldn't be changed to hide it. Can we add a test that
creates a product without a breach, GETs it and sends the defaults back on
update?
>
> ### `WorkingCapitalLoanApplicationDataValidator.java:894`
(`resolveEffectiveBreachId`) vs `WorkingCapitalLoanAssemblerImpl.java:205-211`
> > On loan create the validator returns `null` whenever `breachId` is
present in the request, but the assembler falls back to `product.getBreach()`
when it's null. So `{"breachId": null, "breachGraceDays": 5}` on a product that
has a breach is rejected although the loan would end up with a breach. The
resolution has to match the assembler. Also the product validator's
`resolveEffectiveBreachId` now does the `breachId > 0` validation too, a method
called "resolve" that validates as a side effect.
>
> ### Smaller ones
> * `resolveEffectiveDelinquencyBucketId`, `resolveEffectiveBreachId` and
`rejectIfSpecifiedWithoutParent` are copied into both validators
(`WorkingCapitalLoanProductDataValidator` ~457-480,
`WorkingCapitalLoanApplicationDataValidator` ~881-911). Pls share one helper.
> * `WorkingCapitalLoanProductDataValidator.java:444` changed `!= null` to
`!= null && !isBlank()`, so a blank `breachStartType` is now silently accepted
and persisted as DISBURSEMENT, where before it was `invalid.breach.start.type`.
Out of scope and untested, revert or call it out.
> * `WorkingCapitalStepDef`
`setWorkingCapitalLoanProductsCreate/UpdateRequest` now null
`delinquencyBucketId`/`breachId` for every caller. The negative scenarios in
`WorkingCapitalLoanProduct.feature:202-203` (`delinquencyGraceDays "-1"`,
`delinquencyStartType "INVALID_TYPE"`) only still pass because the assertion is
a substring `contains`, so they don't test the original validation on its own
anymore. Null the parent only in the new scenarios.
> * The 2 new step defs in `WorkingCapitalLoanAccountStepDef` (~3970, 3996)
are near-duplicates and use Java `assert`, which is a no-op without `-ea`. Use
AssertJ.
> * None of the 6 new scenarios has a `@TestRailId`
(`WorkingCapitalLoanProduct.feature:546,554,562,572`,
`WorkingCapitalLoanAccount.feature:1969,1979`).
> * No e2e covers loan modify: modifying a loan without bucket/breach and
sending grace/start type, or clearing the parent in the same request.
> * `testCreateWorkingCapitalLoanProductWithNegativeBreachGraceDays` asserts
a breach exists that another test created, create one when missing like
`getBreachData` does.
> * The title says grace days and start type should become part of the
delinquency bucket, but this PR only adds validation and the fields stay on the
product and loan. Pls fix the title or the scope, and fill in the PR
description since
[FINERACT-2684](https://issues.apache.org/jira/browse/FINERACT-2684) is an
umbrella ticket.
Thanks for the review — addressed.
Went with stop persisting defaults without parent (null instead of
0/DISBURSEMENT), clear children when bucket/breach is removed (shared helper +
unit tests), and Liquibase 0090 for existing orphan rows. Validation rejects
any non-null grace/start type without a parent.
Also: assembler/validator resolveEffectiveBreachId alignment, shared helper,
IT full-builder + without-breach cases, e2e for create/update/modify without
parent. TestRail IDs will be added by QA in a follow-up commit on this PR..
--
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]
