Skip to content

C
Clubphp
Commit 87c22aca authored by DevPilot's avatar DevPilot
Browse files
Options

fix(accounting): stop a failed claim from re-posting the accrual every run 

Booking the accruals against the live books exposed a real defect, and it cost
one duplicate entry before it was caught.

Nine rows in sa_players carry a membership NUMBER in member_id (101, 1015,
897000) instead of a member row id. The accrual run reached the first of them,
the foreign key on accounts_receivable.member_id refused the insert, and the
exception escaped the loop that records claims in posting_accruals. That loop
runs AFTER the journal entry is posted, so the entry stood at the full batch
total with only the claims written before the failure. The next run read the
remaining documents as never accrued and posted a SECOND entry for them — and
would have posted one more every night, because the failure is deterministic.

AccrualService::batch now treats a claim it cannot write as something to report,
not something to throw: the entry is already posted, so abandoning the rest of
the batch is the one response guaranteed to corrupt the ledger. A member id that
does not resolve costs the receivable, never the claim — posting_accruals is the
system of record and already tracks obligations for non-members. Member
existence is resolved once per batch, not once per claim.

Phase_112_003 repairs what the two runs left: the duplicate is reversed rather
than deleted so the correction stays visible, its claims move to the entry that
actually posted their money, and the 110 claims plus 32 receivables that were
never written are backfilled against it. It verifies the entry and its claims
agree before finishing, and does nothing at all unless production matches the
exact broken shape.

Why the clone missed it: the verification harness built tables with
CREATE TABLE ... LIKE, which silently drops foreign keys — so the constraint
that breaks production did not exist in the clone, and the run came back green.
The harness now copies DDL via SHOW CREATE TABLE and asserts the foreign key
count matches the source (409/409).

Verified against a clone of the actual broken production state: net movement for
sa_subscription_accrual is 104,769.00 not 184,938.00, all 135 claims equal their
entry to the cent, 1,316 claims total 835,167.93, trial balance diff 0.00, and
both the repair and the fixed runner post nothing on a second pass.
Co-Authored-By: 's avatarClaude Opus 5 <noreply@anthropic.com>
parent 9f48d4aa
Show whitespace changes
Inline Side-by-side
Showing with 295 additions and 22 deletions
app/Modules/Accounting/Services/Revenue/AccrualService.php
View file @ 87c22aca
...@@ -187,34 +187,76 @@ final class AccrualService ...@@ -187,34 +187,76 @@ final class AccrualService
$entryId = $routed['journal_entry_id']; $entryId = $routed['journal_entry_id'];
// The claims go in after the entry, and each carries the entry that // The claims go in after the entry, and each carries the entry that
// raised it. If this loop dies half way the next run picks up exactly // raised it.
// the ones that never got a row — that is what posting_accruals is for. //
// Nothing in this loop may throw, because the entry is ALREADY posted
// for the full batch total. Abandoning the remaining claims would leave
// that money on the books with nothing recording who owes it, and the
// next run — reading those documents as never accrued — would post a
// SECOND entry for the same obligation. One unresolvable member
// reference would then inflate the ledger a little more every night.
// A claim that cannot be written is reported, not thrown.
$validMembers = self::existingMemberIds($deltas);
$count = 0; $count = 0;
$failed = [];
foreach ($deltas as $item) { foreach ($deltas as $item) {
$documentId = (int) $item['document_id']; $documentId = (int) $item['document_id'];
$dueDate = $item['due_date'] ?? ($opts['due_date'] ?? date('Y-m-d', strtotime('+30 days'))); $dueDate = $item['due_date'] ?? ($opts['due_date'] ?? date('Y-m-d', strtotime('+30 days')));
$desc = (string) ($item['description_ar'] ?? $opts['description_ar'] ?? $streamCode); $desc = (string) ($item['description_ar'] ?? $opts['description_ar'] ?? $streamCode);
$memberId = !empty($item['member_id']) ? (int) $item['member_id'] : null;
try {
SubledgerService::recordAccrual([ SubledgerService::recordAccrual([
'stream_code' => $streamCode, 'stream_code' => $streamCode,
'document_type' => $documentType, 'document_type' => $documentType,
'document_id' => $documentId, 'document_id' => $documentId,
'document_number' => $item['document_number'] ?? null, 'document_number' => $item['document_number'] ?? null,
'accrued_amount' => $item['amount'], 'accrued_amount' => $item['amount'],
'member_id' => $item['member_id'] ?? null, 'member_id' => $memberId,
'counterparty_name' => $item['counterparty_name'] ?? null, 'counterparty_name' => $item['counterparty_name'] ?? null,
'journal_entry_id' => $entryId, 'journal_entry_id' => $entryId,
'document_date' => $opts['entry_date'] ?? date('Y-m-d'), 'document_date' => $opts['entry_date'] ?? date('Y-m-d'),
'due_date' => $dueDate, 'due_date' => $dueDate,
'branch_id' => $item['branch_id'] ?? ($opts['branch_id'] ?? null), 'branch_id' => $item['branch_id'] ?? ($opts['branch_id'] ?? null),
]); ]);
$count++;
} catch (\Throwable $e) {
$failed[] = $documentId;
Logger::error('Accrual claim could not be recorded', [
'stream' => $streamCode,
'document' => $documentType . '#' . $documentId,
'entry' => $entryId,
'error' => $e->getMessage(),
]);
continue;
}
// The member-facing view. Non-members legitimately have no row — // The member-facing view. Non-members legitimately have no row —
// accounts_receivable.member_id is NOT NULL — and their obligation // accounts_receivable.member_id is NOT NULL — and their obligation
// is tracked in posting_accruals above. // is tracked in posting_accruals above.
if (!empty($item['member_id'])) { //
// So does anyone whose member id does not resolve. Some source
// tables carry a membership NUMBER in their member_id column rather
// than a row id, and the receivable's foreign key rightly refuses
// it. That is a data problem in the source module, and it must not
// cost us the claim: the obligation is real either way.
if ($memberId === null) {
continue;
}
if (!isset($validMembers[$memberId])) {
Logger::warning('Accrual claim kept without a receivable — member does not exist', [
'stream' => $streamCode,
'document' => $documentType . '#' . $documentId,
'member_id' => $memberId,
]);
continue;
}
try {
SubledgerService::upsertReceivable([ SubledgerService::upsertReceivable([
'member_id' => (int) $item['member_id'], 'member_id' => $memberId,
'document_type' => $documentType, 'document_type' => $documentType,
'document_id' => $documentId, 'document_id' => $documentId,
'document_number' => $item['document_number'] ?? null, 'document_number' => $item['document_number'] ?? null,
...@@ -225,18 +267,29 @@ final class AccrualService ...@@ -225,18 +267,29 @@ final class AccrualService
'journal_entry_id' => $entryId, 'journal_entry_id' => $entryId,
'branch_id' => $item['branch_id'] ?? ($opts['branch_id'] ?? null), 'branch_id' => $item['branch_id'] ?? ($opts['branch_id'] ?? null),
]); ]);
} catch (\Throwable $e) {
// The claim is already recorded, which is what keeps the books
// straight. The receivable is a view onto it and can be rebuilt.
Logger::error('Receivable could not be written for an accrued claim', [
'stream' => $streamCode,
'document' => $documentType . '#' . $documentId,
'member_id' => $memberId,
'error' => $e->getMessage(),
]);
} }
$count++;
} }
Logger::info('Batch accrual posted', [ Logger::info('Batch accrual posted', [
'stream' => $streamCode, 'entry' => $entryId, 'claims' => $count, 'total' => $total, 'stream' => $streamCode, 'entry' => $entryId, 'claims' => $count, 'total' => $total,
'failed' => \count($failed),
]); ]);
return [ return [
'posted' => true, 'journal_entry_id' => $entryId, 'posted' => true, 'journal_entry_id' => $entryId,
'count' => $count, 'total' => $total, 'error' => null, 'count' => $count, 'total' => $total,
'error' => $failed
? 'تعذّر تسجيل ' . \count($failed) . ' مطالبة رغم ترحيل القيد — راجع السجل'
: null,
]; ];
} }
...@@ -377,6 +430,43 @@ final class AccrualService ...@@ -377,6 +430,43 @@ final class AccrualService
return true; return true;
} }
/**
* Which of a batch's member ids actually exist, as a lookup.
*
* Resolved in one query rather than per claim: a subscription run carries
* hundreds of items, and asking the database once per item turns a two
* second job into a two minute one.
*
* @param array<int, array<string, mixed>> $deltas
* @return array<int, true>
*/
private static function existingMemberIds(array $deltas): array
{
$ids = [];
foreach ($deltas as $item) {
if (!empty($item['member_id'])) {
$ids[(int) $item['member_id']] = true;
}
}
if (!$ids) {
return [];
}
$keys = array_keys($ids);
$rows = App::getInstance()->db()->select(
"SELECT id FROM members WHERE id IN (" . implode(',', array_fill(0, \count($keys), '?')) . ")",
$keys
);
$valid = [];
foreach ($rows as $row) {
$valid[(int) $row['id']] = true;
}
return $valid;
}
private static function money(string $v): string private static function money(string $v): string
{ {
return number_format((float) $v, self::SCALE, '.', ''); return number_format((float) $v, self::SCALE, '.', '');
......
database/migrations/Phase_112_003_repair_sa_subscription_accrual.php 0 → 100644
View file @ 87c22aca
<?php
declare(strict_types=1);
use App\Core\Database;
/**
* Repairs the sports-subscription accrual, which posted twice.
*
* What happened: nine rows in `sa_players` carry a membership NUMBER in
* `member_id` (101, 1015, 897000 …) instead of a member row id. When the accrual
* run reached the first of them, the foreign key on
* `accounts_receivable.member_id` refused the insert and the exception escaped
* the loop that records claims in `posting_accruals`.
*
* That loop runs AFTER the journal entry is posted, so the entry stood at the
* full batch total while only the claims written before the failure existed. The
* next run read the remaining documents as never accrued and posted a SECOND
* entry for them — and would have posted a third, a fourth, and one more every
* night, because the failure is deterministic.
*
* AccrualService::batch no longer lets a single claim end the batch, so this
* cannot recur. This migration cleans up what the two runs left behind:
*
* - the duplicate entry is reversed, not deleted, so the correction is visible
* - its claims move to the entry that actually posted their money
* - the claims that were never written are backfilled against that same entry
*
* Deliberately narrow. It repairs only the exact shape described above and
* verifies the result before finishing; anything else and it does nothing,
* because a wrong guess here would misstate the ledger rather than fix it.
*/
return static function (Database $db): void {
$entries = $db->select(
"SELECT id, entry_number, total_debit
FROM journal_entries
WHERE reference_type = 'sa_subscription_accrual'
AND status = 'posted'
AND reversal_of_id IS NULL
ORDER BY id"
);
if (\count($entries) < 2) {
return; // never broken here, or already repaired
}
$keep = array_shift($entries);
$keepId = (int) $keep['id'];
$keepTotal = (string) $keep['total_debit'];
// Everything the surviving entry is supposed to cover: the same population
// the runner scans, which is what its total was computed from.
$candidates = $db->select(
"SELECT s.id, s.subscription_number, s.final_amount, s.period_start,
p.member_id, p.full_name_ar
FROM sa_subscriptions s
LEFT JOIN sa_players p ON p.id = s.player_id
WHERE s.final_amount > 0
AND COALESCE(s.payment_status, 'unpaid') IN ('unpaid', 'overdue', 'partial')
ORDER BY s.id"
);
$expected = '0.00';
foreach ($candidates as $c) {
$expected = bcadd($expected, number_format((float) $c['final_amount'], 2, '.', ''), 2);
}
// If the kept entry is not the full-population entry, this is not the
// failure described above and guessing would make things worse.
if (bccomp($keepTotal, $expected, 2) !== 0) {
return;
}
$now = date('Y-m-d H:i:s');
// 1. Reverse the duplicates and pull their claims onto the kept entry.
foreach ($entries as $dup) {
$dupId = (int) $dup['id'];
$db->query(
"UPDATE posting_accruals SET journal_entry_id = ?, updated_at = ? WHERE journal_entry_id = ?",
[$keepId, $now, $dupId]
);
$db->query(
"UPDATE accounts_receivable SET journal_entry_id = ?, updated_at = ? WHERE journal_entry_id = ?",
[$keepId, $now, $dupId]
);
$result = \App\Modules\Accounting\Services\JournalService::reverseEntry(
$dupId,
'قيد مكرر — استحقاق اشتراكات النشاط الرياضي اتقيّد مرتين بسبب توقف تسجيل المطالبات في النص. '
. 'المبلغ كله كان اتقيّد بالفعل في القيد ' . $keep['entry_number'] . '.'
);
if (empty($result['success'])) {
throw new \RuntimeException(
'تعذّر عكس القيد المكرر ' . $dup['entry_number'] . ': ' . ($result['error'] ?? 'سبب غير معروف')
);
}
}
// 2. Backfill what the failed loop never wrote, against the entry that
// posted the money — producing exactly the rows the fixed runner would
// have produced, so a later run sees nothing left to do.
$have = [];
foreach ($db->select(
"SELECT document_id FROM posting_accruals WHERE document_type = 'sa_subscription'"
) as $row) {
$have[(int) $row['document_id']] = true;
}
$haveReceivable = [];
foreach ($db->select(
"SELECT document_id FROM accounts_receivable WHERE document_type = 'sa_subscription'"
) as $row) {
$haveReceivable[(int) $row['document_id']] = true;
}
foreach ($candidates as $c) {
$docId = (int) $c['id'];
$amount = number_format((float) $c['final_amount'], 2, '.', '');
$dueDate = $c['period_start'] ?? date('Y-m-d');
$memberId = !empty($c['member_id']) ? (int) $c['member_id'] : null;
// A member id that resolves to nothing is a membership number sitting in
// the wrong column. The claim still stands; only the receivable, whose
// foreign key refuses it, has to be left out.
$realMember = $memberId !== null
&& $db->selectOne("SELECT id FROM members WHERE id = ?", [$memberId]) !== null;
if (!isset($have[$docId])) {
$db->insert('posting_accruals', [
'stream_code' => 'sa:monthly_subscription',
'document_type' => 'sa_subscription',
'document_id' => $docId,
'document_number' => $c['subscription_number'] ?? null,
'accrued_amount' => $amount,
'settled_amount' => '0.00',
'member_id' => $memberId,
'counterparty_name' => $c['full_name_ar'] ?? null,
'journal_entry_id' => $keepId,
'document_date' => date('Y-m-d'),
'due_date' => $dueDate,
'status' => 'open',
'notes' => 'استُكملت بقيد إصلاح — المبلغ كان مرحّلًا في ' . $keep['entry_number']
. ' من غير ما المطالبة تتسجّل.',
'first_accrued_at' => $now,
'updated_at' => $now,
]);
}
// The member-facing side never got written either — the run died on the
// first of these. Without it a member owes money the system will not
// show them.
if ($realMember && !isset($haveReceivable[$docId])) {
\App\Modules\Accounting\Services\SubledgerService::upsertReceivable([
'member_id' => (int) $memberId,
'document_type' => 'sa_subscription',
'document_id' => $docId,
'document_number' => $c['subscription_number'] ?? null,
'document_date' => date('Y-m-d'),
'due_date' => $dueDate,
'description_ar' => 'اشتراك نشاط رياضي ' . ($c['subscription_number'] ?? '')
. ' — ' . ($c['full_name_ar'] ?? ''),
'total_amount' => $amount,
'journal_entry_id' => $keepId,
]);
}
}
// 3. The entry and its claims must now say the same thing.
$sum = $db->selectOne(
"SELECT ROUND(COALESCE(SUM(accrued_amount), 0), 2) s
FROM posting_accruals WHERE journal_entry_id = ? AND status <> 'reversed'",
[$keepId]
);
if (bccomp((string) $sum['s'], $keepTotal, 2) !== 0) {
throw new \RuntimeException(
'إصلاح استحقاق اشتراكات النشاط لم يتوازن: القيد ' . $keepTotal
. ' والمطالبات ' . (string) $sum['s']
);
}
};
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