alberto-art3ch commented on PR #5951:
URL: https://github.com/apache/fineract/pull/5951#issuecomment-6537014053
> Hi @alberto-art3ch, thanks for the PR and for sticking with it through all
the rounds with Adam. Moving it onto the shared `fineract-cob` bases was the
right call, and the locking side looks good to me now: same owners, the orphan
cleanup uses the `lastClosedBusinessDate` guard, and the writer deletes the
lock in the chunk transaction.
>
> There's one thing I won't let in as it is. The interest posting step
breaks when the pivot date config is in play (details inline). Plus the branch
is ~280 commits behind develop now and the Liquibase part numbers clash with
develop. The rest is smaller stuff.
>
> ###
fineract-savings/src/main/java/org/apache/fineract/cob/savings/PostInterestForSavingsBusinessStep.java:41
> ```java
> + final boolean backdatedTxnsAllowedTill =
configurationDomainService.retrievePivotDateConfig();
> + savingsAccountWritePlatformService.postInterest(savingsAccount,
false, DateUtils.getBusinessLocalDate(), backdatedTxnsAllowedTill);
> ```
>
> I think this goes wrong when
`allow-backdated-transaction-before-interest-posting` is disabled, i.e. when
`retrievePivotDateConfig()` returns true. With the flag on,
`SavingsAccount.postInterest` and `updateSummaryWithPivotConfig` work on the
transient `savingsAccountTransactions` list and
`summary.runningBalanceOnPivotDate`. Only
`SavingsAccountAssembler.loadTransactionsToSavingsAccount(account, true)` fills
those. The COB readers (`SavingsItemReader` / `InlineCOBSavingsItemReader`)
only call `setHelpers`, so the posting runs on an empty transaction list with a
zero opening balance, and then the summary gets recomputed from that. On a
tenant with that config this means wrong interest and a broken summary, and
nothing errors out.
>
> The default config doesn't hit it, which is probably why the ITs are
green. Either load the account through
`loadTransactionsToSavingsAccount(account, pivotConfig)` in `afterFind`, or
pass `false` here like the dormancy/escheat services do internally. Whichever
you pick, can we add a test with that config disabled please? Thoughts?
>
> ###
fineract-savings/src/main/resources/db/changelog/tenant/module/savings/parts/module-changelog-master.xml:31
> ```
> + <include file="parts/2006_add_savings_account_locks.xml"
relativeToChangelogFile="true" />
> + <include file="parts/2007_add_savings_cob_job_data.xml"
relativeToChangelogFile="true" />
> + <include file="parts/2008_add_savings_cob_business_steps.xml"
relativeToChangelogFile="true" />
> ```
>
> develop already has
`parts/2006_add_is_rate_chart_overridden_to_deposit_term.xml` in this module,
so these have to move to 2007-2009 when you rebase. Please also check the other
open savings PRs so we don't collide again.
>
> ###
fineract-provider/src/main/java/org/apache/fineract/cob/savings/InlineSavingsCOBBuildExecutionContextTasklet.java:52
> ```java
> +public class InlineSavingsCOBBuildExecutionContextTasklet implements
Tasklet {
> ```
>
> Why the copy? `InlineLoanCOBBuildExecutionContextTasklet<T, B>` already
takes the business step class and the COB job name as constructor params, and
the rest is identical. `new InlineLoanCOBBuildExecutionContextTasklet<>(...,
SavingsCOBBusinessStep.class, SavingsCOBConstant.SAVINGS_COB_JOB_NAME)` in
`SavingsInlineCOBConfig` should do it, and we can drop the 100 lines. If the
`Loan` in the name bothers you, rename it like you did with
`AbstractItemListener`.
>
> ###
fineract-savings/src/main/resources/db/changelog/tenant/module/savings/parts/parts/2008_add_savings_cob_business_steps.xml:53
> ```
> + <column name="step_name"
value="ADD_ACCRUAL_TRANSACTIONS_FOR_SAVINGS"/>
> + <column name="step_order" value="6"/>
> ```
>
> Question about the default order: why is accrual last, after interest
posting and after the dormancy step (which can escheat and close the account)?
On a period-end day I'd expect the accruals to be booked up to the period end
before posting them. Maybe I'm overthinking this and the accrual service copes
with either order, but then I'd love to see that covered by a test that runs
both steps on a posting date. What do you think?
@galovics thanks for the review, all four addressed and pushed on top of
the rebase onto develop.
**1. Pivot date in `PostInterestForSavingsBusinessStep`** — confirmed. The
step now always posts with the full history (`backdatedTxnsAllowedTill =
false`), same as the dormancy, escheat and accrual services do internally. The
COB reader never fills the pivot-date transaction list, so that mode was never
valid here. Added
`inlineSavingsCOBPostsSameInterestWhenBackdatedTransactionsBeforePostingAreDisallowed`:
two identical accounts, one posted with the default config and one with
`allow-backdated-transaction-before-interest-posting` disabled, must produce
the same posting, balance and journal entries.
**2. Liquibase numbering** — parts renumbered to 2007–2009 during the
rebase, doc updated. I checked the open savings/deposit/interest PRs: none of
them touches the `fineract-savings` changelog today.
**3. `InlineSavingsCOBBuildExecutionContextTasklet`** — dropped,
`SavingsInlineCOBConfig` now uses the generic
`InlineLoanCOBBuildExecutionContextTasklet<SavingsAccount,
SavingsCOBBusinessStep>`. The only real difference in the copy was the
exception, so the generic one now throws `CustomJobParameterNotFoundException`
instead of `LoanNotFoundException` when the custom job parameter is missing.
**4. Accrual order** — you are right. Order 6 mirrored the legacy crons
(posting 00:00, accrual 00:01), but with accrual accounting the posting debits
interest payable, so it should run after the accrual that funds it.
`ADD_ACCRUAL_TRANSACTIONS_FOR_SAVINGS` is now step 1 and posting step 2. Added
`inlineSavingsCOBAccruesInterestBeforePostingItOnPostingDate`: one COB run on
the posting date with an accrual-based product must leave 31 January accruals,
one posting whose interest payable debit equals the posted interest, accruals
covering it, an unchanged balance apart from the posting, and no duplicates on
a second run.
--
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]
