feat: extract booking session handling
This commit is contained in:
Björn Fromme
@@ -0,0 +1,125 @@
|
||||
# Service Simplification Plan
|
||||
|
||||
Status: draft
|
||||
Last updated: 2026-04-05
|
||||
|
||||
## Purpose
|
||||
|
||||
This document tracks the next pass of service simplification work. The goal is to reduce orchestration density, make the booking flow easier to follow for a human reader, and keep responsibilities aligned with the actual boundaries in the code.
|
||||
|
||||
The emphasis is not on deleting services for its own sake. The emphasis is on:
|
||||
- keeping one clear owner for each meaningful boundary
|
||||
- removing thin wrappers and pass-through helpers
|
||||
- avoiding services that mostly shuffle data between layers
|
||||
- keeping presentation concerns out of calculation and orchestration code
|
||||
|
||||
## Current Read
|
||||
|
||||
The codebase is already in a better place than it was at the start of the refactor, but a few services still carry more than one responsibility:
|
||||
|
||||
- `BookingService` no longer owns session lifecycle, baseline snapshot handling, or return URL management. That work now lives in `BookingSessionService`, which keeps the booking orchestration boundary narrower.
|
||||
- `BookingService` still covers hydration, booking bootstrap, room grouping, participant counting, service preselection, and booking status rules.
|
||||
- `BookingPriceCalculatorService` is focused on pricing, but it still sits close to display-oriented behavior in adjacent code paths.
|
||||
- `TravelDataService` remains broad and is likely the next larger boundary after booking orchestration is reduced.
|
||||
|
||||
One registry stands out as a real orchestration boundary and should be left alone for now:
|
||||
- `ParticipantFieldHandlerRegistry`
|
||||
|
||||
It is not just a lookup table. It owns execution order, edit-mode mutability gating, and synchronization of submitted form data back into the DTO state.
|
||||
|
||||
## Next Pass
|
||||
|
||||
### 1. Reduce `BookingService`
|
||||
|
||||
Primary goal: make the booking create/edit flow easier to read by splitting unrelated concerns.
|
||||
|
||||
Concrete next steps:
|
||||
- keep booking session lifecycle in one place
|
||||
- extract baseline room snapshot handling into a narrower helper or dedicated service
|
||||
- separate return URL handling if it stays conceptually unrelated
|
||||
- keep `startFreshBooking()` focused on booking bootstrap rather than general session utilities
|
||||
- keep hydration behavior obvious and local to the booking session path
|
||||
|
||||
Decision rule:
|
||||
- if a method only forwards to DTO/session behavior, prefer removing the wrapper
|
||||
- if a method is a genuine workflow owner, keep it and narrow the surrounding API instead of splitting it into generic helpers
|
||||
|
||||
### 2. Keep pricing calculation focused
|
||||
|
||||
Primary goal: keep pricing code about pricing, not rendering.
|
||||
|
||||
Concrete next steps:
|
||||
- keep `BookingPriceCalculatorService` as the pricing boundary
|
||||
- continue removing display formatting from pricing code paths
|
||||
- keep any remaining view-specific formatting in the presentation layer or a dedicated UI helper
|
||||
- avoid introducing another service that only formats values already known to the view
|
||||
|
||||
Decision rule:
|
||||
- if a value is only needed for display, prefer exposing the raw numeric/domain value and formatting it as close to the UI as possible
|
||||
|
||||
### 3. Leave the field-handler registry in place
|
||||
|
||||
Primary goal: avoid unnecessary churn in a class that is already a meaningful orchestration layer.
|
||||
|
||||
Concrete next steps:
|
||||
- do not refactor `ParticipantFieldHandlerRegistry` in this pass
|
||||
- revisit only if a later change can split ordering, mutability, and synchronization into clear collaborators without making the flow harder to trace
|
||||
|
||||
Decision rule:
|
||||
- if a registry owns actual workflow behavior, treat it as a boundary rather than a smell
|
||||
|
||||
## Follow-Up Queue
|
||||
|
||||
After the booking service pass, the next likely candidates are:
|
||||
|
||||
### `TravelDataService`
|
||||
|
||||
This is the largest broad service still in the codebase. It likely needs a later pass if the application should become easier to follow end to end.
|
||||
|
||||
Likely directions:
|
||||
- separate runtime travel loading from cache/snapshot maintenance if the public API still feels too wide
|
||||
- keep the read path explicit and avoid hiding maintenance work behind one large method surface
|
||||
|
||||
### `BookingPriceCalculatorService`
|
||||
|
||||
This service should stay focused on pricing logic, but it may still have room for further internal cleanup if more display or transport aggregation concerns surface.
|
||||
|
||||
Likely directions:
|
||||
- keep calculation responsibilities together
|
||||
- avoid dragging presentation behavior back into the calculator
|
||||
- split only if a sub-boundary becomes obvious and reusable
|
||||
|
||||
### `ParticipantFieldHandlerRegistry`
|
||||
|
||||
This stays on the list only as a future optional refactor, not as an immediate target.
|
||||
|
||||
Likely directions, only if justified later:
|
||||
- isolate ordering/sorting logic if it becomes independently meaningful
|
||||
- split synchronization code if a clearer DTO/form boundary emerges
|
||||
- otherwise leave it as the central orchestration point for participant field processing
|
||||
|
||||
## Progress Tracker
|
||||
|
||||
| Item | Status | Notes |
|
||||
|------|--------|-------|
|
||||
| Participant card DTO cleanup | Done | Card data now uses typed DTOs instead of nested array payloads |
|
||||
| Room label formatting cleanup | Done | Pricing labels now have a dedicated presentation helper |
|
||||
| Booking service split | In progress | Session lifecycle, baseline snapshot, and return URL handling moved to `BookingSessionService` |
|
||||
| Pricing service review | Pending | Keep focused on calculation, not rendering |
|
||||
| Travel data service review | Pending | Broad boundary, likely later pass |
|
||||
| Participant field registry review | Deferred | Real orchestration boundary, intentionally left alone for now |
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
The next booking-service pass is only worth keeping if it:
|
||||
- reduces the number of unrelated responsibilities in `BookingService`
|
||||
- makes the booking flow easier to trace from controller to session/DTO state
|
||||
- preserves existing booking behavior and test coverage
|
||||
- does not replace one large service with several generic “manager” classes
|
||||
|
||||
## Working Agreement
|
||||
|
||||
- Update this document as decisions are made.
|
||||
- Record rejected simplification ideas here with a short reason.
|
||||
- Keep the plan aligned with actual code, not with an abstract architecture ideal.
|
||||
- If a future simplification does not clearly reduce cognitive load, do not add it.
|
||||
@@ -1,79 +0,0 @@
|
||||
# Travel Snapshot Persistence (DB-Primary + Extended Availability)
|
||||
|
||||
## Scope
|
||||
Persist travel payloads in DB and use them as primary local source, with enrichment from BusPro `VERFUEGBARKEIT2` (internal naming: **extended/ext**).
|
||||
|
||||
## Current Implementation Status
|
||||
|
||||
### Completed
|
||||
- [x] Added snapshot persistence entity and repository:
|
||||
- `src/Entity/TravelSnapshot.php`
|
||||
- `src/Repository/TravelSnapshotRepository.php`
|
||||
- [x] Added snapshot application service:
|
||||
- `src/Service/TravelSnapshotService.php`
|
||||
- Uses Symfony Serializer JSON payloads for `Travel`
|
||||
- Supports upsert/hash comparison, load, mapping, product lookup, refresh, purge
|
||||
- [x] Added extended availability integration:
|
||||
- `ApiClient::TYPE_AVAILABILITY_EXTENDED = 'VERFUEGBARKEIT2'`
|
||||
- `ApiClient::getAvailabilitiesExtended()`
|
||||
- parser dispatch in `ApiResponseParser`
|
||||
- `ExtendedAvailabilitiesParser`
|
||||
- models `ExtendedAvailability` and `ExtendedServiceAvailabilityResponse`
|
||||
- [x] Added DB-primary read path in `TravelDataService`:
|
||||
- tries snapshot first
|
||||
- falls back to XML parse + enrichment + snapshot upsert
|
||||
- snapshot mapping merged into `generateFilesMap()`
|
||||
- product lookup includes snapshot metadata fallback
|
||||
- [x] Added refresh command:
|
||||
- `app:travel:snapshot-refresh`
|
||||
- supports batch processing, force mode, optional purge
|
||||
- [x] Added migration for snapshot table:
|
||||
- `migrations/Version20260321120000.php`
|
||||
- [x] Added serializer type metadata/docblocks in relevant model classes to support stable snapshot deserialization.
|
||||
- [x] Removed unified travel cache from `TravelDataService::getTravelData()`:
|
||||
- cache and snapshot-version-token logic removed; travel is now loaded directly from snapshot DB (fast indexed lookup) or XML fallback on every call
|
||||
- `TravelSnapshotService::getCacheVersionToken()` removed along with it
|
||||
- [x] Fixed hotel-specific lookup correctness:
|
||||
- when `hotelId` is provided, snapshot lookup no longer falls back to another hotel of same date.
|
||||
|
||||
### Confirmed Behaviors
|
||||
- [x] Snapshot data survives XML deletion and remains loadable.
|
||||
- [x] Extended refresh updates service-level fields including `uhrzeit_von` -> `Service::timeFrom`.
|
||||
- [x] Runtime availability overlay (`VERFUEGBARKEIT`) remains in place.
|
||||
|
||||
## Design Decisions (Final)
|
||||
- Internal naming uses `extended/ext`; external request type string stays `VERFUEGBARKEIT2`.
|
||||
- DB snapshots are the primary local source for travel loading.
|
||||
- Serializer format is JSON via Symfony Serializer, not PHP `serialize()`.
|
||||
- No application-level cache wraps `getTravelData()`; snapshot DB is the fast path, XML is the fallback.
|
||||
|
||||
## Operational Commands
|
||||
- Refresh snapshots:
|
||||
```bash
|
||||
ddev php bin/console app:travel:snapshot-refresh
|
||||
```
|
||||
- Force refresh:
|
||||
```bash
|
||||
ddev php bin/console app:travel:snapshot-refresh --force
|
||||
```
|
||||
- Force refresh with limit:
|
||||
```bash
|
||||
ddev php bin/console app:travel:snapshot-refresh --force --limit=500
|
||||
```
|
||||
- Refresh + purge:
|
||||
```bash
|
||||
ddev php bin/console app:travel:snapshot-refresh --purge
|
||||
```
|
||||
|
||||
## Open TODOs
|
||||
- [ ] Revisit DB indexes on `travel_snapshot` and remove unused ones if desired (`date_code`, `product_id` currently appear non-critical for active query paths).
|
||||
- [x] Added automated tests:
|
||||
- `TravelSnapshotServiceTest` — upsert, load, exists, generateMapping
|
||||
- `TravelDataServiceTest` — DB-primary + XML fallback paths, insurance rehydration, exception propagation
|
||||
- [ ] Add/expand automated tests:
|
||||
- extended parser coverage
|
||||
- snapshot refresh command behavior
|
||||
- [ ] Define production cron schedule for refresh and cleanup cadence.
|
||||
|
||||
## Notes
|
||||
- If refreshed snapshot data is not visible immediately, ensure the request path is not serving an older in-memory/session DTO. There is no application-level cache on `getTravelData()`; each call reads the snapshot DB directly, so data is current on the next request after a refresh.
|
||||
Reference in New Issue
Block a user