alberto-art3ch commented on PR #6508: URL: https://github.com/apache/fineract/pull/6508#issuecomment-6277110916
> Hi @alberto-art3ch, thanks for the PR. The tests for the successive-business-dates and receivable-product cases are what I'd want, but the code and the description do not match. > > * The description says the lookup "derives the transaction type from the sign of the interest earned". The diff adds `!MathUtil.isZero(getGlAccountIdForInterestReceivable())`, which gates on the receivable GL mapping. Which one is it? `MathUtil.isZero(Long)` is false for null, so this only works because the read service turns null into 0 - is that guaranteed on every path that builds `SavingsAccountData`? An explicit mapped/unmapped flag (or an extracted `hasInterestReceivableMapping()`) would be less fragile. > * `hasInterestToPost` / the zero skip changes behaviour for every savings product: periods that earn zero no longer post a zero-amount transaction. That affects 0% products, zero-balance days, reports that count postings and `recalucateDailyBalanceDetails`. Bugfix? If it is needed, say why in the description and add a test for a plain 0% product, otherwise split it out. > * Why touch the legacy `SavingsAccountPostInterestServiceImpl` at all? The bug is not fixed there and `addPostingTransaction` / `addTransaction` are single-use helpers. Keep this PR to the lookup fix. `Boolean isOverdraft` should be a primitive. > * `testBackdatedWithdrawalReplacesTheInterestPostingWithOverdraftInterest` - its javadoc admits the withdrawal does the correction, not the job, so it passes without the fix. Please remove it. The two near-identical overdraft product builders and 5 count helpers could be one parametrised builder and two helpers. Thanks @galovics, all four points addressed in 5e09f47. Gate vs. description. The description was stale from an earlier iteration, rewritten now. The typed lookup is gated on the interest receivable mapping on purpose: that is the same condition calculateInterestUsing already uses to split each interval into a positive-balance period and an overdraft period sharing one posting date. Only there does the lookup need to tell the two types apart. Every other product gets a single period per date, and the lookup matches either type. On null: you're right that MathUtil.isZero(Long) is false for null. It only worked because the interest posting row mapper reads the column with rs.getLong (null → 0), which is the only path that populates it. I extracted SavingsAccountData.hasInterestReceivableMapping(), which treats null and 0 alike, and used it for both the split and the lookup so they can't drift. Zero skip. Now scoped to receivable-mapped products only. There, one of the two periods sharing a posting date regularly earns zero. A zero amount is created as a regular interest posting whatever the period type, so the typed lookup of a zero-earning overdraft period never matches it and would post it again on every run (covered by testZeroOverdraftInterestPostsNothingAndLeavesTheInterestPostingAlone). Every other product, 0% ones included, keeps posting zero amounts exactly as before, and the factory is back to >= 0. The reason is in the description. Legacy service. Reverted entirely. Its lookup is type-agnostic, so the bug doesn't apply there. isOverdraft is a primitive now. Tests. Removed testBackdatedWithdrawalReplacesTheInterestPostingWithOverdraftInterest and its builder. The two monthly overdraft builders share one parametrised request, and the count helpers are down to one stream helper plus one counter, driven by type predicates. -- 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]
