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/08/20 08:58:51 UTC
[PR] FINERACT-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
mariiaKraievska opened a new pull request, #6299:
URL: https://github.com/apache/fineract/pull/6299
## 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.
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-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
mariiaKraievska commented on PR #6299:
URL: https://github.com/apache/fineract/pull/6299#issuecomment-5407844568
> The core mechanism is right - `DateUtils.getBusinessLocalDate()` already falls back to the tenant date when `enable-business-date` is off, so both config states are covered by construction, and the Liquibase backfill/constraint split is done correctly. Two things need fixing before merge though, plus a flaky assertion.
>
> **1. Breaking API change - response attribute removed**
>
> `WorkingCapitalLoanNearBreachActionData` and `WorkingCapitalLoanPeriodPaymentRateChangeData` drop `OffsetDateTime createdDate` in favor of `LocalDate submittedOnDate`. These records are the API response payloads (`GET .../nearbreachactions`, `GET .../ratechanges`), so this removes a field from the JSON contract - the repo's swagger-brake backward-compatibility check (R014 "Response attribute removed") should flag this. `createdDate` (system-clock audit stamp) and `submittedOnDate` (business date) aren't the same thing either, per your own changelog comment - can we add the new field and keep the old one rather than replacing it?
>
> **2. Liquibase NOT NULL column with no default - rolling deployment hazard**
>
> `parts/0070_...xml` and `parts/0071_...xml` add `submitted_on_date DATE` to existing tables, then `addNotNullConstraint` with no default value. N-1 code that doesn't know about the column will fail with a NOT NULL violation on insert during a rolling deploy. Either `defaultValueComputed="CURRENT_DATE"` on the added column, or keep it nullable in this release and tighten it in a follow-up.
>
> **3. Flaky e2e assertion (`WorkingCapitalNearBreachActionStepDef` / `WorkingCapitalLoanAccountStepDef`)**
>
> ```java
> assertThat(latest.getSubmittedOnDate()).as(...).isEqualTo(Utils.now());
> ```
>
> `Utils.now()` is `LocalDate.now(Clock.systemUTC())`, but the value under test comes from `DateUtils.getLocalDateOfTenant()` - tenant timezone, which defaults to Asia/Kolkata in this stack. Between 18:30 and 24:00 UTC the tenant is already on the next day and this fails. Can we read the tenant date instead of using `Utils.now()` for equality here?
>
> **4. Global config mutation without the repo's isolation mechanism**
>
> Both new e2e scenarios flip `enable-business-date` globally without using the existing `cucumber.execution.exclusive-resources` pattern already used for exactly this kind of shared state. Parallelism is off today so it's masked, but it'll silently corrupt other business-date-dependent scenarios the moment it's turned on.
>
> Smaller: the trailing "Then Global configuration is enabled" step duplicates the existing `@BusinessDateDisabledCheck` after-hook and doesn't run on mid-scenario failure - probably fine to drop in favor of the hook. And the ticket title says "all features supported in WCP" but this only covers rate change + near-breach action - is that the full scope or a first slice?
>
> Recommendation: CHANGES_REQUESTED
@galovics Thank you for your review
1. I’d keep the removal, since createdDate here was audit/system time, not the booking date. For WC history we want submittedOnDate (business/tenant date), same as transactions/charges. Audit stays on created_on_utc; re-exposing both on the response just recreates the confusion this change is meant to fix.
2. Agreed. Dropped NOT NULL from this PR for rolling safety. Follow-up PR https://github.com/apache/fineract/pull/6327 re-backfills any remaining NULLs and adds the constraint (plus entity nullable = false).
--
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-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6299:
URL: https://github.com/apache/fineract/pull/6299#issuecomment-5410996463
> > The core mechanism is right - `DateUtils.getBusinessLocalDate()` already falls back to the tenant date when `enable-business-date` is off, so both config states are covered by construction, and the Liquibase backfill/constraint split is done correctly. Two things need fixing before merge though, plus a flaky assertion.
> > **1. Breaking API change - response attribute removed**
> > `WorkingCapitalLoanNearBreachActionData` and `WorkingCapitalLoanPeriodPaymentRateChangeData` drop `OffsetDateTime createdDate` in favor of `LocalDate submittedOnDate`. These records are the API response payloads (`GET .../nearbreachactions`, `GET .../ratechanges`), so this removes a field from the JSON contract - the repo's swagger-brake backward-compatibility check (R014 "Response attribute removed") should flag this. `createdDate` (system-clock audit stamp) and `submittedOnDate` (business date) aren't the same thing either, per your own changelog comment - can we add the new field and keep the old one rather than replacing it?
> > **2. Liquibase NOT NULL column with no default - rolling deployment hazard**
> > `parts/0070_...xml` and `parts/0071_...xml` add `submitted_on_date DATE` to existing tables, then `addNotNullConstraint` with no default value. N-1 code that doesn't know about the column will fail with a NOT NULL violation on insert during a rolling deploy. Either `defaultValueComputed="CURRENT_DATE"` on the added column, or keep it nullable in this release and tighten it in a follow-up.
> > **3. Flaky e2e assertion (`WorkingCapitalNearBreachActionStepDef` / `WorkingCapitalLoanAccountStepDef`)**
> > ```java
> > assertThat(latest.getSubmittedOnDate()).as(...).isEqualTo(Utils.now());
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > `Utils.now()` is `LocalDate.now(Clock.systemUTC())`, but the value under test comes from `DateUtils.getLocalDateOfTenant()` - tenant timezone, which defaults to Asia/Kolkata in this stack. Between 18:30 and 24:00 UTC the tenant is already on the next day and this fails. Can we read the tenant date instead of using `Utils.now()` for equality here?
> > **4. Global config mutation without the repo's isolation mechanism**
> > Both new e2e scenarios flip `enable-business-date` globally without using the existing `cucumber.execution.exclusive-resources` pattern already used for exactly this kind of shared state. Parallelism is off today so it's masked, but it'll silently corrupt other business-date-dependent scenarios the moment it's turned on.
> > Smaller: the trailing "Then Global configuration is enabled" step duplicates the existing `@BusinessDateDisabledCheck` after-hook and doesn't run on mid-scenario failure - probably fine to drop in favor of the hook. And the ticket title says "all features supported in WCP" but this only covers rate change + near-breach action - is that the full scope or a first slice?
> > Recommendation: CHANGES_REQUESTED
>
> @galovics Thank you for your review
>
> 1. I’d keep the removal, since createdDate here was audit/system time, not the booking date. For WC history we want submittedOnDate (business/tenant date), same as transactions/charges. Audit stays on created_on_utc; re-exposing both on the response just recreates the confusion this change is meant to fix.
> 2. Agreed. Dropped NOT NULL from this PR for rolling safety. Follow-up PR [FINERACT-2455: enforce NOT NULL on WC submitted_on_date columns #6327](https://github.com/apache/fineract/pull/6327) re-backfills any remaining NULLs and adds the constraint (plus entity nullable = false).
I am fine with dropping the audit field from the response:
- 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...
--
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-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "ruzeynalov (via GitHub)" <gi...@apache.org>.
ruzeynalov commented on PR #6299:
URL: https://github.com/apache/fineract/pull/6299#issuecomment-5424784579
Thanks — pushed `74a2c616cf` (amended e2e commit) with both non-blocking points addressed:
- **WC-specific helper**: the tenant-date probe/assert moved out of `BusinessDateHelper` into a new `WorkingCapitalTenantDateHelper`; `BusinessDateHelper` is generic again and the two WC stepdefs inject the new helper.
- **`isNotEqualTo(storedBusinessDate)`**: kept, but reframed as an explicit precondition (`assertThat(storedBusinessDate).isNotIn(before, after)` with a self-explaining message). Rationale: the probe (charge-off template) and the stamp both resolve through `DateUtils.getBusinessLocalDate()`, so a regression that returned the stored row while the config is off would move both together and `isIn(before, after)` alone would still pass — the inequality is the only assertion that proves the fallback path actually ran. The stub date is 01 Jan 2026, already in the past, so the calendar collision cannot occur going forward; if it ever did, the failure now states exactly why.
Both isolated scenarios are green locally (2/2). The `createdDate` restoration for the API-compat check is not part of this push and follows separately.
--
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-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "ruzeynalov (via GitHub)" <gi...@apache.org>.
ruzeynalov commented on PR #6299:
URL: https://github.com/apache/fineract/pull/6299#issuecomment-5409035895
> > The core mechanism is right - `DateUtils.getBusinessLocalDate()` already falls back to the tenant date when `enable-business-date` is off, so both config states are covered by construction, and the Liquibase backfill/constraint split is done correctly. Two things need fixing before merge though, plus a flaky assertion.
> > **1. Breaking API change - response attribute removed**
> > `WorkingCapitalLoanNearBreachActionData` and `WorkingCapitalLoanPeriodPaymentRateChangeData` drop `OffsetDateTime createdDate` in favor of `LocalDate submittedOnDate`. These records are the API response payloads (`GET .../nearbreachactions`, `GET .../ratechanges`), so this removes a field from the JSON contract - the repo's swagger-brake backward-compatibility check (R014 "Response attribute removed") should flag this. `createdDate` (system-clock audit stamp) and `submittedOnDate` (business date) aren't the same thing either, per your own changelog comment - can we add the new field and keep the old one rather than replacing it?
> > **2. Liquibase NOT NULL column with no default - rolling deployment hazard**
> > `parts/0070_...xml` and `parts/0071_...xml` add `submitted_on_date DATE` to existing tables, then `addNotNullConstraint` with no default value. N-1 code that doesn't know about the column will fail with a NOT NULL violation on insert during a rolling deploy. Either `defaultValueComputed="CURRENT_DATE"` on the added column, or keep it nullable in this release and tighten it in a follow-up.
> > **3. Flaky e2e assertion (`WorkingCapitalNearBreachActionStepDef` / `WorkingCapitalLoanAccountStepDef`)**
> > ```java
> > assertThat(latest.getSubmittedOnDate()).as(...).isEqualTo(Utils.now());
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > `Utils.now()` is `LocalDate.now(Clock.systemUTC())`, but the value under test comes from `DateUtils.getLocalDateOfTenant()` - tenant timezone, which defaults to Asia/Kolkata in this stack. Between 18:30 and 24:00 UTC the tenant is already on the next day and this fails. Can we read the tenant date instead of using `Utils.now()` for equality here?
> > **4. Global config mutation without the repo's isolation mechanism**
> > Both new e2e scenarios flip `enable-business-date` globally without using the existing `cucumber.execution.exclusive-resources` pattern already used for exactly this kind of shared state. Parallelism is off today so it's masked, but it'll silently corrupt other business-date-dependent scenarios the moment it's turned on.
> > Smaller: the trailing "Then Global configuration is enabled" step duplicates the existing `@BusinessDateDisabledCheck` after-hook and doesn't run on mid-scenario failure - probably fine to drop in favor of the hook. And the ticket title says "all features supported in WCP" but this only covers rate change + near-breach action - is that the full scope or a first slice?
> > Recommendation: CHANGES_REQUESTED
>
> @galovics Thank you for your review
>
> 1. I’d keep the removal, since createdDate here was audit/system time, not the booking date. For WC history we want submittedOnDate (business/tenant date), same as transactions/charges. Audit stays on created_on_utc; re-exposing both on the response just recreates the confusion this change is meant to fix.
> 2. Agreed. Dropped NOT NULL from this PR for rolling safety. Follow-up PR [FINERACT-2455: enforce NOT NULL on WC submitted_on_date columns #6327](https://github.com/apache/fineract/pull/6327) re-backfills any remaining NULLs and adds the constraint (plus entity nullable = false).
@galovics Thank you for your review
#3 and #4 addressed both
--
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-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6299:
URL: https://github.com/apache/fineract/pull/6299
--
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-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6299:
URL: https://github.com/apache/fineract/pull/6299#issuecomment-5408226977
> > The core mechanism is right - `DateUtils.getBusinessLocalDate()` already falls back to the tenant date when `enable-business-date` is off, so both config states are covered by construction, and the Liquibase backfill/constraint split is done correctly. Two things need fixing before merge though, plus a flaky assertion.
> > **1. Breaking API change - response attribute removed**
> > `WorkingCapitalLoanNearBreachActionData` and `WorkingCapitalLoanPeriodPaymentRateChangeData` drop `OffsetDateTime createdDate` in favor of `LocalDate submittedOnDate`. These records are the API response payloads (`GET .../nearbreachactions`, `GET .../ratechanges`), so this removes a field from the JSON contract - the repo's swagger-brake backward-compatibility check (R014 "Response attribute removed") should flag this. `createdDate` (system-clock audit stamp) and `submittedOnDate` (business date) aren't the same thing either, per your own changelog comment - can we add the new field and keep the old one rather than replacing it?
> > **2. Liquibase NOT NULL column with no default - rolling deployment hazard**
> > `parts/0070_...xml` and `parts/0071_...xml` add `submitted_on_date DATE` to existing tables, then `addNotNullConstraint` with no default value. N-1 code that doesn't know about the column will fail with a NOT NULL violation on insert during a rolling deploy. Either `defaultValueComputed="CURRENT_DATE"` on the added column, or keep it nullable in this release and tighten it in a follow-up.
> > **3. Flaky e2e assertion (`WorkingCapitalNearBreachActionStepDef` / `WorkingCapitalLoanAccountStepDef`)**
> > ```java
> > assertThat(latest.getSubmittedOnDate()).as(...).isEqualTo(Utils.now());
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > `Utils.now()` is `LocalDate.now(Clock.systemUTC())`, but the value under test comes from `DateUtils.getLocalDateOfTenant()` - tenant timezone, which defaults to Asia/Kolkata in this stack. Between 18:30 and 24:00 UTC the tenant is already on the next day and this fails. Can we read the tenant date instead of using `Utils.now()` for equality here?
> > **4. Global config mutation without the repo's isolation mechanism**
> > Both new e2e scenarios flip `enable-business-date` globally without using the existing `cucumber.execution.exclusive-resources` pattern already used for exactly this kind of shared state. Parallelism is off today so it's masked, but it'll silently corrupt other business-date-dependent scenarios the moment it's turned on.
> > Smaller: the trailing "Then Global configuration is enabled" step duplicates the existing `@BusinessDateDisabledCheck` after-hook and doesn't run on mid-scenario failure - probably fine to drop in favor of the hook. And the ticket title says "all features supported in WCP" but this only covers rate change + near-breach action - is that the full scope or a first slice?
> > Recommendation: CHANGES_REQUESTED
>
> @galovics Thank you for your review
>
> 1. I’d keep the removal, since createdDate here was audit/system time, not the booking date. For WC history we want submittedOnDate (business/tenant date), same as transactions/charges. Audit stays on created_on_utc; re-exposing both on the response just recreates the confusion this change is meant to fix.
> 2. Agreed. Dropped NOT NULL from this PR for rolling safety. Follow-up PR [FINERACT-2455: enforce NOT NULL on WC submitted_on_date columns #6327](https://github.com/apache/fineract/pull/6327) re-backfills any remaining NULLs and adds the constraint (plus entity nullable = false).
@mariiaKraievska Do we need a new PR to backfill?
--
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-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration [fineract]
Posted by "mariiaKraievska (via GitHub)" <gi...@apache.org>.
mariiaKraievska commented on PR #6299:
URL: https://github.com/apache/fineract/pull/6299#issuecomment-5408337556
> Do we need a new PR to backfill?
No, backfill of existing rows is already in this PR. The follow-up is for NOT NULL, the extra WHERE IS NULL update there is just a safety net before adding the constraint, not a separate backfill deliverable.
--
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