Skip to content

C
Clubphp
Commit 5a5aba46 authored by Fares's avatar Fares
Browse files
Options

fix(subscriptions): prevent duplicate subscription rows with unique constraint 

Root causes of duplicate members in yearly subscriptions:
1. No DB-level unique constraint allowed race conditions between
   SubscriptionGenerator, SyncService, and RetroactiveMembershipService
2. SyncService set person_id=NULL for member rows vs Generator's person_id=memberId
3. RetroactiveMembershipService did blind INSERTs with no dedup check

Fix:
- Migration removes existing duplicates (keeps paid row, lowest ID tiebreak)
- Normalizes NULL person_id on member rows
- Adds UNIQUE INDEX (member_id, financial_year, person_type, person_id)
- All insert paths catch Duplicate entry exceptions as race guard
- SyncService now sets person_id=memberId matching Generator
- RetroactiveMembershipService checks for existing row before INSERT
Co-Authored-By: 's avatarClaude Opus 4.6 <noreply@anthropic.com>
parent a8c964ee
Hide whitespace changes
Inline Side-by-side
Showing with 241 additions and 100 deletions
app/Modules/Members/Services/RetroactiveMembershipService.php
View file @ 5a5aba46
...@@ -695,6 +695,17 @@ final class RetroactiveMembershipService ...@@ -695,6 +695,17 @@ final class RetroactiveMembershipService
if (!preg_match('/^\d{4}$/', $yearStart)) $yearStart = date('Y'); if (!preg_match('/^\d{4}$/', $yearStart)) $yearStart = date('Y');
$personType = $sub['person_type'] ?? 'member'; $personType = $sub['person_type'] ?? 'member';
$personId = (int) ($sub['person_id'] ?? 0);
// Dedup guard: skip if subscription already exists for this combination
$existing = $db->selectOne(
"SELECT id FROM subscriptions WHERE member_id = ? AND financial_year = ? AND person_type = ? AND person_id = ?",
[$memberId, $financialYear, $personType, $personId]
);
if ($existing) {
return (int) $existing['id'];
}
$baseAmount = (string) ($sub['base_amount'] ?? '0.00'); $baseAmount = (string) ($sub['base_amount'] ?? '0.00');
$devFee = ($personType === 'member') ? '35.00' : '0.00'; $devFee = ($personType === 'member') ? '35.00' : '0.00';
...@@ -729,7 +740,7 @@ final class RetroactiveMembershipService ...@@ -729,7 +740,7 @@ final class RetroactiveMembershipService
'member_id' => $memberId, 'member_id' => $memberId,
'financial_year' => $financialYear, 'financial_year' => $financialYear,
'person_type' => $personType, 'person_type' => $personType,
'person_id' => (int) $sub['person_id'], 'person_id' => $personId,
'person_name' => $sub['person_name'] ?? '', 'person_name' => $sub['person_name'] ?? '',
'base_amount' => $baseAmount, 'base_amount' => $baseAmount,
'development_fee' => $devFee, 'development_fee' => $devFee,
......
app/Modules/Subscriptions/Services/SubscriptionGenerator.php
View file @ 5a5aba46
...@@ -76,22 +76,26 @@ final class SubscriptionGenerator ...@@ -76,22 +76,26 @@ final class SubscriptionGenerator
// Member subscription — discount on base only; dev fee is separate non-discountable charge // Member subscription — discount on base only; dev fee is separate non-discountable charge
$discount = $discountPct ? bcdiv(bcmul($memberRate, $discountPct, 4), '100', 2) : '0.00'; $discount = $discountPct ? bcdiv(bcmul($memberRate, $discountPct, 4), '100', 2) : '0.00';
$total = bcsub($memberRate, $discount, 2); $total = bcsub($memberRate, $discount, 2);
$db->insert('subscriptions', [ try {
'member_id' => $memberId, $db->insert('subscriptions', [
'financial_year' => $financialYear, 'member_id' => $memberId,
'person_type' => 'member', 'financial_year' => $financialYear,
'person_id' => $memberId, 'person_type' => 'member',
'person_name' => $member['full_name_ar'], 'person_id' => $memberId,
'base_amount' => $memberRate, 'person_name' => $member['full_name_ar'],
'development_fee' => $devFee, 'base_amount' => $memberRate,
'discount_amount' => $discount, 'development_fee' => $devFee,
'total_amount' => $total, 'discount_amount' => $discount,
'status' => 'pending', 'total_amount' => $total,
'created_at' => $ts, 'status' => 'pending',
'updated_at' => $ts, 'created_at' => $ts,
'created_by' => $empId, 'updated_at' => $ts,
]); 'created_by' => $empId,
$created++; ]);
$created++;
} catch (\Throwable $e) {
if (str_contains($e->getMessage(), 'Duplicate entry')) { $skipped++; continue; } else { throw $e; }
}
// Spouses — NO dev fee // Spouses — NO dev fee
if ($db->tableExists('spouses')) { if ($db->tableExists('spouses')) {
...@@ -104,22 +108,26 @@ final class SubscriptionGenerator ...@@ -104,22 +108,26 @@ final class SubscriptionGenerator
if ($existingSp) { $skipped++; continue; } if ($existingSp) { $skipped++; continue; }
$spDiscount = $discountPct ? bcdiv(bcmul($spouseRate, $discountPct, 4), '100', 2) : '0.00'; $spDiscount = $discountPct ? bcdiv(bcmul($spouseRate, $discountPct, 4), '100', 2) : '0.00';
$spTotal = bcsub($spouseRate, $spDiscount, 2); $spTotal = bcsub($spouseRate, $spDiscount, 2);
$db->insert('subscriptions', [ try {
'member_id' => $memberId, $db->insert('subscriptions', [
'financial_year' => $financialYear, 'member_id' => $memberId,
'person_type' => 'spouse', 'financial_year' => $financialYear,
'person_id' => (int) $sp['id'], 'person_type' => 'spouse',
'person_name' => $sp['full_name_ar'], 'person_id' => (int) $sp['id'],
'base_amount' => $spouseRate, 'person_name' => $sp['full_name_ar'],
'development_fee' => '0.00', 'base_amount' => $spouseRate,
'discount_amount' => $spDiscount, 'development_fee' => '0.00',
'total_amount' => $spTotal, 'discount_amount' => $spDiscount,
'status' => 'pending', 'total_amount' => $spTotal,
'created_at' => $ts, 'status' => 'pending',
'updated_at' => $ts, 'created_at' => $ts,
'created_by' => $empId, 'updated_at' => $ts,
]); 'created_by' => $empId,
$created++; ]);
$created++;
} catch (\Throwable $e) {
if (str_contains($e->getMessage(), 'Duplicate entry')) { $skipped++; } else { throw $e; }
}
} }
} }
...@@ -134,22 +142,26 @@ final class SubscriptionGenerator ...@@ -134,22 +142,26 @@ final class SubscriptionGenerator
if ($existingCh) { $skipped++; continue; } if ($existingCh) { $skipped++; continue; }
$chDiscount = $discountPct ? bcdiv(bcmul($childRate, $discountPct, 4), '100', 2) : '0.00'; $chDiscount = $discountPct ? bcdiv(bcmul($childRate, $discountPct, 4), '100', 2) : '0.00';
$chTotal = bcsub($childRate, $chDiscount, 2); $chTotal = bcsub($childRate, $chDiscount, 2);
$db->insert('subscriptions', [ try {
'member_id' => $memberId, $db->insert('subscriptions', [
'financial_year' => $financialYear, 'member_id' => $memberId,
'person_type' => 'child', 'financial_year' => $financialYear,
'person_id' => (int) $ch['id'], 'person_type' => 'child',
'person_name' => $ch['full_name_ar'], 'person_id' => (int) $ch['id'],
'base_amount' => $childRate, 'person_name' => $ch['full_name_ar'],
'development_fee' => '0.00', 'base_amount' => $childRate,
'discount_amount' => $chDiscount, 'development_fee' => '0.00',
'total_amount' => $chTotal, 'discount_amount' => $chDiscount,
'status' => 'pending', 'total_amount' => $chTotal,
'created_at' => $ts, 'status' => 'pending',
'updated_at' => $ts, 'created_at' => $ts,
'created_by' => $empId, 'updated_at' => $ts,
]); 'created_by' => $empId,
$created++; ]);
$created++;
} catch (\Throwable $e) {
if (str_contains($e->getMessage(), 'Duplicate entry')) { $skipped++; } else { throw $e; }
}
} }
} }
...@@ -164,22 +176,26 @@ final class SubscriptionGenerator ...@@ -164,22 +176,26 @@ final class SubscriptionGenerator
if ($existingTmp) { $skipped++; continue; } if ($existingTmp) { $skipped++; continue; }
$tmpDiscount = $discountPct ? bcdiv(bcmul($tempRate, $discountPct, 4), '100', 2) : '0.00'; $tmpDiscount = $discountPct ? bcdiv(bcmul($tempRate, $discountPct, 4), '100', 2) : '0.00';
$tmpTotal = bcsub($tempRate, $tmpDiscount, 2); $tmpTotal = bcsub($tempRate, $tmpDiscount, 2);
$db->insert('subscriptions', [ try {
'member_id' => $memberId, $db->insert('subscriptions', [
'financial_year' => $financialYear, 'member_id' => $memberId,
'person_type' => 'temporary', 'financial_year' => $financialYear,
'person_id' => (int) $t['id'], 'person_type' => 'temporary',
'person_name' => $t['full_name_ar'], 'person_id' => (int) $t['id'],
'base_amount' => $tempRate, 'person_name' => $t['full_name_ar'],
'development_fee' => '0.00', 'base_amount' => $tempRate,
'discount_amount' => $tmpDiscount, 'development_fee' => '0.00',
'total_amount' => $tmpTotal, 'discount_amount' => $tmpDiscount,
'status' => 'pending', 'total_amount' => $tmpTotal,
'created_at' => $ts, 'status' => 'pending',
'updated_at' => $ts, 'created_at' => $ts,
'created_by' => $empId, 'updated_at' => $ts,
]); 'created_by' => $empId,
$created++; ]);
$created++;
} catch (\Throwable $e) {
if (str_contains($e->getMessage(), 'Duplicate entry')) { $skipped++; } else { throw $e; }
}
} }
} }
} }
......
app/Modules/Subscriptions/Services/SubscriptionSyncService.php
View file @ 5a5aba46
...@@ -46,8 +46,8 @@ final class SubscriptionSyncService ...@@ -46,8 +46,8 @@ final class SubscriptionSyncService
// ── Member's own subscription row ───────────────────────────── // ── Member's own subscription row ─────────────────────────────
$existing = $db->selectOne( $existing = $db->selectOne(
"SELECT id FROM subscriptions WHERE member_id = ? AND financial_year = ? AND person_type = 'member'", "SELECT id FROM subscriptions WHERE member_id = ? AND financial_year = ? AND person_type = 'member' AND (person_id = ? OR person_id IS NULL)",
[$memberId, $fy] [$memberId, $fy, $memberId]
); );
if (!$existing) { if (!$existing) {
$rateMap = [ $rateMap = [
...@@ -69,22 +69,30 @@ final class SubscriptionSyncService ...@@ -69,22 +69,30 @@ final class SubscriptionSyncService
$devFeeRule = RuleEngine::get('DEVELOPMENT_FEE'); $devFeeRule = RuleEngine::get('DEVELOPMENT_FEE');
$devFee = (string) ($devFeeRule['amount'] ?? '35.00'); $devFee = (string) ($devFeeRule['amount'] ?? '35.00');
$db->insert('subscriptions', [ try {
'member_id' => $memberId, $db->insert('subscriptions', [
'financial_year' => $fy, 'member_id' => $memberId,
'person_type' => 'member', 'financial_year' => $fy,
'person_id' => null, 'person_type' => 'member',
'person_name' => $member['full_name_ar'], 'person_id' => $memberId,
'base_amount' => $rate, 'person_name' => $member['full_name_ar'],
'development_fee' => $devFee, 'base_amount' => $rate,
'discount_amount' => $discount, 'development_fee' => $devFee,
'total_amount' => $total, 'discount_amount' => $discount,
'status' => 'pending', 'total_amount' => $total,
'created_at' => $ts, 'status' => 'pending',
'updated_at' => $ts, 'created_at' => $ts,
'created_by' => $empId, 'updated_at' => $ts,
]); 'created_by' => $empId,
Logger::info("SubscriptionSyncService: added member #{$memberId} to FY {$fy}"); ]);
Logger::info("SubscriptionSyncService: added member #{$memberId} to FY {$fy}");
} catch (\Throwable $e) {
if (str_contains($e->getMessage(), 'Duplicate entry')) {
Logger::info("SubscriptionSyncService: member #{$memberId} FY {$fy} already exists (race guard)");
} else {
throw $e;
}
}
} }
// ── All active dependents ───────────────────────────────────── // ── All active dependents ─────────────────────────────────────
...@@ -183,23 +191,30 @@ final class SubscriptionSyncService ...@@ -183,23 +191,30 @@ final class SubscriptionSyncService
$empId = $employee ? (int) $employee->id : null; $empId = $employee ? (int) $employee->id : null;
$ts = date('Y-m-d H:i:s'); $ts = date('Y-m-d H:i:s');
$db->insert('subscriptions', [ try {
'member_id' => $memberId, $db->insert('subscriptions', [
'financial_year' => $fy, 'member_id' => $memberId,
'person_type' => $personType, 'financial_year' => $fy,
'person_id' => $personId, 'person_type' => $personType,
'person_name' => $person['full_name_ar'], 'person_id' => $personId,
'base_amount' => $rate, 'person_name' => $person['full_name_ar'],
'development_fee' => '0.00', 'base_amount' => $rate,
'discount_amount' => $discount, 'development_fee' => '0.00',
'total_amount' => $total, 'discount_amount' => $discount,
'status' => 'pending', 'total_amount' => $total,
'created_at' => $ts, 'status' => 'pending',
'updated_at' => $ts, 'created_at' => $ts,
'created_by' => $empId, 'updated_at' => $ts,
]); 'created_by' => $empId,
]);
Logger::info("SubscriptionSyncService: added {$personType} #{$personId} to FY {$fy} for member #{$memberId}"); Logger::info("SubscriptionSyncService: added {$personType} #{$personId} to FY {$fy} for member #{$memberId}");
} catch (\Throwable $insertErr) {
if (str_contains($insertErr->getMessage(), 'Duplicate entry')) {
Logger::info("SubscriptionSyncService: {$personType} #{$personId} FY {$fy} already exists (race guard)");
} else {
throw $insertErr;
}
}
} catch (\Throwable $e) { } catch (\Throwable $e) {
Logger::error("SubscriptionSyncService::syncForDependent failed: " . $e->getMessage(), [ Logger::error("SubscriptionSyncService::syncForDependent failed: " . $e->getMessage(), [
'person_type' => $personType, 'person_type' => $personType,
......
database/migrations/Phase_97_001_remove_duplicate_subscriptions_add_unique_index.php 0 → 100644
View file @ 5a5aba46
<?php
declare(strict_types=1);
use App\Core\Database;
return function (Database $db): void {
// Phase 1: Remove duplicate subscription rows.
// For each (member_id, financial_year, person_type, person_id) group with more than one row,
// keep the "best" row (paid > exempt > overdue > pending) with the lowest ID as tiebreaker.
// Delete the rest.
$duplicates = $db->select("
SELECT member_id, financial_year, person_type, person_id, COUNT(*) as cnt
FROM subscriptions
GROUP BY member_id, financial_year, person_type, person_id
HAVING cnt > 1
");
$deleted = 0;
foreach ($duplicates as $dup) {
$personIdCondition = $dup['person_id'] === null
? 'person_id IS NULL'
: 'person_id = ' . (int) $dup['person_id'];
$rows = $db->select("
SELECT id, status, paid_amount, payment_id
FROM subscriptions
WHERE member_id = ? AND financial_year = ? AND person_type = ? AND {$personIdCondition}
ORDER BY
FIELD(status, 'paid', 'exempt', 'overdue', 'pending') ASC,
CASE WHEN paid_amount > 0 THEN 0 ELSE 1 END ASC,
id ASC
", [(int) $dup['member_id'], $dup['financial_year'], $dup['person_type']]);
// Keep the first row (best status), delete the rest
$keepId = (int) $rows[0]['id'];
foreach (array_slice($rows, 1) as $extra) {
$db->delete('subscriptions', 'id = ?', [(int) $extra['id']]);
$deleted++;
}
}
// Phase 2: Handle NULL person_id rows — normalize to avoid unique constraint issues.
// The SubscriptionGenerator sets person_id = member_id for person_type='member',
// but SubscriptionSyncService sets person_id = NULL. Normalize to match the generator.
$db->query("
UPDATE subscriptions
SET person_id = member_id
WHERE person_type = 'member' AND person_id IS NULL
");
// Phase 3: Re-check duplicates after normalization (NULL->member_id may have created new dupes)
$duplicates2 = $db->select("
SELECT member_id, financial_year, person_type, person_id, COUNT(*) as cnt
FROM subscriptions
GROUP BY member_id, financial_year, person_type, person_id
HAVING cnt > 1
");
foreach ($duplicates2 as $dup) {
$rows = $db->select("
SELECT id, status, paid_amount, payment_id
FROM subscriptions
WHERE member_id = ? AND financial_year = ? AND person_type = ? AND person_id = ?
ORDER BY
FIELD(status, 'paid', 'exempt', 'overdue', 'pending') ASC,
CASE WHEN paid_amount > 0 THEN 0 ELSE 1 END ASC,
id ASC
", [(int) $dup['member_id'], $dup['financial_year'], $dup['person_type'], (int) $dup['person_id']]);
$keepId = (int) $rows[0]['id'];
foreach (array_slice($rows, 1) as $extra) {
$db->delete('subscriptions', 'id = ?', [(int) $extra['id']]);
$deleted++;
}
}
// Phase 4: Add unique constraint to prevent future duplicates
$db->raw("
ALTER TABLE subscriptions
ADD UNIQUE INDEX uq_subscription_member_year_person (member_id, financial_year, person_type, person_id)
");
};
docs/architecture-maps/Subscriptions.md
View file @ 5a5aba46
...@@ -135,6 +135,20 @@ service codes. All 204 initially generated subscriptions had `base_amount=0.00`. ...@@ -135,6 +135,20 @@ service codes. All 204 initially generated subscriptions had `base_amount=0.00`.
2. Hard-coded fallback rates as last resort: member/spouse=492, child/temporary=222 2. Hard-coded fallback rates as last resort: member/spouse=492, child/temporary=222
3. All 204 bad rows manually corrected via UPDATE on production DB. 3. All 204 bad rows manually corrected via UPDATE on production DB.
**Fix applied 2026-07-21 (duplicate subscription rows):** Multiple root causes identified:
1. No DB-level UNIQUE constraint — race conditions between SubscriptionGenerator, SyncService, and
RetroactiveMembershipService could create duplicate rows.
2. SyncService set `person_id = NULL` for member rows while Generator set `person_id = $memberId` —
SELECT dedup guards wouldn't match across systems.
3. RetroactiveMembershipService had zero dedup checks (blind INSERT).
Fix:
- Migration Phase_97_001 removes all existing duplicates (keeps paid > overdue > pending, lowest ID tiebreak)
- Normalizes NULL person_id on member rows to member_id
- Adds UNIQUE INDEX on (member_id, financial_year, person_type, person_id)
- All insert paths now catch Duplicate entry exceptions as race-condition guard
- SyncService now sets person_id = memberId for member rows (matches Generator)
- RetroactiveMembershipService now checks for existing row before INSERT
### 5.1b Real-Time Member + Dependent Sync (SubscriptionSyncService) — Added 2026-07-18 ### 5.1b Real-Time Member + Dependent Sync (SubscriptionSyncService) — Added 2026-07-18
When a member activates or a dependent is added, subscription rows are automatically inserted When a member activates or a dependent is added, subscription rows are automatically inserted
...@@ -385,6 +399,8 @@ No subscriptions should exist before this year. ...@@ -385,6 +399,8 @@ No subscriptions should exist before this year.
- If interrupted mid-batch: some members have subscriptions, others don't - If interrupted mid-batch: some members have subscriptions, others don't
- No rollback mechanism for partial generation - No rollback mechanism for partial generation
- Multiple years can be generated (retroactive via manual batch-generate form) - Multiple years can be generated (retroactive via manual batch-generate form)
- **UNIQUE INDEX (member_id, financial_year, person_type, person_id)** prevents duplicates at DB level (added 2026-07-21)
- All insert paths wrap in try/catch for Duplicate entry — safe even under race conditions
### 13.2 Fine Calculation Consistency ### 13.2 Fine Calculation Consistency
- Fine is calculated on `total_amount` (full subscription), not remaining unpaid - Fine is calculated on `total_amount` (full subscription), not remaining unpaid
......
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