You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "mariiaKraievska (via GitHub)" <gi...@apache.org> on 2026/10/15 09:22:29 UTC
[PR] FINERACT-2684: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
mariiaKraievska opened a new pull request, #6662:
URL: https://github.com/apache/fineract/pull/6662
## 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](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
- [ ] 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](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
- [ ] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/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"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
- [ ] 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](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2684: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6662:
URL: https://github.com/apache/fineract/pull/6662#issuecomment-6238869821
@mariiaKraievska Please rebase onto the latest `develop` and resolve the conflicts.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
mariiaKraievska commented on PR #6662:
URL: https://github.com/apache/fineract/pull/6662#issuecomment-6362258650
> Thanks @mariiaKraievska, both blockers from last round are fixed. The breach name in `WorkingCapitalLoanApplicationValidationTest` is random now and IT shard 6 is green on PG, MySQL and MariaDB. The `clear*` logic moved onto `WorkingCapitalLoanProductRelatedDetail(s)` with its own unit test, and the TestRail IDs are in. The current red CI is infra (`spotlessGroovyGradle` fails on `Failed to provision P2 dependencies`, which takes down spotless, the WAR smoke test and all E2E shards), so the E2E changes in [cef7889](https://github.com/apache/fineract/commit/cef78893dfd599d3b026fe89b34b3d96a787b856) haven't run yet. Pls re-run and let's merge only after E2E is green.
>
> A few things on the new push:
>
> * On loan create an explicit `breachId` / `nearBreachId` / `delinquencyStartType` / `breachStartType: null` now clears the value instead of inheriting it from the product. That's an API behavior change, is it intended? There's no IT for it.
> * [cef7889](https://github.com/apache/fineract/commit/cef78893dfd599d3b026fe89b34b3d96a787b856) removes an existing step from C115359 (`WorkingCapitalBreachEvaluation.feature`, around line 448) without explanation. The path should still return `override.not.allowed`, can you confirm why it's gone? That commit is also labelled [FINERACT-2684](https://issues.apache.org/jira/browse/FINERACT-2684) instead of [FINERACT-2968](https://issues.apache.org/jira/browse/FINERACT-2968).
> * Still no IT/e2e for clearing on parent removal (near-breach included), and the breach defaults differ between create (`0` / `DISBURSEMENT`) and update (null).
> * #6542 also adds `parts/0090_*` to the WC module changelog. Different changeset ids so Liquibase is fine, but whichever PR lands second should rename to 0091.
Thanks for the review.
> * Still no IT/e2e for clearing on parent removal (near-breach included), and the breach defaults differ between create (`0` / `DISBURSEMENT`) and update (null).
Covered with a Feign IT (FeignWorkingCapitalLoanParentClearTest): inherit breach / near-breach / delinquency from the product, then clear them with explicit nulls on modify and assert grace/start types are cleared too.
Re-create defaults (0 / DISBURSEMENT) vs update (null): existing product-create behaviour when the parent is set and dependents are omitted. Сlearing on update nulls them instead.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6662:
URL: https://github.com/apache/fineract/pull/6662
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
Posted by "MarianaDmytrivBinariks (via GitHub)" <gi...@apache.org>.
MarianaDmytrivBinariks commented on PR #6662:
URL: https://github.com/apache/fineract/pull/6662#issuecomment-6362626816
> * [cef7889](https://github.com/apache/fineract/commit/cef78893dfd599d3b026fe89b34b3d96a787b856) removes an existing step from C115359 (`WorkingCapitalBreachEvaluation.feature`, around line 448) without explanation. The path should still return `override.not.allowed`, can you confirm why it's gone?
@galovics this step is related to other changes, I've noticed that it's located in a wrong scenarios and removed it, so now those tests are good, we have diff test that covers this deleted step, I left it previously in this test scenarios by mistake, sorry for confusing you.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
mariiaKraievska commented on PR #6662:
URL: https://github.com/apache/fineract/pull/6662#issuecomment-6312911127
> ## Review summary
> Thanks @mariiaKraievska, this is much better. Persisting `null` without a parent fixes the GET -> PUT break, removing the bucket/breach now clears the dependent fields, 0090 cleans up the existing rows, the breach resolution matches the assembler, and the ITs use the full payload again. All the bigger points from last round are covered.
>
> Two things left, and the first one blocks. CI is red because of this PR: IT shard 6 fails on all 3 DBs (see below). And the new clear-on-parent-removal logic lives in a class called `...Validator` while it changes entity state.
>
> TestRail IDs in a follow-up commit on this PR is fine, they just need to be in before merge.
>
> ## Inline comments
> ### `integration-tests/src/test/java/org/apache/fineract/integrationtests/WorkingCapitalLoanApplicationValidationTest.java:1387-1388`
> ```java
> + final Long breachId = breachHelper
> + .create(breachHelper.createBreachRequest("Override WCL Breach", 30, "DAYS", "PERCENTAGE", BigDecimal.valueOf(10)));
> ```
>
> > The name is hardcoded, so every call after the first one hits a duplicate-name 403. That's why `testSubmitWithInvalidBreachId` and `testSubmitWithDelinquencyGraceDaysNotAllowedWhileBreachOverrideIsAllowed` fail in shard 6 on PG, MySQL and MariaDB. Use a random name like you did in `WorkingCapitalLoanProductValidationTest` (`Utils.randomStringGenerator("Breach", 20)`).
>
> ### `fineract-working-capital-loan/src/main/java/org/apache/fineract/portfolio/workingcapitalloanproduct/serialization/WorkingCapitalDependentSettingsValidator.java:84-130`
> ```java
> + public static void clearDelinquencyDependentSettings(final WorkingCapitalLoanProductRelatedDetail relatedDetail,
> + final Map<String, Object> changes) {
> + if (relatedDetail.getDelinquencyGraceDays() != null) {
> + relatedDetail.setDelinquencyGraceDays(null);
> ```
>
> > A validator shouldn't touch a domain object's state, especially a JPA entity. And here the write service and the assembler call into a validator class from the `serialization` package just to null out entity fields. I'd move the 4 `clear*` methods to the embeddables themselves (e.g. `clearDelinquencySettings()` / `clearBreachSettings()` on `WorkingCapitalLoanProductRelatedDetail(s)`) and keep this class for the validation only. Wdyt?
Thanks — both addressed.
1.duplicate breach name — shared breach fixture now uses Utils.randomStringGenerator("Breach", 20) (same pattern as WorkingCapitalLoanProductValidationTest).
2. clearDelinquencySettings / clearBreachSettings moved onto WorkingCapitalLoanProductRelatedDetail(s); WorkingCapitalDependentSettingsValidator is validation-only again.
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2968: WC - Delinquency/breach grace days and start type should be part of delinquency bucket/breach [fineract]
Posted by "MarianaDmytrivBinariks (via GitHub)" <gi...@apache.org>.
MarianaDmytrivBinariks commented on PR #6662:
URL: https://github.com/apache/fineract/pull/6662#issuecomment-6343688815
> Thanks @mariiaKraievska, both blockers from last round are fixed. The breach name in `WorkingCapitalLoanApplicationValidationTest` is random now and IT shard 6 is green on PG, MySQL and MariaDB. The `clear*` logic moved onto `WorkingCapitalLoanProductRelatedDetail(s)` with its own unit test, and the TestRail IDs are in. The current red CI is infra (`spotlessGroovyGradle` fails on `Failed to provision P2 dependencies`, which takes down spotless, the WAR smoke test and all E2E shards), so the E2E changes in [cef7889](https://github.com/apache/fineract/commit/cef78893dfd599d3b026fe89b34b3d96a787b856) haven't run yet. Pls re-run and let's merge only after E2E is green.
>
> A few things on the new push:
>
> * On loan create an explicit `breachId` / `nearBreachId` / `delinquencyStartType` / `breachStartType: null` now clears the value instead of inheriting it from the product. That's an API behavior change, is it intended? There's no IT for it.
> * [cef7889](https://github.com/apache/fineract/commit/cef78893dfd599d3b026fe89b34b3d96a787b856) removes an existing step from C115359 (`WorkingCapitalBreachEvaluation.feature`, around line 448) without explanation. The path should still return `override.not.allowed`, can you confirm why it's gone? That commit is also labelled [FINERACT-2684](https://issues.apache.org/jira/browse/FINERACT-2684) instead of [FINERACT-2968](https://issues.apache.org/jira/browse/FINERACT-2968).
> * Still no IT/e2e for clearing on parent removal (near-breach included), and the breach defaults differ between create (`0` / `DISBURSEMENT`) and update (null).
> * #6542 also adds `parts/0090_*` to the WC module changelog. Different changeset ids so Liquibase is fine, but whichever PR lands second should rename to 0091.
thanks for review @galovics , I would like to clarify reason for this changes:
> On loan create an explicit breachId / nearBreachId / delinquencyStartType / breachStartType: null now clears the value instead of inheriting it from the product. That's an API behavior change, is it intended? There's no IT for it.
this fixes were made to make it possible to have some defined breach/delinquency values on WCLP level, then inherit them to WC loan account level, and just in case user doesn't want to have breachId / nearBreachId / delinquencyStartType / breachStartType at all - to allow clear those values while create/modify loan account IF overrides are allowed, so user is free to inherit OR to have no values per breach/delinquency, that means to override them actually, like value -> no value(empty);
in case override is disallowed - in case use tries to override those values, then corresponding error message is shown on BE(on UI - those fields aren't editable and greyed out with inherited values from WCLP level), could you please let me know is that sounds reasonable to you?
please review, I will appreciate your answer, thanks in advance!
--
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org