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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
