feat: warn about overlapping bookings instead of refusing them

This commit is contained in:
fromme committed 2026-09-18 09:40:14 +02:00
1 parent fae28b1fd7
commit 5a6db29205
8 files changed
+359 -71

No files matched your search

+6 -3
View File
@@ -7,10 +7,10 @@ namespace App\Controller\Booking;
use App\Config\BookingHorizon;
use App\Controller\Traits\BoardResponseTrait;
use App\Controller\Traits\CurrentUserTrait;
use App\Controller\Traits\OverlapWarningTrait;
use App\Entity\BookingSeries;
use App\Entity\Resource;
use App\Enum\RecurrenceRule;
use App\Exception\AlreadyHoldingResourceException;
use App\Exception\ResourceAlreadyBookedException;
use App\Form\BookingRequest;
use App\Form\BookingType;
@@ -34,6 +34,7 @@ final class CreateController extends AbstractController
use BoardResponseTrait;
use CurrentUserTrait;
use HxTrait;
use OverlapWarningTrait;
public function __construct(
private readonly BoardContextFactory $boardContext,
@@ -91,12 +92,14 @@ final class CreateController extends AbstractController
: null;
if (null === $result) {
$this->manager->book($this->currentUser(), $resource, $data->date, $data->timeRange());
$booking = $this->manager->book($this->currentUser(), $resource, $data->date, $data->timeRange());
$this->addFlash('success', \sprintf('%s gebucht (%s).', $resource->getLabel(), $data->timeRange()));
$this->warnAboutOverlaps($booking);
} else {
$this->flashSeriesResult($result, $resource);
$this->warnAboutSeriesOverlaps($result);
}
} catch (AlreadyHoldingResourceException|ResourceAlreadyBookedException $e) {
} catch (ResourceAlreadyBookedException $e) {
return $this->invalidModal($form, $resource, $e->getMessage());
}
+7 -2
View File
@@ -6,12 +6,13 @@ namespace App\Controller\Booking;
use App\Controller\Traits\BoardResponseTrait;
use App\Controller\Traits\CurrentUserTrait;
use App\Controller\Traits\OverlapWarningTrait;
use App\Entity\Booking;
use App\Exception\AlreadyHoldingResourceException;
use App\Exception\ResourceAlreadyBookedException;
use App\Form\BookingRequest;
use App\Form\BookingType;
use App\Htmx\HxTrait;
use App\Repository\BookingRepository;
use App\Service\BoardContextFactory;
use App\Service\BookingManager;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -26,10 +27,12 @@ final class EditController extends AbstractController
use BoardResponseTrait;
use CurrentUserTrait;
use HxTrait;
use OverlapWarningTrait;
public function __construct(
private readonly BoardContextFactory $boardContext,
private readonly BookingManager $manager,
private readonly BookingRepository $bookings,
) {
}
@@ -58,11 +61,13 @@ final class EditController extends AbstractController
if ($form->isSubmitted() && $form->isValid()) {
try {
$this->manager->reschedule($booking, $data->timeRange());
} catch (AlreadyHoldingResourceException|ResourceAlreadyBookedException $e) {
} catch (ResourceAlreadyBookedException $e) {
return $this->invalidModal($form, $booking, $e->getMessage());
}
$this->addFlash('success', \sprintf('%s geändert (%s).', $booking->getResource()->getLabel(), $booking->getTimeRange()));
// Moving a booking can create an overlap that was not there when it was made.
$this->warnAboutOverlaps($booking);
return $this->boardResponse($booking->getResource()->getType(), $booking->getDate(), $boardTime);
}
@@ -0,0 +1,157 @@
<?php
declare(strict_types=1);
namespace App\Controller\Traits;
use App\Entity\Booking;
use App\Model\SeriesResult;
/**
* The hint shown after somebody books a second resource of the same kind for a span they already
* hold one for.
*
* Holding two desks at once is allowed -- one of them is usually for the customer you are
* meeting -- so this is not a guard and never refuses anything; BookingManager explains what is
* still refused. It only says out loud what just happened, because the alternative is finding
* out on the day.
*
* Only same-kind overlaps are reported. A desk next to a parking space next to a monitor is the
* everyday combination, and a toast on every one of those would train everybody to ignore the
* toast that matters.
*
* Expects the using controller to extend AbstractController and to inject
* App\Repository\BookingRepository as $bookings.
*/
trait OverlapWarningTrait
{
private function warnAboutOverlaps(Booking $booking): void
{
$clashes = $this->bookings->findOverlappingForUser(
$booking->getUser(),
$booking->getDate(),
$booking->getTimeRange(),
$booking->getResource()->getType(),
$booking,
);
if ([] === $clashes) {
return;
}
$day = $booking->getDate()->format('d.m.Y');
if (1 === \count($clashes)) {
$this->addFlash('warning', \sprintf(
'Hinweis: Du hast am %s zur gleichen Zeit bereits %s gebucht (%s).',
$day,
$clashes[0]->getResource()->getLabel(),
$clashes[0]->getTimeRange(),
));
return;
}
$this->addFlash('warning', \sprintf(
'Hinweis: Du hast am %s zur gleichen Zeit bereits %d weitere %s gebucht: %s.',
$day,
\count($clashes),
$booking->getResource()->getType()->plural(),
implode(', ', self::labelsOf($clashes)),
));
}
/**
* The same hint for a whole series, aggregated into one flash -- a weekly series running to
* the horizon must not fire a toast per occurrence.
*/
private function warnAboutSeriesOverlaps(SeriesResult $result): void
{
if ($result->isCompletelyBlocked()) {
return;
}
$series = $result->series;
$created = $result->created;
$type = $series->getResource()->getType();
$clashes = $this->bookings->findOverlappingForUserInRange(
$series->getUser(),
$created[0]->getDate(),
$created[array_key_last($created)]->getDate(),
$series->getTimeRange(),
$type,
$series,
);
// One query covers the whole window, so it also returns days between two occurrences --
// days this series never landed on. Only the ones it did are news.
$days = [];
foreach ($clashes as $clash) {
$days[$clash->getDate()->format('Y-m-d')][] = $clash;
}
$days = array_intersect_key($days, array_flip(array_map(
static fn (Booking $booking): string => $booking->getDate()->format('Y-m-d'),
$created,
)));
if ([] === $days) {
return;
}
if (1 === \count($days)) {
$date = array_key_first($days);
$this->addFlash('warning', \sprintf(
'Hinweis: Am %s hast du zur gleichen Zeit bereits %s gebucht.',
(new \DateTimeImmutable($date))->format('j.n.'),
implode(', ', self::labelsOf($days[$date])),
));
return;
}
$this->addFlash('warning', \sprintf(
'Hinweis: An %d von %d Terminen hast du zur gleichen Zeit bereits andere %s gebucht (%s).',
\count($days),
\count($created),
$type->plural(),
self::shortDates(array_keys($days)),
));
}
/**
* @param list<Booking> $bookings
*
* @return list<string>
*/
private static function labelsOf(array $bookings): array
{
return array_values(array_unique(array_map(
static fn (Booking $booking): string => $booking->getResource()->getLabel(),
$bookings,
)));
}
/**
* Cut off after a handful: a series can clash on two dozen days, and a toast listing them all
* would cover the board it is reporting on.
*
* @param list<string> $dates Y-m-d
*/
private static function shortDates(array $dates): string
{
$shown = \array_slice($dates, 0, 5);
$formatted = array_map(
static fn (string $date): string => (new \DateTimeImmutable($date))->format('j.n.'),
$shown,
);
if (\count($dates) > \count($shown)) {
$formatted[] = '…';
}
return implode(', ', $formatted);
}
}
@@ -1,26 +0,0 @@
<?php
declare(strict_types=1);
namespace App\Exception;
use App\Entity\Booking;
/**
* One person may not hold two resources of the same kind at overlapping times. A desk in the
* morning and a different one in the afternoon is fine -- only the overlap is refused.
*/
final class AlreadyHoldingResourceException extends \RuntimeException
{
public function __construct(public readonly Booking $existing)
{
$time = $existing->getTimeRange();
parent::__construct(\sprintf(
'Du hast am %s bereits %s gebucht (%s).',
$existing->getDate()->format('d.m.Y'),
$existing->getResource()->getLabel(),
$time,
));
}
}
+72 -15
View File
@@ -75,9 +75,16 @@ class BookingRepository extends ServiceEntityRepository
}
/**
* One person may not hold two resources of the same kind at the same time -- but they may
* well hold a desk in the morning and a different one in the afternoon, so this is an
* overlap test rather than a per-day test.
* What else the same person already holds of the same kind in that span.
*
* This used to be a guard -- one desk per person at a time -- and is now the input to a
* hint: holding two desks at once is a real thing people do, a second one for a customer
* they are meeting. Nothing here refuses anything, so it returns every clash rather than
* the first one, and it is deliberately limited to the same kind: a desk next to a parking
* space next to a monitor is the everyday combination, and warning about that would train
* everybody to ignore the warning that matters.
*
* @return list<Booking>
*/
public function findOverlappingForUser(
User $user,
@@ -85,25 +92,57 @@ class BookingRepository extends ServiceEntityRepository
TimeRange $time,
ResourceType $type,
?Booking $ignore = null,
): ?Booking {
$qb = $this->createQueryBuilder('b')
->addSelect('r')
->join('b.resource', 'r')
->andWhere('b.user = :user')
): array {
$qb = self::restrictToUserOverlap(
$this->createQueryBuilder('b'),
$user,
$time,
$type,
)
->andWhere('b.date = :date')
->andWhere('r.type = :type')
->setParameter('user', $user)
->setParameter('date', $date->setTime(0, 0))
->setParameter('type', $type)
->setMaxResults(1);
self::restrictToOverlap($qb, $time);
->addOrderBy('r.label', 'ASC');
if (null !== $ignore && null !== $ignore->getId()) {
$qb->andWhere('b.id != :ignore')->setParameter('ignore', $ignore->getId());
}
return $qb->getQuery()->getOneOrNullResult();
return $qb->getQuery()->getResult();
}
/**
* The same hint across a span of days, which is what a series needs -- one query rather than
* one per occurrence, so a weekly series running to the horizon stays a single round trip.
*
* Occurrences of $exclude are left out: a series overlapping itself is not news.
*
* @return list<Booking>
*/
public function findOverlappingForUserInRange(
User $user,
\DateTimeImmutable $from,
\DateTimeImmutable $to,
TimeRange $time,
ResourceType $type,
?BookingSeries $exclude = null,
): array {
$qb = self::restrictToUserOverlap(
$this->createQueryBuilder('b'),
$user,
$time,
$type,
)
->andWhere('b.date BETWEEN :from AND :to')
->setParameter('from', $from->setTime(0, 0))
->setParameter('to', $to->setTime(0, 0))
->orderBy('b.date', 'ASC')
->addOrderBy('r.label', 'ASC');
if (null !== $exclude) {
$qb->andWhere('b.series IS NULL OR b.series != :exclude')->setParameter('exclude', $exclude);
}
return $qb->getQuery()->getResult();
}
/**
@@ -237,6 +276,24 @@ class BookingRepository extends ServiceEntityRepository
->getResult();
}
/**
* What the two per-user overlap finders share: one person, one kind, spans that collide.
* The resource is eager-selected because both callers name it in the message they build.
*/
private static function restrictToUserOverlap(QueryBuilder $qb, User $user, TimeRange $time, ResourceType $type): QueryBuilder
{
$qb->addSelect('r')
->join('b.resource', 'r')
->andWhere('b.user = :user')
->andWhere('r.type = :type')
->setParameter('user', $user)
->setParameter('type', $type);
self::restrictToOverlap($qb, $time);
return $qb;
}
/**
* Half-open overlap, the same test as TimeRange::overlaps(): two spans collide unless one
* ends at or before the other begins. Written once here so the SQL and the PHP cannot drift.
+11 -17
View File
@@ -8,7 +8,6 @@ use App\Entity\Booking;
use App\Entity\BookingSeries;
use App\Entity\Resource;
use App\Entity\User;
use App\Exception\AlreadyHoldingResourceException;
use App\Exception\ResourceAlreadyBookedException;
use App\Model\SeriesResult;
use App\Model\TimeRange;
@@ -32,9 +31,12 @@ use Doctrine\ORM\EntityManagerInterface;
* protection now lives in code, so it belongs to this class alone: nothing else may persist a
* Booking.
*
* The per-user rule ("not two desks at once") is deliberately NOT covered by the lock: it spans
* resources, and locking every resource would serialise the whole app to protect against a user
* racing themselves in two browser tabs.
* **What is NOT guarded here.** One person holding two desks at the same time used to be refused
* from this class. It is allowed now: booking a second desk for a customer you are meeting is a
* real thing people do, and the refusal only ever landed after the modal was filled in, since
* the floor plan offers every free desk regardless of what you already hold. Overlaps of that
* kind are now reported as a warning flash once the booking exists -- see OverlapWarningTrait.
* The only conflict left here is the physical one: two people, or two spans, on one resource.
*/
final readonly class BookingManager
{
@@ -45,8 +47,7 @@ final readonly class BookingManager
}
/**
* @throws AlreadyHoldingResourceException if the user already holds a resource of this kind in that span
* @throws ResourceAlreadyBookedException if somebody else does
* @throws ResourceAlreadyBookedException if the resource is not free for that span
*/
public function book(User $user, Resource $resource, \DateTimeImmutable $date, ?TimeRange $time = null): Booking
{
@@ -56,7 +57,7 @@ final readonly class BookingManager
return $this->em->wrapInTransaction(function () use ($user, $resource, $date, $time): Booking {
$this->em->lock($resource, LockMode::PESSIMISTIC_WRITE);
$this->assertFree($user, $resource, $date, $time);
$this->assertFree($resource, $date, $time);
$booking = new Booking($user, $resource, $date, $time);
$this->em->persist($booking);
@@ -87,8 +88,8 @@ final readonly class BookingManager
foreach ($series->occurrences() as $date) {
try {
$this->assertFree($user, $resource, $date, $time);
} catch (AlreadyHoldingResourceException|ResourceAlreadyBookedException $e) {
$this->assertFree($resource, $date, $time);
} catch (ResourceAlreadyBookedException $e) {
$skipped[$date->format('Y-m-d')] = $e->getMessage();
continue;
@@ -118,7 +119,7 @@ final readonly class BookingManager
$resource = $booking->getResource();
$this->em->lock($resource, LockMode::PESSIMISTIC_WRITE);
$this->assertFree($booking->getUser(), $resource, $booking->getDate(), $time, $booking);
$this->assertFree($resource, $booking->getDate(), $time, $booking);
$booking->changeTime($time);
$this->em->flush();
@@ -161,21 +162,14 @@ final readonly class BookingManager
}
/**
* @throws AlreadyHoldingResourceException
* @throws ResourceAlreadyBookedException
*/
private function assertFree(
User $user,
Resource $resource,
\DateTimeImmutable $date,
TimeRange $time,
?Booking $ignore = null,
): void {
$mine = $this->bookings->findOverlappingForUser($user, $date, $time, $resource->getType(), $ignore);
if (null !== $mine) {
throw new AlreadyHoldingResourceException($mine);
}
foreach ($this->bookings->findOverlapping($resource, $date, $time) as $clash) {
if (null !== $ignore && $clash->getId() === $ignore->getId()) {
continue;
+97 -4
View File
@@ -115,7 +115,12 @@ class BookingControllerTest extends WebTestCase
self::assertStringContainsString('ist am 01.10.2026 bereits belegt', (string) $response->getContent());
}
public function testOnePersonCannotHoldTwoDesksOnTheSameDay(): void
/**
* A second desk at the same time used to be refused. It is allowed now -- the second one is
* usually for the customer somebody is meeting -- and the flash is what tells them they are
* now holding two.
*/
public function testASecondDeskAtTheSameTimeIsBookedAndWarnedAbout(): void
{
$anna = $this->user('[email protected]');
$this->em->persist(new Booking($anna, $this->resource('D1'), new \DateTimeImmutable('2026-10-01')));
@@ -124,12 +129,50 @@ class BookingControllerTest extends WebTestCase
$this->client->loginUser($anna);
$response = $this->submitBookingModal($this->resource('D2'), '2026-10-01');
$content = (string) $response->getContent();
self::assertSame(Response::HTTP_UNPROCESSABLE_ENTITY, $response->getStatusCode());
self::assertStringContainsString('bereits D1 gebucht', (string) $response->getContent());
self::assertSame(Response::HTTP_OK, $response->getStatusCode());
self::assertStringContainsString('D2 gebucht', $content, 'the booking itself still succeeds');
self::assertStringContainsString('zur gleichen Zeit bereits D1 gebucht', $content);
}
public function testADeskAndAParkingSpotOnTheSameDayAreBothAllowed(): void
public function testEveryClashingDeskIsNamedInTheWarning(): void
{
$anna = $this->user('[email protected]');
$this->em->persist(new Booking($anna, $this->resource('D1'), new \DateTimeImmutable('2026-10-01')));
$this->em->persist(new Booking($anna, $this->resource('D2'), new \DateTimeImmutable('2026-10-01')));
$this->em->flush();
$this->client->loginUser($anna);
$response = $this->submitBookingModal($this->resource('D3'), '2026-10-01');
self::assertSame(Response::HTTP_OK, $response->getStatusCode());
self::assertStringContainsString('bereits 2 weitere Schreibtische gebucht: D1, D2', (string) $response->getContent());
}
public function testTwoDesksAtDifferentTimesAreNotWarnedAbout(): void
{
$anna = $this->user('[email protected]');
$this->em->persist(new Booking($anna, $this->resource('D1'), new \DateTimeImmutable('2026-10-01'), TimeRange::fromMinutes(540, 720)));
$this->em->flush();
$this->client->loginUser($anna);
$response = $this->submitBookingModal($this->resource('D2'), '2026-10-01', [
'startTime' => '13:00',
'endTime' => '17:00',
]);
self::assertSame(Response::HTTP_OK, $response->getStatusCode());
self::assertStringNotContainsString('Hinweis', (string) $response->getContent());
}
/**
* The deliberately quiet case: a desk and a parking space at the same time is the everyday
* combination, and a toast for it would train everybody to ignore the toast that matters.
*/
public function testADeskAndAParkingSpotOnTheSameDayAreBothAllowedWithoutAWarning(): void
{
$anna = $this->user('[email protected]');
$this->em->persist(new Booking($anna, $this->resource('D1'), new \DateTimeImmutable('2026-10-01')));
@@ -140,6 +183,56 @@ class BookingControllerTest extends WebTestCase
$response = $this->submitBookingModal($this->resource('P1', ResourceType::PARKING), '2026-10-01');
self::assertSame(Response::HTTP_OK, $response->getStatusCode());
self::assertStringNotContainsString('Hinweis', (string) $response->getContent());
}
public function testASeriesIsWarnedAboutOnceForAllItsClashingDays(): void
{
$anna = $this->user('[email protected]');
$other = $this->resource('D2');
// Two of the four Mondays the series lands on, and one Tuesday it does not.
$this->em->persist(new Booking($anna, $other, new \DateTimeImmutable('2026-10-05'), TimeRange::fromMinutes(600, 660)));
$this->em->persist(new Booking($anna, $other, new \DateTimeImmutable('2026-10-19'), TimeRange::fromMinutes(600, 660)));
$this->em->persist(new Booking($anna, $other, new \DateTimeImmutable('2026-10-20'), TimeRange::fromMinutes(600, 660)));
$this->em->flush();
$this->client->loginUser($anna);
$response = $this->submitBookingModal($this->resource('D1'), '2026-10-05', [
'startTime' => '09:00',
'endTime' => '12:00',
'repeat' => 'weekly',
'until' => '2026-10-30',
]);
$content = (string) $response->getContent();
self::assertSame(Response::HTTP_OK, $response->getStatusCode());
self::assertSame(1, substr_count($content, 'Hinweis:'), 'one flash for the whole series');
self::assertStringContainsString('An 2 von 4 Terminen', $content);
self::assertStringContainsString('(5.10., 19.10.)', $content);
}
public function testMovingABookingOntoAnotherOneWarns(): void
{
$anna = $this->user('[email protected]');
$mine = new Booking($anna, $this->resource('D1'), new \DateTimeImmutable('2026-10-01'), TimeRange::fromMinutes(780, 1020));
$this->em->persist($mine);
$this->em->persist(new Booking($anna, $this->resource('D2'), new \DateTimeImmutable('2026-10-01'), TimeRange::fromMinutes(540, 720)));
$this->em->flush();
$this->client->loginUser($anna);
$url = \sprintf('/bookings/%d/edit', $mine->getId());
$crawler = $this->client->request('GET', $url, server: ['HTTP_HX-Request' => 'true']);
$this->client->request('POST', $url, ['booking' => [
'startTime' => '10:00',
'endTime' => '11:00',
'_token' => $crawler->filter('#booking__token')->attr('value'),
]], server: ['HTTP_HX-Request' => 'true']);
$response = $this->client->getResponse();
self::assertSame(Response::HTTP_OK, $response->getStatusCode());
self::assertStringContainsString('zur gleichen Zeit bereits D2 gebucht', (string) $response->getContent());
}
public function testAnInvalidCsrfTokenIsRejected(): void
+9 -4
View File
@@ -10,7 +10,6 @@ use App\Entity\Resource;
use App\Entity\User;
use App\Enum\RecurrenceRule;
use App\Enum\ResourceType;
use App\Exception\AlreadyHoldingResourceException;
use App\Exception\ResourceAlreadyBookedException;
use App\Model\TimeRange;
use App\Service\BookingManager;
@@ -69,13 +68,19 @@ class BookingManagerTest extends KernelTestCase
self::assertNotNull($second->getId());
}
public function testOnePersonMayNotHoldTwoDesksAtOnce(): void
/**
* This was a refusal once. It is allowed now -- the second desk is usually for the customer
* somebody is meeting -- and the overlap is reported as a flash instead, which
* BookingControllerTest covers.
*/
public function testOnePersonMayHoldTwoDesksAtOnce(): void
{
$user = $this->user('[email protected]');
$this->manager->book($user, $this->resource('D1'), $this->date(), TimeRange::fromMinutes(540, 780));
$this->expectException(AlreadyHoldingResourceException::class);
$this->manager->book($user, $this->resource('D2'), $this->date(), TimeRange::fromMinutes(600, 720));
$second = $this->manager->book($user, $this->resource('D2'), $this->date(), TimeRange::fromMinutes(600, 720));
self::assertNotNull($second->getId());
}
public function testOnePersonMayHoldTwoDesksAtDifferentTimes(): void