From 51ce9b6e45087ffeb9deb58b6db054339a9d1f84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Tue, 24 Feb 2026 18:16:52 +0100 Subject: [PATCH] fix: don't restore selections from draft for immutable fields --- src/Service/BookingEditDraftService.php | 154 +++--- .../BookingEditDraftServiceMutabilityTest.php | 448 ++++++++++++++++++ 2 files changed, 530 insertions(+), 72 deletions(-) create mode 100644 tests/Service/BookingEditDraftServiceMutabilityTest.php diff --git a/src/Service/BookingEditDraftService.php b/src/Service/BookingEditDraftService.php index ba3ce8a..c55c026 100644 --- a/src/Service/BookingEditDraftService.php +++ b/src/Service/BookingEditDraftService.php @@ -331,91 +331,101 @@ class BookingEditDraftService */ private function applyServiceSelections(ParticipantDto $participant, array $data, Travel $travel): void { - // Ski pass (single service) - merge strategy: only apply if resolves to valid service - if (true === array_key_exists('skiPass', $data) && null !== $data['skiPass']) { - $draftSkiPassId = $data['skiPass']; - $resolved = $this->resolveService($draftSkiPassId, $travel->additionalServices); + // Additional services category — only apply draft data when services are mutable. + // When immutable, the booking's current services must be preserved as-is to avoid + // API rejection (stale draft data could differ from the locked booking state). + if (true === $travel->additionalServicesMutable) { + // Ski pass (single service) - merge strategy: only apply if resolves to valid service + if (true === array_key_exists('skiPass', $data) && null !== $data['skiPass']) { + $draftSkiPassId = $data['skiPass']; + $resolved = $this->resolveService($draftSkiPassId, $travel->additionalServices); - if (null !== $resolved) { - $participant->skiPass = $resolved; + if (null !== $resolved) { + $participant->skiPass = $resolved; + } + } + + // Courses (array) - overwrite strategy: user can deselect all + if (true === array_key_exists('courses', $data) && true === is_array($data['courses'])) { + $participant->courses = $this->resolveServiceArray($data['courses'], $travel->additionalServices); + } + + // Board (array) - overwrite strategy: user can deselect all + if (true === array_key_exists('board', $data) && true === is_array($data['board'])) { + $participant->board = $this->resolveServiceArray($data['board'], $travel->additionalServices); + } + + // Rentals (array) - overwrite strategy: user can deselect all + if (true === array_key_exists('rentals', $data) && true === is_array($data['rentals'])) { + $participant->rentals = $this->resolveServiceArray($data['rentals'], $travel->additionalServices); + } + + // Rental insurance (single) - merge strategy: only apply if resolves to valid service + if (true === array_key_exists('rentalInsurance', $data) && null !== $data['rentalInsurance']) { + $resolved = $this->resolveService($data['rentalInsurance'], $travel->additionalServices); + if (null !== $resolved) { + $participant->rentalInsurance = $resolved; + $participant->rentalInsuranceSelected = true; + } + } + + // Additional services (array) - overwrite strategy with mandatory service preservation + // User can deselect optional services, but mandatory services from API must be preserved + if (true === array_key_exists('additionalServices', $data) && true === is_array($data['additionalServices'])) { + $resolvedFromDraft = $this->resolveServiceArray($data['additionalServices'], $travel->additionalServices); + $participant->additionalServices = $this->preserveMandatoryServices( + $resolvedFromDraft, + $participant->additionalServices, + $travel + ); } } - // Courses (array) - overwrite strategy: user can deselect all - if (true === array_key_exists('courses', $data) && true === is_array($data['courses'])) { - $participant->courses = $this->resolveServiceArray($data['courses'], $travel->additionalServices); - } + // Transportation category — only apply draft data when transportation is mutable + if (true === $travel->transportationServicesMutable) { + // Transportation outbound (single) - merge strategy: only apply if resolves to valid service + if (true === array_key_exists('transportationOutbound', $data) && null !== $data['transportationOutbound']) { + $resolved = $this->resolveService($data['transportationOutbound'], $travel->transportationServices); + if (null !== $resolved) { + $participant->transportationOutbound = $resolved; + } + } - // Board (array) - overwrite strategy: user can deselect all - if (true === array_key_exists('board', $data) && true === is_array($data['board'])) { - $participant->board = $this->resolveServiceArray($data['board'], $travel->additionalServices); - } + // Transportation inbound (single) - merge strategy: only apply if resolves to valid service + if (true === array_key_exists('transportationInbound', $data) && null !== $data['transportationInbound']) { + $resolved = $this->resolveService($data['transportationInbound'], $travel->transportationServices); + if (null !== $resolved) { + $participant->transportationInbound = $resolved; + } + } - // Rentals (array) - overwrite strategy: user can deselect all - if (true === array_key_exists('rentals', $data) && true === is_array($data['rentals'])) { - $participant->rentals = $this->resolveServiceArray($data['rentals'], $travel->additionalServices); - } - - // Rental insurance (single) - merge strategy: only apply if resolves to valid service - if (true === array_key_exists('rentalInsurance', $data) && null !== $data['rentalInsurance']) { - $resolved = $this->resolveService($data['rentalInsurance'], $travel->additionalServices); - if (null !== $resolved) { - $participant->rentalInsurance = $resolved; - $participant->rentalInsuranceSelected = true; + // Parking (boolean) - overwrite strategy: user can uncheck + if (true === array_key_exists('parking', $data)) { + $participant->parking = (bool) $data['parking']; } } - // Additional services (array) - overwrite strategy with mandatory service preservation - // Additional services (array) - overwrite strategy with mandatory service preservation - // User can deselect optional services, but mandatory services from API must be preserved - if (true === array_key_exists('additionalServices', $data) && true === is_array($data['additionalServices'])) { - $resolvedFromDraft = $this->resolveServiceArray($data['additionalServices'], $travel->additionalServices); - $participant->additionalServices = $this->preserveMandatoryServices( - $resolvedFromDraft, - $participant->additionalServices, - $travel - ); - } + // Pickups category — only apply draft data when pickups are mutable + if (true === $travel->pickupsMutable) { + // Pickup (single) - merge strategy: only apply if resolves to valid pickup + if (true === array_key_exists('pickup', $data) && null !== $data['pickup']) { + $resolved = $this->resolvePickup($data['pickup'], $travel); + if (null !== $resolved) { + $participant->pickup = $resolved; + } + } - // Transportation outbound (single) - merge strategy: only apply if resolves to valid service - if (true === array_key_exists('transportationOutbound', $data) && null !== $data['transportationOutbound']) { - $resolved = $this->resolveService($data['transportationOutbound'], $travel->transportationServices); - if (null !== $resolved) { - $participant->transportationOutbound = $resolved; + // Drop-off (single) - merge strategy: only apply if resolves to valid drop-off + if (true === array_key_exists('dropOff', $data) && null !== $data['dropOff']) { + $resolved = $this->resolveDropOff($data['dropOff'], $travel); + if (null !== $resolved) { + $participant->dropOff = $resolved; + $participant->differentDropOff = true; + } } } - // Transportation inbound (single) - merge strategy: only apply if resolves to valid service - if (true === array_key_exists('transportationInbound', $data) && null !== $data['transportationInbound']) { - $resolved = $this->resolveService($data['transportationInbound'], $travel->transportationServices); - if (null !== $resolved) { - $participant->transportationInbound = $resolved; - } - } - - // Pickup (single) - merge strategy: only apply if resolves to valid pickup - if (true === array_key_exists('pickup', $data) && null !== $data['pickup']) { - $resolved = $this->resolvePickup($data['pickup'], $travel); - if (null !== $resolved) { - $participant->pickup = $resolved; - } - } - - // Drop-off (single) - merge strategy: only apply if resolves to valid drop-off - if (true === array_key_exists('dropOff', $data) && null !== $data['dropOff']) { - $resolved = $this->resolveDropOff($data['dropOff'], $travel); - if (null !== $resolved) { - $participant->dropOff = $resolved; - $participant->differentDropOff = true; - } - } - - // Parking (boolean) - overwrite strategy: user can uncheck - if (true === array_key_exists('parking', $data)) { - $participant->parking = (bool) $data['parking']; - } - - // Insurance (single) - merge strategy: only apply if resolves to valid insurance + // Insurance (not governed by mutability categories — always apply from draft) if (true === array_key_exists('insurance', $data) && null !== $data['insurance']) { $resolved = $this->resolveInsurance($data['insurance'], $travel); if (null !== $resolved) { diff --git a/tests/Service/BookingEditDraftServiceMutabilityTest.php b/tests/Service/BookingEditDraftServiceMutabilityTest.php new file mode 100644 index 0000000..5b2c0ec --- /dev/null +++ b/tests/Service/BookingEditDraftServiceMutabilityTest.php @@ -0,0 +1,448 @@ +service = new BookingEditDraftService( + $this->createMock(BookingEditDraftRepository::class), + $this->createMock(EntityManagerInterface::class), + $this->createMock(BookingFingerprintService::class), + new NullLogger(), + ); + } + + // --- Additional Services Mutability --- + + public function testDraftSkipsAdditionalServicesWhenImmutable(): void + { + $originalService = $this->createService(10, 'Original'); + $participant = new ParticipantDto(); + $participant->additionalServices = [$originalService]; + $participant->courses = []; + $participant->board = []; + $participant->rentals = []; + $participant->skiPass = null; + $participant->rentalInsurance = null; + + $travel = $this->createTravel( + additionalServicesMutable: false, + additionalServices: [10 => $originalService, 99 => $this->createService(99, 'Draft Added')], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'additionalServices' => [99], + 'courses' => [99], + 'board' => [99], + 'rentals' => [99], + 'skiPass' => 99, + 'rentalInsurance' => 99, + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + // All additional services category fields must remain unchanged + $this->assertSame([$originalService], $participant->additionalServices); + $this->assertSame([], $participant->courses); + $this->assertSame([], $participant->board); + $this->assertSame([], $participant->rentals); + $this->assertNull($participant->skiPass); + $this->assertNull($participant->rentalInsurance); + } + + public function testDraftAppliesAdditionalServicesWhenMutable(): void + { + $originalService = $this->createService(10, 'Original'); + $draftService = $this->createService(99, 'Draft Added'); + + $participant = new ParticipantDto(); + $participant->additionalServices = [$originalService]; + $participant->courses = []; + $participant->skiPass = null; + + $travel = $this->createTravel( + additionalServicesMutable: true, + additionalServices: [10 => $originalService, 99 => $draftService], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'additionalServices' => [99], + 'courses' => [99], + 'skiPass' => 99, + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + // Draft values should be applied + $this->assertCount(1, $participant->additionalServices); + $this->assertSame(99, $participant->additionalServices[0]->id); + $this->assertCount(1, $participant->courses); + $this->assertSame(99, $participant->courses[0]->id); + $this->assertNotNull($participant->skiPass); + $this->assertSame(99, $participant->skiPass->id); + } + + // --- Transportation Mutability --- + + public function testDraftSkipsTransportationWhenImmutable(): void + { + $originalOutbound = $this->createService(20, 'Outbound', isTransportation: true); + $originalInbound = $this->createService(21, 'Inbound', isTransportation: true); + + $participant = new ParticipantDto(); + $participant->transportationOutbound = $originalOutbound; + $participant->transportationInbound = $originalInbound; + $participant->parking = false; + + $travel = $this->createTravel( + transportationServicesMutable: false, + transportationServices: [ + 20 => $originalOutbound, + 21 => $originalInbound, + 88 => $this->createService(88, 'Draft Transport', isTransportation: true), + ], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'transportationOutbound' => 88, + 'transportationInbound' => 88, + 'parking' => true, + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + // Transportation fields must remain unchanged + $this->assertSame($originalOutbound, $participant->transportationOutbound); + $this->assertSame($originalInbound, $participant->transportationInbound); + $this->assertFalse($participant->parking); + } + + public function testDraftAppliesTransportationWhenMutable(): void + { + $originalOutbound = $this->createService(20, 'Outbound', isTransportation: true); + $draftTransport = $this->createService(88, 'Draft Transport', isTransportation: true); + + $participant = new ParticipantDto(); + $participant->transportationOutbound = $originalOutbound; + $participant->parking = false; + + $travel = $this->createTravel( + transportationServicesMutable: true, + transportationServices: [20 => $originalOutbound, 88 => $draftTransport], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'transportationOutbound' => 88, + 'parking' => true, + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + $this->assertSame($draftTransport, $participant->transportationOutbound); + $this->assertTrue($participant->parking); + } + + // --- Pickups Mutability --- + + public function testDraftSkipsPickupsWhenImmutable(): void + { + $originalPickup = $this->createPickup(30, 'Original Pickup'); + + $participant = new ParticipantDto(); + $participant->pickup = $originalPickup; + $participant->dropOff = null; + $participant->differentDropOff = false; + + $travel = $this->createTravel( + pickupsMutable: false, + pickups: [30 => $originalPickup, 77 => $this->createPickup(77, 'Draft Pickup')], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'pickup' => 77, + 'dropOff' => 77, + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + // Pickup fields must remain unchanged + $this->assertSame($originalPickup, $participant->pickup); + $this->assertNull($participant->dropOff); + $this->assertFalse($participant->differentDropOff); + } + + public function testDraftAppliesPickupsWhenMutable(): void + { + $draftPickup = $this->createPickup(77, 'Draft Pickup'); + + $participant = new ParticipantDto(); + $participant->pickup = null; + + $travel = $this->createTravel( + pickupsMutable: true, + pickups: [77 => $draftPickup], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'pickup' => 77, + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + $this->assertSame($draftPickup, $participant->pickup); + } + + // --- Insurance (always applied, not governed by mutability) --- + + public function testDraftAlwaysAppliesInsuranceRegardlessOfMutability(): void + { + $draftInsurance = $this->createInsurance('INS-50'); + + $participant = new ParticipantDto(); + $participant->insurance = null; + + $travel = $this->createTravel( + additionalServicesMutable: false, + transportationServicesMutable: false, + pickupsMutable: false, + insurances: ['INS-50' => $draftInsurance], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'insurance' => 'INS-50', + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + $this->assertSame($draftInsurance, $participant->insurance); + } + + // --- Mixed mutability: only immutable categories are protected --- + + public function testMixedMutabilityProtectsOnlyImmutableCategories(): void + { + $originalAdditional = $this->createService(10, 'Original Additional'); + $draftTransport = $this->createService(88, 'Draft Transport', isTransportation: true); + $draftPickup = $this->createPickup(77, 'Draft Pickup'); + + $participant = new ParticipantDto(); + $participant->additionalServices = [$originalAdditional]; + $participant->transportationOutbound = null; + $participant->pickup = null; + + $travel = $this->createTravel( + additionalServicesMutable: false, // immutable + transportationServicesMutable: true, // mutable + pickupsMutable: true, // mutable + additionalServices: [10 => $originalAdditional, 99 => $this->createService(99, 'Draft')], + transportationServices: [88 => $draftTransport], + pickups: [77 => $draftPickup], + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'services' => [ + 'additionalServices' => [99], + 'transportationOutbound' => 88, + 'pickup' => 77, + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + // Additional services: immutable → unchanged + $this->assertSame([$originalAdditional], $participant->additionalServices); + + // Transportation: mutable → draft applied + $this->assertSame($draftTransport, $participant->transportationOutbound); + + // Pickups: mutable → draft applied + $this->assertSame($draftPickup, $participant->pickup); + } + + // --- Personal data is always applied regardless of mutability --- + + public function testDraftAlwaysAppliesPersonalDataRegardlessOfMutability(): void + { + $participant = new ParticipantDto(); + $participant->firstName = 'Original'; + + $travel = $this->createTravel( + additionalServicesMutable: false, + transportationServicesMutable: false, + pickupsMutable: false, + ); + + $dto = $this->createBookingDto($travel, [$participant]); + + $draft = $this->createDraft([ + 'participants' => [ + 0 => [ + 'personalData' => [ + 'firstName' => 'Updated', + ], + ], + ], + ]); + + $this->service->applyDraftToDto($draft, $dto, $travel); + + $this->assertSame('Updated', $participant->firstName); + } + + // --- Helpers --- + + private function createService(int $id, string $label = 'Test', bool $isTransportation = false): Service + { + $service = new Service(); + $service->id = $id; + $service->label = $label; + + return $service; + } + + private function createPickup(int $id, string $label = 'Test Pickup'): Pickup + { + $pickup = new Pickup(); + $pickup->id = $id; + $pickup->city = $label; + + return $pickup; + } + + private function createInsurance(string $id): Insurance + { + $insurance = new Insurance(); + $insurance->id = $id; + $insurance->label = 'Test Insurance'; + + return $insurance; + } + + private function createTravel( + bool $additionalServicesMutable = true, + bool $transportationServicesMutable = true, + bool $pickupsMutable = true, + array $additionalServices = [], + array $transportationServices = [], + array $pickups = [], + array $dropOffs = [], + array $insurances = [], + ): Travel { + $travel = new Travel(); + $travel->additionalServicesMutable = $additionalServicesMutable; + $travel->transportationServicesMutable = $transportationServicesMutable; + $travel->pickupsMutable = $pickupsMutable; + $travel->additionalServices = $additionalServices; + $travel->transportationServices = $transportationServices; + $travel->pickups = $pickups; + $travel->dropOffs = $dropOffs; + $travel->insurances = $insurances; + + return $travel; + } + + private function createBookingDto(Travel $travel, array $participants): BookingDto + { + $dto = new BookingDto($travel, 1); + $dto->participants = $participants; + + return $dto; + } + + private function createDraft(array $formData): BookingEditDraft + { + $user = $this->createMock(User::class); + + return new BookingEditDraft($user, 123, new \DateTimeImmutable(), $formData); + } +}