183 lines
6.8 KiB
Markdown
183 lines
6.8 KiB
Markdown
# Booking Data Processor Refactoring Plan
|
|
|
|
## Current Status (2025-10-05)
|
|
|
|
We discovered a structural inconsistency between the UPDATE and CREATE booking flows while implementing the booking submission feature.
|
|
|
|
## Problem Statement
|
|
|
|
The UPDATE and CREATE flows use fundamentally different data structures, leading to code duplication and complexity:
|
|
|
|
### UPDATE Flow (Current)
|
|
- `BookingEditDto` contains a `Booking` object
|
|
- `Booking` has centralized service arrays with participant mappings:
|
|
- `booking.additionalServices` - array of Service objects with `mapping` property (0-based indices)
|
|
- `booking.transportationServices` - array of Service objects with `mapping` property
|
|
- `booking.pickupsOutbound` - array of Pickup objects with `mapping` property
|
|
- `BookingDataProcessor.createUpdateRequestPayload()`:
|
|
- Resets all service mappings
|
|
- Iterates through participants
|
|
- Rebuilds service mappings by looking up services in booking data
|
|
- Removes unused services
|
|
- Converts 0-based indices to 1-based for API
|
|
|
|
### CREATE Flow (Current)
|
|
- `BookingCreateDto` contains only `participants` array
|
|
- Each `ParticipantDto` has direct service references:
|
|
- `courses`, `skiPass`, `additionalServices`, `board`, `rentals`
|
|
- `transportationOutbound`, `transportationInbound`
|
|
- `pickupOutbound`, `pickupInbound`
|
|
- `insurance`
|
|
- `BookingDataProcessor.createBookingRequestPayload()`:
|
|
- Collects services directly from participants
|
|
- Groups by service ID
|
|
- Converts to 1-based participant IDs for API
|
|
|
|
## Root Cause
|
|
|
|
The UPDATE flow was designed to work with API-sourced `Booking` objects that already have centralized service mappings. The CREATE flow was designed from scratch with a simpler participant-centric approach.
|
|
|
|
## Proposed Solution
|
|
|
|
**Align both flows to use the participant-centric structure:**
|
|
|
|
1. **Both DTOs work the same way:**
|
|
- Both have `participants` array
|
|
- Services are attached directly to participants
|
|
- No centralized service objects with mappings
|
|
|
|
2. **Unified payload generation:**
|
|
- Use same `collect*Mappings()` methods for both flows
|
|
- Use same `addServicesFromMap()` helper
|
|
- Remove complex service manipulation in update flow
|
|
|
|
3. **Benefits:**
|
|
- Single source of truth for service mappings
|
|
- Less code duplication
|
|
- Easier to understand and maintain
|
|
- Consistent patterns across all booking operations
|
|
|
|
## Implementation Steps
|
|
|
|
### Phase 1: Refactor BookingEditDto.fromBooking()
|
|
- [x] Already populates participant services correctly
|
|
- [x] Services are already attached to participants
|
|
- [ ] Verify all service types are covered
|
|
|
|
### Phase 2: Refactor BookingDataProcessor.createUpdateRequestPayload()
|
|
- [ ] Remove `resetServiceMappings()`
|
|
- [ ] Remove `processParticipantServices()` (complex service lookup)
|
|
- [ ] Remove `processAdditionalServices()`
|
|
- [ ] Remove `processTransportationServices()`
|
|
- [ ] Remove `processPickupLocations()`
|
|
- [ ] Remove `removeUnusedServices()`
|
|
- [ ] Use `collect*Mappings()` methods instead (same as create flow)
|
|
- [ ] Update `buildServicesPayload()` to use collected maps
|
|
- [ ] Update `buildPickupPayload()` to use collected maps
|
|
|
|
### Phase 3: Add convertServicesToMap() Helper
|
|
- [ ] Create helper to convert service objects with mapping to ID => participant IDs map
|
|
- [ ] This bridges the gap between old structure (if needed) and new structure
|
|
|
|
### Phase 4: Testing
|
|
- [ ] Test update flow with all service types
|
|
- [ ] Test create flow (should remain unchanged)
|
|
- [ ] Verify API payloads are identical before/after refactoring
|
|
- [ ] Test edge cases (no services, all services, mixed scenarios)
|
|
|
|
### Phase 5: Cleanup
|
|
- [ ] Remove unused methods from BookingDataProcessor
|
|
- [ ] Remove unused properties from Booking model (if any)
|
|
- [ ] Update documentation
|
|
|
|
## Risk Assessment
|
|
|
|
**MEDIUM RISK** - This refactoring touches critical booking update functionality that is already working in production.
|
|
|
|
### Risks:
|
|
1. Breaking existing update flow
|
|
2. Subtle bugs in service mapping
|
|
3. Data loss if participant service references are incorrect
|
|
4. Payment/bank account handling might break
|
|
|
|
### Mitigation:
|
|
1. Comprehensive testing before deployment
|
|
2. Keep git history clean with atomic commits
|
|
3. Test with real booking data from sandbox
|
|
4. Verify XML payloads match exactly (before/after)
|
|
5. Have rollback plan ready
|
|
|
|
## Decision Point
|
|
|
|
**Should we refactor NOW or LATER?**
|
|
|
|
### Arguments for NOW:
|
|
- We're already in BookingDataProcessor
|
|
- Fresh understanding of both flows
|
|
- Prevents further divergence
|
|
- Makes current task (booking submission) cleaner
|
|
|
|
### Arguments for LATER:
|
|
- Current task (booking submission) is incomplete
|
|
- Refactoring is significant and risky
|
|
- Could introduce bugs in working update flow
|
|
- Should be separate PR with focused testing
|
|
- Current booking submission is more urgent
|
|
|
|
## Decision
|
|
|
|
**REFACTOR LATER** - Complete the booking submission task first, then do this refactoring as a separate focused effort.
|
|
|
|
**AGREED:** The CREATE flow's participant-centric structure is the new standard. The UPDATE flow should adopt this architecture in the future refactoring.
|
|
|
|
### Reasoning:
|
|
1. Booking submission is nearly complete and is the immediate business need
|
|
2. Update flow is working and tested - don't break what works
|
|
3. Refactoring deserves dedicated focus and testing
|
|
4. Can create comprehensive tests for both flows first
|
|
5. Allows for proper code review and QA
|
|
|
|
### Short-term Solution:
|
|
- Keep both flows separate for now
|
|
- Add the `convertServicesToMap()` helper to bridge structures
|
|
- Complete booking submission with current architecture
|
|
- Document this technical debt
|
|
|
|
### Long-term Plan:
|
|
- Create separate refactoring task/issue
|
|
- Write comprehensive tests for update flow first
|
|
- Perform refactoring in dedicated branch
|
|
- Extensive testing with sandbox data
|
|
- Separate PR with focused review
|
|
|
|
## Current Task: Booking Submission
|
|
|
|
We are 60% complete with booking submission implementation:
|
|
|
|
### Completed:
|
|
- [x] BookingResponse, PriceItem, PaymentTerms models
|
|
- [x] BookingResponseParser with pricing data
|
|
- [x] createBookingRequestPayload() in BookingDataProcessor
|
|
- [x] Payment type ID constants
|
|
- [x] Helper method addServicesFromMap()
|
|
|
|
### Remaining:
|
|
- [ ] Add TYPE_BOOKING constant to ApiClient
|
|
- [ ] Add createBookingInquiry() and createBooking() to ApiClient
|
|
- [ ] Update ApiResponseParser to handle BUCHUNG response type
|
|
- [ ] Implement two-phase submission in CreateStep4Controller
|
|
- [ ] Add clearBookingCreateDto() to BookingService
|
|
- [ ] Create BookingSuccessController and template
|
|
- [ ] Test with sandbox API
|
|
|
|
## Next Steps
|
|
|
|
1. **IMMEDIATE:** Continue with booking submission task
|
|
2. **AFTER COMPLETION:** Create refactoring issue/task
|
|
3. **FUTURE:** Dedicated refactoring effort with proper testing
|
|
|
|
---
|
|
|
|
**Document Created:** 2025-10-05
|
|
**Status:** Deferred - Continue with booking submission
|
|
**Related:** Booking submission implementation (in progress) |