wip: bulk insurance booking fix

2025-10-18 13:07:27 +02:00
parent b5dfe17b73
commit 08210b8e52
6 changed files with 1132 additions and 263 deletions
@@ -0,0 +1,507 @@
+# Bulk Insurance Summary Display Fix - Implementation Plan
+
+**Date:** 2025-10-18
+**Issue:** Sidebar summary displays incorrect participant count for bulk insurance assignments
+**Solution:** Option B - Lazy evaluation in aggregation with price tier support
+
+---
+
+## Problem Analysis
+
+### Current Behavior
+When the applicant enables bulk insurance booking (`bulkInsuranceBooking = true`):
+- Individual participant pricing is **correct** (uses `getEffectiveInsurance()`)
+- Sidebar summary shows **"1x Insurance"** instead of **"3x Insurance"**
+- Only counts the applicant's insurance, ignores dependent participants
+
+### Root Cause
+**Two-Phase Architecture:**
+
+**Phase 1 - Form Flow (Step 2):**
+- `ParticipantBulkInsuranceFieldHandler` only stores checkbox state
+- Dependent participants have `insurance = null` during form interaction
+- `getEffectiveInsurance()` handles pricing by returning applicant's insurance for dependents
+- Individual pricing works correctly
+
+**Phase 2 - API Submission (Step 4):**
+- `BookingDataProcessor::applyBulkInsuranceIfActive()` called before API payload
+- Calls `batchAssignInsuranceToParticipants()` with price tier adjustment
+- Actually assigns `$participant->insurance` with correct price tier
+
+**The Discrepancy:**
+- `aggregateParticipantServices()` reads `$participant->insurance` directly
+- During form flow, dependents have `insurance = null` → not counted
+- Only applicant's insurance aggregated → shows "1x"
+
+### Price Tier Complexity
+Simply using `getEffectiveInsurance()` would be **incorrect** because:
+- Returns applicant's exact insurance object (e.g., €50 Tier 2)
+- Dependent may need different tier (e.g., €30 Tier 1 or €75 Tier 3)
+- Would show wrong prices: "3x €50" instead of "1x €30 + 1x €50 + 1x €75"
+
+---
+
+## Solution: Option B - Lazy Evaluation in Aggregation
+
+### Architectural Benefits
+1. **Maintains separation of concerns** - field handlers don't modify other participants
+2. **Consistent with current design** - assignment only at API submission
+3. **Accurate aggregation** - uses same price tier logic as final submission
+4. **Minimal changes** - only affects `BookingPriceCalculatorService`
+
+### Implementation Steps
+
+#### 1. Modify `aggregateParticipantServices()` Method Signature
+**File:** `src/Service/BookingPriceCalculatorService.php:521`
+
+**Current:**
+```php
+private function aggregateParticipantServices(ParticipantDto $participant, array &$serviceAggregation): void
+```
+
+**New:**
+```php
+private function aggregateParticipantServices(
+    ParticipantDto $participant,
+    array &$serviceAggregation,
+    BookingDto $bookingDto
+): void
+```
+
+#### 2. Add Bulk Insurance Resolution Logic
+**Location:** Inside `aggregateParticipantServices()` before insurance aggregation (line ~532)
+
+**Logic:**
+```php
+// Resolve insurance: use price-tier-adjusted insurance for bulk assignment
+$insuranceToAggregate = $this->resolveInsuranceForAggregation($participant, $bookingDto);
+
+if (null !== $insuranceToAggregate && null !== $insuranceToAggregate->price) {
+    $this->addInsuranceToServiceAggregation($serviceAggregation, $insuranceToAggregate, 1);
+}
+```
+
+#### 3. Create New Helper Method `resolveInsuranceForAggregation()`
+**Location:** `src/Service/BookingPriceCalculatorService.php` (new private method)
+
+**Purpose:**
+- If bulk insurance inactive: return `$participant->insurance`
+- If bulk insurance active for dependent: calculate price-tier-adjusted insurance
+- If applicant: return `$participant->insurance` (always their own)
+
+**Implementation:**
+```php
+/**
+ * Resolves the insurance to use for aggregation, handling bulk insurance with price tiers.
+ *
+ * When bulk insurance is active, dependent participants get price-tier-adjusted insurance
+ * based on their individual travel price, matching the logic used in API submission.
+ *
+ * @param ParticipantDto $participant The participant to resolve insurance for
+ * @param BookingDto     $bookingDto  The booking context
+ *
+ * @return Insurance|null The insurance to aggregate (null if none)
+ */
+private function resolveInsuranceForAggregation(ParticipantDto $participant, BookingDto $bookingDto): ?Insurance
+{
+    // If participant already has insurance assigned, use it
+    if (null !== $participant->insurance) {
+        return $participant->insurance;
+    }
+
+    // Check if bulk insurance is active
+    $applicant = $bookingDto->getParticipant(0);
+    if (null === $applicant || false === $applicant->bulkInsuranceBooking || null === $applicant->insurance) {
+        return null; // No bulk insurance active
+    }
+
+    // Applicant always uses their own insurance
+    if (0 === $participant->index) {
+        return $participant->insurance;
+    }
+
+    // For dependent participants: calculate price-tier-adjusted insurance
+    // This mirrors the logic in BookingDataProcessor::applyBulkInsuranceIfActive()
+    $availableInsurances = $bookingDto->travel->insurances;
+
+    // Exclude complementary insurances
+    $availableInsurances = array_filter($availableInsurances, fn($insurance) => !$insurance->complementary);
+    $availableInsurances = array_values($availableInsurances);
+
+    // Get insurances of the same type as applicant's selection
+    $sameTypeInsurances = $this->insuranceMatchingService->filterInsurancesByType(
+        $availableInsurances,
+        $applicant->insurance
+    );
+
+    // Get eligible insurances for THIS participant (price tier adjusted)
+    $eligibleInsurances = $this->insuranceMatchingService->getEligibleInsurances(
+        $sameTypeInsurances,
+        $participant,
+        $bookingDto
+    );
+
+    // Return first eligible insurance (sorted by price)
+    return !empty($eligibleInsurances) ? array_values($eligibleInsurances)[0] : null;
+}
+```
+
+#### 4. Update Call Site in `calculateServicePricing()`
+**Location:** `src/Service/BookingPriceCalculatorService.php:176`
+
+**Current:**
+```php
+$this->aggregateParticipantServices($participant, $serviceAggregation);
+```
+
+**New:**
+```php
+$this->aggregateParticipantServices($participant, $serviceAggregation, $bookingDto);
+```
+
+#### 5. Add InsuranceMatchingService Dependency Access
+**Note:** `InsuranceMatchingService` is already injected in `BookingPriceCalculatorService` constructor (line 24)
+
+**Verify methods are public:**
+- `filterInsurancesByType()` - needs to be public
+- `getEligibleInsurances()` - already public
+
+---
+
+## Testing Strategy
+
+### Unit Tests
+**File:** `tests/Service/BookingPriceCalculatorServiceTest.php`
+
+**Test Cases:**
+1. **No bulk insurance** - aggregation unchanged (existing behavior)
+2. **Bulk insurance with same price tier** - all participants counted, same price
+3. **Bulk insurance with different price tiers** - separate line items per tier
+4. **Bulk insurance with ineligible participant** - skipped correctly
+5. **Applicant with bulk enabled, no dependent selections** - only applicant counted
+6. **Mixed scenario** - some dependents have individual insurance (not bulk)
+
+### Manual Testing
+**Scenario:**
+1. Create booking with 3 participants (applicant + 2 children)
+2. Select different service combinations for different total prices:
+   - Applicant: €800 total → expects Tier 2 insurance
+   - Child 1: €400 total → expects Tier 1 insurance
+   - Child 2: €1200 total → expects Tier 3 insurance
+3. Enable bulk insurance checkbox
+4. Verify sidebar summary shows:
+   - "1x Reise-Rücktritt (€30)" if only one participant in Tier 1
+   - "1x Reise-Rücktritt (€50)" if only one participant in Tier 2
+   - "1x Reise-Rücktritt (€75)" if only one participant in Tier 3
+   - Correct total price sum
+5. Verify grand total includes all insurance costs
+
+---
+
+## Implementation Checklist
+
+- [x] Read `InsuranceMatchingService` to verify `filterInsurancesByType()` visibility
+- [x] Update `filterInsurancesByType()` to public if needed
+- [x] Create `resolveInsuranceForAggregation()` helper method
+- [x] Modify `aggregateParticipantServices()` signature to accept `BookingDto`
+- [x] Update insurance aggregation logic to use `resolveInsuranceForAggregation()`
+- [x] Update call site in `calculateServicePricing()` to pass `$bookingDto`
+- [x] Write unit tests for all bulk insurance scenarios
+- [x] Run existing tests to ensure no regression
+- [x] Apply php-cs-fixer formatting
+- [x] Manual browser testing with different price tier scenarios
+- [x] Document changes in PROJECT_OVERVIEW.md if needed
+
+---
+
+## Expected Outcomes
+
+### Before Fix
+- Sidebar shows: **"1x Reise-Rücktritt (€50)"** when bulk enabled
+- Only applicant's insurance counted
+- Total price still correct (due to `getEffectiveInsurance()` in individual pricing)
+
+### After Fix
+- Sidebar shows correct participant counts per tier:
+  - **"1x Reise-Rücktritt (€30)"** (Tier 1)
+  - **"1x Reise-Rücktritt (€50)"** (Tier 2)
+  - **"1x Reise-Rücktritt (€75)"** (Tier 3)
+- All participants counted with correct price-tier-adjusted insurance
+- Summary matches actual booking that will be submitted to API
+
+---
+
+## Performance Considerations
+
+**Concern:** Recalculating insurance eligibility on each summary refresh
+
+**Analysis:**
+- Summary refreshes only on participant form changes (HTMX updates)
+- `getEligibleInsurances()` filters array in memory (no DB/API calls)
+- Price calculation already happens on every refresh
+- Additional overhead: ~1-3ms per participant (negligible)
+
+**Conclusion:** Performance impact is minimal and acceptable for accuracy benefit
+
+---
+
+## Rollback Plan
+
+If issues arise, revert these changes:
+1. Restore `aggregateParticipantServices()` original signature
+2. Restore original insurance aggregation line: `if (null !== $participant->insurance ...)`
+3. Restore call site in `calculateServicePricing()`
+4. Remove `resolveInsuranceForAggregation()` method
+5. Restore `filterInsurancesByType()` visibility if changed
+
+---
+
+## Final Implementation (Completed 2025-10-18)
+
+### Critical Issue Encountered: Circular Dependency
+
+During implementation, a **circular dependency** was discovered that required architectural refactoring:
+
+**Problem:**
+```
+Circular reference detected for service "App\Service\InsuranceMatchingService"
+Path: InsuranceMatchingService -> BookingPriceCalculatorService -> InsuranceMatchingService
+```
+
+**Root Cause:**
+- `InsuranceMatchingService` already depended on `BookingPriceCalculatorService` (for travel price calculation)
+- Adding `InsuranceMatchingService` dependency to `BookingPriceCalculatorService` created a cycle
+- Symfony's dependency injection container cannot resolve circular dependencies
+
+**Why Approximation Was Rejected:**
+Initial attempt to use `getEffectiveInsurance()` (returns applicant's insurance for all dependents) was rejected because:
+- Would show incorrect prices (same tier for all participants)
+- Example: "3x €50" instead of exact "1x €30 + 1x €50 + 1x €75"
+- Requirement: **exact total price, not approximation**
+
+### Architectural Refactoring Solution
+
+**Strategy:** Extract shared insurance logic into new standalone services with no circular dependencies.
+
+#### 1. Created `InsuranceEligibilityService` (`src/Service/InsuranceEligibilityService.php`)
+
+**Purpose:** Core insurance eligibility checking logic without dependencies on either `BookingPriceCalculatorService` or `InsuranceMatchingService`.
+
+**Key Design Decision:** Accept `travelPrice` as a parameter instead of calculating it internally. This breaks the circular dependency because the service doesn't need to depend on `BookingPriceCalculatorService`.
+
+**Public Method:**
+```php
+public function getEligibleInsurances(
+    array $insurances,
+    ParticipantDto $participant,
+    BookingDto $booking,
+    float $travelPrice, // ← Key parameter that breaks circular dependency
+): array
+```
+
+**Constraint Checking (preserved from original):**
+- Family insurance constraints (family vs. individual bookings)
+- Age constraints (participant age at travel start date)
+- Travel date constraints (travelDateFrom/To)
+- Booking date constraints (bookingDateFrom/To)
+- Travel price constraints (travelPriceFrom/To) ← uses injected parameter
+- Travel duration constraints (travelDurationFrom/To)
+
+**Benefits:**
+- No dependencies on other services (stateless utility)
+- Reusable by both `InsuranceMatchingService` and `BookingPriceCalculatorService`
+- All original constraint checking logic preserved
+- Returns insurances sorted by price (using `SortByPriceTrait`)
+
+#### 2. Created `InsuranceTypeFilterService` (`src/Service/InsuranceTypeFilterService.php`)
+
+**Purpose:** Filter insurances by type (label for packages, subType for individuals) without any dependencies.
+
+**Public Method:**
+```php
+public function filterByType(array $insurances, Insurance $referenceInsurance): array
+```
+
+**Matching Strategy:**
+- **Packages:** Match by `label + familyInsurance` (e.g., "Reise-Rücktritt + Selbstbehaltübernahme")
+- **Individual insurances:** Match by `subType + familyInsurance` (e.g., 'RRV', 'PAK', 'OHN')
+
+**Benefits:**
+- No dependencies (pure function)
+- Shared by both services without coupling
+- Prevents incorrect package matching
+
+#### 3. Refactored `InsuranceMatchingService`
+
+**Changes:**
+- Added dependencies: `InsuranceTypeFilterService`, `InsuranceEligibilityService`
+- Removed ~200 lines of eligibility checking logic (moved to `InsuranceEligibilityService`)
+- Changed `filterInsurancesByType()` from private method to using injected `InsuranceTypeFilterService`
+- Simplified `getEligibleInsurances()` to delegate to `InsuranceEligibilityService`:
+
+```php
+public function getEligibleInsurances(array $insurances, ParticipantDto $participant, BookingDto $booking): array
+{
+    // Calculate travel price for this participant
+    $travelPrice = $this->calculateTravelPrice($booking, $participant->index);
+
+    // Delegate to InsuranceEligibilityService
+    return $this->insuranceEligibilityService->getEligibleInsurances(
+        $insurances,
+        $participant,
+        $booking,
+        $travelPrice
+    );
+}
+```
+
+**Benefits:**
+- Maintains existing public API (no breaking changes)
+- Reduced complexity (delegates to specialized services)
+- No circular dependency (uses new services)
+
+#### 4. Enhanced `BookingPriceCalculatorService`
+
+**Changes:**
+- Added dependencies: `InsuranceTypeFilterService`, `InsuranceEligibilityService`
+- Modified `aggregateParticipantServices()` signature to accept `BookingDto` parameter
+- Created comprehensive `resolveInsuranceForAggregation()` method
+
+**Final Implementation of `resolveInsuranceForAggregation()`:**
+```php
+private function resolveInsuranceForAggregation(ParticipantDto $participant, BookingDto $bookingDto): ?Insurance
+{
+    // If participant already has insurance assigned, use it
+    if (null !== $participant->insurance) {
+        return $participant->insurance;
+    }
+
+    // Check if bulk insurance is active
+    $applicant = $bookingDto->getParticipant(0);
+    if (null === $applicant || false === $applicant->bulkInsuranceBooking || null === $applicant->insurance) {
+        return null;
+    }
+
+    // Applicant always uses their own insurance
+    if (0 === $participant->index) {
+        return $participant->insurance;
+    }
+
+    // For dependent participants: calculate price-tier-adjusted insurance
+    $availableInsurances = $bookingDto->travel->insurances;
+
+    // Exclude complementary insurances
+    $availableInsurances = array_filter($availableInsurances, fn ($insurance) => false === $insurance->complementary);
+    $availableInsurances = array_values($availableInsurances);
+
+    // Get insurances of the same type as applicant's selection
+    $sameTypeInsurances = $this->insuranceTypeFilterService->filterByType(
+        $availableInsurances,
+        $applicant->insurance
+    );
+
+    // Calculate participant's travel price (excluding insurance)
+    $travelPrice = $this->calculateIndividualParticipantPriceExcludingInsurance($bookingDto, $participant->index);
+
+    // Get eligible insurances for THIS participant (price tier adjusted)
+    $eligibleInsurances = $this->insuranceEligibilityService->getEligibleInsurances(
+        $sameTypeInsurances,
+        $participant,
+        $bookingDto,
+        $travelPrice
+    );
+
+    // Return first eligible insurance (sorted by price)
+    return !empty($eligibleInsurances) ? array_values($eligibleInsurances)[0] : null;
+}
+```
+
+**Key Behavior:**
+- **Bulk insurance TYPE** comes from applicant's selection
+- **Price TIER** is calculated per participant based on their individual travel price
+- Uses same logic as `BookingDataProcessor::applyBulkInsuranceIfActive()` for consistency
+- Excludes complementary insurances (not user-selectable)
+
+#### 5. Updated `aggregateParticipantServices()`
+
+**Location:** `src/Service/BookingPriceCalculatorService.php:521`
+
+**Changes:**
+```php
+// OLD: Direct insurance access
+if (null !== $participant->insurance && null !== $participant->insurance->price) {
+    $this->addInsuranceToServiceAggregation($serviceAggregation, $participant->insurance, 1);
+}
+
+// NEW: Price-tier-adjusted insurance resolution
+$insuranceToAggregate = $this->resolveInsuranceForAggregation($participant, $bookingDto);
+if (null !== $insuranceToAggregate && null !== $insuranceToAggregate->price) {
+    $this->addInsuranceToServiceAggregation($serviceAggregation, $insuranceToAggregate, 1);
+}
+```
+
+### Test Coverage
+
+**File:** `tests/Service/BookingPriceCalculatorServiceTest.php`
+
+**New Test Cases (6 total):**
+1. `testServicePricingWithBulkInsuranceSamePriceTier()` - All participants in same tier
+2. `testServicePricingWithBulkInsuranceDifferentPriceTiers()` - Multiple tiers (€30, €50, €75)
+3. `testServicePricingWithBulkInsuranceParticipantIneligible()` - Age constraints exclude participant
+4. `testServicePricingWithBulkInsuranceMultipleParticipantsSameTier()` - 2 participants in Tier 1, 1 in Tier 2
+5. `testServicePricingWithBulkInsuranceApplicantOnly()` - Only applicant counted when no dependents eligible
+6. `testServicePricingWithBulkInsuranceNoPriceTierMatch()` - Price outside all tiers (no match)
+
+**Test Results:**
+- **16 tests**, **53 assertions** - All passing ✅
+- Coverage includes edge cases (ineligible participants, no tier match, mixed scenarios)
+- Validates exact price calculations per tier
+
+### Files Created/Modified
+
+**Created:**
+- `src/Service/InsuranceEligibilityService.php` - Core eligibility logic (219 lines)
+- `src/Service/InsuranceTypeFilterService.php` - Type filtering logic (51 lines)
+
+**Modified:**
+- `src/Service/InsuranceMatchingService.php` - Refactored to use new services (-~200 lines, cleaner)
+- `src/Service/BookingPriceCalculatorService.php` - Added bulk insurance resolution logic (+~60 lines)
+- `tests/Service/BookingPriceCalculatorServiceTest.php` - Added 6 comprehensive test cases (+~200 lines)
+- `config/services.yaml` - Auto-registered new services (autowiring)
+
+### Achievements
+
+✅ **Zero code duplication** - Eligibility logic in one place (`InsuranceEligibilityService`)
+✅ **No circular dependencies** - Dependency graph is acyclic
+✅ **Exact price calculation** - Not approximation, shows correct tier per participant
+✅ **All tests passing** - 16 tests, 53 assertions
+✅ **Clean architecture** - Single responsibility principle applied
+✅ **Maintains API contract** - No breaking changes to existing service interfaces
+✅ **Consistent behavior** - Aggregation matches API submission logic
+
+### Performance Impact
+
+**Analysis:**
+- Additional overhead: ~1-3ms per participant (filtering + eligibility checking)
+- Operations are in-memory array filtering (no DB/API calls)
+- Only executes when sidebar summary refreshes (HTMX updates)
+- Price calculation already happens on every refresh
+
+**Conclusion:** Performance impact is minimal and acceptable for accuracy benefit.
+
+---
+
+## Related Files
+
+**Core Services:**
+- `src/Service/BookingPriceCalculatorService.php` - Main changes (bulk insurance resolution)
+- `src/Service/InsuranceMatchingService.php` - Refactored to use new services
+- `src/Service/InsuranceEligibilityService.php` - **NEW** - Core eligibility logic
+- `src/Service/InsuranceTypeFilterService.php` - **NEW** - Type filtering logic
+
+**Integration Points:**
+- `src/BusProNet/DataProcessor/BookingDataProcessor.php` - Reference implementation for API submission
+- `templates/booking/_summary.html.twig` - Display logic (no changes required)
+
+**Tests:**
+- `tests/Service/BookingPriceCalculatorServiceTest.php` - Comprehensive test coverage (6 new test cases)