From 6c0a512c05926fa3baca6888cee6d33ffd735ef6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Sun, 27 Sep 2026 15:18:35 +0200 Subject: [PATCH] fix: refuse the import while MyE&P reports no cost unit or country code --- src/Command/BpnImportCommand.php | 86 ++++++++++++--- tests/Command/BpnImportCommandTest.php | 147 +++++++++++++++++++++++++ 2 files changed, 215 insertions(+), 18 deletions(-) create mode 100644 tests/Command/BpnImportCommandTest.php diff --git a/src/Command/BpnImportCommand.php b/src/Command/BpnImportCommand.php index b278b57..a0d8695 100644 --- a/src/Command/BpnImportCommand.php +++ b/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 $hotels + * + * @return list + */ + 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; + } } diff --git a/tests/Command/BpnImportCommandTest.php b/tests/Command/BpnImportCommandTest.php new file mode 100644 index 0000000..3d36ab1 --- /dev/null +++ b/tests/Command/BpnImportCommandTest.php @@ -0,0 +1,147 @@ +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 $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 $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) + ; + } +}