Fix cron cleanup leaving order/invoice data inconsistent (#4180)

🐛 Fix cron cleanup leaving order/invoice data inconsistent

onAfterAdminCronRun() removed overdue unpaid invoices with a raw
SQL DELETE, bypassing rmInvoice(). This left client_order.unpaid_
invoice_id pointing at a deleted invoice, orphaned invoice_item
rows, and skipped releasing reserved stock/promo redemptions.

Add InvoiceRepository::findUnpaidOlderThan() and route the cron
cleanup through rmInvoice() for each matching invoice so related
orders, invoice items, and reserved resources stay consistent.

Fixes #4178


Claude-Session: https://claude.ai/code/session_01KdphsdrmmWYBi7mFZK94QQ

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

src/modules/Invoice/Repository/InvoiceRepository.php

@@ -246,6 +246,25 @@ class InvoiceRepository extends EntityRepository
             ->getResult();
     }
 
+    /**
+     * Unpaid invoices whose due date is more than the given number of days
+     * in the past. Used by the cron cleanup that expires stale unpaid
+     * invoices.
+     *
+     * @return Invoice[]
+     */
+    public function findUnpaidOlderThan(int $days): array
+    {
+        return $this->createQueryBuilder('i')
+            ->andWhere('i.status = :status')
+            ->andWhere('i.dueAt IS NOT NULL')
+            ->andWhere('DATE_DIFF(CURRENT_TIMESTAMP(), i.dueAt) > :days')
+            ->setParameter('status', Invoice::STATUS_UNPAID)
+            ->setParameter('days', $days)
+            ->getQuery()
+            ->getResult();
+    }
+
     /**
      * Latest invoice that has a non-null nr — fallback for invoice
      * number generation.

src/modules/Invoice/Service.php

@@ -619,10 +619,16 @@ class Service implements InjectionAwareInterface
         $systemService = $di['mod_service']('System');
         $remove_after_days = $systemService->getParamValue('remove_after_days');
         if (isset($remove_after_days) && $remove_after_days) {
-            // removing old invoices
+            // removing old unpaid invoices, through rmInvoice() so related
+            // orders, invoice items, and reserved resources stay consistent
             $days = (int) $remove_after_days;
-            $sql = 'DELETE FROM invoice WHERE status = :status AND DATEDIFF(NOW(), due_at) > :days';
-            $di['em']->getConnection()->executeStatement($sql, ['days' => $days, 'status' => Invoice::STATUS_UNPAID]);
+            $service = $di['mod_service']('invoice');
+            $invoices = $service->getInvoiceRepository()->findUnpaidOlderThan($days);
+            foreach ($invoices as $invoiceModel) {
+                $id = $invoiceModel->getId();
+                $service->rmInvoice($invoiceModel);
+                $di['logger']->info('Removed expired unpaid invoice #{id}', ['id' => $id]);
+            }
         }
     }
 

src/modules/Invoice/tests/Unit/Repository/InvoiceRepositoryTest.php

@@ -242,3 +242,42 @@ test('getInvoiceTotals omits invoices without items', function (): void {
 
     expect($totals)->toBe([]);
 });
+
+test('findUnpaidOlderThan returns only unpaid invoices whose due date is far enough in the past', function (): void {
+    $config = ORMSetup::createAttributeMetadataConfig([Path::join(__DIR__, '..', '..', '..', 'Entity')], true);
+    $config->setProxyDir(sys_get_temp_dir());
+    $config->setProxyNamespace('FOSSBilling\\Tests\\DoctrineProxies');
+    $entityManager = new EntityManager(DriverManager::getConnection(['driver' => 'pdo_sqlite', 'memory' => true]), $config);
+
+    $metadata = array_map(
+        $entityManager->getClassMetadata(...),
+        [Invoice::class, InvoiceItem::class],
+    );
+    (new Doctrine\ORM\Tools\SchemaTool($entityManager))->createSchema($metadata);
+
+    $farOverdue = new Invoice();
+    $farOverdue->setStatus(Invoice::STATUS_UNPAID);
+    $farOverdue->setDueAt(new DateTime('-10 days'));
+    $entityManager->persist($farOverdue);
+
+    $recentlyOverdue = new Invoice();
+    $recentlyOverdue->setStatus(Invoice::STATUS_UNPAID);
+    $recentlyOverdue->setDueAt(new DateTime('-2 days'));
+    $entityManager->persist($recentlyOverdue);
+
+    $noDueDate = new Invoice();
+    $noDueDate->setStatus(Invoice::STATUS_UNPAID);
+    $entityManager->persist($noDueDate);
+
+    $paidButOverdue = new Invoice();
+    $paidButOverdue->setStatus(Invoice::STATUS_PAID);
+    $paidButOverdue->setDueAt(new DateTime('-10 days'));
+    $entityManager->persist($paidButOverdue);
+
+    $entityManager->flush();
+
+    $result = $entityManager->getRepository(Invoice::class)->findUnpaidOlderThan(5);
+
+    expect($result)->toHaveCount(1)
+        ->and($result[0]->getId())->toBe($farOverdue->getId());
+});

src/modules/Invoice/tests/Unit/ServiceTest.php

@@ -451,7 +451,6 @@ test('handles after admin invoice reminder sent event', function (): void {
 });
 
 test('handles after admin cron run event', function (): void {
-    $service = new Service();
     $eventMock = Mockery::mock('\Box_Event');
 
     $remove_after_days = 64;
@@ -461,20 +460,31 @@ test('handles after admin cron run event', function (): void {
         ->atLeast()->once()
         ->andReturn($remove_after_days);
 
-    $connection = Mockery::mock(Doctrine\DBAL\Connection::class);
-    $connection->shouldReceive('executeStatement')
-        ->atLeast()->once();
+    $invoiceModel = createEntity(Invoice::class, ['id' => 1]);
+
+    $invoiceServiceMock = Mockery::mock(Service::class)->makePartial()->shouldAllowMockingProtectedMethods();
+    $invoiceServiceMock->shouldReceive('rmInvoice')
+        ->once()
+        ->with($invoiceModel)
+        ->andReturn(true);
 
     $di = container();
-    $di['mod_service'] = $di->protect(fn (): Mockery\MockInterface => $systemServiceMock);
-    $di['em']->shouldReceive('getConnection')->andReturn($connection);
+    $di['em']->getRepository(Invoice::class)->shouldReceive('findUnpaidOlderThan')
+        ->with(64)
+        ->once()
+        ->andReturn([$invoiceModel]);
+    $di['mod_service'] = $di->protect(fn (string $name = ''): object => match (strtolower($name)) {
+        'system' => $systemServiceMock,
+        'invoice' => $invoiceServiceMock,
+        default => Mockery::mock()->shouldIgnoreMissing(),
+    });
 
-    $service->setDi($di);
+    $invoiceServiceMock->setDi($di);
     $eventMock->shouldReceive('getDi')
         ->atLeast()->once()
         ->andReturn($di);
 
-    $service->onAfterAdminCronRun($eventMock);
+    Service::onAfterAdminCronRun($eventMock);
 });
 
 test('uses the client billing email for invoice notifications', function (): void {

tests/Helpers/Container.php

@@ -273,6 +273,7 @@ function container(): Container
         $invoiceRepository->shouldReceive('findPaid')->byDefault()->andReturn([]);
         $invoiceRepository->shouldReceive('findByClientId')->byDefault()->andReturn([]);
         $invoiceRepository->shouldReceive('findUnpaidApprovedNotRemindedBefore')->byDefault()->andReturn([]);
+        $invoiceRepository->shouldReceive('findUnpaidOlderThan')->byDefault()->andReturn([]);
         $invoiceRepository->shouldReceive('findPaidByRelId')->byDefault()->andReturn([]);
 
         $invoiceItemRepository = \Mockery::mock(\Box\Mod\Invoice\Repository\InvoiceItemRepository::class)->shouldIgnoreMissing();