Commit 2f77d3e1 authored by Mahmoud Aglan's avatar Mahmoud Aglan

fix(accounting): opening balances were double-counted in three reports

The opening figures live in TWO places in this ledger: the
chart_of_accounts.opening_balance column AND 24 posted journal entries dated
2024-07-01 with reference_type='opening', totalling 90,601,962.36.

Three reports read the column and then also summed the ledger movement that
already contained those same entries, counting the opening twice:

- Trial balance (LedgerService::getTrialBalance)
- General ledger  (LedgerService::getAccountLedger)
- Balance sheet   (FinancialReportService::getBalanceSheet, and the
                   consolidated sheet which delegates to it)

Measured on live data, trial balance over FY 2024/2025:
  1103 مشروعات تحت التنفيذ   reported 85,627,410.75  actual 43,923,543.75
  210201 أرباح مرحلة         reported -146,645,270   actual -73,322,635
i.e. exactly double on every account carrying an opening balance. The report
still footed, because opening balances net to zero across debit and credit —
so it looked right and every line was wrong. Only periods containing
2024-07-01 were affected; a 2026 trial balance was already correct.

The opening column is now derived as cumulative posted movement BEFORE the
period start, which is the standard definition, removes the double count
structurally, and works for any period rather than only a year boundary. The
trial balance query is also restructured into two independent aggregates so
no row multiplication is possible and an account whose only movement predates
the period still appears.

Income statement was already correct and is unchanged.

Also in this commit:

- LedgerService::rebuildBalances() + a seed that runs it. The opening import
  wrote journal rows without going through JournalService, so 24 accounts had
  a cached current_balance disagreeing with the ledger — retained earnings
  cached 0.00 against an actual 73,322,635.00. The reports read the ledger and
  were fine, but the Chart of Accounts screen and the bank-reconciliation
  opening figure read the cache, which is precisely where an accountant would
  find a number contradicting the trial balance.

- Carnet guest entry never posted. Accounting listened on
  'carnet.guest_entry_recorded'; GuestEntryService dispatches
  'carnet_guest.entry_recorded' (underscore, not dot). Notifications listens on
  the correct name, which is why notifications worked and the ledger entry
  never appeared. Fees were recorded in carnet_guest_entries.amount_paid and
  posted nowhere.

- 'tournament.fee_collected' has no dispatcher anywhere. Documented as dead
  rather than left looking wired.

- Two fiscal years were flagged is_current; the seed leaves exactly the one
  containing today. FiscalYear::findByDate now resolves overlapping years
  deterministically (open first, then narrowest range) instead of taking
  whatever the database returned — this chart has calendar years overlapping a
  July-June year, so Jul-Dec 2024 matches two. No entry is reassigned; all 795
  are already inside their assigned year.

- PostingRouter and postViaRule now probe App::db() with try/catch. It is typed
  `: Database` and throws when unbound rather than returning null, so the
  previous null guards could never fire.
Co-Authored-By: 's avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent fb3097a2
...@@ -26,13 +26,35 @@ class FiscalYear extends Model ...@@ -26,13 +26,35 @@ class FiscalYear extends Model
return $instance; return $instance;
} }
/**
* Resolve the fiscal year a date belongs to.
*
* This chart carries overlapping fiscal years — calendar years 2018-2025 alongside
* a July-June "2024/2025" year — so a date in Jul-Dec 2024 matches two of them.
* Without an explicit order the database returns whichever it likes, and two
* entries on the same day can land in different years.
*
* Resolution order, most specific first:
* 1. an OPEN year (a closed year must not absorb new postings)
* 2. the NARROWEST range containing the date
* 3. the one flagged current
* 4. lowest id, purely so the answer is stable
*/
public static function findByDate(string $date): ?static public static function findByDate(string $date): ?static
{ {
$row = static::query() $db = \App\Core\App::getInstance()->db();
->where('start_date', '<=', $date)
->where('end_date', '>=', $date) $row = $db->selectOne(
->where('is_archived', '=', 0) "SELECT * FROM fiscal_years
->first(); WHERE start_date <= ? AND end_date >= ? AND is_archived = 0
ORDER BY (status = 'open') DESC,
DATEDIFF(end_date, start_date) ASC,
is_current DESC,
id ASC
LIMIT 1",
[$date, $date]
);
if ($row === null) { if ($row === null) {
return null; return null;
} }
...@@ -41,6 +63,26 @@ class FiscalYear extends Model ...@@ -41,6 +63,26 @@ class FiscalYear extends Model
return $instance; return $instance;
} }
/** Fiscal years whose ranges overlap another — surfaced as a warning in the UI. */
public static function overlapping(): array
{
$db = \App\Core\App::getInstance()->db();
return $db->select(
"SELECT a.id, a.name_ar, a.start_date, a.end_date,
b.id AS other_id, b.name_ar AS other_name,
b.start_date AS other_start, b.end_date AS other_end
FROM fiscal_years a
JOIN fiscal_years b
ON b.id > a.id
AND b.is_archived = 0
AND a.start_date <= b.end_date
AND b.start_date <= a.end_date
WHERE a.is_archived = 0
ORDER BY a.start_date"
);
}
public function isOpen(): bool public function isOpen(): bool
{ {
return $this->status === 'open'; return $this->status === 'open';
......
...@@ -140,9 +140,12 @@ final class AccountingIntegrationService ...@@ -140,9 +140,12 @@ final class AccountingIntegrationService
return false; return false;
} }
$db = App::getInstance()->db(); // App::db() is typed `: Database` and throws when unbound (CLI) rather than
if ($db === null) { // returning null, so probe it rather than null-checking the result.
return false; // CLI context without a bound connection try {
$db = App::getInstance()->db();
} catch (\Throwable $e) {
return false;
} }
$payment = $paymentId > 0 ? $db->selectOne("SELECT * FROM payments WHERE id = ?", [$paymentId]) : null; $payment = $paymentId > 0 ? $db->selectOne("SELECT * FROM payments WHERE id = ?", [$paymentId]) : null;
......
...@@ -146,11 +146,15 @@ final class FinancialReportService ...@@ -146,11 +146,15 @@ final class FinancialReportService
$totalEquity = '0.00'; $totalEquity = '0.00';
foreach ($allAccounts as $acc) { foreach ($allAccounts as $acc) {
$opening = (string) $acc['opening_balance']; // Cumulative posted movement up to asOfDate IS the balance.
// chart_of_accounts.opening_balance is deliberately NOT added on top: the
// opening figures already sit in the ledger as posted 'opening' entries
// dated 2024-07-01, which are inside "up to asOfDate". Adding the column
// as well double-counted every account that carries an opening balance.
if ($acc['account_nature'] === 'debit') { if ($acc['account_nature'] === 'debit') {
$balance = bcadd($opening, bcsub((string) $acc['total_debit'], (string) $acc['total_credit'], 2), 2); $balance = bcsub((string) $acc['total_debit'], (string) $acc['total_credit'], 2);
} else { } else {
$balance = bcadd($opening, bcsub((string) $acc['total_credit'], (string) $acc['total_debit'], 2), 2); $balance = bcsub((string) $acc['total_credit'], (string) $acc['total_debit'], 2);
} }
if (bccomp($balance, '0.00', 2) === 0) { if (bccomp($balance, '0.00', 2) === 0) {
......
...@@ -11,6 +11,110 @@ use App\Core\App; ...@@ -11,6 +11,110 @@ use App\Core\App;
*/ */
final class LedgerService final class LedgerService
{ {
/**
* Recompute the denormalised balance caches from the posted ledger.
*
* `chart_of_accounts.current_balance` and the `account_balances` period table are
* maintained incrementally by JournalService as entries post. Anything that writes
* journal rows without going through it — a bulk opening-balance import, a direct
* SQL fix — leaves them stale, and then the Chart of Accounts screen and the bank
* reconciliation opening figure disagree with the trial balance.
*
* This rebuilds both from journal_entry_lines, which is the only source of truth.
* Safe to run at any time: it derives, it never invents.
*
* @return array{accounts:int, periods:int, corrected:int, drift_before:string}
*/
public static function rebuildBalances(): array
{
$db = App::getInstance()->db();
// What the ledger actually says, per account.
$actual = $db->select(
"SELECT jel.account_id,
SUM(jel.debit) AS dr,
SUM(jel.credit) AS cr
FROM journal_entry_lines jel
JOIN journal_entries je ON je.id = jel.journal_entry_id
WHERE je.status = 'posted' AND je.is_archived = 0
GROUP BY jel.account_id"
);
$byAccount = [];
foreach ($actual as $row) {
$byAccount[(int) $row['account_id']] = $row;
}
$accounts = $db->select(
"SELECT id, account_nature, current_balance FROM chart_of_accounts WHERE is_archived = 0"
);
$corrected = 0;
$driftBefore = '0.00';
foreach ($accounts as $acc) {
$id = (int) $acc['id'];
$dr = (string) ($byAccount[$id]['dr'] ?? '0.00');
$cr = (string) ($byAccount[$id]['cr'] ?? '0.00');
$balance = $acc['account_nature'] === 'credit'
? bcsub($cr, $dr, 2)
: bcsub($dr, $cr, 2);
$current = (string) $acc['current_balance'];
if (bccomp($balance, $current, 2) !== 0) {
$driftBefore = bcadd($driftBefore, bcsub($balance, $current, 2) >= '0'
? bcsub($balance, $current, 2)
: bcsub($current, $balance, 2), 2);
$db->update('chart_of_accounts', ['current_balance' => $balance], '`id` = ?', [$id]);
$corrected++;
}
}
// Rebuild the per-period table wholesale — partial repair cannot fix a row
// that should not exist.
$db->raw("DELETE FROM account_balances");
$periods = $db->select(
"SELECT jel.account_id,
je.fiscal_year_id,
DATE_FORMAT(je.entry_date, '%Y-%m') AS period,
jel.cost_center_id,
jel.branch_id,
SUM(jel.debit) AS dr,
SUM(jel.credit) AS cr
FROM journal_entry_lines jel
JOIN journal_entries je ON je.id = jel.journal_entry_id
WHERE je.status = 'posted' AND je.is_archived = 0
GROUP BY jel.account_id, je.fiscal_year_id, period, jel.cost_center_id, jel.branch_id"
);
$now = date('Y-m-d H:i:s');
foreach ($periods as $p) {
$db->insert('account_balances', [
'account_id' => (int) $p['account_id'],
'fiscal_year_id' => (int) $p['fiscal_year_id'],
'period' => $p['period'],
'opening_debit' => '0.00',
'opening_credit' => '0.00',
'period_debit' => $p['dr'],
'period_credit' => $p['cr'],
'closing_debit' => $p['dr'],
'closing_credit' => $p['cr'],
'cost_center_id' => $p['cost_center_id'],
'branch_id' => $p['branch_id'],
'created_at' => $now,
'updated_at' => $now,
]);
}
return [
'accounts' => count($accounts),
'periods' => count($periods),
'corrected' => $corrected,
'drift_before' => $driftBefore,
];
}
/** /**
* Get general ledger for a specific account within a date range. * Get general ledger for a specific account within a date range.
* *
...@@ -56,13 +160,15 @@ final class LedgerService ...@@ -56,13 +160,15 @@ final class LedgerService
$openingDebit = $openingRow['total_debit'] ?? '0.00'; $openingDebit = $openingRow['total_debit'] ?? '0.00';
$openingCredit = $openingRow['total_credit'] ?? '0.00'; $openingCredit = $openingRow['total_credit'] ?? '0.00';
// Opening balance adjusted for account nature // Opening balance = cumulative posted movement before the period, adjusted for
// account nature. chart_of_accounts.opening_balance is deliberately NOT added:
// the opening figures are already in the ledger as posted 'opening' entries
// dated 2024-07-01, so adding the column too counted them twice and made this
// statement disagree with the trial balance.
if ($account['account_nature'] === 'debit') { if ($account['account_nature'] === 'debit') {
$openingBalance = bcsub($openingDebit, $openingCredit, 2); $openingBalance = bcsub($openingDebit, $openingCredit, 2);
$openingBalance = bcadd($openingBalance, (string) $account['opening_balance'], 2);
} else { } else {
$openingBalance = bcsub($openingCredit, $openingDebit, 2); $openingBalance = bcsub($openingCredit, $openingDebit, 2);
$openingBalance = bcadd($openingBalance, (string) $account['opening_balance'], 2);
} }
// Ledger entries within the period // Ledger entries within the period
...@@ -123,34 +229,71 @@ final class LedgerService ...@@ -123,34 +229,71 @@ final class LedgerService
): array { ): array {
$db = App::getInstance()->db(); $db = App::getInstance()->db();
$extraWhere = ''; // Dimension filters apply identically to the prior-period and in-period legs,
$params = [$dateFrom, $dateTo]; // otherwise the opening column would be scoped differently from the movement.
$dimWhere = '';
$dimParams = [];
if ($costCenterId !== null) { if ($costCenterId !== null) {
$extraWhere .= ' AND COALESCE(jel.cost_center_id, je.cost_center_id) = ?'; $dimWhere .= ' AND COALESCE(jel.cost_center_id, je.cost_center_id) = ?';
$params[] = $costCenterId; $dimParams[] = $costCenterId;
} }
if ($branchId !== null) { if ($branchId !== null) {
$extraWhere .= ' AND COALESCE(jel.branch_id, je.branch_id) = ?'; $dimWhere .= ' AND COALESCE(jel.branch_id, je.branch_id) = ?';
$params[] = $branchId; $dimParams[] = $branchId;
} }
// Get all accounts with their period movement // The opening balance is CUMULATIVE POSTED MOVEMENT BEFORE dateFrom — not the
// chart_of_accounts.opening_balance column.
//
// Those two are not interchangeable here: this ledger carries the opening
// figures BOTH in that column AND as 24 posted journal entries dated
// 2024-07-01 (reference_type='opening'). Reading the column while the period
// also contains those entries counts the opening twice — a trial balance run
// over FY 2024/2025 reported retained earnings at exactly double.
//
// Deriving the opening from the ledger is also the standard definition, so
// any period start works, not only a fiscal-year boundary.
//
// Each leg is its own aggregate so no row multiplication can occur, and an
// account whose only movement predates the period still appears with its
// opening balance.
$params = array_merge(
[$dateFrom], $dimParams,
[$dateFrom, $dateTo], $dimParams
);
$accounts = $db->select( $accounts = $db->select(
"SELECT coa.id, coa.account_code, coa.name_ar, coa.name_en, "SELECT coa.id, coa.account_code, coa.name_ar, coa.name_en,
coa.account_type, coa.account_nature, coa.level, coa.is_header, coa.account_type, coa.account_nature, coa.level, coa.is_header,
coa.opening_balance, coa.current_balance, coa.opening_balance, coa.current_balance,
COALESCE(SUM(jel.debit), 0) as total_debit, COALESCE(prior.dr, 0) AS prior_debit,
COALESCE(SUM(jel.credit), 0) as total_credit COALESCE(prior.cr, 0) AS prior_credit,
COALESCE(cur.dr, 0) AS total_debit,
COALESCE(cur.cr, 0) AS total_credit
FROM chart_of_accounts coa FROM chart_of_accounts coa
LEFT JOIN journal_entry_lines jel ON jel.account_id = coa.id LEFT JOIN (
LEFT JOIN journal_entries je ON je.id = jel.journal_entry_id SELECT jel.account_id,
AND je.status = 'posted' AND je.entry_date >= ? AND je.entry_date <= ? AND je.is_archived = 0 SUM(jel.debit) AS dr,
{$extraWhere} SUM(jel.credit) AS cr
WHERE coa.is_archived = 0 AND (jel.id IS NULL OR je.id IS NOT NULL) FROM journal_entry_lines jel
GROUP BY coa.id, coa.account_code, coa.name_ar, coa.name_en, JOIN journal_entries je ON je.id = jel.journal_entry_id
coa.account_type, coa.account_nature, coa.level, coa.is_header, WHERE je.status = 'posted' AND je.is_archived = 0
coa.opening_balance, coa.current_balance AND je.entry_date < ?
{$dimWhere}
GROUP BY jel.account_id
) prior ON prior.account_id = coa.id
LEFT JOIN (
SELECT jel.account_id,
SUM(jel.debit) AS dr,
SUM(jel.credit) AS cr
FROM journal_entry_lines jel
JOIN journal_entries je ON je.id = jel.journal_entry_id
WHERE je.status = 'posted' AND je.is_archived = 0
AND je.entry_date >= ? AND je.entry_date <= ?
{$dimWhere}
GROUP BY jel.account_id
) cur ON cur.account_id = coa.id
WHERE coa.is_archived = 0
ORDER BY coa.account_code ASC", ORDER BY coa.account_code ASC",
$params $params
); );
...@@ -168,7 +311,9 @@ final class LedgerService ...@@ -168,7 +311,9 @@ final class LedgerService
foreach ($accounts as &$acc) { foreach ($accounts as &$acc) {
$debitMovement = (string) $acc['total_debit']; $debitMovement = (string) $acc['total_debit'];
$creditMovement = (string) $acc['total_credit']; $creditMovement = (string) $acc['total_credit'];
$opening = (string) $acc['opening_balance'];
// Signed opening: positive = net debit, negative = net credit.
$opening = bcsub((string) $acc['prior_debit'], (string) $acc['prior_credit'], 2);
// Opening debit/credit split // Opening debit/credit split
if (bccomp($opening, '0.00', 2) >= 0) { if (bccomp($opening, '0.00', 2) >= 0) {
......
...@@ -35,7 +35,10 @@ final class PostingRouter ...@@ -35,7 +35,10 @@ final class PostingRouter
return null; return null;
} }
$db = App::getInstance()->db(); $db = self::db();
if ($db === null) {
return null;
}
$stream = $db->selectOne( $stream = $db->selectOne(
"SELECT id FROM revenue_streams WHERE stream_code = ? AND is_active = 1", "SELECT id FROM revenue_streams WHERE stream_code = ? AND is_active = 1",
...@@ -99,7 +102,7 @@ final class PostingRouter ...@@ -99,7 +102,7 @@ final class PostingRouter
*/ */
public static function accountFor(string $streamCode, ?string $fallbackCode = null, string $stage = 'payment'): ?int public static function accountFor(string $streamCode, ?string $fallbackCode = null, string $stage = 'payment'): ?int
{ {
$db = App::getInstance()->db(); $db = self::db();
if ($db === null) { if ($db === null) {
return null; return null;
} }
...@@ -153,15 +156,29 @@ final class PostingRouter ...@@ -153,15 +156,29 @@ final class PostingRouter
return (int) $acc['id']; return (int) $acc['id'];
} }
/**
* App::db() is typed `: Database` and throws on an uninitialised property rather
* than returning null, so every entry point guards with a try/catch instead of a
* null check. An unbound connection (CLI) simply means "not ready".
*/
private static function db(): ?\App\Core\Database
{
try {
return App::getInstance()->db();
} catch (\Throwable $e) {
return null;
}
}
private static function ready(): bool private static function ready(): bool
{ {
if (self::$tablesReady !== null) { if (self::$tablesReady !== null) {
return self::$tablesReady; return self::$tablesReady;
} }
$db = App::getInstance()->db(); $db = self::db();
if ($db === null) { if ($db === null) {
return false; // CLI without a bound connection — do not cache this return false; // unbound connection — do not cache, it may bind later
} }
try { try {
......
...@@ -164,7 +164,7 @@ final class RevenueStreamRegistry ...@@ -164,7 +164,7 @@ final class RevenueStreamRegistry
'carnet:guest_entry' => [ 'carnet:guest_entry' => [
'name_ar' => 'دخول ضيوف بالكارنيه', 'name_en' => 'Carnet Guest Entry', 'name_ar' => 'دخول ضيوف بالكارنيه', 'name_en' => 'Carnet Guest Entry',
'module' => 'carnets', 'key' => null, 'category' => 'facility', 'module' => 'carnets', 'key' => null, 'category' => 'facility',
'event' => 'carnet.guest_entry_recorded', 'legacy_account' => '410518', 'event' => 'carnet_guest.entry_recorded', 'legacy_account' => '410518',
], ],
// ── Penalties ──────────────────────────────────────────────── // ── Penalties ────────────────────────────────────────────────
......
...@@ -376,17 +376,29 @@ EventBus::listen('facility.entry_recorded', function (array $data): void { ...@@ -376,17 +376,29 @@ EventBus::listen('facility.entry_recorded', function (array $data): void {
}, 50); }, 50);
// ── Carnet Guest Entry ────────────────────────────────────── // ── Carnet Guest Entry ──────────────────────────────────────
// When a carnet guest entry is recorded (invitation book usage) // When a carnet guest entry is recorded (invitation book usage).
EventBus::listen('carnet.guest_entry_recorded', function (array $data): void { //
// The event name is 'carnet_guest.entry_recorded' — underscore, not a dot, between
// carnet and guest. This listener was previously registered on
// 'carnet.guest_entry_recorded', which nothing dispatches, so every guest-entry fee
// collected was recorded in carnet_guest_entries.amount_paid and never posted to the
// ledger. GuestEntryService.php:75 is the dispatcher; Notifications/bootstrap.php
// listens on the correct name, which is why notifications worked and accounting did not.
EventBus::listen('carnet_guest.entry_recorded', function (array $data): void {
try { try {
AccountingIntegrationService::onGuestEntry($data); AccountingIntegrationService::onGuestEntry($data);
} catch (\Throwable $e) { } catch (\Throwable $e) {
\App\Core\Logger::error('Accounting auto-post failed (carnet.guest_entry_recorded): ' . $e->getMessage()); \App\Core\Logger::error('Accounting auto-post failed (carnet_guest.entry_recorded): ' . $e->getMessage());
} }
}, 50); }, 50);
// ── Tournament Fees ───────────────────────────────────────── // ── Tournament Fees ─────────────────────────────────────────
// When a tournament registration fee is collected // 'tournament.fee_collected' has no dispatcher anywhere in the codebase —
// TournamentService only fires 'tournament.participant_registered', which carries no
// amount. Tournament fees are therefore collected through the generic payment screen
// (payment_type 'other') rather than from the tournament itself. The listener is kept
// so the path works the moment a fee is wired into registration, but it is dead today
// and is reported as such on the diagnostics page rather than left looking healthy.
EventBus::listen('tournament.fee_collected', function (array $data): void { EventBus::listen('tournament.fee_collected', function (array $data): void {
try { try {
AccountingIntegrationService::onTournamentFee($data); AccountingIntegrationService::onTournamentFee($data);
......
<?php
declare(strict_types=1);
use App\Modules\Accounting\Services\LedgerService;
/**
* Rebuild the denormalised balance caches from the posted ledger.
*
* `chart_of_accounts.current_balance` and the `account_balances` period table are
* maintained incrementally by JournalService as entries post. The opening-balance
* import wrote journal rows directly, bypassing that, so 24 accounts carried a cached
* balance that disagreed with the ledger — retained earnings showed 0.00 against an
* actual 73,322,635.00 credit.
*
* The trial balance, general ledger, income statement and balance sheet all read the
* ledger and were therefore unaffected. The stale cache surfaced on the Chart of
* Accounts screen (the balance column beside each account) and as the opening book
* balance in bank reconciliation — i.e. exactly where an accountant would spot a
* number that contradicts the trial balance.
*
* Derives only; invents nothing. Safe to re-run.
*/
return function (\App\Core\Database $db): void {
// LedgerService reads the connection off the App singleton, which is unbound in
// CLI. setDb is idempotent, so bind unconditionally rather than probing db() —
// App::db() is typed `: Database` and throws on an uninitialised property.
\App\Core\App::getInstance()->setDb($db);
$result = LedgerService::rebuildBalances();
echo " balances rebuilt: {$result['corrected']} of {$result['accounts']} accounts corrected, "
. "{$result['periods']} period rows rewritten\n";
// Exactly one fiscal year may be current. Two were flagged (2024/2025 and 2026),
// which makes "the current year" ambiguous wherever the UI relies on the flag.
// The year containing today wins; the rest are cleared. No entry is touched —
// fiscal_year_id assignments stay exactly as they are.
$today = date('Y-m-d');
$current = $db->selectOne(
"SELECT id FROM fiscal_years
WHERE start_date <= ? AND end_date >= ? AND is_archived = 0
ORDER BY (status = 'open') DESC, DATEDIFF(end_date, start_date) ASC, id ASC
LIMIT 1",
[$today, $today]
);
if ($current) {
$db->query("UPDATE fiscal_years SET is_current = 0 WHERE id <> ?", [(int) $current['id']]);
$db->query("UPDATE fiscal_years SET is_current = 1 WHERE id = ?", [(int) $current['id']]);
echo " fiscal year #{$current['id']} set as the single current year\n";
}
};
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment