fix: refuse the import while MyE&P reports no cost unit or country code

src/Command/BpnImportCommand.php

@@ -4,6 +4,7 @@ namespace App\Command;
 
 use App\BusProNet\DataProvider\HotelDataProvider;
 use App\BusProNet\DataProvider\PickupDataProvider;
+use App\BusProNet\Model\Hotel;
 use Doctrine\DBAL\Connection;
 use League\Flysystem\FilesystemException;
 use League\Flysystem\FilesystemOperator;
@@ -35,6 +36,36 @@ class BpnImportCommand extends Command
     {
         $io = new SymfonyStyle($input, $output);
 
+        // Resolve the base data once, before any file is read. A destination cannot be
+        // written without its hotel, so an unavailable hotel list would otherwise skip every
+        // record and still report success. Holding both lists locally also keeps the loop off
+        // the providers, which would otherwise be asked per pickup of all 174 files.
+        $hotels = $this->hotelDataProvider->getAll();
+        $pickups = $this->pickupDataProvider->getAll();
+
+        if ([] === $hotels) {
+            $io->error('Hotel-Stammdaten konnten nicht von MyE&P geladen werden – Import abgebrochen.');
+            $this->logger->error('Import aborted: hotel base data unavailable');
+
+            return Command::FAILURE;
+        }
+
+        // Every destination this import touches gets its cost unit and its country written
+        // from the hotel, on update as well as on insert. If MyE&P does not report those two
+        // fields yet, a run would quietly blank them on rows that have them: the cost unit
+        // then drops off the invoices (App\Service\Pdf\InvoiceApprover) and a missing
+        // country code blanks the flag icons and takes the row out of the country filters.
+        // Refusing to run is the recoverable failure; overwriting good data is not.
+        if ([] !== $missingFields = $this->missingHotelFields($hotels)) {
+            $io->error(sprintf(
+                'Die MyE&P-API liefert keine %s – Import abgebrochen, damit bestehende Werte nicht überschrieben werden.',
+                implode(' und ', $missingFields),
+            ));
+            $this->logger->error('Import aborted: the MyE&P hotel payload carries no '.implode(', ', $missingFields));
+
+            return Command::FAILURE;
+        }
+
         // Get all xml files
         try {
             $xmlFiles = $this
@@ -56,28 +87,13 @@ class BpnImportCommand extends Command
             return Command::FAILURE;
         }
 
-        // Resolve the BusProNet base data once, before touching a single row. A destination
-        // cannot be written without its hotel, so an unavailable hotel list would otherwise
-        // skip every record and still report success. Holding both lists locally also keeps
-        // the loop off the providers: on a failed fetch nothing is cached, and a per-lookup
-        // ->get() would re-open the socket for every pickup of all 174 files.
-        $hotels = $this->hotelDataProvider->getAll();
-        $pickups = $this->pickupDataProvider->getAll();
-
-        if ([] === $hotels) {
-            $io->error('Hotel-Stammdaten konnten nicht von BusProNet geladen werden – Import abgebrochen.');
-            $this->logger->error('BPN import aborted: hotel base data unavailable');
-
-            return Command::FAILURE;
-        }
-
         $io->info('Found '.$totalCount.' XML files');
         $addedCount = 0;
         $updatedCount = 0;
         $warnings = [];
 
         if ([] === $pickups) {
-            $warnings[] = 'Zustiegs-Stammdaten konnten nicht von BusProNet geladen werden';
+            $warnings[] = 'Zustiegs-Stammdaten konnten nicht von MyE&P geladen werden';
         }
 
         $progressBar = $io->createProgressBar($totalCount);
@@ -144,7 +160,7 @@ class BpnImportCommand extends Command
                                 'product' => $product,
                                 'pickups' => json_encode($datePickups),
                                 'cost_unit' => $hotel->getCostUnit(),
-                                'country' => $hotel->getCountry(),
+                                'country' => $hotel->getCountryCode(),
                                 'updated_at' => (new \DateTimeImmutable())->format('Y-m-d H:i:s'),
                             ], [
                                 'id' => $id,
@@ -166,7 +182,7 @@ class BpnImportCommand extends Command
                                 'hotel_bus_pro_id' => $hotelBusProId,
                                 'pickups' => json_encode($datePickups),
                                 'cost_unit' => $hotel->getCostUnit(),
-                                'country' => $hotel->getCountry(),
+                                'country' => $hotel->getCountryCode(),
                                 'created_at' => (new \DateTimeImmutable())->format('Y-m-d H:i:s'),
                             ])
                         ;
@@ -192,4 +208,38 @@ class BpnImportCommand extends Command
 
         return Command::SUCCESS;
     }
+
+    /**
+     * Names the hotel fields MyE&P is not reporting, German labels for the operator.
+     *
+     * Asserted across the whole list rather than per hotel, because a single hotel
+     * legitimately has neither: only a minority of them carry a kostenstelle upstream, and
+     * some carry no country at all. A list where not one hotel has the field is a field
+     * missing from the response, not a gap in the data.
+     *
+     * @param array<int, Hotel> $hotels
+     *
+     * @return list<string>
+     */
+    private function missingHotelFields(array $hotels): array
+    {
+        $required = [
+            'Kostenstellen' => static fn (Hotel $hotel): bool => '' !== (string) $hotel->getCostUnit(),
+            'Länderkennungen' => static fn (Hotel $hotel): bool => '' !== (string) $hotel->getCountryCode(),
+        ];
+
+        $missing = [];
+
+        foreach ($required as $label => $isReported) {
+            foreach ($hotels as $hotel) {
+                if ($isReported($hotel)) {
+                    continue 2;
+                }
+            }
+
+            $missing[] = $label;
+        }
+
+        return $missing;
+    }
 }

tests/Command/BpnImportCommandTest.php

@@ -0,0 +1,147 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Command;
+
+use App\BusProNet\DataProvider\HotelDataProvider;
+use App\BusProNet\DataProvider\PickupDataProvider;
+use App\BusProNet\Model\Hotel;
+use App\Command\BpnImportCommand;
+use Doctrine\DBAL\Connection;
+use League\Flysystem\FilesystemOperator;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Tester\CommandTester;
+
+/**
+ * The import writes a destination's cost unit and country from the hotel base data, on
+ * update as well as on insert, so it can only run once MyE&P reports both fields. Until it
+ * does, a run would blank them on the 2,206 destinations that carry a cost unit and the
+ * 2,259 that carry a country - silently, because nothing reads them back until an invoice
+ * is stamped. These tests pin the refusal, and that it happens before any write.
+ */
+class BpnImportCommandTest extends TestCase
+{
+    public function testItRefusesToRunWhenNoHotelReportsACostUnit(): void
+    {
+        $tester = $this->runImport([$this->hotel(costUnit: null, countryCode: 'ch')]);
+
+        $this->assertSame(Command::FAILURE, $tester->getStatusCode());
+        $this->assertStringContainsString('Kostenstellen', $tester->getDisplay());
+        $this->assertStringNotContainsString('Länderkennungen', $tester->getDisplay());
+    }
+
+    public function testItRefusesToRunWhenNoHotelReportsACountryCode(): void
+    {
+        $tester = $this->runImport([$this->hotel(costUnit: '4711', countryCode: null)]);
+
+        $this->assertSame(Command::FAILURE, $tester->getStatusCode());
+        $this->assertStringContainsString('Länderkennungen', $tester->getDisplay());
+    }
+
+    public function testItNamesBothFieldsWhenNeitherIsReported(): void
+    {
+        $tester = $this->runImport([$this->hotel(costUnit: null, countryCode: null)]);
+
+        $this->assertSame(Command::FAILURE, $tester->getStatusCode());
+        $this->assertStringContainsString('Kostenstellen und Länderkennungen', $tester->getDisplay());
+    }
+
+    public function testAnEmptyStringDoesNotCountAsAReportedField(): void
+    {
+        $tester = $this->runImport([$this->hotel(costUnit: '', countryCode: '')]);
+
+        $this->assertSame(Command::FAILURE, $tester->getStatusCode());
+        $this->assertStringContainsString('Kostenstellen und Länderkennungen', $tester->getDisplay());
+    }
+
+    public function testOneHotelCarryingAFieldIsEnoughToPassTheGuard(): void
+    {
+        // most hotels legitimately have no kostenstelle upstream, so the guard asks whether
+        // the field is reported at all, not whether every hotel fills it
+        $tester = $this->runImport([
+            $this->hotel(costUnit: null, countryCode: null),
+            $this->hotel(costUnit: '4711', countryCode: 'ch'),
+        ]);
+
+        $display = $tester->getDisplay();
+
+        $this->assertStringNotContainsString('Kostenstellen', $display);
+        $this->assertStringNotContainsString('Länderkennungen', $display);
+    }
+
+    public function testItRefusesToRunWithoutAnyHotelBaseData(): void
+    {
+        $tester = $this->runImport([]);
+
+        $this->assertSame(Command::FAILURE, $tester->getStatusCode());
+        $this->assertStringContainsString('Hotel-Stammdaten', $tester->getDisplay());
+    }
+
+    /**
+     * @param list<Hotel> $hotels
+     */
+    private function runImport(array $hotels): CommandTester
+    {
+        $keyed = [];
+
+        foreach ($hotels as $index => $hotel) {
+            $keyed[$index + 1] = $hotel;
+        }
+
+        $tester = new CommandTester(new BpnImportCommand(
+            $this->refusingConnection(),
+            $this->provider(HotelDataProvider::class, $keyed),
+            $this->provider(PickupDataProvider::class, []),
+            new NullLogger(),
+            // an auto stubbed listing, so it yields no files. The guards under test all run
+            // before it, and the one case that gets past them stops there without writing.
+            $this->createMock(FilesystemOperator::class),
+        ));
+
+        $tester->execute([]);
+
+        return $tester;
+    }
+
+    /**
+     * @template T of HotelDataProvider|PickupDataProvider
+     *
+     * @param class-string<T> $class
+     *
+     * @return T&MockObject
+     */
+    private function provider(string $class, array $all): object
+    {
+        $provider = $this->createMock($class);
+        $provider->method('getAll')->willReturn($all);
+
+        return $provider;
+    }
+
+    /**
+     * Any query at all means the guard let a write through, which is the whole failure this
+     * covers, so the connection fails the test rather than the command.
+     */
+    private function refusingConnection(): Connection&MockObject
+    {
+        $connection = $this->createMock(Connection::class);
+        $connection->expects($this->never())->method($this->anything());
+
+        return $connection;
+    }
+
+    private function hotel(?string $costUnit, ?string $countryCode): Hotel
+    {
+        return (new Hotel())
+            ->setBusProId(163959)
+            ->setCode('LPJGR')
+            ->setName('Sportclub Jenatsch')
+            ->setCostUnit($costUnit)
+            ->setCountryCode($countryCode)
+        ;
+    }
+}