From 8a6ce7606f227d9c5a0ffd7697b3f7b5615b3731 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= <bjoern.fromme@ep-reisen.de>
Date: Tue, 29 Sep 2026 16:18:50 +0200
Subject: [PATCH] feat: add a cleanup command for accounts without recent
 activity

---
 src/Command/CleanupUsersCommand.php       | 199 +++++++++++++++++++++
 src/Repository/UserRepository.php         |  88 ++++++++++
 tests/Command/CleanupUsersCommandTest.php | 201 ++++++++++++++++++++++
 3 files changed, 488 insertions(+)
 create mode 100644 src/Command/CleanupUsersCommand.php
 create mode 100644 tests/Command/CleanupUsersCommandTest.php

diff --git a/src/Command/CleanupUsersCommand.php b/src/Command/CleanupUsersCommand.php
new file mode 100644
index 00000000..e05732fa
--- /dev/null
+++ b/src/Command/CleanupUsersCommand.php
@@ -0,0 +1,199 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Command;
+
+use App\Entity\User;
+use App\Repository\UserRepository;
+use App\Security\Role;
+use Doctrine\ORM\EntityManagerInterface;
+use Psr\Log\LoggerInterface;
+use Symfony\Component\Console\Attribute\AsCommand;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Input\InputOption;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Console\Style\SymfonyStyle;
+
+/**
+ * Removes local accounts that carry nothing worth keeping.
+ *
+ * Nothing here destroys customer data: BusPro owns the person, the bookings and the roles, and
+ * BpnAuthenticator rebuilds the local row on the next login (`findOneBy(email) ?? new User()`),
+ * re-deriving the roles, the hotel codes and the profile completeness from the CRM as it goes.
+ * An account is a cache of what BusPro says, so dropping one costs its owner nothing.
+ *
+ * One thing is not rebuilt, and it is the reason every removal gets a log line of its own: the
+ * uuid, minted in the constructor and exported as the OAuth2 `sub` claim (see UserinfoController).
+ * A returning account gets a fresh one, so the old subject cannot be tied to the new row by
+ * anything except that log entry. The SSO clients that gate on a role — E&P Team, E&P Hub — never
+ * see these accounts anyway, since the roles they require are exactly the ones excluded below.
+ *
+ * An account is removed when all of the following hold:
+ *
+ *  - it holds no effective role beyond ROLE_CUSTOMER, that role being the fallback for an account
+ *    with no privileges at all (see Role::withCustomerFallback()). An account left with no roles
+ *    at all is the same state, spelled the way it was before the fallback landed;
+ *  - it carries no pending nomination, which would otherwise disappear from /admin/user before
+ *    anybody had ruled on it;
+ *  - it holds no booking edit draft, and nothing in the Groups subsystem names it.
+ *
+ * --inactive-for adds a "has not signed in for N months" condition on top. It is off by default:
+ * a returning customer is recreated on sight, so age says nothing about whether a row may go.
+ *
+ * Deleting requires --force. The removal cannot be undone and the first run clears thousands of
+ * rows, so a bare invocation reports what it would do and stops. The guard is a flag rather than
+ * an $io->confirm() because the scheduler hands the command a StringInput, which reports itself
+ * as interactive — a confirmation would sit waiting on a STDIN nobody is attached to. A flag
+ * behaves the same way whether a person or cron is running it, and it makes the schedule entry
+ * say out loud that the task destroys data.
+ */
+#[AsCommand(
+    name: 'app:cleanup:users',
+    description: 'Removes accounts that hold no role, no draft and no group booking reference',
+)]
+class CleanupUsersCommand extends Command
+{
+    /**
+     * How many accounts are removed between two flushes. The unit of work holds every scheduled
+     * removal until then, and the first run has thousands of rows to get through, so it is a
+     * memory bound rather than a policy one.
+     */
+    private const BATCH_SIZE = 500;
+
+    public function __construct(
+        private readonly UserRepository $userRepository,
+        private readonly EntityManagerInterface $entityManager,
+        private readonly LoggerInterface $logger,
+    ) {
+        parent::__construct();
+    }
+
+    protected function configure(): void
+    {
+        $this
+            ->addOption(
+                'inactive-for',
+                null,
+                InputOption::VALUE_REQUIRED,
+                'Only consider accounts without a login for this many months (default: no such limit)',
+            )
+            ->addOption(
+                'limit',
+                null,
+                InputOption::VALUE_REQUIRED,
+                'Maximum number of accounts to delete in one run (default: no limit)',
+            )
+            ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Show what would be deleted without actually deleting')
+            ->addOption('force', null, InputOption::VALUE_NONE, 'Actually delete; without it the command only reports what it would do')
+        ;
+    }
+
+    protected function execute(InputInterface $input, OutputInterface $output): int
+    {
+        $io = new SymfonyStyle($input, $output);
+
+        $months = null !== $input->getOption('inactive-for') ? (int) $input->getOption('inactive-for') : null;
+        $limit = null !== $input->getOption('limit') ? (int) $input->getOption('limit') : null;
+        $dryRun = (bool) $input->getOption('dry-run');
+        $force = (bool) $input->getOption('force');
+
+        if ((null !== $months && $months < 1) || (null !== $limit && $limit < 1)) {
+            $io->error('--inactive-for and --limit have to be at least 1 when given.');
+
+            return Command::INVALID;
+        }
+
+        $inactiveSince = null !== $months ? new \DateTimeImmutable(sprintf('-%d months', $months)) : null;
+
+        $candidates = array_values(array_filter(
+            $this->userRepository->findCleanupCandidates($inactiveSince),
+            static fn (User $user): bool => self::isDisposable($user),
+        ));
+
+        // After the role filter, never before it: --limit caps the accounts actually deleted, and
+        // the candidate query cannot tell which ones those are.
+        if (null !== $limit) {
+            $candidates = \array_slice($candidates, 0, $limit);
+        }
+
+        if ([] === $candidates) {
+            $io->success('No accounts to remove.');
+
+            return Command::SUCCESS;
+        }
+
+        if (true === $dryRun || false === $force) {
+            $io->table(
+                ['E-Mail', 'Letzter Login', 'Rollen'],
+                array_map(static fn (User $user): array => [
+                    $user->getEmail(),
+                    $user->getLastLoginAt()?->format('Y-m-d') ?? '-',
+                    implode(', ', Role::assignedOnly($user->getRoles())) ?: '-',
+                ], $candidates),
+            );
+
+            $io->note(sprintf('Would delete %d account(s).', \count($candidates)));
+
+            if (true === $dryRun) {
+                return Command::SUCCESS;
+            }
+
+            // Not a dry run, just an invocation that forgot to say so. Reported as a failure so a
+            // schedule entry missing --force turns into a mail rather than a task that quietly
+            // does nothing every week.
+            $io->error('Refusing to delete without --force. Pass --dry-run to preview, or --force to delete.');
+
+            return Command::INVALID;
+        }
+
+        foreach ($candidates as $index => $user) {
+            // One line per account, before the removal rather than after: once the row is gone
+            // this is the only place left that records which account the uuid belonged to. Worth
+            // a log_entry row per deletion, including the several thousand the first run writes —
+            // they age out with the six-month log retention.
+            $this->logger->info('Deleting account without roles', [
+                'uuid' => $user->getUuid(),
+                'email' => $user->getEmail(),
+                'lastLoginAt' => $user->getLastLoginAt()?->format(\DateTimeInterface::ATOM),
+                'roles' => Role::assignedOnly($user->getRoles()),
+            ]);
+
+            $this->entityManager->remove($user);
+
+            if (0 === ($index + 1) % self::BATCH_SIZE) {
+                $this->entityManager->flush();
+            }
+        }
+
+        $this->entityManager->flush();
+
+        $count = \count($candidates);
+
+        $this->logger->info('Deleted accounts without roles', [
+            'count' => $count,
+            'inactiveSince' => $inactiveSince?->format('Y-m-d'),
+        ]);
+
+        $io->success(sprintf('Deleted %d account(s).', $count));
+
+        return Command::SUCCESS;
+    }
+
+    /**
+     * Whether the account's role set says it may go. ROLE_CUSTOMER is the fallback rather than a
+     * baseline, so "customer only" and "nothing at all" are the same state — the second being the
+     * spelling accounts kept that have not signed in since the fallback landed.
+     *
+     * Pending markers grant nothing, but they are a nomination waiting for an administrator, so an
+     * account carrying one stays until somebody has ruled on it.
+     */
+    private static function isDisposable(User $user): bool
+    {
+        $roles = $user->getRoles();
+
+        return [] === Role::pendingOnly($roles)
+            && [] === array_diff(Role::effectiveOnly($roles), [Role::CUSTOMER]);
+    }
+}
diff --git a/src/Repository/UserRepository.php b/src/Repository/UserRepository.php
index 2d4ed577..b142c6f9 100644
--- a/src/Repository/UserRepository.php
+++ b/src/Repository/UserRepository.php
@@ -4,7 +4,12 @@ declare(strict_types=1);
 
 namespace App\Repository;
 
+use App\Entity\BookingEditDraft;
+use App\Entity\Groups\Accommodation;
 use App\Entity\Groups\AccommodationBooking;
+use App\Entity\Groups\AccommodationPrice;
+use App\Entity\Groups\AdditionalService;
+use App\Entity\Groups\BoardService;
 use App\Entity\User;
 use App\Form\Model\Filter\UserFilterDto;
 use App\Repository\Filter\AppliesListFiltersTrait;
@@ -122,4 +127,87 @@ class UserRepository extends ServiceEntityRepository
 
         return $users;
     }
+
+    /**
+     * The entities that point at a User and would silently lose that pointer if the account were
+     * deleted. Every one of these join columns is ON DELETE SET NULL, so the database does not
+     * stand in the way — it just erases who created, last touched or looks after a group booking.
+     * The cleanup therefore has to ask, rather than let the constraint decide.
+     *
+     * @var array<class-string, string[]>
+     */
+    private const USER_REFERENCES = [
+        Accommodation::class => ['createdBy', 'updatedBy'],
+        AccommodationPrice::class => ['createdBy', 'updatedBy'],
+        AccommodationBooking::class => ['createdBy', 'updatedBy', 'managedBy'],
+        AdditionalService::class => ['createdBy', 'updatedBy'],
+        BoardService::class => ['createdBy', 'updatedBy'],
+    ];
+
+    /**
+     * Accounts the cleanup may consider: holding no booking edit draft and named by nothing in the
+     * Groups subsystem. $inactiveSince narrows that to accounts dormant since then, and is the
+     * caller's choice rather than this method's default — BpnAuthenticator recreates an account on
+     * sight, so how long ago somebody last signed in says nothing about whether the row may go.
+     *
+     * Deliberately unbounded: the caller still has to drop everything whose roles disqualify it,
+     * so a LIMIT here would cap the rows examined rather than the rows acted on, and a caller
+     * asking for ten deletions would get however many of the first ten candidates survived the
+     * role filter. Ordered oldest login first so a caller that does cap the result takes the
+     * least recently seen accounts.
+     *
+     * The roles are deliberately not part of this query. They live in a JSON array column with no
+     * JSON_CONTAINS registered, and the column holds more than one spelling of the same set — rows
+     * written by different Doctrine versions differ in whitespace — so a LIKE or an equality test
+     * on the raw JSON would quietly miss accounts or, far worse, match the wrong ones. The caller
+     * filters the shortlist through Role::effectiveOnly(), which is the only thing that knows what
+     * a role set actually means.
+     *
+     * @return User[]
+     */
+    public function findCleanupCandidates(?\DateTimeImmutable $inactiveSince = null): array
+    {
+        $qb = $this->createQueryBuilder('u');
+
+        if (null !== $inactiveSince) {
+            $qb
+                ->andWhere('u.lastLoginAt IS NOT NULL')
+                ->andWhere('u.lastLoginAt < :inactiveSince')
+                ->setParameter('inactiveSince', $inactiveSince)
+            ;
+        }
+
+        $qb->andWhere($qb->expr()->not($qb->expr()->exists(
+            $this->getEntityManager()->createQueryBuilder()
+                ->select('draft.id')
+                ->from(BookingEditDraft::class, 'draft')
+                ->andWhere('draft.user = u')
+                ->getDQL()
+        )));
+
+        $index = 0;
+
+        foreach (self::USER_REFERENCES as $class => $fields) {
+            foreach ($fields as $field) {
+                $alias = sprintf('ref%d%s', $index++, $field);
+
+                $qb->andWhere($qb->expr()->not($qb->expr()->exists(
+                    $this->getEntityManager()->createQueryBuilder()
+                        ->select($alias.'.id')
+                        ->from($class, $alias)
+                        ->andWhere(sprintf('%s.%s = u', $alias, $field))
+                        ->getDQL()
+                )));
+            }
+        }
+
+        /** @var User[] $users */
+        $users = $qb
+            ->orderBy('u.lastLoginAt', 'ASC')
+            ->getQuery()
+            ->getResult()
+        ;
+
+        return $users;
+    }
 }
diff --git a/tests/Command/CleanupUsersCommandTest.php b/tests/Command/CleanupUsersCommandTest.php
new file mode 100644
index 00000000..334ca541
--- /dev/null
+++ b/tests/Command/CleanupUsersCommandTest.php
@@ -0,0 +1,201 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Command;
+
+use App\Command\CleanupUsersCommand;
+use App\Entity\User;
+use App\Repository\UserRepository;
+use App\Security\Role;
+use Doctrine\ORM\EntityManagerInterface;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\LoggerInterface;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Tester\CommandTester;
+
+class CleanupUsersCommandTest extends TestCase
+{
+    public function testDeletesAccountsWithoutAnEffectiveRole(): void
+    {
+        $customer = $this->user('customer@example.test', [Role::CUSTOMER]);
+        $roleless = $this->user('legacy@example.test', []);
+
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $removed = [];
+        $entityManager->expects(self::exactly(2))
+            ->method('remove')
+            ->willReturnCallback(static function (User $user) use (&$removed): void {
+                $removed[] = $user->getEmail();
+            });
+        $entityManager->expects(self::once())->method('flush');
+
+        $tester = new CommandTester($this->command([$customer, $roleless], $entityManager));
+        $tester->execute(['--force' => true]);
+
+        self::assertSame(Command::SUCCESS, $tester->getStatusCode());
+        self::assertSame(['customer@example.test', 'legacy@example.test'], $removed);
+        self::assertStringContainsString('Deleted 2 account(s).', $tester->getDisplay());
+    }
+
+    /**
+     * The candidate query cannot read the roles, so everything the role policy protects has to
+     * survive this filter — including an account whose only effective role sits beside the
+     * ROLE_CUSTOMER fallback that older rows still carry.
+     *
+     * @param string[] $roles
+     */
+    #[DataProvider('protectedRoleSets')]
+    public function testKeepsAccountsHoldingAnythingBeyondTheCustomerFallback(array $roles): void
+    {
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager->expects(self::never())->method('remove');
+
+        $tester = new CommandTester($this->command([$this->user('kept@example.test', $roles)], $entityManager));
+        $tester->execute(['--force' => true]);
+
+        self::assertSame(Command::SUCCESS, $tester->getStatusCode());
+        self::assertStringContainsString('No accounts to remove.', $tester->getDisplay());
+    }
+
+    /**
+     * @return iterable<string, array{string[]}>
+     */
+    public static function protectedRoleSets(): iterable
+    {
+        yield 'teamer' => [[Role::TEAMER]];
+        yield 'employee' => [[Role::EMPLOYEE]];
+        yield 'approved administrative role' => [[Role::TEAMER, Role::MANAGER]];
+        yield 'house manager' => [[Role::HOUSE_MANAGER]];
+        yield 'pending nomination awaiting approval' => [[Role::CUSTOMER, Role::pending(Role::ADMIN)]];
+        yield 'stale fallback beside a real role' => [[Role::CUSTOMER, Role::TEAMER]];
+    }
+
+    /**
+     * The whole point of the guard: a bare invocation is a preview, not a deletion. Symfony's
+     * VALUE_REQUIRED only says an option needs a value when it is given, so nothing in the
+     * definition ever forces the caller to state an intent — this does.
+     */
+    public function testRefusesToDeleteWithoutForce(): void
+    {
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager->expects(self::never())->method('remove');
+        $entityManager->expects(self::never())->method('flush');
+
+        $tester = new CommandTester($this->command([$this->user('customer@example.test', [Role::CUSTOMER])], $entityManager));
+        $tester->execute([]);
+
+        self::assertSame(Command::INVALID, $tester->getStatusCode());
+        self::assertStringContainsString('Would delete 1 account(s).', $tester->getDisplay());
+        self::assertStringContainsString('Refusing to delete without --force', $tester->getDisplay());
+    }
+
+    public function testDryRunDeletesNothing(): void
+    {
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager->expects(self::never())->method('remove');
+        $entityManager->expects(self::never())->method('flush');
+
+        $tester = new CommandTester($this->command([$this->user('customer@example.test', [Role::CUSTOMER])], $entityManager));
+        $tester->execute(['--dry-run' => true]);
+
+        self::assertSame(Command::SUCCESS, $tester->getStatusCode());
+        self::assertStringContainsString('Would delete 1 account(s).', $tester->getDisplay());
+        self::assertStringNotContainsString('Refusing to delete', $tester->getDisplay());
+        self::assertStringContainsString('customer@example.test', $tester->getDisplay());
+    }
+
+    /**
+     * --limit caps the deletions, not the candidates examined, so a run that finds one deletable
+     * account among protected ones still deletes it rather than spending the budget on rows it
+     * then discards.
+     */
+    public function testLimitCountsDeletionsRatherThanCandidates(): void
+    {
+        $users = [
+            $this->user('teamer@example.test', [Role::TEAMER]),
+            $this->user('customer@example.test', [Role::CUSTOMER]),
+        ];
+
+        $entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager->expects(self::once())
+            ->method('remove')
+            ->with(self::callback(static fn (User $user): bool => 'customer@example.test' === $user->getEmail()));
+
+        $tester = new CommandTester($this->command($users, $entityManager));
+        $tester->execute(['--limit' => 1, '--force' => true]);
+
+        self::assertSame(Command::SUCCESS, $tester->getStatusCode());
+        self::assertStringContainsString('Deleted 1 account(s).', $tester->getDisplay());
+    }
+
+    public function testInactivityWindowIsOffUnlessAskedFor(): void
+    {
+        $repository = $this->createMock(UserRepository::class);
+        $repository->expects(self::once())
+            ->method('findCleanupCandidates')
+            ->with(null)
+            ->willReturn([]);
+
+        $tester = new CommandTester($this->commandWith($repository, $this->createStub(EntityManagerInterface::class)));
+        $tester->execute(['--force' => true]);
+
+        self::assertSame(Command::SUCCESS, $tester->getStatusCode());
+    }
+
+    public function testInactivityWindowIsPassedOnAsADate(): void
+    {
+        $repository = $this->createMock(UserRepository::class);
+        $repository->expects(self::once())
+            ->method('findCleanupCandidates')
+            ->with(self::callback(static function (?\DateTimeImmutable $since): bool {
+                $expected = new \DateTimeImmutable('-6 months');
+
+                return null !== $since && $since->format('Y-m-d') === $expected->format('Y-m-d');
+            }))
+            ->willReturn([]);
+
+        $tester = new CommandTester($this->commandWith($repository, $this->createStub(EntityManagerInterface::class)));
+        $tester->execute(['--inactive-for' => 6, '--force' => true]);
+
+        self::assertSame(Command::SUCCESS, $tester->getStatusCode());
+    }
+
+    public function testRejectsANonPositiveWindow(): void
+    {
+        $repository = $this->createMock(UserRepository::class);
+        $repository->expects(self::never())->method('findCleanupCandidates');
+
+        $tester = new CommandTester($this->commandWith($repository, $this->createStub(EntityManagerInterface::class)));
+        $tester->execute(['--inactive-for' => 0]);
+
+        self::assertSame(Command::INVALID, $tester->getStatusCode());
+    }
+
+    /**
+     * @param string[] $roles
+     */
+    private function user(string $email, array $roles): User
+    {
+        return (new User($email))
+            ->setRoles($roles)
+            ->setLastLoginAt(new \DateTimeImmutable('-2 years'));
+    }
+
+    /**
+     * @param User[] $candidates
+     */
+    private function command(array $candidates, EntityManagerInterface $entityManager): CleanupUsersCommand
+    {
+        $repository = $this->createStub(UserRepository::class);
+        $repository->method('findCleanupCandidates')->willReturn($candidates);
+
+        return $this->commandWith($repository, $entityManager);
+    }
+
+    private function commandWith(UserRepository $repository, EntityManagerInterface $entityManager): CleanupUsersCommand
+    {
+        return new CleanupUsersCommand($repository, $entityManager, $this->createStub(LoggerInterface::class));
+    }
+}