FINERACT-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration - #6299
Conversation
…ted in WCP should follow business date or system date based on configuration
…features supported in WCP should follow business date or system date based on configuration
galovics
left a comment
There was a problem hiding this comment.
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)
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
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!
Your assigned reviewer(s) will follow our guidelines for code reviews.