mariiaKraievska commented on PR #6299:
URL: https://github.com/apache/fineract/pull/6299#issuecomment-5424947608
> * It was a mistake to be introduced
> * No real business logic depends on it
> * Working capital functionality was not finished completely by 1.15, and
it was highlighted, changes still might happen, even breaking...in some cases...
> Nice work on the follow-up. Three of the four points from last round are
properly addressed:
>
> * **Liquibase**: nullable column + separate backfill changeset now,
rolling-deploy safe.
> * **The flaky e2e assertion**: `Utils.now()` is gone, replaced with a
server-round-trip capture-before/after pattern - genuinely fixes the
tenant-timezone issue rather than papering over it.
> * **Test isolation**: proper `exclusive-resources` entry plus an `@After`
restore hook for `enable-business-date`, matching the existing pattern in this
file.
>
> One thing is still open though, and CI is now red on exactly it:
`createdDate` is still being _replaced_ by `submittedOnDate` on both
`WorkingCapitalLoanNearBreachActionData` and
`WorkingCapitalLoanPeriodPaymentRateChangeData` instead of the new field being
added alongside it. `run-api-backward-compatibility / api-compatibility-check`
is failing with:
>
> ```
> R014 createdDate was removed from response default in GET
/v1/working-capital-loans/{loanId}/rate-changes
> R014 createdDate was removed from response default in GET
/v1/working-capital-loans/external-id/{loanExternalId}/rate-changes
> R014 createdDate was removed from response default in GET
/v1/working-capital-loans/{loanId}/near-breach-actions
> R014 createdDate was removed from response default in GET
/v1/working-capital-loans/external-id/{loanExternalId}/near-breach-actions
> ```
>
> Could you restore `createdDate` on both records (and the mapper mapping /
constructor arg that were removed alongside it) and keep `submittedOnDate` as
an addition rather than a replacement? If the intent is to phase `createdDate`
out eventually, `swaggerBrake { deprecatedApiDeletionAllowed = true }` is
already configured in `fineract-provider/build.gradle`, so marking it
`@Schema(deprecated = true)` first and removing it in a later PR would work too.
>
> Two small non-blocking things while I was back in there:
>
> * The new `BusinessDateHelper` methods
(`captureCurrentTenantDateBeforeWorkingCapitalAction`,
`getEffectiveWorkingCapitalDateFromServer`) hard-depend on the WC charge-off
template endpoint even though `BusinessDateHelper` is otherwise a generic
helper - might be cleaner in a WC-specific helper.
> * `assertStampedOnCurrentTenantDate` also asserts
`.isNotEqualTo(storedBusinessDate)` against the hardcoded `01 January 2026`
stub date - harmless today, but it's a latent false failure if this suite is
ever run on that calendar date. The `.isIn(before, after)` check already proves
the point on its own.
>
> Recommendation: CHANGES_REQUESTED (down from the prior review - just the
API compat piece left)
Regarding createdDate, as it was mentioned earlier:
" - It was a mistake to be introduced
- No real business logic depends on it
- WC functionality was not finished completely, changes still might
happen"
--
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]
