From 2329fe17a04d9aa20c5df202a0772eea9b67a93e 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 15:16:10 +0200
Subject: [PATCH] fix: report an unresolvable hotel instead of failing on it

---
 .../System/Destination/DestinationTrait.php   |  38 +++++-
 src/Model/DestinationDto.php                  |   2 +
 src/Validator/Constraints/ResolvableHotel.php |  21 +++
 .../Constraints/ResolvableHotelValidator.php  |  40 ++++++
 .../Destination/DestinationTraitTest.php      | 122 ++++++++++++++++++
 .../ResolvableHotelValidatorTest.php          | 101 +++++++++++++++
 6 files changed, 317 insertions(+), 7 deletions(-)
 create mode 100644 src/Validator/Constraints/ResolvableHotel.php
 create mode 100644 src/Validator/Constraints/ResolvableHotelValidator.php
 create mode 100644 tests/Controller/Administrative/System/Destination/DestinationTraitTest.php
 create mode 100644 tests/Validator/Constraints/ResolvableHotelValidatorTest.php

diff --git a/src/Controller/Administrative/System/Destination/DestinationTrait.php b/src/Controller/Administrative/System/Destination/DestinationTrait.php
index 74a4aa4..5b23e11 100644
--- a/src/Controller/Administrative/System/Destination/DestinationTrait.php
+++ b/src/Controller/Administrative/System/Destination/DestinationTrait.php
@@ -7,27 +7,51 @@ use App\Model\DestinationDto;
 
 trait DestinationTrait
 {
+    /**
+     * Resolves the chosen boarding points against the base data.
+     *
+     * A stop the base data cannot resolve is kept with null city and street rather than dropped,
+     * the same choice App\Command\MyEpImportCommand makes for the export's own unresolvable ids:
+     * it is still a real departure point, and App\Service\Common\PickupOptions falls back to
+     * "Zustieg {busProId}" for one with no town. Dropping it would hide a departure the coach
+     * actually makes.
+     *
+     * The id comes from the form rather than from the resolved stop, so it survives a lookup
+     * that does not.
+     */
     private function mapPickups(DestinationDto $destinationDto): array
     {
         $pickups = [];
 
         foreach ($destinationDto->getPickups() as $pickupId) {
             $pickup = $this->pickupDataProvider->get($pickupId);
+
             $pickups[] = [
-                'busProId' => $pickup->getBusProId(),
-                'city' => $pickup->getCity(),
-                'street' => $pickup->getStreet(),
-                'time' => $pickup->getTime(),
+                'busProId' => (int) $pickupId,
+                'city' => $pickup?->getCity(),
+                'street' => $pickup?->getStreet(),
+                'time' => $pickup?->getTime(),
             ];
         }
 
         return $pickups;
     }
 
+    /**
+     * @throws \LogicException when the hotel does not resolve
+     */
     private function updateEntity(Destination $destination, DestinationDto $destinationDto): void
     {
-        $hotel = $this->hotelDataProvider->get($destinationDto->getHotelBusProId());
-        $pickups = $this->mapPickups($destinationDto);
+        $hotelBusProId = $destinationDto->getHotelBusProId();
+
+        // App\Validator\Constraints\ResolvableHotel is what reports this to the operator, on the
+        // field and in German, and every caller here runs behind $form->isValid(). This is the
+        // second line only: a caller that skips validation would otherwise write a destination
+        // with no house name, no cost unit and no country, and nothing reads those back until an
+        // invoice is stamped. Failing loudly beats a half built row.
+        if (null === $hotel = $this->hotelDataProvider->get((int) $hotelBusProId)) {
+            throw new \LogicException(sprintf('Refusing to write destination %s: hotel %s does not resolve against the MyE&P base data', $destination->getId() ?? 'new', var_export($hotelBusProId, true)));
+        }
 
         $destination
             ->setProduct($destinationDto->getProduct())
@@ -37,7 +61,7 @@ trait DestinationTrait
             ->setHotelCode($hotel->getCode())
             ->setHotelBusProId($hotel->getBusProId())
             ->setCountry($destinationDto->getCountry())
-            ->setPickups($pickups)
+            ->setPickups($this->mapPickups($destinationDto))
             ->setCostUnit($hotel->getCostUnit())
         ;
     }
diff --git a/src/Model/DestinationDto.php b/src/Model/DestinationDto.php
index a90d75e..878e8c5 100644
--- a/src/Model/DestinationDto.php
+++ b/src/Model/DestinationDto.php
@@ -3,6 +3,7 @@
 namespace App\Model;
 
 use App\Entity\Destination;
+use App\Validator\Constraints as AppAssert;
 use Symfony\Component\Validator\Constraints as Assert;
 
 class DestinationDto
@@ -36,6 +37,7 @@ class DestinationDto
     private ?\DateTimeImmutable $dateTo = null;
 
     #[Assert\NotBlank(message: 'Bitte angeben')]
+    #[AppAssert\ResolvableHotel]
     private ?string $hotelBusProId = null;
 
     private ?string $costUnit = null;
diff --git a/src/Validator/Constraints/ResolvableHotel.php b/src/Validator/Constraints/ResolvableHotel.php
new file mode 100644
index 0000000..f6a4a22
--- /dev/null
+++ b/src/Validator/Constraints/ResolvableHotel.php
@@ -0,0 +1,21 @@
+<?php
+
+namespace App\Validator\Constraints;
+
+use Symfony\Component\Validator\Constraint;
+
+/**
+ * The chosen hotel must still resolve against MyE&P's base data.
+ *
+ * Everything a destination records about its house is derived from that lookup - name, code,
+ * cost unit, country - so an id that no longer resolves cannot produce a complete row. It used
+ * to reach App\Controller\Administrative\System\Destination\DestinationTrait::updateEntity()
+ * and fail there on a null, which is a 500 on a form submit rather than a message on a field.
+ */
+#[\Attribute]
+class ResolvableHotel extends Constraint
+{
+    public string $unknownMessage = 'Dieser Vertragspartner ist nicht mehr verfügbar';
+
+    public string $unavailableMessage = 'Die Stammdaten sind gerade nicht erreichbar – bitte später erneut versuchen';
+}
diff --git a/src/Validator/Constraints/ResolvableHotelValidator.php b/src/Validator/Constraints/ResolvableHotelValidator.php
new file mode 100644
index 0000000..661fb0c
--- /dev/null
+++ b/src/Validator/Constraints/ResolvableHotelValidator.php
@@ -0,0 +1,40 @@
+<?php
+
+namespace App\Validator\Constraints;
+
+use App\BusProNet\DataProvider\HotelDataProvider;
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidator;
+
+class ResolvableHotelValidator extends ConstraintValidator
+{
+    public function __construct(private readonly HotelDataProvider $hotelDataProvider)
+    {
+    }
+
+    public function validate(mixed $value, Constraint $constraint): void
+    {
+        if (false === $constraint instanceof ResolvableHotel) {
+            return;
+        }
+
+        // Whether a hotel was chosen at all is NotBlank's business, not this one's.
+        if (null === $value || '' === $value) {
+            return;
+        }
+
+        // An empty list is MyE&P being unreachable rather than a bad choice: StaleWhileErrorCache
+        // answers [] instead of throwing, so this is the only place the two are still
+        // distinguishable. Reporting an outage as an invalid selection sends the operator looking
+        // for a different answer when the one they gave was right.
+        if ([] === $hotels = $this->hotelDataProvider->getAll()) {
+            $this->context->buildViolation($constraint->unavailableMessage)->addViolation();
+
+            return;
+        }
+
+        if (false === isset($hotels[(int) $value])) {
+            $this->context->buildViolation($constraint->unknownMessage)->addViolation();
+        }
+    }
+}
diff --git a/tests/Controller/Administrative/System/Destination/DestinationTraitTest.php b/tests/Controller/Administrative/System/Destination/DestinationTraitTest.php
new file mode 100644
index 0000000..e92568e
--- /dev/null
+++ b/tests/Controller/Administrative/System/Destination/DestinationTraitTest.php
@@ -0,0 +1,122 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controller\Administrative\System\Destination;
+
+use App\BusProNet\DataProvider\HotelDataProvider;
+use App\BusProNet\DataProvider\PickupDataProvider;
+use App\BusProNet\Model\Hotel;
+use App\BusProNet\Model\Pickup;
+use App\Controller\Administrative\System\Destination\DestinationTrait;
+use App\Entity\Destination;
+use App\Model\DestinationDto;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * The shared mapping behind the destination create, edit and duplicate forms.
+ *
+ * Both behaviours here are about what happens when MyE&P cannot resolve something, which is
+ * the state the whole form lands in during an outage: the hotel stops the write, a boarding
+ * point does not.
+ */
+class DestinationTraitTest extends TestCase
+{
+    /**
+     * The same choice App\Command\MyEpImportCommand makes for the export's own unresolvable ids.
+     * Dropping the stop would hide a departure the coach actually makes; PickupOptions renders
+     * one without a town as "Zustieg {busProId}".
+     */
+    public function testAnUnresolvablePickupIsKeptWithoutItsAddress(): void
+    {
+        $harness = $this->harness($this->hotel(), [4 => $this->pickup()]);
+
+        $destination = new Destination();
+        $harness->map($destination, $this->dto([4, 999]));
+
+        $this->assertSame([
+            ['busProId' => 4, 'city' => 'Karlsruhe', 'street' => 'A5 - Autohof Bruchsal', 'time' => null],
+            ['busProId' => 999, 'city' => null, 'street' => null, 'time' => null],
+        ], $destination->getPickups());
+    }
+
+    /**
+     * ResolvableHotel reports this on the field, and every caller runs behind $form->isValid().
+     * This is the second line: a half built destination has no house name, no cost unit and no
+     * country, and nothing reads those back until an invoice is stamped.
+     */
+    public function testAnUnresolvableHotelRefusesToWriteAnything(): void
+    {
+        $harness = $this->harness(hotel: null);
+        $destination = new Destination();
+
+        try {
+            $harness->map($destination, $this->dto([]));
+            $this->fail('Expected a LogicException');
+        } catch (\LogicException $e) {
+            $this->assertStringContainsString('does not resolve', $e->getMessage());
+        }
+
+        $this->assertNull($destination->getHotel());
+        $this->assertNull($destination->getProduct());
+    }
+
+    /**
+     * @param ?Hotel             $hotel   null stands for a hotel the base data cannot resolve
+     * @param array<int, Pickup> $pickups keyed by busProId; anything absent does not resolve
+     */
+    private function harness(?Hotel $hotel, array $pickups = []): object
+    {
+        $hotelProvider = $this->createMock(HotelDataProvider::class);
+        $hotelProvider->method('get')->willReturn($hotel);
+
+        $pickupProvider = $this->createMock(PickupDataProvider::class);
+        $pickupProvider->method('get')->willReturnCallback(fn ($id) => $pickups[$id] ?? null);
+
+        return new class($hotelProvider, $pickupProvider) {
+            use DestinationTrait;
+
+            public function __construct(
+                private readonly HotelDataProvider $hotelDataProvider,
+                private readonly PickupDataProvider $pickupDataProvider,
+            ) {
+            }
+
+            public function map(Destination $destination, DestinationDto $dto): void
+            {
+                $this->updateEntity($destination, $dto);
+            }
+        };
+    }
+
+    private function dto(array $pickupIds): DestinationDto
+    {
+        return (new DestinationDto())
+            ->setProduct('Lenzerheide - Sportclub Jenatsch')
+            ->setDateFrom(new \DateTimeImmutable('2026-04-04'))
+            ->setDateTo(new \DateTimeImmutable('2026-04-11'))
+            ->setHotelBusProId('163959')
+            ->setCountry('ch')
+            ->setPickups($pickupIds)
+        ;
+    }
+
+    private function hotel(): Hotel
+    {
+        return (new Hotel())
+            ->setBusProId(163959)
+            ->setCode('LPJGR')
+            ->setName('Sportclub Jenatsch')
+            ->setCostUnit('4711')
+        ;
+    }
+
+    private function pickup(): Pickup
+    {
+        return (new Pickup())
+            ->setBusProId(4)
+            ->setCity('Karlsruhe')
+            ->setStreet('A5 - Autohof Bruchsal')
+        ;
+    }
+}
diff --git a/tests/Validator/Constraints/ResolvableHotelValidatorTest.php b/tests/Validator/Constraints/ResolvableHotelValidatorTest.php
new file mode 100644
index 0000000..23cccac
--- /dev/null
+++ b/tests/Validator/Constraints/ResolvableHotelValidatorTest.php
@@ -0,0 +1,101 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Validator\Constraints;
+
+use App\BusProNet\DataProvider\HotelDataProvider;
+use App\BusProNet\Model\Hotel;
+use App\Validator\Constraints\ResolvableHotel;
+use App\Validator\Constraints\ResolvableHotelValidator;
+use PHPUnit\Framework\MockObject\MockObject;
+use Symfony\Component\Validator\Test\ConstraintValidatorTestCase;
+
+/**
+ * Everything a destination records about its house is derived from this lookup, so an id that
+ * does not resolve used to reach DestinationTrait::updateEntity() and fail there on a null -
+ * a 500 on a form submit rather than a message on a field.
+ *
+ * The two causes are reported apart on purpose. An unreachable MyE&P is not the operator's
+ * doing, and telling them their choice was invalid sends them hunting for a different answer
+ * when the one they gave was right.
+ */
+class ResolvableHotelValidatorTest extends ConstraintValidatorTestCase
+{
+    public function testAResolvableHotelPasses(): void
+    {
+        $this->validator = $this->createValidator([163959 => $this->hotel()]);
+        $this->validator->initialize($this->context);
+
+        $this->validator->validate('163959', new ResolvableHotel());
+
+        $this->assertNoViolation();
+    }
+
+    public function testAHotelMissingFromALoadedListIsRejectedAsUnknown(): void
+    {
+        $constraint = new ResolvableHotel();
+
+        $this->validator = $this->createValidator([163959 => $this->hotel()]);
+        $this->validator->initialize($this->context);
+
+        $this->validator->validate('999999', $constraint);
+
+        $this->buildViolation($constraint->unknownMessage)->assertRaised();
+    }
+
+    /**
+     * StaleWhileErrorCache answers [] rather than throwing, so an empty list is the only signal
+     * that MyE&P could not be reached - and on a cold cache there is no stale copy to fall back to.
+     */
+    public function testAnEmptyListIsReportedAsAnOutageRatherThanABadChoice(): void
+    {
+        $constraint = new ResolvableHotel();
+
+        $this->validator = $this->createValidator([]);
+        $this->validator->initialize($this->context);
+
+        $this->validator->validate('163959', $constraint);
+
+        $this->buildViolation($constraint->unavailableMessage)->assertRaised();
+    }
+
+    /**
+     * Whether a hotel was chosen at all belongs to NotBlank, which already sits on the property.
+     */
+    public function testAnEmptyValueIsLeftToNotBlank(): void
+    {
+        $this->validator = $this->createValidator([]);
+        $this->validator->initialize($this->context);
+
+        $this->validator->validate(null, new ResolvableHotel());
+        $this->validator->validate('', new ResolvableHotel());
+
+        $this->assertNoViolation();
+    }
+
+    protected function createValidator(array $hotels = []): ResolvableHotelValidator
+    {
+        return new ResolvableHotelValidator($this->provider($hotels));
+    }
+
+    /**
+     * @return HotelDataProvider&MockObject
+     */
+    private function provider(array $hotels): object
+    {
+        $provider = $this->createMock(HotelDataProvider::class);
+        $provider->method('getAll')->willReturn($hotels);
+
+        return $provider;
+    }
+
+    private function hotel(): Hotel
+    {
+        return (new Hotel())
+            ->setBusProId(163959)
+            ->setCode('LPJGR')
+            ->setName('Sportclub Jenatsch')
+        ;
+    }
+}