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