diff --git a/src/Controller/Booking/Create/Step2ParticipantController.php b/src/Controller/Booking/Create/Step2ParticipantController.php index 1b5e3b87..69e75532 100644 --- a/src/Controller/Booking/Create/Step2ParticipantController.php +++ b/src/Controller/Booking/Create/Step2ParticipantController.php @@ -153,12 +153,6 @@ class Step2ParticipantController extends AbstractController } $bookingDto = $result; - // Internal agency bookings are pre-populated with a non-travelling applicant already, - // so the choice is neither offered nor accepted for them. - if ($bookingDto->isInternalAgencyBooking()) { - return $this->htmxRedirect($request, $this->generateUrl('app_booking_create_step_2')); - } - $this->travelDataService->enrichWithFreshAvailabilities($bookingDto->travel); // Captured before handleRequest(), which binds the new choice straight onto the DTO. @@ -227,7 +221,7 @@ class Step2ParticipantController extends AbstractController } $bookingDto = $result; - if (true === $bookingDto->applicantTravels || $bookingDto->isInternalAgencyBooking()) { + if (true === $bookingDto->applicantTravels) { return $this->redirectToRoute('app_booking_create_step_2'); } diff --git a/src/Form/Model/BookingDto.php b/src/Form/Model/BookingDto.php index 25de5ed2..c99d694e 100644 --- a/src/Form/Model/BookingDto.php +++ b/src/Form/Model/BookingDto.php @@ -68,7 +68,7 @@ class BookingDto * * Create mode only: it is the booking-level choice behind "Ich reise selbst mit". * In edit mode the split is derived from the loaded booking instead, see - * isFirstParticipantApplicant(). Internal agency bookings never flip this. + * isFirstParticipantApplicant(). */ public bool $applicantTravels = true; @@ -201,21 +201,23 @@ class BookingDto * same thing, which only held because the internal agency was the one case where * applicant and participant[0] differed. * - * - internal agency: always false, their applicant is a staff member and never travels - * - create: the booking-level choice - * - edit: derived from the loaded booking. A null person id on either side counts as - * "same", so incomplete BPN data keeps today's stricter, locked-down behaviour. + * - create: the booking-level choice. The agency plays no part here: internal agency + * bookings are made in the BusPro backend and only ever edited in this application. + * - edit, internal agency: always false, their applicant is a customer who is not the + * first traveller + * - edit, otherwise: derived from the loaded booking. A null person id on either side + * counts as "same", so incomplete BPN data keeps today's stricter, locked-down behaviour. */ public function isFirstParticipantApplicant(): bool { - if ($this->isInternalAgencyBooking()) { - return false; - } - if (self::MODE_CREATE === $this->getMode()) { return $this->applicantTravels; } + if ($this->isInternalAgencyBooking()) { + return false; + } + $applicantPersonId = $this->booking?->applicant?->personId; $firstParticipantPersonId = $this->getParticipant(0)?->personId; @@ -289,17 +291,15 @@ class BookingDto } /** - * Requires a complete address from participant[0] only where they supply the anmelder block. + * Requires a complete address from participant[0] only where they are the applicant. * * BusPro does not require an anschrift from an ordinary participant, so on a split booking - * the traveller at index 0 is not asked for one. Keyed on $applicantTravels rather than - * isFirstParticipantApplicant() so internal agency bookings, whose payload still builds the - * anmelder from participant[0], keep asking for it. + * the traveller at index 0 is not asked for one. */ #[Assert\Callback(groups: ['strict_required'])] public function validateFirstParticipantAddress(ExecutionContextInterface $context): void { - if (false === $this->applicantTravels) { + if (false === $this->isFirstParticipantApplicant()) { return; } diff --git a/src/Form/Model/ParticipantEditDto.php b/src/Form/Model/ParticipantEditDto.php index e2f12134..fedff1ef 100644 --- a/src/Form/Model/ParticipantEditDto.php +++ b/src/Form/Model/ParticipantEditDto.php @@ -105,7 +105,7 @@ class ParticipantEditDto return; } - if (false === $this->bookingContext->applicantTravels) { + if (false === $this->bookingContext->isFirstParticipantApplicant()) { return; } diff --git a/src/Form/Service/Condition/FirstParticipantSuppliesApplicantDataCondition.php b/src/Form/Service/Condition/FirstParticipantIsApplicantCondition.php similarity index 61% rename from src/Form/Service/Condition/FirstParticipantSuppliesApplicantDataCondition.php rename to src/Form/Service/Condition/FirstParticipantIsApplicantCondition.php index 7f499632..4b483425 100644 --- a/src/Form/Service/Condition/FirstParticipantSuppliesApplicantDataCondition.php +++ b/src/Form/Service/Condition/FirstParticipantIsApplicantCondition.php @@ -8,22 +8,16 @@ use App\Form\Model\BookingDto; use App\Form\Service\Contract\FieldConditionInterface; /** - * Condition for "participant[0] is the person the block is built from". + * Condition for "participant[0] is the booking's applicant". * * Use it where a field is only mandatory because BusPro asks it of the Anmelder rather than of * an ordinary participant, such as mobile and address (an empty anschrift passes for a * participant, measured on 2036). * - * It deliberately tracks $applicantTravels, the same condition - * BookingPayloadBuilder::buildCreatePayload() keys its $applicantSource on. That keeps internal - * agency bookings unchanged: their applicant never travels, but their payload still takes the - * anmelder from participant[0], so participant[0] still owes the Anmelder's fields. This is why - * it is not isFirstParticipantApplicant(), which answers false for them. - * * FirstParticipantCondition remains the right choice where plain index 0 is meant, such as * showing a field on the first card regardless of who that person is. */ -class FirstParticipantSuppliesApplicantDataCondition implements FieldConditionInterface +class FirstParticipantIsApplicantCondition implements FieldConditionInterface { /** * @param BookingDto $bookingDto The current booking data @@ -32,7 +26,7 @@ class FirstParticipantSuppliesApplicantDataCondition implements FieldConditionIn */ public function evaluate(BookingDto $bookingDto, int $participantIndex, array $formData): bool { - return 0 === $participantIndex && $bookingDto->applicantTravels; + return 0 === $participantIndex && $bookingDto->isFirstParticipantApplicant(); } /** @@ -45,6 +39,6 @@ class FirstParticipantSuppliesApplicantDataCondition implements FieldConditionIn public function getDescription(): string { - return 'Checks if participant[0] supplies the anmelder block'; + return 'Checks if participant[0] is the applicant'; } } diff --git a/src/Form/Service/Condition/FirstParticipantReadOnlyCondition.php b/src/Form/Service/Condition/FirstParticipantReadOnlyCondition.php index 1709767e..dc43f4da 100644 --- a/src/Form/Service/Condition/FirstParticipantReadOnlyCondition.php +++ b/src/Form/Service/Condition/FirstParticipantReadOnlyCondition.php @@ -12,14 +12,14 @@ use App\Form\Service\Contract\FieldConditionInterface; * * It fires only where participant[0] IS the booking's applicant, which * BookingDto::isFirstParticipantApplicant() decides: the booking-level choice in create mode, - * and anmelder vs. participant[0] person id in edit mode. Internal agency bookings answer - * false there, which preserves the behaviour this class used to spell out itself. + * and anmelder vs. participant[0] person id in edit mode, where internal agency bookings + * always answer false. * * Business Rules: * - CREATE mode (anonymous user): First participant data is editable (user must fill it in) * - CREATE mode (logged-in, participant[0] is the applicant): data is prepopulated, read-only - * - CREATE mode (applicant does not travel, or internal agency): participant[0] is an ordinary - * traveller, data is editable + * - CREATE mode (applicant does not travel): participant[0] is an ordinary traveller, data is + * editable * - EDIT mode (participant[0] is the applicant): always read-only * - EDIT mode (split booking, or internal agency): participant[0] ≠ applicant, editable here * (PersonalDataMutabilityCondition still applies separately) diff --git a/src/Form/Service/CreateFieldStateProvider.php b/src/Form/Service/CreateFieldStateProvider.php index 3668f940..8c6ea302 100644 --- a/src/Form/Service/CreateFieldStateProvider.php +++ b/src/Form/Service/CreateFieldStateProvider.php @@ -16,8 +16,8 @@ use App\Form\Service\Condition\FamilyInsuranceActiveCondition; use App\Form\Service\Condition\FieldValueCondition; use App\Form\Service\Condition\FinalBookingOnlyCondition; use App\Form\Service\Condition\FirstParticipantCondition; +use App\Form\Service\Condition\FirstParticipantIsApplicantCondition; use App\Form\Service\Condition\FirstParticipantReadOnlyCondition; -use App\Form\Service\Condition\FirstParticipantSuppliesApplicantDataCondition; use App\Form\Service\Condition\MultipleParticipantsCondition; use App\Form\Service\Condition\RentalInsuranceAvailableCondition; use App\Form\Service\Condition\RentalSelectionCondition; @@ -88,7 +88,7 @@ class CreateFieldStateProvider extends AbstractFieldStateProvider ); // First participant read-only condition - // Render personal data fields as static text for first participant in non-internal agency bookings + // Render personal data fields as static text for the first participant where they are the applicant // when user is logged in (data is prepopulated from BPN account) $firstParticipantReadOnlyCondition = new FirstParticipantReadOnlyCondition(); @@ -122,7 +122,7 @@ class CreateFieldStateProvider extends AbstractFieldStateProvider // applicant (guest bookings) — BusPro does not ask a telefonmobil of an ordinary participant. $this->fieldStateConditions['mobile'] = [ 'static_text' => $firstParticipantReadOnlyCondition, - 'required' => new FirstParticipantSuppliesApplicantDataCondition(), + 'required' => new FirstParticipantIsApplicantCondition(), ]; // Address subfields - must render as static text to prevent creating duplicate BPN records @@ -319,7 +319,7 @@ class CreateFieldStateProvider extends AbstractFieldStateProvider // ordinary participant. Also rendered as static text for the applicant when read-only. $this->fieldStateConditions['address'] = [ 'static_text' => $firstParticipantReadOnlyCondition, - 'required' => new FirstParticipantSuppliesApplicantDataCondition(), + 'required' => new FirstParticipantIsApplicantCondition(), ]; // Room assignment field: diff --git a/src/Form/Service/ParticipantEmailFieldHandler.php b/src/Form/Service/ParticipantEmailFieldHandler.php index 68052107..e19f48b0 100644 --- a/src/Form/Service/ParticipantEmailFieldHandler.php +++ b/src/Form/Service/ParticipantEmailFieldHandler.php @@ -14,7 +14,8 @@ use App\Form\Service\Abstract\AbstractParticipantFieldHandler; * BusPro requires a Mail-Adresse for every participant once a room is present and rejects the * step-3 Anfrage with 650 otherwise, children included. It does accept an address identical to * the Anmelder's, so children inherit the applicant's rather than being asked for one they - * typically do not have. The value stays editable and stays mandatory. + * typically do not have. The value stays editable and stays mandatory. This happens in the + * create flow only; in edit mode the handler just stores what was submitted. * * The copy only ever fills a blank, so an address somebody typed is never overwritten. A later * correction to the applicant's own address therefore does not propagate to children who already @@ -65,6 +66,13 @@ class ParticipantEmailFieldHandler extends AbstractParticipantFieldHandler */ public function applyApplicantEmail(BookingDto $bookingDto, ParticipantDto $participant): void { + // Create mode only. An existing booking already carries whatever BusPro accepted, a + // blank may be intended where personal data is optional, and participant[0] is not + // reliably the applicant there. + if (BookingDto::MODE_CREATE !== $bookingDto->getMode()) { + return; + } + if (false === $participant->isChild()) { return; } diff --git a/src/Service/ParticipantDataPrefiller.php b/src/Service/ParticipantDataPrefiller.php index 9f2bfdbc..5c70d14c 100644 --- a/src/Service/ParticipantDataPrefiller.php +++ b/src/Service/ParticipantDataPrefiller.php @@ -132,9 +132,6 @@ class ParticipantDataPrefiller return self::TOKEN === $participant->lastName; } - /** - * Fills the participant DTO with generated dummy personal data. - */ /** * Fills the applicant DTO with generated dummy contact data. * @@ -162,6 +159,9 @@ class ParticipantDataPrefiller $applicant->address = $address; } + /** + * Fills the participant DTO with generated dummy personal data. + */ public function fillDummyParticipant(ParticipantDto $participant, int $participantIndex): void { $participantNumber = $participantIndex + 1; diff --git a/templates/booking/create/step_2.html.twig b/templates/booking/create/step_2.html.twig index eb5dbe7b..290a7d6e 100644 --- a/templates/booking/create/step_2.html.twig +++ b/templates/booking/create/step_2.html.twig @@ -54,27 +54,25 @@ {% endif %} {% set bookingDto = bookingCreateContext.bookingDto %} - {% if not bookingDto.isInternalAgencyBooking() %} - {# No form_start here: this sits inside the step 2 form and nested - form tags are invalid. htmx posts the inputs enclosed below, - including the CSRF token, to the toggle route. #} -
- {{ form_row(applicantTravelsForm.applicantTravels) }} - {{ form_rest(applicantTravelsForm) }} + {# No form_start here: this sits inside the step 2 form and nested + form tags are invalid. htmx posts the inputs enclosed below, + including the CSRF token, to the toggle route. #} +
+ {{ form_row(applicantTravelsForm.applicantTravels) }} + {{ form_rest(applicantTravelsForm) }} - {% if not bookingDto.applicantTravels %} - {% include 'booking/create/_applicant_card.html.twig' with { - 'applicant': bookingDto.applicant, - 'isSubmitted': bookingCreateContext.isSubmitted, - 'applicantErrors': applicantErrors - } %} - {% endif %} -
- {% endif %} + {% if not bookingDto.applicantTravels %} + {% include 'booking/create/_applicant_card.html.twig' with { + 'applicant': bookingDto.applicant, + 'isSubmitted': bookingCreateContext.isSubmitted, + 'applicantErrors': applicantErrors + } %} + {% endif %} +
{% for cardData in bookingCreateContext.cardsData %} diff --git a/tests/Form/Model/BookingDtoTest.php b/tests/Form/Model/BookingDtoTest.php index 92753ba1..73c18ff1 100644 --- a/tests/Form/Model/BookingDtoTest.php +++ b/tests/Form/Model/BookingDtoTest.php @@ -148,14 +148,25 @@ class BookingDtoTest extends TestCase $this->assertFalse($dto->isFirstParticipantApplicant()); } - public function testInternalAgencyBookingNeverHasTheFirstParticipantAsApplicant(): void + public function testInternalAgencyBookingNeverHasTheFirstParticipantAsApplicantInEditMode(): void { - $dto = $this->createCreateModeDto(); + $dto = $this->createEditModeDto(100, 100); $dto->agencyCode = AgencyLoader::INTERNAL_AGENCY_CODE; $this->assertFalse( $dto->isFirstParticipantApplicant(), - 'Their applicant is a staff member who never travels' + 'Their applicant is a customer who is not the first traveller, whatever the ids say' + ); + } + + public function testAgencyCodeDoesNotDecideAnythingInCreateMode(): void + { + $dto = $this->createCreateModeDto(); + $dto->agencyCode = AgencyLoader::INTERNAL_AGENCY_CODE; + + $this->assertTrue( + $dto->isFirstParticipantApplicant(), + 'Internal agency bookings are never created here, so the code is only passed on' ); } @@ -358,6 +369,42 @@ class BookingDtoTest extends TestCase $this->assertContains('Die anmeldende Person muss volljährig sein.', $messages); } + public function testEditModeDoesNotAskASplitBookingsFirstParticipantForAnAddress(): void + { + $dto = $this->createEditModeDto(100, 200); + $dto->participants[0]->index = 0; + $dto->participants[0]->address = null; + + $this->assertSame([], $this->collectFirstParticipantAddressPaths($dto)); + } + + public function testEditModeStillAsksTheApplicantForAnAddress(): void + { + $dto = $this->createEditModeDto(100, 100); + $dto->participants[0]->index = 0; + $dto->participants[0]->address = null; + + $this->assertSame(['participants[0].address'], $this->collectFirstParticipantAddressPaths($dto)); + } + + /** + * @return list + */ + private function collectFirstParticipantAddressPaths(BookingDto $dto): array + { + $validator = Validation::createValidatorBuilder()->enableAttributeMapping()->getValidator(); + $violations = $validator->validate($dto, null, ['booking_edit', 'strict_required']); + + $paths = []; + foreach ($violations as $violation) { + if (str_starts_with($violation->getPropertyPath(), 'participants[0].address')) { + $paths[] = $violation->getPropertyPath(); + } + } + + return $paths; + } + private function createAddress(): Address { $address = new Address(); diff --git a/tests/Form/Model/ParticipantEditDtoTest.php b/tests/Form/Model/ParticipantEditDtoTest.php index 2f04f7e8..a1c06e43 100644 --- a/tests/Form/Model/ParticipantEditDtoTest.php +++ b/tests/Form/Model/ParticipantEditDtoTest.php @@ -8,6 +8,7 @@ use App\BusProNet\Constants; use App\BusProNet\Model\Address; use App\BusProNet\Model\Booking; use App\BusProNet\Model\Insurance; +use App\BusProNet\Model\PersonalData; use App\BusProNet\Model\Service; use App\BusProNet\Model\Travel; use App\BusProNet\XmlLoader\AgencyLoader; @@ -703,7 +704,6 @@ class ParticipantEditDtoTest extends TestCase $bookingDto = new BookingDto($travel, 1); $bookingDto->booking = new Booking(); - $bookingDto->agencyCode = AgencyLoader::INTERNAL_AGENCY_CODE; $participant0 = $this->createAdultParticipant('first@example.com'); $participant0->index = 0; @@ -740,6 +740,38 @@ class ParticipantEditDtoTest extends TestCase $this->assertCount(0, $addressViolations1, 'Non-applicant address should not be required in edit submission'); } + public function testEditSubmissionDoesNotAskASplitBookingsFirstParticipantForAnAddress(): void + { + $travel = $this->createTravelOfferingSkiPasses(); + + $applicant = new PersonalData(); + $applicant->personId = 100; + + $bookingDto = new BookingDto($travel, 1); + $bookingDto->booking = new Booking(); + $bookingDto->booking->applicant = $applicant; + + $participant0 = $this->createAdultParticipant('first@example.com'); + $participant0->index = 0; + $participant0->personId = 200; + $participant0->address = null; + + $bookingDto->participants = [$participant0]; + + $wrapper0 = new ParticipantEditDto( + participant: $bookingDto->participants[0], + bookingContext: $bookingDto, + ); + + $violations0 = $this->validator->validate($wrapper0, null, ['booking_edit', 'strict_required']); + $addressViolations0 = array_filter( + iterator_to_array($violations0), + fn ($v) => 'address' === $v->getPropertyPath() || 'participant.address' === $v->getPropertyPath() + ); + + $this->assertCount(0, $addressViolations0, 'A traveller who is not the applicant owes no address'); + } + public function testSkiPassAgeCalculatedAtTravelDate(): void { $travel = $this->createTravelOfferingSkiPasses(); diff --git a/tests/Form/Service/ParticipantEmailFieldHandlerTest.php b/tests/Form/Service/ParticipantEmailFieldHandlerTest.php index 156deeac..20a4cf63 100644 --- a/tests/Form/Service/ParticipantEmailFieldHandlerTest.php +++ b/tests/Form/Service/ParticipantEmailFieldHandlerTest.php @@ -4,7 +4,9 @@ declare(strict_types=1); namespace App\Tests\Form\Service; +use App\BusProNet\Model\Booking; use App\BusProNet\Model\Travel; +use App\BusProNet\XmlLoader\AgencyLoader; use App\Form\Model\BookingDto; use App\Form\Model\ParticipantDto; use App\Form\Service\ParticipantDateOfBirthFieldHandler; @@ -79,6 +81,30 @@ class ParticipantEmailFieldHandlerTest extends TestCase $this->assertSame('elternteil@example.com', $bookingDto->participants[0]->email); } + public function testEditModeNeverPreFills(): void + { + $bookingDto = $this->createBookingDto(); + $bookingDto->booking = new Booking(); + + $this->handler->processField(['email' => ''], $bookingDto, 1); + + $this->assertNull( + $bookingDto->participants[1]->email, + 'participant[0] is not reliably the applicant in edit mode, and a blank may be intended' + ); + } + + public function testEditModeLeavesAnInternalAgencyChildWithoutEmail(): void + { + $bookingDto = $this->createBookingDto(); + $bookingDto->booking = new Booking(); + $bookingDto->agencyCode = AgencyLoader::INTERNAL_AGENCY_CODE; + + $this->handler->processField(['email' => ''], $bookingDto, 1); + + $this->assertNull($bookingDto->participants[1]->email); + } + public function testHandlerRunsAfterDateOfBirth(): void { $this->assertSame(