diff --git a/src/Controller/Booking/CreateController.php b/src/Controller/Booking/CreateController.php index 7b00138..7f61fea 100644 --- a/src/Controller/Booking/CreateController.php +++ b/src/Controller/Booking/CreateController.php @@ -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()); } diff --git a/src/Controller/Booking/EditController.php b/src/Controller/Booking/EditController.php index 0019d58..76bd2bb 100644 --- a/src/Controller/Booking/EditController.php +++ b/src/Controller/Booking/EditController.php @@ -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); } diff --git a/src/Controller/Traits/OverlapWarningTrait.php b/src/Controller/Traits/OverlapWarningTrait.php new file mode 100644 index 0000000..a325607 --- /dev/null +++ b/src/Controller/Traits/OverlapWarningTrait.php @@ -0,0 +1,157 @@ +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 $bookings + * + * @return list + */ + 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 $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); + } +} diff --git a/src/Exception/AlreadyHoldingResourceException.php b/src/Exception/AlreadyHoldingResourceException.php deleted file mode 100644 index 03dc6da..0000000 --- a/src/Exception/AlreadyHoldingResourceException.php +++ /dev/null @@ -1,26 +0,0 @@ -getTimeRange(); - - parent::__construct(\sprintf( - 'Du hast am %s bereits %s gebucht (%s).', - $existing->getDate()->format('d.m.Y'), - $existing->getResource()->getLabel(), - $time, - )); - } -} diff --git a/src/Repository/BookingRepository.php b/src/Repository/BookingRepository.php index a25fdc9..5286183 100644 --- a/src/Repository/BookingRepository.php +++ b/src/Repository/BookingRepository.php @@ -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 */ 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 + */ + 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. diff --git a/src/Service/BookingManager.php b/src/Service/BookingManager.php index 63ed6e9..afd2951 100644 --- a/src/Service/BookingManager.php +++ b/src/Service/BookingManager.php @@ -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; diff --git a/tests/Controller/BookingControllerTest.php b/tests/Controller/BookingControllerTest.php index ab9a6e1..994f9bd 100644 --- a/tests/Controller/BookingControllerTest.php +++ b/tests/Controller/BookingControllerTest.php @@ -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('anna@ep-reisen.de'); $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('anna@ep-reisen.de'); + $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('anna@ep-reisen.de'); + $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('anna@ep-reisen.de'); $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('anna@ep-reisen.de'); + $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('anna@ep-reisen.de'); + $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 diff --git a/tests/Service/BookingManagerTest.php b/tests/Service/BookingManagerTest.php index 1755a10..b99666f 100644 --- a/tests/Service/BookingManagerTest.php +++ b/tests/Service/BookingManagerTest.php @@ -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('a@ep-reisen.de'); $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