From 6aa655a8a34334f18ea36f56125d4198de680b8d Mon Sep 17 00:00:00 2001 From: Nitesh Yadav Date: Thu, 10 Sep 2026 12:36:38 +0530 Subject: [PATCH] fix(cash): correct branch-day reconciliation, voucher classification and duplicate posting bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BranchDayRegisterApiResource: - Make openDay/closeDay transactional and add an atomic closed_at guard so a double-close or a mid-write failure cannot leave the register half-updated. - Fix bookBreakdown()'s slip/closure withdrawal queries: m_clerk_withdrawal_request and m_clerk_closure_request store cashier_id as an appuser id in some registers and an m_cashiers id in others; try both via a dual EXISTS instead of assuming one, which was silently dropping real withdrawals from expected cash and producing a false "Short" on close. - Office-scope vault-in/settled-out and close-preview queries so a multi-office tenant's close no longer mixes another branch's cash into this one's. - closeDay requires closingCash and uses DateUtils.getBusinessLocalDate() for late-close detection instead of the server's own LocalDate.now(). - saveDenominationDetail() now deletes any prior snapshot for the same register/leg/teller before inserting, so a repeated save no longer sums duplicate denomination rows into the vault position. - resolveOffice() enforces office-hierarchy authorization via PlatformSecurityContext.validateAccessRights(). CashReconciliationApiResource / CashReportApiResource / DenominationBreakdownWriter: - Add a MAX_PLAUSIBLE_TXN_ID filter (consistent with the existing cash report guard) so a fabricated/oversized transaction id can no longer distort a cashier's expected or counted position. - Tolerate sub-rupee (paisa) rounding differences instead of requiring an exact amount match — Indian cash has no paisa coins, so exact-equality was rejecting correctly-counted cash. - CashReconciliationApiResource now trusts its own server-computed expected count rather than the client-submitted one when recording a reconciliation. VaultPositionService: - Only fall back to the branch-total OPENING leg when the office has exactly one teller; a multi-teller branch was getting one teller's vault entry applied to all of them. CashSettlementApiResource / CashInterCashierApiResource / CashExchangeApiResource: - Make settlement creation transactional; add a partial unique index (0540) blocking two active settlements for the same cashier/day, closing a double-submit race the application-level count check alone could not. - Validate a cash transfer against the sending cashier's actual denomination holdings and require both cashiers to be in the same office. - Default cash exchange date to the business date instead of null, and add the missing CREATE/REVERSE permission checks (0539 seeds them). CashierDepositRequestApiResource: - Namespace pending-deposit denomination rows with a +500,000,000 offset so they cannot collide with real transaction ids during approve/reject. ClerkDisbursementRequestApiResource (Disbursement/Collection/Fund Transfer/Bank Transfer register): - executeSavingsWithdrawal() was missing the withOwnVoucher() suppression its sibling methods already had, so every Collection-mode approval raised a second, orphaned voucher for the same money on top of the request's own — double-counting the figure in Received and Payments on My Vouchers. - Voucher numbering no longer hard-codes CR/DR by money direction. It now checks whether the payment type the clerk actually selected is cash (m_payment_type.is_cash_payment) and numbers into the Journal series when it is not, since a Transfer/Cheque/DD instrument never crosses the counter regardless of which GL it posts against. Bank Transfer vouchers (always account-to-account, never cash) are now unconditionally Journal too. - The voucher's TYPE column stopped being hard-coded to "DISBURSEMENT" for every request this register raises; it now stores the clerk's own request type (Disbursement/Collection/Fund Transfer/Bank Transfer) so My Vouchers shows what was actually submitted. VoucherVerificationApiResource (/my-work): - Classify a voucher's side (Received/Payment/Journal) by whether either of its lines actually touches the till (Cash in Hand or a cash-at-vault/teller GL), the same test the register listing already uses, instead of trusting the voucher number's own CR/DR prefix at face value. A Collection or Disbursement posted against a plain GL (no cash involved) was being counted as real cash received/paid; it now reads as Journal, matching what actually happened at the counter. Existing CR/DR requests with no lines yet are left alone, since there is nothing to test the till against. --- .../api/BranchDayRegisterApiResource.java | 186 +++++++++++++++--- .../api/CashExchangeApiResource.java | 19 +- .../api/CashierDepositRequestApiResource.java | 30 ++- .../api/CashInterCashierApiResource.java | 87 ++++++++ .../api/CashReconciliationApiResource.java | 46 ++++- .../cashreport/api/CashReportApiResource.java | 10 +- .../service/VaultPositionService.java | 28 ++- .../api/CashSettlementApiResource.java | 9 + .../ClerkDisbursementRequestApiResource.java | 53 ++++- .../service/DenominationBreakdownWriter.java | 33 +++- .../api/VoucherVerificationApiResource.java | 33 +++- .../tenant/final-changelog-tenant.xml | 2 + .../0539_seed_cash_exchange_permissions.xml | 62 ++++++ ...active_cash_settlement_per_cashier_day.xml | 53 +++++ 14 files changed, 591 insertions(+), 60 deletions(-) create mode 100644 fineract-provider/src/main/resources/db/changelog/tenant/parts/0539_seed_cash_exchange_permissions.xml create mode 100644 fineract-provider/src/main/resources/db/changelog/tenant/parts/0540_unique_active_cash_settlement_per_cashier_day.xml diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/branchday/api/BranchDayRegisterApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/branchday/api/BranchDayRegisterApiResource.java index cbe7fdd6f..1006863bf 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/branchday/api/BranchDayRegisterApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/branchday/api/BranchDayRegisterApiResource.java @@ -45,6 +45,7 @@ import org.apache.fineract.portfolio.cashreport.service.VaultPositionService; import org.apache.fineract.useradministration.domain.AppUser; import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.stereotype.Component; +import org.springframework.transaction.annotation.Transactional; /** * Branch day register — the formal opening and closing of a branch's business day. @@ -70,6 +71,13 @@ public class BranchDayRegisterApiResource { private final VaultPositionService vaultPositionService; private final Gson gson = new Gson(); + // Mirrors CashReportApiResource.MAX_PLAUSIBLE_TXN_ID: a genuine m_savings_account_transaction id + // is always small. Some cashier-GL deposit legs stamp a synthetic, implausibly large txn_id (a + // millisecond timestamp, decoded back out of hex) that never links to a real transaction and is + // never corrected -- without this same filter here, the close-preview's counted-in/counted-out + // figures would include that phantom amount, on top of the real cash the branch actually counted. + private static final long MAX_PLAUSIBLE_TXN_ID = 1_000_000_000_000L; + // ── GET /v1/branchday ── the register itself ────────────────────────────── /** @@ -183,14 +191,33 @@ public class BranchDayRegisterApiResource { // What the cashier's own denomination entries add up to for the day. Where this and the // expected figure agree, the day tallies and the closing figure is not in question. + // Scoped to this office through the officer who took the cash (appuser -> staff -> cashier -> + // teller -> office). Unscoped, a branch's own "does the physical count agree with the books" + // cross-check was being compared against every branch's counted cash at once, which makes it + // wrong at every office but the first. A row whose officer maps to no cashier is still + // counted, for the same reason as the withdrawal legs in bookBreakdown. final BigDecimal countedIn = zero(jdbcTemplate.queryForObject(""" - SELECT COALESCE(SUM(total_amount), 0) FROM m_savings_txn_denomination - WHERE txn_date = ? AND txn_type = 'DEPOSIT' - """, BigDecimal.class, Date.valueOf(businessDate))); + SELECT COALESCE(SUM(std.total_amount), 0) FROM m_savings_txn_denomination std + WHERE std.txn_date = ? AND std.txn_type = 'DEPOSIT' + AND (std.txn_id IS NULL OR std.txn_id < ?) + AND (std.officer_id IS NULL + OR NOT EXISTS (SELECT 1 FROM m_appuser au JOIN m_cashiers c ON c.staff_id = au.staff_id + WHERE au.id = std.officer_id) + OR EXISTS (SELECT 1 FROM m_appuser au JOIN m_cashiers c ON c.staff_id = au.staff_id + JOIN m_tellers t ON t.id = c.teller_id + WHERE au.id = std.officer_id AND t.office_id = ?)) + """, BigDecimal.class, Date.valueOf(businessDate), MAX_PLAUSIBLE_TXN_ID, officeId)); final BigDecimal countedOut = zero(jdbcTemplate.queryForObject(""" - SELECT COALESCE(SUM(total_amount), 0) FROM m_savings_txn_denomination - WHERE txn_date = ? AND txn_type = 'WITHDRAWAL' - """, BigDecimal.class, Date.valueOf(businessDate))); + SELECT COALESCE(SUM(std.total_amount), 0) FROM m_savings_txn_denomination std + WHERE std.txn_date = ? AND std.txn_type = 'WITHDRAWAL' + AND (std.txn_id IS NULL OR std.txn_id < ?) + AND (std.officer_id IS NULL + OR NOT EXISTS (SELECT 1 FROM m_appuser au JOIN m_cashiers c ON c.staff_id = au.staff_id + WHERE au.id = std.officer_id) + OR EXISTS (SELECT 1 FROM m_appuser au JOIN m_cashiers c ON c.staff_id = au.staff_id + JOIN m_tellers t ON t.id = c.teller_id + WHERE au.id = std.officer_id AND t.office_id = ?)) + """, BigDecimal.class, Date.valueOf(businessDate), MAX_PLAUSIBLE_TXN_ID, officeId)); result.put("denominationIn", countedIn); result.put("denominationOut", countedOut); @@ -199,9 +226,15 @@ public class BranchDayRegisterApiResource { // ── POST /v1/branchday/open ─────────────────────────────────────────────── + // Transactional because opening a day is three separate writes -- the register row, its + // branch-total OPENING breakdown, and each teller's carried-forward OPENING_VAULT -- and a + // failure part-way through used to commit whatever had already run. A register left open with no + // OPENING_VAULT is not a cosmetic gap: the next close's carry-forward reaches past it to an older + // register instead, and the vault's opening balance silently regresses to a stale figure. @POST @Path("open") @Consumes(MediaType.APPLICATION_JSON) + @Transactional public String openDay(final String body) { final AppUser currentUser = this.context.authenticatedUser(); requireDayAuthority(currentUser); @@ -461,9 +494,15 @@ public class BranchDayRegisterApiResource { // ── POST /v1/branchday/close ────────────────────────────────────────────── + // Transactional for the same reason as openDay: the register UPDATE, the branch-total CLOSING + // breakdown and each teller's CLOSING_VAULT snapshot are separate writes. This database still + // carries the evidence of them committing independently -- a register whose closed_at stayed + // NULL while an orphaned CLOSING_VAULT snapshot survived, which later got summed alongside a + // second snapshot and doubled a vault's carried-forward opening balance. @POST @Path("close") @Consumes(MediaType.APPLICATION_JSON) + @Transactional public String closeDay(final String body) { final AppUser currentUser = this.context.authenticatedUser(); requireDayAuthority(currentUser); @@ -481,6 +520,16 @@ public class BranchDayRegisterApiResource { // counted note-by-note wins over a typed-in total. final BigDecimal closingCash = denominationTotal != null ? denominationTotal : this.fromJsonHelper.extractBigDecimalWithLocaleNamed("closingCash", root); + // A close needs a figure. Without one the row commits with closing_cash and cash_difference + // NULL, and every carry-forward lookup keys on "closing_cash IS NOT NULL" -- so the next open + // skips this register entirely and resurrects the day-before's note-for-note breakdown as + // tomorrow's opening cash, with no difference to explain and nothing on screen to suggest the + // chain broke. The UI always sends one; this endpoint is directly reachable and did not check. + if (closingCash == null) { + throw new PlatformDataIntegrityException("error.msg.branchday.closing.cash.required", + "Record what was counted before closing business day " + businessDate + + " -- either a closing figure or a denomination breakdown."); + } final String remarks = this.fromJsonHelper.extractStringNamed("remarks", root); final String forceReason = this.fromJsonHelper.extractStringNamed("forceCloseReason", root); @@ -577,7 +626,14 @@ public class BranchDayRegisterApiResource { // Closing a past date is allowed — otherwise a day left open could never be settled at all — // but it must say why it was left open, and it is stamped as a force close. - final LocalDate today = LocalDate.now(DateUtils.getDateTimeZoneOfTenant()); + // + // Judged against the tenant's BUSINESS date, not the wall clock: a branch that finishes + // counting at 00:20 is still closing its own current day, and stamping that as a forced late + // close (with a mandatory "why was this left open" reason) both annoys the closer and + // destroys the one distinction this register exists for -- closed that evening versus closed + // nine days later. The same method already uses getBusinessLocalDate() for its own + // arithmetic; using two different notions of "today" in one close was the inconsistency. + final LocalDate today = DateUtils.getBusinessLocalDate(); final boolean isLate = businessDate.isBefore(today); if (isLate && (forceReason == null || forceReason.isBlank())) { throw new PlatformDataIntegrityException("error.msg.branchday.force.reason.required", @@ -587,11 +643,18 @@ public class BranchDayRegisterApiResource { final BigDecimal bookCash = (BigDecimal) bookBreakdown(officeId, businessDate).get("expectedCash"); final BigDecimal difference = closingCash != null ? closingCash.subtract(bookCash) : null; - jdbcTemplate.update(""" + // "closed_at IS NULL" in the WHERE, not just the read above: the already-closed check and + // this write are two separate statements, so two managers pressing Close at once (or one + // double-click) both got past the check. The second UPDATE then overwrote the first's + // closing figure, closer and difference with no trace, and the two denomination writes + // interleaved into exactly the duplicate CLOSING rows saveDenominationDetail's + // delete-then-insert exists to prevent. Letting the database arbitrate makes the loser of + // that race fail cleanly instead. + final int closed = jdbcTemplate.update(""" UPDATE m_branch_day_register SET closed_by = ?, closed_by_name = ?, closed_at = ?, closing_cash = ?, book_cash = ?, cash_difference = ?, force_closed = ?, force_close_reason = ?, remarks = ? - WHERE office_id = ? AND business_date = ? + WHERE office_id = ? AND business_date = ? AND closed_at IS NULL """, // Forced either way: a late close, or a close over a drawer nobody counted. The reason is // stored whenever it was needed, not only when the date was in the past — a same-day @@ -601,6 +664,10 @@ public class BranchDayRegisterApiResource { bookCash, difference, isLate || forcedOverCashiers, (isLate || forcedOverCashiers) ? overrideReason : null, remarks, officeId, Date.valueOf(businessDate)); + if (closed == 0) { + throw new PlatformDataIntegrityException("error.msg.branchday.already.closed", + "Business day " + businessDate + " is already closed."); + } final Long registerId = ((Number) found.get(0).get("id")).longValue(); saveDenominationDetail(registerId, "CLOSING", denominations); @@ -725,12 +792,30 @@ public class BranchDayRegisterApiResource { * {@code tellerId} is null for the branch-total OPENING/CLOSING legs (vault + every cashier * added together) and set for the vault-only OPENING_VAULT/CLOSING_VAULT legs — see * {@link VaultPositionService} for why the two are never allowed to mix. + * + * Replaces whatever this exact (register, leg, teller) already held rather than adding to it. + * Without that, a second call for the same snapshot -- a retry, a re-submitted close after an + * earlier attempt stalled partway, {@link #snapshotVaultDenominations} running twice for the + * same close -- leaves the old rows sitting alongside the new ones. Nothing downstream + * deduplicates by denomination before summing, so a vault's carried-forward opening balance (or + * a branch's own opening/closing figure) silently doubles instead of being replaced, exactly the + * kind of self-inflicted "the vault has more cash than the whole branch's book figure" gap this + * table exists to prevent, not cause. */ private void saveDenominationDetail(final Long registerId, final String leg, final Long tellerId, final List> denominations) { if (denominations == null || denominations.isEmpty()) { return; } + // The bare "? IS NULL" comparison (no column on either side to infer a type from) makes + // PostgreSQL's JDBC driver fail the whole statement with "could not determine data type of + // parameter" -- unlike "teller_id = ?", which gets its type from the teller_id column even + // when the bound value is null. Casting to BIGINT gives the standalone parameter a type of + // its own, independent of context. + jdbcTemplate.update(""" + DELETE FROM m_branch_day_register_detail + WHERE register_id = ? AND leg = ? AND (teller_id = ? OR (teller_id IS NULL AND CAST(? AS BIGINT) IS NULL)) + """, registerId, leg, tellerId, tellerId); for (final Map row : denominations) { final int value = (int) row.get("denominationValue"); final int count = (int) row.get("count"); @@ -939,11 +1024,32 @@ public class BranchDayRegisterApiResource { // disbursements use APPROVED; withdrawal is the one register that doesn't) -- filtering on // APPROVED alone silently excluded every completed withdrawal from this total, overstating // the branch's expected cash by exactly what had actually been paid out that day. + // Scoped to this office through the cashier who paid it out. Unscoped, this summed EVERY + // branch's withdrawals into whichever branch happened to be closing: with 6-8 offices on + // these tenants, Head Office's expected cash was being reduced by cash another branch had + // paid over its own counter. + // + // Two id conventions have to be tolerated, because cashier_id here is NOT an m_cashiers id: + // these registers store the APPUSER id of the cashier who submitted (cashier_id = 4 = + // nitesh.cashier, while that user's m_cashiers row is id 1). Resolving it only as an + // m_cashiers id matched nothing and silently excluded every withdrawal from expected cash -- + // reporting a phantom shortage of exactly what had been paid out. Accepting either reading + // keeps this correct whichever convention a given register follows. + // + // A row whose cashier is not set at all is still counted, matching the "OR office_id IS NULL" + // leniency the deposits query above applies: no cashier means no branch to attribute it to, + // and dropping it silently would understate the total instead. final BigDecimal slipWithdrawals = zero(jdbcTemplate.queryForObject(""" - SELECT COALESCE(SUM(amount), 0) FROM m_clerk_withdrawal_request - WHERE status = 'WITHDRAWN' - AND COALESCE(txn_date, CAST(submitted_at AS DATE)) = ? - """, BigDecimal.class, date)); + SELECT COALESCE(SUM(w.amount), 0) FROM m_clerk_withdrawal_request w + WHERE w.status = 'WITHDRAWN' + AND COALESCE(w.txn_date, CAST(w.submitted_at AS DATE)) = ? + AND (w.cashier_id IS NULL + OR EXISTS (SELECT 1 FROM m_appuser au JOIN m_cashiers c ON c.staff_id = au.staff_id + JOIN m_tellers t ON t.id = c.teller_id + WHERE au.id = w.cashier_id AND t.office_id = ?) + OR EXISTS (SELECT 1 FROM m_cashiers c JOIN m_tellers t ON t.id = c.teller_id + WHERE c.id = w.cashier_id AND t.office_id = ?)) + """, BigDecimal.class, date, officeId, officeId)); // Account closures pay cash over the same counter but live in their own register, so they // were missing from this total entirely -- the branch's expected cash stayed high by exactly @@ -955,24 +1061,36 @@ public class BranchDayRegisterApiResource { // since a closure takes nothing out of the drawer until the cashier hands it over (a // nil-balance closure never reaches a cashier at all, hence the closure_date fallback). final BigDecimal closureWithdrawals = zero(jdbcTemplate.queryForObject(""" - SELECT COALESCE(SUM(cashier_disbursed_amount), 0) FROM m_clerk_closure_request - WHERE status = 'CLOSED' - AND COALESCE(CAST(disbursed_at AS DATE), closure_date) = ? - """, BigDecimal.class, date)); + SELECT COALESCE(SUM(cr.cashier_disbursed_amount), 0) FROM m_clerk_closure_request cr + WHERE cr.status = 'CLOSED' + AND COALESCE(CAST(cr.disbursed_at AS DATE), cr.closure_date) = ? + AND (cr.cashier_id IS NULL + OR EXISTS (SELECT 1 FROM m_appuser au JOIN m_cashiers c ON c.staff_id = au.staff_id + JOIN m_tellers t ON t.id = c.teller_id + WHERE au.id = cr.cashier_id AND t.office_id = ?) + OR EXISTS (SELECT 1 FROM m_cashiers c JOIN m_tellers t ON t.id = c.teller_id + WHERE c.id = cr.cashier_id AND t.office_id = ?)) + """, BigDecimal.class, date, officeId, officeId)); final BigDecimal withdrawals = slipWithdrawals.add(closureWithdrawals); - // Cash drawn from the vault to a cashier — more physical cash sitting in the branch. + // Cash drawn from the vault to a cashier — more physical cash sitting in the branch. Scoped + // to this office by the vault it came out of, for the same reason as the withdrawals above. final BigDecimal vaultIn = zero(jdbcTemplate.queryForObject(""" - SELECT COALESCE(SUM(transfer_amount), 0) FROM m_cash_transfer - WHERE transfer_date = ? AND COALESCE(is_reversed, false) = false - """, BigDecimal.class, date)); + SELECT COALESCE(SUM(ct.transfer_amount), 0) FROM m_cash_transfer ct + WHERE ct.transfer_date = ? AND COALESCE(ct.is_reversed, false) = false + AND (ct.from_teller_id IS NULL + OR EXISTS (SELECT 1 FROM m_tellers t WHERE t.id = ct.from_teller_id AND t.office_id = ?)) + """, BigDecimal.class, date, officeId)); - // Cash a cashier settled back to the vault at the end of the day. + // Cash a cashier settled back to the vault at the end of the day, scoped to the vault it + // went back into. final BigDecimal settledOut = zero(jdbcTemplate.queryForObject(""" - SELECT COALESCE(SUM(settlement_amount), 0) FROM m_cash_settlement - WHERE settlement_date = ? AND COALESCE(is_reversed, false) = false - """, BigDecimal.class, date)); + SELECT COALESCE(SUM(cs.settlement_amount), 0) FROM m_cash_settlement cs + WHERE cs.settlement_date = ? AND COALESCE(cs.is_reversed, false) = false + AND (cs.to_teller_id IS NULL + OR EXISTS (SELECT 1 FROM m_tellers t WHERE t.id = cs.to_teller_id AND t.office_id = ?)) + """, BigDecimal.class, date, officeId)); final BigDecimal expected = opening.add(deposits).subtract(withdrawals); @@ -1025,8 +1143,26 @@ public class BranchDayRegisterApiResource { cashAccountId, cashAccountId); } + /** + * A caller-supplied officeId is checked against the caller's own office hierarchy, not taken on + * trust. Without that check, any user whose role name happens to contain "clerk"/"manager"/"head + * cashier" (see {@link #canOperateDay}) could open or close ANOTHER branch's business day, and + * read its register, close preview and cash-GL balance, just by naming its id in the request -- + * the authority check confirmed they may operate A day, never WHOSE day. + * {@code validateAccessRights} is the same office-hierarchy gate the client and group write + * services already use, so a head-office user retains access to their branches' days and a + * branch user is confined to their own office and anything below it. + */ private Long resolveOffice(final Long requested, final AppUser user) { if (requested != null) { + final String hierarchy = jdbcTemplate + .query("SELECT hierarchy FROM m_office WHERE id = ?", (rs, n) -> rs.getString(1), requested).stream() + .findFirst().orElse(null); + if (hierarchy == null) { + throw new PlatformDataIntegrityException("error.msg.branchday.office.not.found", + "Office " + requested + " was not found."); + } + this.context.validateAccessRights(hierarchy); return requested; } return user.getOffice() != null ? user.getOffice().getId() : 1L; diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashexchange/api/CashExchangeApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashexchange/api/CashExchangeApiResource.java index 3ec138689..5741531de 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashexchange/api/CashExchangeApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashexchange/api/CashExchangeApiResource.java @@ -53,15 +53,28 @@ public class CashExchangeApiResource { @POST @Transactional public String create(String body) { + // Gated like every sibling register in this flow (CREATE_CASHTRANSFER, + // CREATE_CASHSETTLEMENT, CREATE_CASHINTERCASHIER). Cash exchange had no check at all, so any + // authenticated user -- not just a cashier -- could write legs that move a drawer's + // denomination position. Permission seeded and granted to the existing cash-handling roles by + // migration 0539. + this.context.authenticatedUser().validateHasPermissionTo("CREATE_CASHEXCHANGE"); + JsonObject json = JsonParser.parseString(body).getAsJsonObject(); final Long accountId = json.has("accountId") && !json.get("accountId").isJsonNull() ? json.get("accountId").getAsLong() : null; final String accountNo = json.has("accountNo") && !json.get("accountNo").isJsonNull() ? json.get("accountNo").getAsString() : null; final Long clientId = json.has("clientId") && !json.get("clientId").isJsonNull() ? json.get("clientId").getAsLong() : null; + // Defaulted, never left null: every as-of-date position query filters + // "ce.exchange_date <= CAST(? AS DATE)", which is false for NULL, so a null-dated exchange + // vanished from the cashier position and reconciliation reports while still counting in the + // undated stock query (DenominationBreakdownWriter.getCashierNetPosition). The drawer then + // read as holding the notes it had given away and none of the ones it had taken in. The + // sibling registers (cash transfer, settlement) already default the same way. final LocalDate exchangeDate = json.has("exchangeDate") && !json.get("exchangeDate").isJsonNull() ? LocalDate.parse(json.get("exchangeDate").getAsString()) - : null; + : DateUtils.getBusinessLocalDate(); final String note = json.has("note") && !json.get("note").isJsonNull() ? json.get("note").getAsString() : null; final String customerIdProof = json.has("customerIdProof") && !json.get("customerIdProof").isJsonNull() ? json.get("customerIdProof").getAsString() : null; @@ -153,6 +166,10 @@ public class CashExchangeApiResource { @POST @Path("{id}/reverse") public String reverse(@PathParam("id") Long id, String body) { + // Reversing moves the same cash back, so it is gated the same way REVERSE_CASHTRANSFER and + // REVERSE_CASHSETTLEMENT already are. See create() above and migration 0539. + this.context.authenticatedUser().validateHasPermissionTo("REVERSE_CASHEXCHANGE"); + CashExchange exchange = repository.findById(id) .orElseThrow(() -> new RuntimeException("Cash exchange not found: " + id)); if (Boolean.TRUE.equals(exchange.getIsReversed())) { diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java index 6dba8fb89..290d81092 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashierdeposit/api/CashierDepositRequestApiResource.java @@ -77,6 +77,19 @@ public class CashierDepositRequestApiResource { // activity JsonArray, and a field of the same name reads as that variable at a glance. private static final Logger LOG = LoggerFactory.getLogger(CashierDepositRequestApiResource.class); + // The temporary txn_id a pending deposit's denomination rows are tagged with (see + // savePendingDenomination) is this request's own id -- but m_savings_txn_denomination.txn_id has + // no reference-type discriminator, and a real m_savings_account_transaction id is drawn from the + // exact same small-integer range. Request #5 and transaction #5 coexisting is the ordinary case, + // not a rare one, so approve()'s "WHERE txn_id = " UPDATE (and reject()'s + // DELETE) would silently rewrite or erase an unrelated real transaction's own denomination + // breakdown whenever the two ids happened to match. Offsetting the temporary id into a range no + // real transaction id will reach in this system's lifetime makes every such match unambiguous, + // without a schema change. Kept well under CashReportApiResource.MAX_PLAUSIBLE_TXN_ID (10^12) so + // a request still sitting PENDING continues to count in cashier-position reports -- the cash is + // physically in the drawer already, approval or not. + private static final long PENDING_DEPOSIT_TXN_ID_OFFSET = 500_000_000L; + private final JdbcTemplate jdbcTemplate; private final FromJsonHelper fromJsonHelper; private final PlatformSecurityContext context; @@ -452,13 +465,15 @@ public class CashierDepositRequestApiResource { } // Denomination was already saved at submit time (cashier physically received cash then). - // Now update the temporary txn_id (request id) to the real Fineract transaction id. - // parseTxnIdToLong handles both decimal (savings/current) and hex (GL journal entry). + // Now update the temporary txn_id (offset request id -- see PENDING_DEPOSIT_TXN_ID_OFFSET) to + // the real Fineract transaction id. parseTxnIdToLong handles both decimal (savings/current) + // and hex (GL journal entry). if (txnId != null) { final Long realTxnId = parseTxnIdToLong(txnId); if (realTxnId != null) { jdbcTemplate.update( - "UPDATE m_savings_txn_denomination SET txn_id = ? WHERE txn_id = ?", realTxnId, id); + "UPDATE m_savings_txn_denomination SET txn_id = ? WHERE txn_id = ?", realTxnId, + id + PENDING_DEPOSIT_TXN_ID_OFFSET); } } @@ -510,9 +525,10 @@ public class CashierDepositRequestApiResource { throw apiError(409, "This deposit request was already actioned by another manager."); } - // Reverse denomination — cashier physically returned cash to customer on rejection. - // The temporary denomination records (saved at submit time with txn_id = request id) are removed. - jdbcTemplate.update("DELETE FROM m_savings_txn_denomination WHERE txn_id = ?", id); + // Reverse denomination — cashier physically returned cash to customer on rejection. The + // temporary denomination records (saved at submit time with txn_id = offset request id -- see + // PENDING_DEPOSIT_TXN_ID_OFFSET) are removed. + jdbcTemplate.update("DELETE FROM m_savings_txn_denomination WHERE txn_id = ?", id + PENDING_DEPOSIT_TXN_ID_OFFSET); return gson.toJson(Map.of("resourceId", id, "status", "REJECTED")); } @@ -875,7 +891,7 @@ public class CashierDepositRequestApiResource { denomination_value, count, total_amount, txn_date, txn_type, officer_id, officer_username, payment_type) VALUES (?, ?, ?, ?, ?, ?, ?, ?, CURRENT_DATE, 'DEPOSIT', ?, ?, ?) """, - requestId, savingsAccountId, accountNo, clientId, denomId, + requestId + PENDING_DEPOSIT_TXN_ID_OFFSET, savingsAccountId, accountNo, clientId, denomId, value, count, BigDecimal.valueOf((long) value * count), cashierId, cashierUsername, paymentTypeName ); diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashintercashier/api/CashInterCashierApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashintercashier/api/CashInterCashierApiResource.java index 2f72e2ad2..5fccf713a 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashintercashier/api/CashInterCashierApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashintercashier/api/CashInterCashierApiResource.java @@ -23,12 +23,14 @@ import java.util.stream.Collectors; import lombok.RequiredArgsConstructor; import org.apache.fineract.infrastructure.core.exception.PlatformDataIntegrityException; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; +import org.apache.fineract.portfolio.cashreport.api.CashReportApiResource; import org.apache.fineract.portfolio.cashreport.service.CashierRoleHelper; import org.apache.fineract.portfolio.cashintercashier.domain.CashInterCashier; import org.apache.fineract.portfolio.cashintercashier.domain.CashInterCashierDetail; import org.apache.fineract.portfolio.cashintercashier.domain.CashInterCashierDetailRepository; import org.apache.fineract.portfolio.cashintercashier.domain.CashInterCashierRepository; import org.apache.fineract.useradministration.domain.AppUser; +import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.stereotype.Component; import org.springframework.transaction.annotation.Transactional; import org.apache.fineract.infrastructure.core.service.DateUtils; @@ -44,6 +46,10 @@ public class CashInterCashierApiResource { private final CashInterCashierDetailRepository detailRepository; private final PlatformSecurityContext context; private final CashierRoleHelper cashierRoleHelper; + private final JdbcTemplate jdbcTemplate; + // The same figure the Cashier Position screen shows, so a cashier cannot hand on notes the + // report says they are not holding -- CashSettlementApiResource depends on it the same way. + private final CashReportApiResource cashReportApiResource; private final Gson gson = new Gson(); @POST @@ -85,6 +91,21 @@ public class CashInterCashierApiResource { throw new PlatformDataIntegrityException("error.msg.cashintercashier.empty", "At least one denomination with a positive count is required."); } + + // The sending cashier has to actually hold these notes. Cash transfer validates the vault's + // stock and settlement validates the cashier's; this register validated neither, so a cashier + // holding five ₹500 notes could hand 200 of them to a colleague. Both sides are then wrong + // for ever, on every future date: the receiver's position, reconciliation and settlement + // allowance all rise by ₹1,00,000 of notes that do not exist, and the sender's reads + // -195x₹500 with nothing to net it back. + validateCashierHoldsDenominations(fromCashierId, denominations, transferDate); + + // Both cashiers must sit under the same office. Nothing stopped a cross-branch inter-cashier + // transfer, which moves physical cash between two branches with no vault leg on either side + // -- so one branch's day-close is short and the other's is over, with no record of a transfer + // between them anywhere in either branch's register. + validateSameOffice(fromCashierId, toCashierId); + final BigDecimal total = sumTotal(denominations); CashInterCashier header = new CashInterCashier(); @@ -206,6 +227,72 @@ public class CashInterCashierApiResource { return false; } + /** + * Same check, same source of truth and same message shape as + * {@code CashSettlementApiResource.validateCashierHoldsDenominations} -- the Cashier Position + * figure, so a cashier cannot hand over notes that report says they are not holding. An empty + * position is left alone rather than refused, matching the settlement path: a cashier with no + * recorded position yet is a setup state, not an overdraft. + */ + private void validateCashierHoldsDenominations(final Long fromCashierId, final JsonArray denominations, + final LocalDate transferDate) { + + final Map position = cashReportApiResource.buildPositionForCashier(fromCashierId, + transferDate.toString()); + + @SuppressWarnings("unchecked") + final List> heldRows = (List>) position.get("denominations"); + if (heldRows == null || heldRows.isEmpty()) { + return; + } + + final Map held = new java.util.HashMap<>(); + for (final Map row : heldRows) { + final Object id = row.get("denominationId"); + final Object count = row.get("count"); + if (id instanceof Number && count instanceof Number) { + held.merge(((Number) id).longValue(), ((Number) count).longValue(), Long::sum); + } + } + + for (final JsonElement el : denominations) { + final JsonObject item = el.getAsJsonObject(); + final long count = item.has("count") && !item.get("count").isJsonNull() ? item.get("count").getAsLong() : 0; + if (count <= 0 || !item.has("denominationId") || item.get("denominationId").isJsonNull()) { + continue; + } + final long denominationId = item.get("denominationId").getAsLong(); + final int denominationValue = item.has("denominationValue") && !item.get("denominationValue").isJsonNull() + ? item.get("denominationValue").getAsInt() : 0; + final long available = held.getOrDefault(denominationId, 0L); + if (count > available) { + throw new PlatformDataIntegrityException("error.msg.cashintercashier.insufficient.cashier.denomination", + "Cashier is not holding that much of denomination " + denominationValue + ": has " + available + + ", transferring " + count + ". Check the Cashier Position report first."); + } + } + } + + /** Both cashiers must belong to the same office, via their own teller. */ + private void validateSameOffice(final Long fromCashierId, final Long toCashierId) { + final List> offices = jdbcTemplate.queryForList(""" + SELECT c.id AS cashier_id, t.office_id FROM m_cashiers c JOIN m_tellers t ON t.id = c.teller_id + WHERE c.id IN (?, ?) + """, fromCashierId, toCashierId); + if (offices.size() < 2) { + // One of them has no teller/office resolvable -- leave it to the existing party checks + // rather than inventing a new failure here. + return; + } + final Object first = offices.get(0).get("office_id"); + final Object second = offices.get(1).get("office_id"); + if (first != null && second != null && !first.equals(second)) { + throw new PlatformDataIntegrityException("error.msg.cashintercashier.cross.office", + "Both cashiers must be in the same branch. Cash moving between branches has to go through" + + " each branch's own vault, not a direct cashier-to-cashier transfer."); + } + } + private void saveDetails(Long interCashierId, JsonArray items) { for (JsonElement el : items) { JsonObject item = el.getAsJsonObject(); diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreconciliation/api/CashReconciliationApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreconciliation/api/CashReconciliationApiResource.java index cc922d638..c303214ea 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreconciliation/api/CashReconciliationApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreconciliation/api/CashReconciliationApiResource.java @@ -47,6 +47,13 @@ public class CashReconciliationApiResource { private final JdbcTemplate jdbcTemplate; private final Gson gson = new Gson(); + // Mirrors CashReportApiResource.MAX_PLAUSIBLE_TXN_ID: a genuine m_savings_account_transaction id + // is always small. Some cashier-GL deposit legs stamp a synthetic, implausibly large txn_id (a + // millisecond timestamp, decoded back out of hex) that never links to a real transaction and is + // never corrected -- without this same filter here, this endpoint (unlike the cashier-position + // report) would count that phantom figure into a cashier's expected cash. + private static final long MAX_PLAUSIBLE_TXN_ID = 1_000_000_000_000L; + /** * GET /cashreconciliation/expected?cashierId=X&asOfDate=yyyy-MM-dd * Returns denomination-wise expected balance for a cashier. @@ -89,6 +96,7 @@ public class CashReconciliationApiResource { + "LEFT JOIN m_denomination_master dm ON dm.id = std.denomination_id " + "WHERE std.txn_type = 'DEPOSIT' " + "AND std.txn_date <= CAST(? AS DATE) " + + "AND (std.txn_id IS NULL OR std.txn_id < " + MAX_PLAUSIBLE_TXN_ID + ") " + "AND std.officer_id IN (" + " SELECT au.id FROM m_appuser au " + " JOIN m_cashiers mc ON mc.staff_id = au.staff_id WHERE mc.id = ?) " @@ -118,6 +126,7 @@ public class CashReconciliationApiResource { + "LEFT JOIN m_denomination_master dm ON dm.id = std.denomination_id " + "WHERE std.txn_type = 'WITHDRAWAL' " + "AND std.txn_date <= CAST(? AS DATE) " + + "AND (std.txn_id IS NULL OR std.txn_id < " + MAX_PLAUSIBLE_TXN_ID + ") " + "AND std.officer_id IN (" + " SELECT au.id FROM m_appuser au " + " JOIN m_cashiers mc ON mc.staff_id = au.staff_id WHERE mc.id = ?) " @@ -259,6 +268,14 @@ public class CashReconciliationApiResource { final JsonArray denominations = json.has("denominations") && json.get("denominations").isJsonArray() ? json.getAsJsonArray("denominations") : new JsonArray(); + // Expected counts come from the server's own computation, never from the request body. + // Reading the client's expectedCount made the whole reconciliation self-certifying: a cashier + // Rs 40,000 short could post expectedCount == physicalCount on every row, and the record + // stored variance 0 / status OK. That row then satisfied the mandatory reconciliation + // precondition on settlement, so the shortage never surfaced anywhere in the system. The + // physical count is the cashier's to declare; what the books expect is not. + final Map serverExpected = expectedCountsByDenomination(cashierId, reconDate); + BigDecimal expectedTotal = BigDecimal.ZERO; BigDecimal physicalTotal = BigDecimal.ZERO; List details = new ArrayList<>(); @@ -266,7 +283,10 @@ public class CashReconciliationApiResource { for (JsonElement el : denominations) { JsonObject item = el.getAsJsonObject(); int denomValue = item.has("denominationValue") ? item.get("denominationValue").getAsInt() : 0; - int expectedCount = item.has("expectedCount") ? item.get("expectedCount").getAsInt() : 0; + final Long denomIdForExpected = item.has("denominationId") && !item.get("denominationId").isJsonNull() + ? item.get("denominationId").getAsLong() : null; + int expectedCount = denomIdForExpected != null + ? serverExpected.getOrDefault(denomIdForExpected, 0L).intValue() : 0; int physicalCount = item.has("physicalCount") ? item.get("physicalCount").getAsInt() : 0; int varianceCount = physicalCount - expectedCount; BigDecimal varianceAmount = BigDecimal.valueOf((long) denomValue * varianceCount); @@ -348,6 +368,30 @@ public class CashReconciliationApiResource { rs.getLong("total_count") }; + /** + * The server's own expected note/coin counts for a cashier as of a date, keyed by denomination + * id. Delegates to {@link #getExpected} rather than re-deriving the eight legs: two + * implementations of "what should this drawer hold" drift apart, and the one place that matters + * is the record a settlement is later allowed on the strength of. + */ + private Map expectedCountsByDenomination(final Long cashierId, final LocalDate asOfDate) { + final Map expected = new java.util.HashMap<>(); + final JsonObject computed = JsonParser.parseString(getExpected(cashierId, asOfDate.toString())).getAsJsonObject(); + if (!computed.has("denominations") || !computed.get("denominations").isJsonArray()) { + return expected; + } + for (final JsonElement el : computed.getAsJsonArray("denominations")) { + final JsonObject row = el.getAsJsonObject(); + if (!row.has("denominationId") || row.get("denominationId").isJsonNull()) { + continue; + } + final long count = row.has("expectedCount") && !row.get("expectedCount").isJsonNull() + ? row.get("expectedCount").getAsLong() : 0L; + expected.merge(row.get("denominationId").getAsLong(), count, Long::sum); + } + return expected; + } + private void addToBalance(Map balance, List rows, int sign) { for (Object[] row : rows) { Long denomId = (Long) row[0]; diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/api/CashReportApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/api/CashReportApiResource.java index b0094cb29..7dc86fdf6 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/api/CashReportApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/api/CashReportApiResource.java @@ -76,14 +76,20 @@ public class CashReportApiResource { // recorded against a ₹918 deposit is not ₹1,60,000 of cash in anybody's drawer. Seven // such transactions in this database were carrying ₹1.68 lakh that never existed. // + // The tolerance is sub-rupee, not exact: Indian cash has no paisa coins in circulation, + // so a paisa-exact ledger amount (e.g. 12998.56) is legitimately handed over as the + // nearest whole rupee (12999) -- a 44-paisa gap is real rounding, not a mis-keyed + // breakdown, and treating it as one silently dropped an entire genuine withdrawal (and + // everything counted after it) out of the cashier's position. + // // Compared per transaction inside a correlated subquery rather than as a join, so a // breakdown with no transaction behind it -- a Demand Draft keyed by its request id -- // is untouched by this test and still handled by the id-magnitude test above. + "AND NOT EXISTS (" + " SELECT 1 FROM m_savings_account_transaction mt " + " WHERE mt.id = std.txn_id" - + " AND mt.amount <> (SELECT SUM(sd.count * sd.denomination_value)" - + " FROM m_savings_txn_denomination sd WHERE sd.txn_id = std.txn_id)) "; + + " AND ABS(mt.amount - (SELECT SUM(sd.count * sd.denomination_value)" + + " FROM m_savings_txn_denomination sd WHERE sd.txn_id = std.txn_id)) >= 1) "; /** * Day book — every voucher raised on one date, in one list, newest first. diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/service/VaultPositionService.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/service/VaultPositionService.java index 1e7510f31..b0138d066 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/service/VaultPositionService.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashreport/service/VaultPositionService.java @@ -89,14 +89,26 @@ public class VaultPositionService { """, officeId, dateFilter, tellerId); String anchorLeg = "OPENING_VAULT"; if (anchorRows.isEmpty()) { - anchorLeg = "OPENING"; - anchorRows = jdbcTemplate.queryForList(""" - SELECT r.id AS register_id, r.business_date FROM m_branch_day_register r - WHERE r.office_id = ? AND r.business_date <= CAST(? AS DATE) - AND EXISTS (SELECT 1 FROM m_branch_day_register_detail d - WHERE d.register_id = r.id AND d.leg = 'OPENING') - ORDER BY r.business_date DESC LIMIT 1 - """, officeId, dateFilter); + // The branch-total OPENING leg mixes the vault with whatever every cashier under it + // holds, so it is only a defensible stand-in for a vault when the office has exactly one + // teller. With two, EVERY teller without a vault-scoped row of its own would anchor on + // the same office-wide breakdown, and the frontend's Branch Total -- which sums each + // teller's vault -- would report roughly twice the branch's actual cash. Worse, a close + // then snapshots that inflated figure as this teller's CLOSING_VAULT and the next open + // carries it forward, making the phantom permanent. A second teller with no history of + // its own genuinely holds nothing, and returning no breakdown says so honestly. + final Integer tellerCount = jdbcTemplate.queryForObject("SELECT COUNT(*) FROM m_tellers WHERE office_id = ?", + Integer.class, officeId); + if (tellerCount != null && tellerCount == 1) { + anchorLeg = "OPENING"; + anchorRows = jdbcTemplate.queryForList(""" + SELECT r.id AS register_id, r.business_date FROM m_branch_day_register r + WHERE r.office_id = ? AND r.business_date <= CAST(? AS DATE) + AND EXISTS (SELECT 1 FROM m_branch_day_register_detail d + WHERE d.register_id = r.id AND d.leg = 'OPENING') + ORDER BY r.business_date DESC LIMIT 1 + """, officeId, dateFilter); + } } if (anchorRows.isEmpty()) { diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashsettlement/api/CashSettlementApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashsettlement/api/CashSettlementApiResource.java index 82dbae991..847ea66a8 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashsettlement/api/CashSettlementApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/cashsettlement/api/CashSettlementApiResource.java @@ -63,7 +63,16 @@ public class CashSettlementApiResource { private final CashReportApiResource cashReportApiResource; private final Gson gson = new Gson(); + // Transactional so the reconciliation precondition and the duplicate-settlement check below are + // read in the same transaction that writes the settlement. They used to run in autocommit while + // the insert happened later inside its own TransactionTemplate, leaving a window where a + // double-clicked Submit passed both checks twice and wrote two settlements for the same + // cashier/date -- each subtracted cumulatively, so the drawer read permanently negative by the + // settlement amount. Migration 0540 adds the unique index that makes the race impossible rather + // than merely unlikely; this annotation is what stops the two halves of the check-then-act from + // being separated by another request's commit. @POST + @Transactional public String create(String body) { context.authenticatedUser().validateHasPermissionTo("CREATE_CASHSETTLEMENT"); diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java index 626f6e5c0..a9b60edf5 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/clerkdisbursement/api/ClerkDisbursementRequestApiResource.java @@ -187,12 +187,26 @@ public class ClerkDisbursementRequestApiResource { activityLog, Timestamp.from(Instant.now()), requestType ); - // A collection pulls money into the GL, so it reads as a credit; a disbursement and a - // bank transfer both move money out of the source side, so they read as a debit. - final String voucherPrefix = "COLLECTION".equals(requestType) - ? VoucherService.PREFIX_CREDIT : VoucherService.PREFIX_DEBIT; + // A collection pulls money into the GL, so it reads as a credit; a disbursement moves + // money out of the source side, so it reads as a debit — but only when the TXN TP. the + // clerk actually picked is Cash. That dropdown is the real-world signal for whether notes + // cross the counter, the same way it already governs every teller screen in this system + // (isCashPayment): a clerk who selects Transfer, Cheque, DD or NEFT/RTGS is saying no cash + // moves for this request, whatever GL it happens to post against, so it belongs in the + // Journal series instead of being counted as real cash received or paid on My Vouchers. + final boolean isCashPayment = paymentTypeId != null && Boolean.TRUE.equals(jdbcTemplate.queryForObject( + "SELECT is_cash_payment FROM m_payment_type WHERE id = ?", Boolean.class, paymentTypeId)); + final String voucherPrefix = !isCashPayment ? VoucherService.PREFIX_JOURNAL + : "COLLECTION".equals(requestType) ? VoucherService.PREFIX_CREDIT : VoucherService.PREFIX_DEBIT; + // The voucher's own TYPE column used to be hard-coded to VoucherService.TYPE_DISBURSEMENT + // for every request this register raises, so My Vouchers showed "DISBURSEMENT" even for a + // Collection or a Fund Transfer the clerk had explicitly picked a different TYPE for on the + // entry screen — the one place this register's actual four-way distinction ought to have + // been visible was throwing it away. Stored as the clerk's own requestType instead, in the + // same words the TYPE dropdown already uses. + final String voucherTypeLabel = voucherTypeLabelFor(requestType); final String voucherNo = voucherService.createVoucher( - VoucherService.TYPE_DISBURSEMENT, VoucherService.REF_CLERK_DISBURSEMENT_REQUEST, + voucherTypeLabel, VoucherService.REF_CLERK_DISBURSEMENT_REQUEST, newId, txnAmount, currentUser, voucherPrefix); jdbcTemplate.update( "UPDATE m_clerk_disbursement_request SET voucher_no = ? WHERE id = ?", voucherNo, newId); @@ -1010,13 +1024,26 @@ public class ClerkDisbursementRequestApiResource { */ /** * Bank-transfer settlement voucher, drawn from the same central register as every other - * voucher so the two numbering schemes cannot drift apart. A settlement moves money out of - * the debited accounts, so it reads as a debit: DR/2108/0001. + * voucher so the two numbering schemes cannot drift apart. + * + * Numbered Journal rather than Debit: both sides of a bank transfer are member accounts, and + * the money never touches Cash in Hand or the vault either way (see the "no CashVault entry + * created anywhere" comments where these accounts are debited/credited directly). A DR number + * would have read as real cash paid out, double-counting the figure in My Vouchers on top of + * the day's actual Payment total. */ private String generateBtVoucherNo(final long logId, final AppUser currentUser, final BigDecimal amount) { return voucherService.createVoucher( - VoucherService.TYPE_DISBURSEMENT, VoucherService.REF_BANK_TRANSFER_LOG, - logId, amount, currentUser, VoucherService.PREFIX_DEBIT); + "Bank Transfer", VoucherService.REF_BANK_TRANSFER_LOG, + logId, amount, currentUser, VoucherService.PREFIX_JOURNAL); + } + + /** The TYPE column's label, in the same words the clerk's own TYPE dropdown uses for it. */ + private static String voucherTypeLabelFor(final String requestType) { + if ("COLLECTION".equals(requestType)) return "Collection"; + if ("BANK_TRANSFER".equals(requestType)) return "Bank Transfer"; + if ("FUND_TRANSFER".equals(requestType)) return "Fund Transfer"; + return "Disbursement"; } // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -1250,7 +1277,13 @@ public class ClerkDisbursementRequestApiResource { .withJson(payload.toString()) .build(); fineractRequestContextHolder.setAttribute(SynchronousCommandProcessingService.COMMAND_SOURCE_ID, null); - final CommandProcessingResult result = commandsSourceWritePlatformService.logCommandSource(cmd); + // This disbursement/collection request already raised its own voucher at submission and + // attaches every leg to it in addDisbursementVoucherLines() after approval succeeds — without + // withOwnVoucher() the plain savings withdrawal command below raises a second, orphaned + // voucher of its own for the same money (see VoucherService.withOwnVoucher's javadoc), which + // is what was double-counting a Collection's amount in both Received and Payments on My Vouchers. + final CommandProcessingResult result = voucherService + .withOwnVoucher(() -> commandsSourceWritePlatformService.logCommandSource(cmd)); return result.getResourceId() != null ? result.getResourceId().toString() : null; } diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/denominationmaster/service/DenominationBreakdownWriter.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/denominationmaster/service/DenominationBreakdownWriter.java index bf72dd855..8e4c9b5fb 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/denominationmaster/service/DenominationBreakdownWriter.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/denominationmaster/service/DenominationBreakdownWriter.java @@ -40,6 +40,14 @@ public class DenominationBreakdownWriter { private final PlatformSecurityContext context; private final JdbcTemplate jdbcTemplate; + // Mirrors CashReportApiResource.MAX_PLAUSIBLE_TXN_ID: a genuine m_savings_account_transaction id + // is always small. Some cashier-GL deposit legs stamp a synthetic, implausibly large txn_id (a + // millisecond timestamp, decoded back out of hex) that never links to a real transaction and is + // never corrected -- without this same filter here, a cashier's computed stock (and the + // insufficient-stock check that guards a withdrawal against it) would count that phantom figure + // as real notes and coins in the drawer. + private static final long MAX_PLAUSIBLE_TXN_ID = 1_000_000_000_000L; + // ── Public API ──────────────────────────────────────────────────────────── /** @@ -126,8 +134,17 @@ public class DenominationBreakdownWriter { "Denomination breakdown validation errors exist.", errors); } - // 5. Grand total must match the transaction amount - if (expectedAmount != null && computedTotal.compareTo(expectedAmount) != 0) { + // 5. Grand total must match the transaction amount, to within a rupee. + // + // Exact equality made a whole class of legitimate transaction impossible to complete: an + // interest-bearing withdrawal or closure settles on a paisa figure (₹12,998.56), Indian cash + // has no paisa coins, so NO set of notes can ever sum to it and every breakdown the cashier + // entered was rejected. Sub-rupee is therefore accepted as the rounding it is -- the same + // tolerance CashReportApiResource's own breakdown check applies on the reporting side, so + // the two agree instead of one accepting what the other rejects. A genuinely mis-keyed + // breakdown (₹1,60,000 of notes against a ₹918 deposit) is off by far more than a rupee and + // is still refused. + if (expectedAmount != null && computedTotal.subtract(expectedAmount).abs().compareTo(BigDecimal.ONE) >= 0) { throw new PlatformDataIntegrityException( "error.msg.denomination.total.mismatch", label + " denomination breakdown total (" + computedTotal @@ -277,6 +294,7 @@ public class DenominationBreakdownWriter { + " SELECT std.denomination_id, CAST(SUM(std.count) AS BIGINT)" + " FROM m_savings_txn_denomination std" + " WHERE std.txn_type = 'DEPOSIT'" + + " AND (std.txn_id IS NULL OR std.txn_id < " + MAX_PLAUSIBLE_TXN_ID + ")" + " AND std.officer_id IN (SELECT au.id FROM m_appuser au" + " JOIN m_cashiers mc ON mc.staff_id = au.staff_id WHERE mc.id = ?)" + " GROUP BY std.denomination_id" @@ -300,6 +318,7 @@ public class DenominationBreakdownWriter { + " SELECT std.denomination_id, CAST(-SUM(std.count) AS BIGINT)" + " FROM m_savings_txn_denomination std" + " WHERE std.txn_type = 'WITHDRAWAL'" + + " AND (std.txn_id IS NULL OR std.txn_id < " + MAX_PLAUSIBLE_TXN_ID + ")" + " AND std.officer_id IN (SELECT au.id FROM m_appuser au" + " JOIN m_cashiers mc ON mc.staff_id = au.staff_id WHERE mc.id = ?)" + " GROUP BY std.denomination_id" @@ -309,7 +328,13 @@ public class DenominationBreakdownWriter { + " WHERE ce.cashier_id = ? AND ced.direction = 'GIVEN'" + " AND (ce.is_reversed IS NULL OR ce.is_reversed = false)" + " GROUP BY ced.denomination_id" - + " ) raw GROUP BY denomination_id HAVING SUM(net_count) > 0" + // No "HAVING SUM(net_count) > 0" here: dropping the denominations that net negative + // reports MORE cash than the drawer holds, because the shortfall they represent is + // silently discarded instead of subtracted. Net -20x₹500 with +30x₹100 is -₹7,000, + // not +₹3,000 -- and this figure is what validateCashierHasTotal lets a withdrawal + // through on, so the optimistic version hands out money that is not there. It also + // disagreed with getCashierNetPosition below, which has never filtered them. + + " ) raw GROUP BY denomination_id" + ") t JOIN m_denomination_master dm ON dm.id = t.denomination_id"; final BigDecimal total = jdbcTemplate.queryForObject(sql, BigDecimal.class, @@ -390,6 +415,7 @@ public class DenominationBreakdownWriter { + " SELECT std.denomination_id, CAST(SUM(std.count) AS BIGINT)" + " FROM m_savings_txn_denomination std" + " WHERE std.txn_type = 'DEPOSIT'" + + " AND (std.txn_id IS NULL OR std.txn_id < " + MAX_PLAUSIBLE_TXN_ID + ")" + " AND std.officer_id IN (SELECT au.id FROM m_appuser au" + " JOIN m_cashiers mc ON mc.staff_id = au.staff_id WHERE mc.id = ?)" + " GROUP BY std.denomination_id" @@ -413,6 +439,7 @@ public class DenominationBreakdownWriter { + " SELECT std.denomination_id, CAST(-SUM(std.count) AS BIGINT)" + " FROM m_savings_txn_denomination std" + " WHERE std.txn_type = 'WITHDRAWAL'" + + " AND (std.txn_id IS NULL OR std.txn_id < " + MAX_PLAUSIBLE_TXN_ID + ")" + " AND std.officer_id IN (SELECT au.id FROM m_appuser au" + " JOIN m_cashiers mc ON mc.staff_id = au.staff_id WHERE mc.id = ?)" + " GROUP BY std.denomination_id" diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/voucher/api/VoucherVerificationApiResource.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/voucher/api/VoucherVerificationApiResource.java index 51cb1b193..ae91fc825 100644 --- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/voucher/api/VoucherVerificationApiResource.java +++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/voucher/api/VoucherVerificationApiResource.java @@ -785,7 +785,15 @@ public class VoucherVerificationApiResource { cr.party_name AS credit_party, cr.account_no AS credit_account, (SELECT COUNT(*) FROM m_voucher_line l WHERE l.voucher_no = v.voucher_no) AS line_count, (v.created_by_username = ? OR mk.maker_id = ?) AS raised_by_me, - (ver.verified_by = ? OR ap.approver_id = ?) AS verified_by_me + (ver.verified_by = ? OR ap.approver_id = ?) AS verified_by_me, + -- Whether either side of the voucher is the till itself — same test as the + -- register listing below, and for the same reason: a Collection/Disbursement + -- request is numbered in the CR/DR series (it debits or credits a member's + -- own account), but when its other leg is a GL account rather than the vault, + -- no cash actually crossed the counter. Counting it under Received or Payment + -- read as double-counting the exact figure already shown on the Journal-free + -- day totals, when it was really a book entry between two ledgers. + till.touched AS till_touched FROM m_voucher v LEFT JOIN m_office o ON o.id = v.branch_office_id LEFT JOIN m_voucher_verification ver ON ver.voucher_no = v.voucher_no @@ -799,6 +807,14 @@ public class VoucherVerificationApiResource { SELECT party_name, account_no FROM m_voucher_line WHERE voucher_no = v.voucher_no AND entry_type = 'CR' ORDER BY line_no LIMIT 1) cr ON TRUE + LEFT JOIN LATERAL ( + SELECT TRUE AS touched FROM m_voucher_line cl + WHERE cl.voucher_no = v.voucher_no + AND (cl.gl_account_id IN ( + SELECT fa.gl_account_id FROM acc_gl_financial_activity_account fa + WHERE fa.financial_activity_type IN (101, 102)) + OR (cl.gl_account_id IS NULL AND cl.party_name = 'Cash in Hand')) + LIMIT 1) till ON TRUE WHERE v.created_at >= ? AND v.created_at < ? AND (v.created_by_username = ? OR mk.maker_id = ? OR ver.verified_by = ? OR ap.approver_id = ?) ORDER BY v.created_at DESC, v.voucher_no DESC @@ -816,11 +832,23 @@ public class VoucherVerificationApiResource { for (final Map r : rows) { final BigDecimal amount = (BigDecimal) r.get("amount"); final String voucherNo = (String) r.get("voucher_no"); + final int lineCount = ((Number) r.get("line_count")).intValue(); // The series the number was issued in is the side: a register's own prefix is the one // thing that cannot disagree with the voucher, whereas re-deriving CR/DR from the type // would have to keep a list of types in step with the ones that issue them. - final String side = voucherNo == null ? null + final String rawSide = voucherNo == null ? null : voucherNo.startsWith("CR") ? "CR" : voucherNo.startsWith("DR") ? "DR" : "JV"; + // A CR/DR voucher whose lines never touch the till (the same test bookBreakdown() uses + // below) is a book entry between two ledgers that happened to be numbered in the cash + // series — a Collection/Disbursement request posted against a GL account rather than the + // vault, e.g. a savings account debited straight into a Sundry GL account. Reading its + // prefix at face value counted that figure a second time, on top of whatever Journal + // already showed, as if real cash had crossed the counter. A voucher with no lines yet + // (raised but not actioned) is left alone — there is nothing to test the till against, + // and it is still real expected cash, not yet a book entry either way. + final boolean tillTouched = Boolean.TRUE.equals(r.get("till_touched")); + final String side = ("CR".equals(rawSide) || "DR".equals(rawSide)) && lineCount > 0 && !tillTouched + ? "JV" : rawSide; if (amount != null) { if ("CR".equals(side)) { @@ -856,7 +884,6 @@ public class VoucherVerificationApiResource { m.put("debitAccount", r.get("debit_account")); m.put("creditParty", r.get("credit_party")); m.put("creditAccount", r.get("credit_account")); - final int lineCount = ((Number) r.get("line_count")).intValue(); m.put("extraLines", Math.max(0, lineCount - 2)); m.put("myRole", raisedByMe && verifiedByMe ? "RAISED_AND_VERIFIED" : raisedByMe ? "RAISED" : "VERIFIED"); out.add(m); diff --git a/fineract-provider/src/main/resources/db/changelog/tenant/final-changelog-tenant.xml b/fineract-provider/src/main/resources/db/changelog/tenant/final-changelog-tenant.xml index 27d34f83c..6b737c847 100644 --- a/fineract-provider/src/main/resources/db/changelog/tenant/final-changelog-tenant.xml +++ b/fineract-provider/src/main/resources/db/changelog/tenant/final-changelog-tenant.xml @@ -65,4 +65,6 @@ + + diff --git a/fineract-provider/src/main/resources/db/changelog/tenant/parts/0539_seed_cash_exchange_permissions.xml b/fineract-provider/src/main/resources/db/changelog/tenant/parts/0539_seed_cash_exchange_permissions.xml new file mode 100644 index 000000000..bd5af3235 --- /dev/null +++ b/fineract-provider/src/main/resources/db/changelog/tenant/parts/0539_seed_cash_exchange_permissions.xml @@ -0,0 +1,62 @@ + + + + + + + SELECT COUNT(*) FROM m_permission WHERE code = 'CREATE_CASHEXCHANGE' + + + + + + + + + + + + + + + + + + + + + + + + + + + INSERT INTO m_role_permission (role_id, permission_id) + SELECT DISTINCT existing.role_id, newperm.id + FROM m_role_permission existing + JOIN m_permission sibling ON sibling.id = existing.permission_id + CROSS JOIN m_permission newperm + WHERE sibling.code = 'CREATE_CASHTRANSFER' + AND newperm.code IN ('CREATE_CASHEXCHANGE', 'READ_CASHEXCHANGE', 'REVERSE_CASHEXCHANGE') + AND NOT EXISTS ( + SELECT 1 FROM m_role_permission rp + WHERE rp.role_id = existing.role_id AND rp.permission_id = newperm.id + ) + + + + diff --git a/fineract-provider/src/main/resources/db/changelog/tenant/parts/0540_unique_active_cash_settlement_per_cashier_day.xml b/fineract-provider/src/main/resources/db/changelog/tenant/parts/0540_unique_active_cash_settlement_per_cashier_day.xml new file mode 100644 index 000000000..7312f7676 --- /dev/null +++ b/fineract-provider/src/main/resources/db/changelog/tenant/parts/0540_unique_active_cash_settlement_per_cashier_day.xml @@ -0,0 +1,53 @@ + + + + + + + + + SELECT COUNT(*) FROM ( + SELECT from_cashier_id, settlement_date + FROM m_cash_settlement + WHERE COALESCE(is_reversed, false) = false + AND from_cashier_id IS NOT NULL + GROUP BY from_cashier_id, settlement_date + HAVING COUNT(*) > 1 + ) dupes + + + SELECT COUNT(*) FROM pg_indexes + WHERE tablename = 'm_cash_settlement' + AND indexname = 'uq_cash_settlement_active_per_cashier_day' + + + + + CREATE UNIQUE INDEX uq_cash_settlement_active_per_cashier_day + ON m_cash_settlement (from_cashier_id, settlement_date) + WHERE COALESCE(is_reversed, false) = false AND from_cashier_id IS NOT NULL + + + DROP INDEX IF EXISTS uq_cash_settlement_active_per_cashier_day + + + +