fix(db): RESTRICT delete of a Payment that has a FinancialBreakdown (H4)
The ledger FK used ON DELETE CASCADE on a non-nullable column, so deleting a Payment silently destroyed its immutable financial breakdown rows. Switch to RESTRICT — a settled payment can no longer be deleted out from under its ledger. Regression: tests/Settlement/FinancialBreakdownIntegrityTest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -40,7 +40,7 @@ _None outstanding._
|
||||
| ✅H1 | IDOR write: `createAppointment` trusts request `doctor_uuid`, no scope check — any staff books onto any doctor's calendar | src/Appointment/Controller/MyAppointmentsController.php:59 | security-idor | **DONE** — `canBookForDoctor()` scope gate + `tests/Appointment/BookingScopeTest` |
|
||||
| ✅H2 | No UNIQUE `(doctor_id, slot_start)` on Appointment → double-booking race (index is non-unique) | src/Appointment/Entity/Appointment.php | db-unique | **DONE** — nullable unique `active_slot_key` (occupying = pending/confirmed, mirrors `isSlotTaken`); `bookAtomically` catches the unique violation + expires lapsed pendings in-txn; all 3 booking paths (online/my/admin) routed through it. Migration backfills one row per slot (non-destructive). `tests/Appointment/SlotUniquenessTest`. **NB:** backfill surfaced a real pre-existing double-booked slot in dev data (doctor 1764, two `expired` rows) — harmless (both expired = key NULL). |
|
||||
| ✅H3 | `Payment.referenceId` not unique → same gateway callback credited twice | src/Payment/Entity/Payment.php:61-62 | db-unique | **DONE** — unique index on `reference_id` (NULL until success → no collision) + callback pre-check rejects replays. `tests/Payment/PaymentCallbackAmountTest::testReplayedGatewayReferenceIsRejected` |
|
||||
| H4 | `FinancialBreakdown.payment` onDelete CASCADE on non-nullable FK → deleting a Payment destroys ledger rows; should be RESTRICT | src/Settlement/Entity/FinancialBreakdown.php:28-30 | db-ondelete | Delete a Payment w/ breakdown → expect FK restrict error, ledger preserved |
|
||||
| ✅H4 | `FinancialBreakdown.payment` onDelete CASCADE on non-nullable FK → deleting a Payment destroys ledger rows; should be RESTRICT | src/Settlement/Entity/FinancialBreakdown.php:28-30 | db-ondelete | **DONE** — onDelete RESTRICT + migration. `tests/Settlement/FinancialBreakdownIntegrityTest` |
|
||||
| H5 | Insurance pricing/coverage modeled as raw int FKs (no FK/onDelete) → orphan rows on delete: `EntityInsurancePricing.entity_id/insurance_id`, `TenantInsurance.entity_id/insurance_id`, `TenantServiceCoverage.tenant_insurance_id` | src/Insurance/Entity/EntityInsurancePricing.php:25-29 · TenantInsurance.php:29,32 · TenantServiceCoverage.php:23-24 | db-ondelete | Delete insurance/tenant → children removed or restricted, no dangling rows |
|
||||
| H6 | N+1: clinic doctors list lazy-loads `specialties` per doctor | src/Clinic/Controller/ClinicController.php:326 · src/Doctor/Repository/DoctorRepository.php:49 | perf-nplus1 | SQL profiler on clinic doctors list → 1 query/doctor for specialties |
|
||||
| H7 | N+1: comments list lazy-loads `likes`→`user`, `replies` (recursive), author realName | src/Rating/Controller/RatingController.php:278,352 | perf-nplus1 | Profiler GET comments → query count scales w/ comments |
|
||||
|
||||
@@ -0,0 +1,33 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace DoctrineMigrations;
|
||||
|
||||
use Doctrine\DBAL\Schema\Schema;
|
||||
use Doctrine\Migrations\AbstractMigration;
|
||||
|
||||
/**
|
||||
* Auto-generated Migration: Please modify to your needs!
|
||||
*/
|
||||
final class Version20260628153855 extends AbstractMigration
|
||||
{
|
||||
public function getDescription(): string
|
||||
{
|
||||
return '';
|
||||
}
|
||||
|
||||
public function up(Schema $schema): void
|
||||
{
|
||||
// this up() migration is auto-generated, please modify it to your needs
|
||||
$this->addSql('ALTER TABLE financial_breakdowns DROP FOREIGN KEY `FK_D66D9FF44C3A3BB`');
|
||||
$this->addSql('ALTER TABLE financial_breakdowns ADD CONSTRAINT FK_D66D9FF44C3A3BB FOREIGN KEY (payment_id) REFERENCES payments (id) ON DELETE RESTRICT');
|
||||
}
|
||||
|
||||
public function down(Schema $schema): void
|
||||
{
|
||||
// this down() migration is auto-generated, please modify it to your needs
|
||||
$this->addSql('ALTER TABLE financial_breakdowns DROP FOREIGN KEY FK_D66D9FF44C3A3BB');
|
||||
$this->addSql('ALTER TABLE financial_breakdowns ADD CONSTRAINT `FK_D66D9FF44C3A3BB` FOREIGN KEY (payment_id) REFERENCES payments (id) ON DELETE CASCADE');
|
||||
}
|
||||
}
|
||||
@@ -26,7 +26,7 @@ class FinancialBreakdown
|
||||
private string $uuid;
|
||||
|
||||
#[ORM\ManyToOne(targetEntity: Payment::class)]
|
||||
#[ORM\JoinColumn(name: 'payment_id', referencedColumnName: 'id', nullable: false, onDelete: 'CASCADE')]
|
||||
#[ORM\JoinColumn(name: 'payment_id', referencedColumnName: 'id', nullable: false, onDelete: 'RESTRICT')]
|
||||
private Payment $payment;
|
||||
|
||||
#[ORM\Column(type: 'string', length: 20)]
|
||||
|
||||
@@ -0,0 +1,37 @@
|
||||
<?php
|
||||
|
||||
namespace App\Tests\Settlement;
|
||||
|
||||
use App\Payment\Entity\Payment;
|
||||
use App\Settlement\Entity\FinancialBreakdown;
|
||||
use App\Tests\ApiTestCase;
|
||||
use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException;
|
||||
|
||||
/**
|
||||
* A FinancialBreakdown is an immutable ledger row. Deleting the Payment it
|
||||
* settles must be blocked (ON DELETE RESTRICT) rather than silently cascading
|
||||
* the ledger row away.
|
||||
*/
|
||||
class FinancialBreakdownIntegrityTest extends ApiTestCase
|
||||
{
|
||||
public function testDeletingPaymentWithBreakdownIsRestricted(): void
|
||||
{
|
||||
$user = $this->createUser();
|
||||
$payment = new Payment($user, 100_000, 'mellat', Payment::TYPE_APPOINTMENT);
|
||||
$this->em->persist($payment);
|
||||
|
||||
$breakdown = new FinancialBreakdown(
|
||||
$payment,
|
||||
FinancialBreakdown::SOURCE_APPOINTMENT,
|
||||
$user,
|
||||
100_000, 0, '0.00', 0, 100_000, '10.00', 10_000, 90_000,
|
||||
null, null, null,
|
||||
);
|
||||
$this->em->persist($breakdown);
|
||||
$this->em->flush();
|
||||
|
||||
$this->em->remove($payment);
|
||||
$this->expectException(ForeignKeyConstraintViolationException::class);
|
||||
$this->em->flush();
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user