diff --git a/config/services.yaml b/config/services.yaml index dab19d3..787d54c 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -105,6 +105,13 @@ services: $emailTextsFile: '%kernel.project_dir%/config/email_texts.yaml' $mailingBusTransport: '%mailing_bus_transport%' + # Tagged by interface rather than listed by hand: order between collectors is + # irrelevant, so the only thing an explicit list could add here is the chance to + # forget one, which would silently stop a metric being recorded. + _instanceof: + App\Service\Statistics\Collector\StatisticsCollectorInterface: + tags: ['app.statistics_collector'] + App\: resource: '../src/' exclude: @@ -246,6 +253,10 @@ services: $datevEmailRecipient: '%env(DATEV_EMAIL_RECIPIENT)%' $datevEmailSender: '%env(DATEV_EMAIL_SENDER)%' + App\EventListener\StatisticsChangeSetListener: + arguments: + $collectors: !tagged_iterator app.statistics_collector + App\RequiredTeamerCheck\RequiredTeamerCheckRegistry: arguments: # Presentation order only. Each check must point at a page that clears diff --git a/docs/statistics.md b/docs/statistics.md new file mode 100644 index 0000000..45c3bb5 --- /dev/null +++ b/docs/statistics.md @@ -0,0 +1,337 @@ +# Statistics: event collection and evaluation + +Reference for how statistics are collected, what is stored, and how to read or extend it. +Describes the behaviour as implemented — not a plan. Evaluation screens for these metrics do +not exist yet; this is what they will be built on. + +The governing rule, from which the rest follows: + +> **Columns are for what you `GROUP BY`. The payload is for what you read back.** +> +> A value that will end up in a `GROUP BY` needs its own indexed column. Everything else — +> old and new values, reasons, scopes, flags — lives in the JSON `payload`. That split is +> what lets a new metric ship without a migration, and it is the only rule that has to hold +> as the table grows. + +Two corollaries that surprise people: + +- **Rows are never updated or deleted.** A statistics event is a record of something that + happened, not a view of current state. If the answer can be recomputed from live data at + any time, it does not belong here — see *What does not belong* below. +- **Nothing here is a foreign key.** `assignment_id` and friends are plain integers, so + statistics outlive the rows they describe. Applications are hard deleted; their history is + not. + +--- + +## Why the table exists + +Three of the four metrics could not be answered from live data: + +- `Assignment::$jobProfile` holds only the current value. Every edit destroyed the previous + one permanently. +- Applications are hard deleted (no soft delete), so any count over them silently shrank + over time. +- Call-offs persisted, but not *when* they happened — `updatedAt` is overwritten by any + later edit — nor at what scope. + +The `log` table is not an alternative. It is a monolog dump (`App\Logging\DatabaseHandler`) +with free-form `context`/`extra`, excluded from migrations by +`schema_filter: ~^(?!log)~` in `config/packages/doctrine.yaml`. It is an audit trail for +humans to read, not a schema to aggregate over. + +--- + +## The table + +`statistics_event`, created by `migrations/Version20260826000000.php`, mapped by +`src/Entity/StatisticsEvent.php`. + +| Column | Type | Purpose | +|--------|------|---------| +| `id` | int, PK | | +| `name` | varchar(64) | the metric, `enumType: StatisticsEventName` | +| `occurred_at` | datetime | when it happened | +| `actor_id` | int, null | `User::getId()` of whoever acted | +| `actor_label` | varchar(180), null | their `getUserIdentifier()` | +| `actor_role` | varchar(32), null | `enumType: StatisticsActorRole` | +| `assignment_id` | int, null | dimension | +| `disposition_id` | int, null | dimension | +| `application_id` | int, null | dimension | +| `teamer_id` | int, null | dimension | +| `destination_id` | int, null | dimension | +| `job_profile_id` | int, null | dimension | +| `hotel_code` | varchar(8), null | **normalized** code, frozen at write time | +| `payload` | json | everything metric-specific | + +Indexed on `(name, occurred_at)`, `(name, hotel_code)`, `(assignment_id)`, `(teamer_id)`. + +### Two traps in the columns + +**`hotel_code` is normalized here and raw in `feedback`.** This table stores `ALB`; +`feedback.hotel_code` stores `SERALB` and normalizes at query time. Same column name, +opposite conventions — comparing or joining them directly returns nothing, silently. The +`SER`-stripping rule lives in `Destination::getHotelCodeNormalized()` +(`src/Entity/Destination.php`), and is reimplemented in DQL in `DispositionRepository` and +in raw SQL in `FeedbackRepository`. Reuse the entity method in PHP; do not add a fourth copy. + +**Dimensions are frozen at write time, deliberately.** A later correction to a destination's +hotel code does not rewrite past periods. This is the same pattern `Feedback::fromAssignment()` +(`src/Entity/Feedback.php`) already uses. + +--- + +## The four metrics + +Defined in `src/Enum/StatisticsEventName.php`. Adding a case here plus one collector that +records it is the entire cost of a new metric. + +| Name | Recorded when | Payload | +|------|---------------|---------| +| `assignment.job_profile_changed` | the job profile column changes on any write path | `from`, `to` (each `{id, name}` or null) | +| `assignment.called_off` | an assignment's status becomes `called_off` | `dispositions_affected`, `previous_status` | +| `disposition.called_off` | a disposition's status becomes `called_off` | `called_off_by`, `scope`, `reason`, `previous_status` | +| `application.created` | an application is persisted, by any path | — | + +Which dimensions each one fills: + +| Metric | assignment | destination | job_profile | hotel_code | teamer | disposition | application | +|--------|:---:|:---:|:---:|:---:|:---:|:---:|:---:| +| `assignment.job_profile_changed` | ✓ | ✓ | ✓ | ✓ | — | — | — | +| `assignment.called_off` | ✓ | ✓ | ✓ | ✓ | — | — | — | +| `disposition.called_off` | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | — | +| `application.created` | ✓ | ✓ | ✓ | ✓ | ✓ | — | ✓ | + +### Call-offs: three different "who" and "how far" + +This is the part most likely to be misread when building a chart. + +- **`actor_role`** — who *clicked*, from the security token. Always `admin` for a call-off; + only administrative users can perform one. +- **`payload.called_off_by`** — whose *decision* it was, `teamer` or `office`, as declared by + an admin on the call-off form (`src/Form/DispositionCallOffType.php`). There is no + teamer-initiated call-off route; a teamer's decision is recorded on their behalf. +- **`payload.scope`** — how far it *reached*, `assignment` or `disposition` + (`src/Enum/CallOffScope.php`). The office can cancel one placement just as a + whole trip can fall away, so `called_off_by` does not imply this. + +Cancelling a trip cascades onto every disposition on it in one flush, so **one office +decision produces one `assignment.called_off` row plus N `disposition.called_off` rows**. +Counting the disposition rows therefore counts *affected placements*, not decisions. To count +decisions: + +```sql +-- decisions +SELECT COUNT(*) FROM statistics_event WHERE name = 'assignment.called_off'; +SELECT COUNT(*) FROM statistics_event + WHERE name = 'disposition.called_off' + AND JSON_UNQUOTE(JSON_EXTRACT(payload,'$.scope')) = 'disposition'; + +-- affected placements +SELECT COUNT(*) FROM statistics_event WHERE name = 'disposition.called_off'; +``` + +`assignment.called_off` carries `dispositions_affected` so a cancelled trip's cost reads off +a single row. It is frequently `0` — a trip cancelled before anyone was staffed is still a +decision worth counting. + +--- + +## How events are recorded + +One mechanism, writing through `App\Service\Statistics\StatisticsRecorder`. + +**Doctrine changesets** — `src/EventListener/StatisticsChangeSetListener.php`, on `onFlush` +and `postFlush`. Used for state changes. `onFlush` is the only place the *previous* value +still exists, and it catches every write path — edit form, duplication, imports, console — +rather than the one route someone remembered to hook. Recording happens in `postFlush`, after +commit, so a rolled-back flush leaves no phantom statistics: the batch belongs to one flush, +is cleared at the start of the next, and goes away with the service reset that a worker does +between messages. + +The listener itself knows nothing about any metric. It owns only the timing, and delegates +to **collectors** — implementations of +`App\Service\Statistics\Collector\StatisticsCollectorInterface`, autoconfigured by interface +so a new one needs no wiring and cannot be silently forgotten. + +A collector receives the whole flush (`StatisticsFlush`), not one entity at a time. That is +deliberate: `CallOffCollector` has to see which *assignments* are becoming `called_off` +before it can tell a cascaded disposition from a teamer dropping out, and it counts the +cascade to fill `dispositions_affected`. Neither is expressible from a single entity's +changeset, and that distinction exists only inside the flush. + +**Group collectors by concern, not by metric.** Both call-off metrics live in one collector +because they can only be told apart together; a job profile change shares nothing with them +and lives in its own. Order between collectors is irrelevant and nothing may depend on it. + +Collectors see **insertions** as well as updates, so a metric about something being created +is a collector too. `ApplicationCreatedCollector` reads `insertionsOf(Application::class)` +rather than subscribing to `ApplicationCreatedEvent`, which is what an earlier version did. +Reading the flush counts every path that persists an application; subscribing counted only +the paths that remember to dispatch, and would have silently missed an admin applying on +someone's behalf, an import, or a fixture. + +A newly inserted entity has no id during `onFlush`. That is fine — Doctrine assigns it during +the commit, and recording happens afterwards in `postFlush`. + +> **There is deliberately only one mechanism.** If a future metric is not a persisted state +> change at all — an email sent, a login, a document downloaded — call `StatisticsRecorder` +> directly from wherever that happens. Do not reintroduce a second listener layer for it. + +**Writes go through raw DBAL**, not the ORM, so recording never touches the UnitOfWork — a +listener running inside a flush would otherwise trigger a nested flush. Failures are caught +and reported to the audit logger: statistics are a side effect and must never break the +business action that produced them. + +--- + +## Reading the data + +`src/Repository/StatisticsEventRepository.php`. + +```php +public function countGroupedBy( + StatisticsEventName $name, + string $dimension, // whitelisted + ?\DateTimeImmutable $dateFrom = null, + ?\DateTimeImmutable $dateTo = null, + StatisticsDateBasis $dateBasis = StatisticsDateBasis::SEASON, +): array +``` + +`getCountGroupedByQuery()` returns the same query unexecuted, which is how its semantics are +pinned in tests without a database. + +### A period means the season, not when the row was written + +`StatisticsDateBasis::SEASON` (the default) filters on the assignment's dates, falling back to +the destination's — the translation of `Assignment::getEffectivePeriod()` into DQL, which +lives in `src/Repository/Filter/SeasonPeriodFilter.php` and is shared with +`DispositionRepository`. This matches every other statistics screen and +`StatisticsFilterType`. Change what a season means there, once, not per screen. + +`StatisticsDateBasis::OCCURRENCE` filters `occurred_at` instead. Reach for it deliberately, +for genuine time series such as call-offs per month. + +The two routinely disagree, because an application for the coming winter arrives months +before the season it belongs to. Picking the wrong one does not fail — it quietly answers a +different question from the screen next to it. + +`SEASON` reaches the assignment through an arbitrary join and therefore excludes events with +no `assignment_id`. All four metrics set it. + +### Crossing with live data + +The plain-integer dimension columns cost nothing in queryability — Doctrine's arbitrary join +works: + +``` +SELECT destination.hotel AS hotel, COUNT(event.id) AS applications + FROM App\Entity\StatisticsEvent event + JOIN App\Entity\Assignment assignment WITH assignment.id = event.assignmentId + JOIN assignment.destination destination + WHERE event.name = :name + GROUP BY hotel +``` + +Group by `hotel_code`, though, not by the hotel *name*: they are not one-to-one. One +normalized code can span several destinations with different names. + +--- + +## Backfilling + +`bin/console app:statistics:backfill` seeds what current data still contains. Idempotent — +it skips subjects already recorded — and supports `--dry-run`. It is never run automatically: +nothing in `CronCommand`, the scheduler, or any deploy script invokes it. + +It writes only to `statistics_event`, so it is safe to run against production at any time. + +| Metric | Backfillable | `occurred_at` from | +|--------|--------------|--------------------| +| `application.created` | yes | `createdAt` — exact | +| `disposition.called_off` | yes | `updatedAt` — **approximate** | +| `assignment.called_off` | yes | `updatedAt` — **approximate** | +| `assignment.job_profile_changed` | **no** | history does not exist | + +Every seeded row carries `"backfilled": true`; the approximate ones also carry +`"approximate_date": true`. So the choice is per chart, never table-wide: + +```sql +WHERE JSON_EXTRACT(payload,'$.backfilled') IS NULL -- organically recorded only +``` + +Three limits on seeded rows, all of which matter when charting: + +- **Dates from `updatedAt` are a proxy**, since any later edit overwrites it. Exclude those + rows from time series; keep them for totals. +- **`actor_role` is `system`.** The command runs on the CLI with no security token, so the + original actor is not recoverable. +- **Frozen dimensions are frozen at *backfill* time**, not at the time of the event, and + `scope` is left unset because it cannot be established retroactively. + +--- + +## Extending it + +The cost depends entirely on whether the new metric needs a new *dimension* or just a new +*name*. Almost all need only a name. + +**No migration — a new kind of event.** Add a case to `StatisticsEventName`, then one +`StatisticsCollectorInterface` implementation that records it. Metric-specific detail goes in +`payload`. `StatisticsChangeSetListener` is never edited. This covers most requests — +application rejections, contract upload delays, feedback submitted, any status transition, +anything created or deleted. + +Reuse an existing collector when the new metric is part of a concern already covered — +especially when it needs to see the same entities in the same flush. Write a new one when it +is genuinely independent. + +**One migration — a new dimension to slice by.** Only when you need to group by something no +column carries (a season, a country, a fee band). Add the column and an index, fill it in +`StatisticsRecorder`, and accept `NULL` on older rows. Resist when the dimension is already +reachable through a join. + +**Migration plus backfill — history for something not yet recorded.** Extend +`StatisticsBackfillCommand` if current data still holds the answer. If it has been +overwritten, as with job profiles, the metric simply starts the day it ships. + +--- + +## What does not belong here + +**Anything recomputable from live data that cannot be lost.** Feedback statistics +(`src/Controller/Administrative/Statistics/`) are the worked example: `Feedback` already +freezes its own dimensions at creation, is never deleted, and its aggregations average a +per-question `ratings` array. It is already an append-only record; copying it into +`statistics_event` would duplicate every row and lose the structure the charts need. This +table counts occurrences — it has no shape for averaging structured arrays. + +**Current state.** "How many applications are open right now" is a question for the +`application` table. This table answers "how many were ever received", which is a different +and equally valid question — just not the same one. + +**An audit trail for humans.** That is the `log` table and `/admin/log`. + +--- + +## Where things live + +| Concern | Path | +|---------|------| +| Entity | `src/Entity/StatisticsEvent.php` | +| Repository | `src/Repository/StatisticsEventRepository.php` | +| Enums | `src/Enum/Statistics*.php` | +| Write path | `src/Service/Statistics/StatisticsRecorder.php` | +| Flush orchestration | `src/EventListener/StatisticsChangeSetListener.php` | +| Collectors (one per concern) | `src/Service/Statistics/Collector/` | +| Flush context passed to collectors | `src/Service/Statistics/StatisticsFlush.php` | +| What a period means, shared with the disposition statistics | `src/Repository/Filter/SeasonPeriodFilter.php` | +| Backfill | `src/Command/StatisticsBackfillCommand.php` | +| Migration | `migrations/Version20260826000000.php` | +| Tests | `tests/EventListener/`, `tests/Repository/`, `tests/Service/Statistics/` | + +Existing statistics screens, for the filter/handler pattern a future evaluation UI should +reuse: `src/Controller/Administrative/Statistics/`, +`src/Service/Common/StatisticsFilterHandler.php`, `src/Model/StatisticsFilterDto.php`, +`src/Form/StatisticsFilterType.php`. diff --git a/migrations/Version20260826000000.php b/migrations/Version20260826000000.php new file mode 100644 index 0000000..c4b8b35 --- /dev/null +++ b/migrations/Version20260826000000.php @@ -0,0 +1,54 @@ +addSql(<<<'SQL' + CREATE TABLE statistics_event ( + id INT AUTO_INCREMENT NOT NULL, + name VARCHAR(64) NOT NULL, + occurred_at DATETIME NOT NULL COMMENT '(DC2Type:datetime_immutable)', + actor_id INT DEFAULT NULL, + actor_label VARCHAR(180) DEFAULT NULL, + actor_role VARCHAR(32) DEFAULT NULL, + assignment_id INT DEFAULT NULL, + disposition_id INT DEFAULT NULL, + application_id INT DEFAULT NULL, + teamer_id INT DEFAULT NULL, + destination_id INT DEFAULT NULL, + job_profile_id INT DEFAULT NULL, + hotel_code VARCHAR(8) DEFAULT NULL, + payload JSON NOT NULL, + INDEX IDX_A343D13C5E237E0687C03D1B (name, occurred_at), + INDEX IDX_A343D13C5E237E06C0D0E610 (name, hotel_code), + INDEX IDX_A343D13CD19302F8 (assignment_id), + INDEX IDX_A343D13C4302FF75 (teamer_id), + PRIMARY KEY(id) + ) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB + SQL); + } + + public function down(Schema $schema): void + { + $this->addSql('DROP TABLE statistics_event'); + } +} diff --git a/src/Command/StatisticsBackfillCommand.php b/src/Command/StatisticsBackfillCommand.php new file mode 100644 index 0000000..36acd6c --- /dev/null +++ b/src/Command/StatisticsBackfillCommand.php @@ -0,0 +1,214 @@ +addOption('dry-run', null, InputOption::VALUE_NONE, 'Report what would be written without writing it'); + } + + /** + * Recovers the statistics that current data still contains, so charts do not start + * from an empty table on the day this ships. + * + * Runs on the CLI, where there is no security token, so every row it writes is + * attributed to the system actor rather than to whoever originally acted. Safe to run + * more than once: subjects already recorded are skipped. + */ + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $dryRun = (bool) $input->getOption('dry-run'); + + if ($dryRun) { + $io->note('Dry run - nothing is written.'); + } + + $applications = $this->backfillApplications($dryRun); + $io->success(sprintf('%d application(s) backfilled.', $applications)); + + $callOffs = $this->backfillCallOffs($dryRun); + $io->success(sprintf('%d called off disposition(s) backfilled.', $callOffs)); + + $assignmentCallOffs = $this->backfillAssignmentCallOffs($dryRun); + $io->success(sprintf('%d called off assignment(s) backfilled.', $assignmentCallOffs)); + + $io->warning( + 'Job profile changes cannot be backfilled: only the current profile is stored, ' + .'so the history does not exist. That metric starts from now.' + ); + + return Command::SUCCESS; + } + + private function backfillApplications(bool $dryRun): int + { + $recorded = array_flip($this->statisticsEventRepository->findRecordedSubjectIds( + StatisticsEventName::APPLICATION_CREATED, + 'applicationId', + )); + + $count = 0; + + foreach ($this->iterateAll(Application::class) as $application) { + if (isset($recorded[$application->getId()])) { + continue; + } + + ++$count; + + if ($dryRun) { + continue; + } + + $this->recorder->recordForApplication( + StatisticsEventName::APPLICATION_CREATED, + $application, + ['backfilled' => true], + $application->getCreatedAt(), + ); + } + + return $count; + } + + private function backfillCallOffs(bool $dryRun): int + { + $recorded = array_flip($this->statisticsEventRepository->findRecordedSubjectIds( + StatisticsEventName::DISPOSITION_CALLED_OFF, + 'dispositionId', + )); + + $count = 0; + + foreach ($this->iterateAll(Disposition::class, ['status' => Disposition::STATUS_CALLED_OFF]) as $disposition) { + if (isset($recorded[$disposition->getId()])) { + continue; + } + + ++$count; + + if ($dryRun) { + continue; + } + + $this->recorder->recordForDisposition( + StatisticsEventName::DISPOSITION_CALLED_OFF, + $disposition, + [ + 'called_off_by' => $disposition->getCalledOffBy(), + 'reason' => $disposition->getCalledOffReason(), + 'backfilled' => true, + // updatedAt is whenever the row was last touched for any reason, which + // is only the call-off date if nothing happened afterwards. + 'approximate_date' => true, + ], + $disposition->getUpdatedAt() ?? $disposition->getCreatedAt(), + ); + } + + return $count; + } + + private function backfillAssignmentCallOffs(bool $dryRun): int + { + $recorded = array_flip($this->statisticsEventRepository->findRecordedSubjectIds( + StatisticsEventName::ASSIGNMENT_CALLED_OFF, + 'assignmentId', + )); + + $count = 0; + + foreach ($this->iterateAll(Assignment::class, ['status' => Assignment::STATUS_CALLED_OFF]) as $assignment) { + if (isset($recorded[$assignment->getId()])) { + continue; + } + + ++$count; + + if ($dryRun) { + continue; + } + + $affected = 0; + + foreach ($assignment->getDispositions() as $disposition) { + if (Disposition::STATUS_CALLED_OFF === $disposition->getStatus()) { + ++$affected; + } + } + + $this->recorder->recordForAssignment( + StatisticsEventName::ASSIGNMENT_CALLED_OFF, + $assignment, + [ + // Counted as things stand now, not as they stood on the day. An + // assignment called off through the old status dropdown left its + // dispositions running, so this can legitimately be 0. + 'dispositions_affected' => $affected, + 'backfilled' => true, + 'approximate_date' => true, + ], + $assignment->getUpdatedAt() ?? $assignment->getCreatedAt(), + ); + } + + return $count; + } + + /** + * @template T of object + * + * @param class-string $entityClass + * @param array $criteria + * + * @return iterable + */ + private function iterateAll(string $entityClass, array $criteria = []): iterable + { + $repository = $this->entityManager->getRepository($entityClass); + + $qb = $repository->createQueryBuilder('entity'); + + foreach ($criteria as $field => $value) { + $qb + ->andWhere($qb->expr()->eq(sprintf('entity.%s', $field), ':'.$field)) + ->setParameter($field, $value) + ; + } + + // Kept out of the identity map: these tables are large and every row is read once. + foreach ($qb->getQuery()->toIterable() as $entity) { + yield $entity; + + $this->entityManager->detach($entity); + } + } +} diff --git a/src/Entity/StatisticsEvent.php b/src/Entity/StatisticsEvent.php new file mode 100644 index 0000000..e96286a --- /dev/null +++ b/src/Entity/StatisticsEvent.php @@ -0,0 +1,239 @@ + */ + #[ORM\Column(type: Types::JSON)] + private array $payload = []; + + public function __construct(StatisticsEventName $name, \DateTimeImmutable $occurredAt) + { + $this->name = $name; + $this->occurredAt = $occurredAt; + } + + public function getId(): ?int + { + return $this->id; + } + + public function getName(): StatisticsEventName + { + return $this->name; + } + + public function getOccurredAt(): \DateTimeImmutable + { + return $this->occurredAt; + } + + public function getActorId(): ?int + { + return $this->actorId; + } + + public function setActorId(?int $actorId): static + { + $this->actorId = $actorId; + + return $this; + } + + public function getActorLabel(): ?string + { + return $this->actorLabel; + } + + public function setActorLabel(?string $actorLabel): static + { + $this->actorLabel = $actorLabel; + + return $this; + } + + public function getActorRole(): ?StatisticsActorRole + { + return $this->actorRole; + } + + public function setActorRole(?StatisticsActorRole $actorRole): static + { + $this->actorRole = $actorRole; + + return $this; + } + + public function getAssignmentId(): ?int + { + return $this->assignmentId; + } + + public function setAssignmentId(?int $assignmentId): static + { + $this->assignmentId = $assignmentId; + + return $this; + } + + public function getDispositionId(): ?int + { + return $this->dispositionId; + } + + public function setDispositionId(?int $dispositionId): static + { + $this->dispositionId = $dispositionId; + + return $this; + } + + public function getApplicationId(): ?int + { + return $this->applicationId; + } + + public function setApplicationId(?int $applicationId): static + { + $this->applicationId = $applicationId; + + return $this; + } + + public function getTeamerId(): ?int + { + return $this->teamerId; + } + + public function setTeamerId(?int $teamerId): static + { + $this->teamerId = $teamerId; + + return $this; + } + + public function getDestinationId(): ?int + { + return $this->destinationId; + } + + public function setDestinationId(?int $destinationId): static + { + $this->destinationId = $destinationId; + + return $this; + } + + public function getJobProfileId(): ?int + { + return $this->jobProfileId; + } + + public function setJobProfileId(?int $jobProfileId): static + { + $this->jobProfileId = $jobProfileId; + + return $this; + } + + public function getHotelCode(): ?string + { + return $this->hotelCode; + } + + public function setHotelCode(?string $hotelCode): static + { + $this->hotelCode = $hotelCode; + + return $this; + } + + /** + * @return array + */ + public function getPayload(): array + { + return $this->payload; + } + + /** + * @param array $payload + */ + public function setPayload(array $payload): static + { + $this->payload = $payload; + + return $this; + } +} diff --git a/src/Enum/CallOffScope.php b/src/Enum/CallOffScope.php new file mode 100644 index 0000000..4e7ad85 --- /dev/null +++ b/src/Enum/CallOffScope.php @@ -0,0 +1,19 @@ + 'Tätigkeitsprofil geändert', + self::ASSIGNMENT_CALLED_OFF => 'Reise abgesagt', + self::DISPOSITION_CALLED_OFF => 'Einsatz abgesagt', + self::APPLICATION_CREATED => 'Bewerbung eingegangen', + }; + } +} diff --git a/src/EventListener/StatisticsChangeSetListener.php b/src/EventListener/StatisticsChangeSetListener.php new file mode 100644 index 0000000..91bee48 --- /dev/null +++ b/src/EventListener/StatisticsChangeSetListener.php @@ -0,0 +1,102 @@ + + */ + private array $pending = []; + + /** + * @param iterable $collectors + */ + public function __construct( + private readonly StatisticsRecorder $recorder, + private readonly iterable $collectors, + ) { + } + + public function onFlush(OnFlushEventArgs $args): void + { + // Each flush starts from empty: anything still here was collected by a flush that + // never committed, and writing it now would date it wrongly and attribute it to + // whoever happens to be acting instead. + $this->pending = []; + + $flush = new StatisticsFlush($args->getObjectManager()->getUnitOfWork()); + + foreach ($this->collectors as $collector) { + foreach ($collector->collect($flush) as $event) { + $this->pending[] = $event; + } + } + } + + public function postFlush(PostFlushEventArgs $args): void + { + if (0 === count($this->pending)) { + return; + } + + // Taken and cleared up front: recording must not be repeated if anything + // downstream flushes again. + $pending = $this->pending; + $this->pending = []; + + foreach ($pending as $event) { + $subject = $event->subject; + + if ($subject instanceof Assignment) { + $this->recorder->recordForAssignment($event->name, $subject, $event->payload); + + continue; + } + + if ($subject instanceof Disposition) { + $this->recorder->recordForDisposition($event->name, $subject, $event->payload); + + continue; + } + + $this->recorder->recordForApplication($event->name, $subject, $event->payload); + } + } + + /** + * Long-running processes reset their services between messages, which is where a failed + * flush's leftovers would otherwise sit waiting for an unrelated flush to write them. + */ + public function reset(): void + { + $this->pending = []; + } +} diff --git a/src/Form/AssignmentType.php b/src/Form/AssignmentType.php index 4e6b6bd..bb2a27c 100644 --- a/src/Form/AssignmentType.php +++ b/src/Form/AssignmentType.php @@ -22,6 +22,19 @@ use Symfony\Component\OptionsResolver\OptionsResolver; class AssignmentType extends AbstractType { + /** + * The statuses an admin may move an assignment between at will. + * + * Calling off is deliberately absent: it has to go through the dedicated action, which + * also cancels every disposition on the assignment. Choosing it here used to set the + * assignment to called_off and leave its teamers holding a live placement on a + * cancelled trip. + */ + private const STATUS_CHOICES = [ + 'Entwurf' => Assignment::STATUS_DRAFT, + 'veröffentlicht' => Assignment::STATUS_PUBLISHED, + ]; + public function __construct(private readonly EntityManagerInterface $entityManager) { } @@ -29,14 +42,8 @@ class AssignmentType extends AbstractType public function buildForm(FormBuilderInterface $builder, array $options): void { $builder - ->add('status', ChoiceType::class, [ - 'label' => 'Status', - 'choices' => [ - 'Entwurf' => Assignment::STATUS_DRAFT, - 'veröffentlicht' => Assignment::STATUS_PUBLISHED, - 'abgesagt' => Assignment::STATUS_CALLED_OFF, - ], - ]) + // status is added in PRE_SET_DATA, where the assignment behind the form is + // known - an already called off one has to keep that value in the list. ->add('availableDispositions', IntegerType::class, [ 'label' => 'zu vergeben', 'required' => false, @@ -138,6 +145,9 @@ class AssignmentType extends AbstractType ->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event) { $form = $event->getForm(); $data = $event->getData(); + + $this->addStatusField($data, $form); + $destination = $data->getDestination(); if (null === $destination) { @@ -170,6 +180,38 @@ class AssignmentType extends AbstractType ; } + /** + * The statuses this form may set on a given assignment. + * + * An assignment that is already called off has to keep that value in the list, or the + * form could not render it and any save would silently reset the status. It stays + * switchable back to published, since there is no other way to undo a call-off made by + * mistake - what is blocked is entering the state, not leaving it. + * + * ChoiceType validates a submission against whatever this returns, so it holds against + * a hand-crafted POST too, not just the rendered select. + * + * @return array + */ + public static function statusChoices(?Assignment $assignment): array + { + $choices = self::STATUS_CHOICES; + + if (null !== $assignment && Assignment::STATUS_CALLED_OFF === $assignment->getStatus()) { + $choices['abgesagt'] = Assignment::STATUS_CALLED_OFF; + } + + return $choices; + } + + private function addStatusField(?Assignment $assignment, FormInterface $form): void + { + $form->add('status', ChoiceType::class, [ + 'label' => 'Status', + 'choices' => self::statusChoices($assignment), + ]); + } + private function addPickupField(Destination $destination, FormInterface $form): void { $choices = []; diff --git a/src/Repository/AssignmentRepository.php b/src/Repository/AssignmentRepository.php index b65bb80..4b9c925 100644 --- a/src/Repository/AssignmentRepository.php +++ b/src/Repository/AssignmentRepository.php @@ -417,29 +417,4 @@ class AssignmentRepository extends ServiceEntityRepository ->getQuery() ; } - - public function getSelectableHotelCodes(): array - { - $qb = $this->createQueryBuilder('assignment'); - - $codes = []; - - $result = $qb - ->select('destination.hotelCode') - ->innerJoin('assignment.destination', 'destination') - ->groupBy('destination.hotelCode') - ->orderBy('destination.hotelCode', 'ASC') - ->getQuery() - ->getArrayResult(); - - foreach ($result as $row) { - if (str_starts_with($row['hotelCode'], 'SER')) { - $codes[] = substr($row['hotelCode'], 2, 3); - } else { - $codes[] = substr($row['hotelCode'], 0, 3); - } - } - - return array_unique($codes); - } } diff --git a/src/Repository/DispositionRepository.php b/src/Repository/DispositionRepository.php index 91fb7df..e5614e4 100644 --- a/src/Repository/DispositionRepository.php +++ b/src/Repository/DispositionRepository.php @@ -6,6 +6,7 @@ use App\Entity\Assignment; use App\Entity\Disposition; use App\Entity\Teamer; use App\Entity\Upload; +use App\Repository\Filter\SeasonPeriodFilter; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; use Doctrine\ORM\Query; use Doctrine\ORM\Query\Expr\Join; @@ -343,38 +344,7 @@ class DispositionRepository extends ServiceEntityRepository ->orderBy('destination.hotel', 'ASC') ; - // Filter by effective date range (assignment date or fallback to destination date) - if (null !== $dateFrom) { - $qb - ->andWhere($qb->expr()->orX( - $qb->expr()->andX( - $qb->expr()->isNotNull('assignment.dateFrom'), - $qb->expr()->gte('assignment.dateFrom', ':dateFrom') - ), - $qb->expr()->andX( - $qb->expr()->isNull('assignment.dateFrom'), - $qb->expr()->gte('destination.dateFrom', ':dateFrom') - ) - )) - ->setParameter('dateFrom', $dateFrom) - ; - } - - if (null !== $dateTo) { - $qb - ->andWhere($qb->expr()->orX( - $qb->expr()->andX( - $qb->expr()->isNotNull('assignment.dateTo'), - $qb->expr()->lte('assignment.dateTo', ':dateTo') - ), - $qb->expr()->andX( - $qb->expr()->isNull('assignment.dateTo'), - $qb->expr()->lte('destination.dateTo', ':dateTo') - ) - )) - ->setParameter('dateTo', $dateTo) - ; - } + SeasonPeriodFilter::apply($qb, $dateFrom, $dateTo); $results = $qb->getQuery()->getResult(); @@ -435,37 +405,7 @@ class DispositionRepository extends ServiceEntityRepository ->orderBy('hotelCode', 'ASC') ; - if (null !== $dateFrom) { - $qb - ->andWhere($qb->expr()->orX( - $qb->expr()->andX( - $qb->expr()->isNotNull('assignment.dateFrom'), - $qb->expr()->gte('assignment.dateFrom', ':dateFrom') - ), - $qb->expr()->andX( - $qb->expr()->isNull('assignment.dateFrom'), - $qb->expr()->gte('destination.dateFrom', ':dateFrom') - ) - )) - ->setParameter('dateFrom', $dateFrom) - ; - } - - if (null !== $dateTo) { - $qb - ->andWhere($qb->expr()->orX( - $qb->expr()->andX( - $qb->expr()->isNotNull('assignment.dateTo'), - $qb->expr()->lte('assignment.dateTo', ':dateTo') - ), - $qb->expr()->andX( - $qb->expr()->isNull('assignment.dateTo'), - $qb->expr()->lte('destination.dateTo', ':dateTo') - ) - )) - ->setParameter('dateTo', $dateTo) - ; - } + SeasonPeriodFilter::apply($qb, $dateFrom, $dateTo); $results = $qb->getQuery()->getResult(); diff --git a/src/Repository/Filter/SeasonPeriodFilter.php b/src/Repository/Filter/SeasonPeriodFilter.php new file mode 100644 index 0000000..40a2243 --- /dev/null +++ b/src/Repository/Filter/SeasonPeriodFilter.php @@ -0,0 +1,59 @@ +andWhere($qb->expr()->orX( + $qb->expr()->andX( + $qb->expr()->isNotNull($assignmentAlias.'.dateFrom'), + $qb->expr()->gte($assignmentAlias.'.dateFrom', ':dateFrom') + ), + $qb->expr()->andX( + $qb->expr()->isNull($assignmentAlias.'.dateFrom'), + $qb->expr()->gte($destinationAlias.'.dateFrom', ':dateFrom') + ) + )) + ->setParameter('dateFrom', $dateFrom) + ; + } + + if (null !== $dateTo) { + $qb + ->andWhere($qb->expr()->orX( + $qb->expr()->andX( + $qb->expr()->isNotNull($assignmentAlias.'.dateTo'), + $qb->expr()->lte($assignmentAlias.'.dateTo', ':dateTo') + ), + $qb->expr()->andX( + $qb->expr()->isNull($assignmentAlias.'.dateTo'), + $qb->expr()->lte($destinationAlias.'.dateTo', ':dateTo') + ) + )) + ->setParameter('dateTo', $dateTo) + ; + } + } +} diff --git a/src/Repository/StatisticsEventRepository.php b/src/Repository/StatisticsEventRepository.php new file mode 100644 index 0000000..796e6c5 --- /dev/null +++ b/src/Repository/StatisticsEventRepository.php @@ -0,0 +1,156 @@ + + * + * @method StatisticsEvent|null find($id, $lockMode = null, $lockVersion = null) + * @method StatisticsEvent|null findOneBy(array $criteria, array $orderBy = null) + * @method StatisticsEvent[] findAll() + * @method StatisticsEvent[] findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null) + */ +class StatisticsEventRepository extends ServiceEntityRepository +{ + public function __construct(ManagerRegistry $registry) + { + parent::__construct($registry, StatisticsEvent::class); + } + + /** + * Counts events of one metric, grouped by one of the dimension columns. + * + * The grouping column is whitelisted rather than interpolated freely, since it + * goes into the DQL string. + * + * The period defaults to the season the assignment runs in, because that is what + * every other statistics screen means by a date range - filtering these events by + * when they were recorded instead would silently answer a different question from + * the screen next to it. Pass OCCURRENCE deliberately for a real time series. + * + * SEASON reaches the assignment through an arbitrary join, since the dimension + * columns are plain integers rather than relations. Events without an assignmentId + * therefore drop out of a season-filtered count; all metrics recorded so far set it. + * + * @return array + */ + public function countGroupedBy( + StatisticsEventName $name, + string $dimension, + ?\DateTimeImmutable $dateFrom = null, + ?\DateTimeImmutable $dateTo = null, + StatisticsDateBasis $dateBasis = StatisticsDateBasis::SEASON, + ): array { + $rows = $this + ->getCountGroupedByQuery($name, $dimension, $dateFrom, $dateTo, $dateBasis) + ->getResult() + ; + + // Doctrine hands COUNT() back as a string. Cast it once here rather than leaving + // every caller to discover it through a === comparison that quietly never matches. + return array_map( + static fn (array $row): array => [ + 'value' => $row['value'], + 'total' => (int) $row['total'], + ], + $rows, + ); + } + + /** + * The query behind countGroupedBy(), exposed unexecuted so its semantics can be + * pinned without a database. + */ + public function getCountGroupedByQuery( + StatisticsEventName $name, + string $dimension, + ?\DateTimeImmutable $dateFrom = null, + ?\DateTimeImmutable $dateTo = null, + StatisticsDateBasis $dateBasis = StatisticsDateBasis::SEASON, + ): Query { + $allowed = ['actorRole', 'hotelCode', 'destinationId', 'jobProfileId', 'teamerId', 'assignmentId']; + + if (!in_array($dimension, $allowed, true)) { + throw new \InvalidArgumentException(sprintf('Cannot group statistics by "%s".', $dimension)); + } + + $qb = $this->createQueryBuilder('event'); + + $qb + ->select(sprintf('event.%s AS value', $dimension), 'COUNT(event.id) AS total') + ->where($qb->expr()->eq('event.name', ':name')) + ->setParameter('name', $name->value) + ->groupBy('value') + ->orderBy('total', 'DESC') + ; + + $filtered = null !== $dateFrom || null !== $dateTo; + + if (StatisticsDateBasis::SEASON === $dateBasis) { + if ($filtered) { + $qb + ->innerJoin(Assignment::class, 'assignment', Join::WITH, 'assignment.id = event.assignmentId') + ->innerJoin('assignment.destination', 'destination') + ; + } + + // The same translation the disposition statistics use, so a period cannot come + // to mean one thing here and another on the screen beside it. + SeasonPeriodFilter::apply($qb, $dateFrom, $dateTo); + } else { + if (null !== $dateFrom) { + $qb + ->andWhere($qb->expr()->gte('event.occurredAt', ':dateFrom')) + ->setParameter('dateFrom', $dateFrom) + ; + } + + if (null !== $dateTo) { + $qb + ->andWhere($qb->expr()->lte('event.occurredAt', ':dateTo')) + ->setParameter('dateTo', $dateTo) + ; + } + } + + return $qb->getQuery(); + } + + /** + * The subject ids already recorded for a metric, used by the backfill to stay + * idempotent. + * + * @return array + */ + public function findRecordedSubjectIds(StatisticsEventName $name, string $dimension): array + { + $allowed = ['assignmentId', 'dispositionId', 'applicationId']; + + if (!in_array($dimension, $allowed, true)) { + throw new \InvalidArgumentException(sprintf('Cannot look up statistics by "%s".', $dimension)); + } + + $qb = $this->createQueryBuilder('event'); + + $rows = $qb + ->select(sprintf('event.%s AS subjectId', $dimension)) + ->where($qb->expr()->eq('event.name', ':name')) + ->andWhere($qb->expr()->isNotNull(sprintf('event.%s', $dimension))) + ->setParameter('name', $name->value) + ->getQuery() + ->getResult() + ; + + return array_map(static fn (array $row): int => (int) $row['subjectId'], $rows); + } +} diff --git a/src/Service/Statistics/CollectedStatisticsEvent.php b/src/Service/Statistics/CollectedStatisticsEvent.php new file mode 100644 index 0000000..40c4700 --- /dev/null +++ b/src/Service/Statistics/CollectedStatisticsEvent.php @@ -0,0 +1,27 @@ + $payload + */ + public function __construct( + public readonly StatisticsEventName $name, + public readonly Assignment|Disposition|Application $subject, + public readonly array $payload = [], + ) { + } +} diff --git a/src/Service/Statistics/Collector/ApplicationCreatedCollector.php b/src/Service/Statistics/Collector/ApplicationCreatedCollector.php new file mode 100644 index 0000000..20ac8b9 --- /dev/null +++ b/src/Service/Statistics/Collector/ApplicationCreatedCollector.php @@ -0,0 +1,31 @@ +insertionsOf(Application::class) as $application) { + yield new CollectedStatisticsEvent( + StatisticsEventName::APPLICATION_CREATED, + $application, + ); + } + } +} diff --git a/src/Service/Statistics/Collector/CallOffCollector.php b/src/Service/Statistics/Collector/CallOffCollector.php new file mode 100644 index 0000000..c79ba55 --- /dev/null +++ b/src/Service/Statistics/Collector/CallOffCollector.php @@ -0,0 +1,83 @@ + $cancelledTrips */ + $cancelledTrips = new \SplObjectStorage(); + + foreach ($flush->updatesOf(Assignment::class) as $assignment) { + if ($flush->transitionsTo($assignment, 'status', $calledOff)) { + $cancelledTrips[$assignment] = 0; + } + } + + $dispositionEvents = []; + + foreach ($flush->updatesOf(Disposition::class) as $disposition) { + if (!$flush->transitionsTo($disposition, 'status', $calledOff)) { + continue; + } + + $assignment = $disposition->getAssignment(); + $cascaded = null !== $assignment && $cancelledTrips->contains($assignment); + + if ($cascaded) { + $cancelledTrips[$assignment] = $cancelledTrips[$assignment] + 1; + } + + $dispositionEvents[] = new CollectedStatisticsEvent( + StatisticsEventName::DISPOSITION_CALLED_OFF, + $disposition, + [ + // Whose decision it was, as declared on the call-off form. The role of + // whoever clicked is recorded separately, in actor_role. + 'called_off_by' => $disposition->getCalledOffBy(), + // How far it reached. The office can cancel one placement as well as a + // whole trip, so called_off_by does not imply this. + 'scope' => ($cascaded ? CallOffScope::ASSIGNMENT : CallOffScope::DISPOSITION)->value, + 'reason' => $disposition->getCalledOffReason(), + 'previous_status' => $flush->previousValue($disposition, 'status'), + ], + ); + } + + yield from $dispositionEvents; + + // Emitted last, once the cascade has been counted: a cancelled trip is worth one row + // stating what it cost, rather than N rows that have to be counted back into a + // decision. Frequently zero - a trip cancelled before anyone was staffed is still a + // decision worth recording. + foreach ($cancelledTrips as $assignment) { + yield new CollectedStatisticsEvent( + StatisticsEventName::ASSIGNMENT_CALLED_OFF, + $assignment, + [ + 'dispositions_affected' => $cancelledTrips[$assignment], + 'previous_status' => $flush->previousValue($assignment, 'status'), + ], + ); + } + } +} diff --git a/src/Service/Statistics/Collector/JobProfileChangeCollector.php b/src/Service/Statistics/Collector/JobProfileChangeCollector.php new file mode 100644 index 0000000..5a089c3 --- /dev/null +++ b/src/Service/Statistics/Collector/JobProfileChangeCollector.php @@ -0,0 +1,61 @@ +updatesOf(Assignment::class) as $assignment) { + $changeSet = $flush->changeSet($assignment); + + if (!isset($changeSet['jobProfile'])) { + continue; + } + + [$old, $new] = $changeSet['jobProfile']; + + if ($old === $new) { + continue; + } + + yield new CollectedStatisticsEvent( + StatisticsEventName::ASSIGNMENT_JOB_PROFILE_CHANGED, + $assignment, + [ + 'from' => $this->describe($old), + 'to' => $this->describe($new), + ], + ); + } + } + + /** + * @return array{id: int|null, name: string|null}|null + */ + private function describe(?JobProfile $jobProfile): ?array + { + if (null === $jobProfile) { + return null; + } + + return [ + 'id' => $jobProfile->getId(), + 'name' => $jobProfile->getName(), + ]; + } +} diff --git a/src/Service/Statistics/Collector/StatisticsCollectorInterface.php b/src/Service/Statistics/Collector/StatisticsCollectorInterface.php new file mode 100644 index 0000000..9e8dd72 --- /dev/null +++ b/src/Service/Statistics/Collector/StatisticsCollectorInterface.php @@ -0,0 +1,25 @@ + + */ + public function collect(StatisticsFlush $flush): iterable; +} diff --git a/src/Service/Statistics/StatisticsFlush.php b/src/Service/Statistics/StatisticsFlush.php new file mode 100644 index 0000000..900747c --- /dev/null +++ b/src/Service/Statistics/StatisticsFlush.php @@ -0,0 +1,105 @@ + + */ + public function updates(): array + { + return $this->unitOfWork->getScheduledEntityUpdates(); + } + + /** + * @template T of object + * + * @param class-string $class + * + * @return array + */ + public function updatesOf(string $class): array + { + return array_values(array_filter( + $this->updates(), + static fn (object $entity): bool => $entity instanceof $class, + )); + } + + /** + * @return array + */ + public function insertions(): array + { + return $this->unitOfWork->getScheduledEntityInsertions(); + } + + /** + * Newly created entities have no id yet at this point - Doctrine assigns it during the + * commit. Collectors may safely yield them anyway, because recording happens in + * postFlush, by which time the id is there. + * + * @template T of object + * + * @param class-string $class + * + * @return array + */ + public function insertionsOf(string $class): array + { + return array_values(array_filter( + $this->insertions(), + static fn (object $entity): bool => $entity instanceof $class, + )); + } + + /** + * @return array + */ + public function changeSet(object $entity): array + { + return $this->unitOfWork->getEntityChangeSet($entity); + } + + /** + * Whether a field is moving to a value it did not already hold. + * + * The second half matters: re-saving a record that is already in the target state is not + * a transition, and must not be counted as one. + */ + public function transitionsTo(object $entity, string $field, mixed $value): bool + { + $changeSet = $this->changeSet($entity); + + if (!isset($changeSet[$field])) { + return false; + } + + [$old, $new] = $changeSet[$field]; + + return $value === $new && $value !== $old; + } + + public function previousValue(object $entity, string $field): mixed + { + return $this->changeSet($entity)[$field][0] ?? null; + } +} diff --git a/src/Service/Statistics/StatisticsRecorder.php b/src/Service/Statistics/StatisticsRecorder.php new file mode 100644 index 0000000..93a1a9e --- /dev/null +++ b/src/Service/Statistics/StatisticsRecorder.php @@ -0,0 +1,154 @@ + $dimensions + * @param array $payload + */ + public function record( + StatisticsEventName $name, + array $dimensions = [], + array $payload = [], + ?\DateTimeImmutable $occurredAt = null, + ): void { + $data = array_merge([ + 'assignment_id' => null, + 'disposition_id' => null, + 'application_id' => null, + 'teamer_id' => null, + 'destination_id' => null, + 'job_profile_id' => null, + 'hotel_code' => null, + ], $dimensions); + + $data['name'] = $name->value; + $data['occurred_at'] = ($occurredAt ?? new \DateTimeImmutable())->format('Y-m-d H:i:s'); + + $user = $this->security->getUser(); + + $data['actor_id'] = $user instanceof User ? $user->getId() : null; + $data['actor_label'] = $user?->getUserIdentifier(); + $data['actor_role'] = $this->resolveActorRole()->value; + + try { + // Encoded inside the try: a payload that will not encode costs the event either + // way, and this is the difference between a log line naming the payload and one + // naming a column that rejected the literal false. + $data['payload'] = json_encode($payload, JSON_THROW_ON_ERROR); + + $this->connection->insert('statistics_event', $data); + } catch (Exception|\JsonException $e) { + $this->logger->error('Could not record statistic event', [ + 'statistics_event' => $name->value, + 'error' => $e->getMessage(), + ]); + } + } + + /** + * @param array $payload + */ + public function recordForAssignment( + StatisticsEventName $name, + Assignment $assignment, + array $payload = [], + ?\DateTimeImmutable $occurredAt = null, + ): void { + $this->record($name, $this->dimensionsFromAssignment($assignment), $payload, $occurredAt); + } + + /** + * @param array $payload + */ + public function recordForDisposition( + StatisticsEventName $name, + Disposition $disposition, + array $payload = [], + ?\DateTimeImmutable $occurredAt = null, + ): void { + $dimensions = $this->dimensionsFromAssignment($disposition->getAssignment()); + $dimensions['disposition_id'] = $disposition->getId(); + $dimensions['teamer_id'] = $disposition->getTeamer()?->getId(); + + $this->record($name, $dimensions, $payload, $occurredAt); + } + + /** + * @param array $payload + */ + public function recordForApplication( + StatisticsEventName $name, + Application $application, + array $payload = [], + ?\DateTimeImmutable $occurredAt = null, + ): void { + $dimensions = $this->dimensionsFromAssignment($application->getAssignment()); + $dimensions['application_id'] = $application->getId(); + $dimensions['teamer_id'] = $application->getTeamer()?->getId(); + + $this->record($name, $dimensions, $payload, $occurredAt); + } + + public function resolveActorRole(): StatisticsActorRole + { + if (null === $this->security->getUser()) { + return StatisticsActorRole::SYSTEM; + } + + if ($this->security->isGranted('ROLE_ADMINISTRATIVE')) { + return StatisticsActorRole::ADMIN; + } + + if ($this->security->isGranted('ROLE_TEAMER')) { + return StatisticsActorRole::TEAMER; + } + + return StatisticsActorRole::SYSTEM; + } + + /** + * @return array + */ + private function dimensionsFromAssignment(?Assignment $assignment): array + { + $destination = $assignment?->getDestination(); + + return [ + 'assignment_id' => $assignment?->getId(), + 'destination_id' => $destination?->getId(), + 'job_profile_id' => $assignment?->getJobProfile()?->getId(), + 'hotel_code' => $destination?->getHotelCodeNormalized(), + ]; + } +} diff --git a/tests/EventListener/StatisticsChangeSetListenerTest.php b/tests/EventListener/StatisticsChangeSetListenerTest.php new file mode 100644 index 0000000..80c60ea --- /dev/null +++ b/tests/EventListener/StatisticsChangeSetListenerTest.php @@ -0,0 +1,393 @@ +recorder = $this->createMock(StatisticsRecorder::class); + } + + public function testRecordsBothSidesOfAJobProfileChange(): void + { + $assignment = new Assignment(); + $old = (new JobProfile())->setName('Skilehrer'); + $new = (new JobProfile())->setName('Hausleitung'); + + $this->recorder + ->expects($this->once()) + ->method('recordForAssignment') + ->with( + StatisticsEventName::ASSIGNMENT_JOB_PROFILE_CHANGED, + $assignment, + $this->callback(static function (array $payload): bool { + return 'Skilehrer' === $payload['from']['name'] + && 'Hausleitung' === $payload['to']['name']; + }) + ) + ; + + $this->flush([[$assignment, ['jobProfile' => [$old, $new]]]]); + } + + /** + * An assignment gets edited constantly. Only the job profile column may produce a row. + */ + public function testIgnoresAnAssignmentEditThatLeavesTheJobProfileAlone(): void + { + $this->recorder->expects($this->never())->method('recordForAssignment'); + + $this->flush([[new Assignment(), ['remarks' => ['alt', 'neu'], 'pickup' => [1, 2]]]]); + } + + /** + * A newly created assignment gets its profile as an insert, not an update, and is not a + * change anybody made. + */ + public function testIgnoresAJobProfileThatDidNotActuallyChange(): void + { + $profile = (new JobProfile())->setName('Skilehrer'); + + $this->recorder->expects($this->never())->method('recordForAssignment'); + + $this->flush([[new Assignment(), ['jobProfile' => [$profile, $profile]]]]); + } + + public function testRecordsACallOffWithWhoseDecisionItWas(): void + { + $disposition = $this->disposition(); + $disposition + ->setCalledOffBy(Disposition::CALLED_OFF_BY_TEAMER) + ->setCalledOffReason('Krankheit') + ; + + $this->recorder + ->expects($this->once()) + ->method('recordForDisposition') + ->with( + StatisticsEventName::DISPOSITION_CALLED_OFF, + $disposition, + $this->callback(static function (array $payload): bool { + return Disposition::CALLED_OFF_BY_TEAMER === $payload['called_off_by'] + && 'Krankheit' === $payload['reason'] + && Disposition::STATUS_CONFIRMED === $payload['previous_status']; + }) + ) + ; + + $this->flush([[$disposition, ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]]]); + } + + public function testIgnoresAnyOtherDispositionStatusChange(): void + { + $this->recorder->expects($this->never())->method('recordForDisposition'); + + $this->flush([[$this->disposition(), [ + 'status' => [Disposition::STATUS_NEW, Disposition::STATUS_CONFIRMED], + ]]]); + } + + /** + * Calling off a whole assignment cascades the status onto every disposition in one + * flush, which is exactly why this listens on the changeset instead of on + * DispositionCalledOffEvent - that event is never dispatched for the cascade. + */ + public function testRecordsEveryDispositionOfACascadedAssignmentCallOff(): void + { + $assignment = new Assignment(); + + $this->recorder->expects($this->exactly(2))->method('recordForDisposition'); + + $this->flush([ + [$assignment, ['status' => [Assignment::STATUS_PUBLISHED, Assignment::STATUS_CALLED_OFF]]], + [$this->disposition($assignment), ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]], + [$this->disposition($assignment), ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]], + ]); + } + + /** + * One office decision that cancels a trip with two teamers must not read as two office + * decisions. The scope is what separates the decision from its cost. + */ + public function testACascadedDispositionIsScopedToTheAssignment(): void + { + $assignment = new Assignment(); + + $this->recorder + ->expects($this->exactly(2)) + ->method('recordForDisposition') + ->with( + StatisticsEventName::DISPOSITION_CALLED_OFF, + $this->anything(), + $this->callback(static fn (array $payload): bool => CallOffScope::ASSIGNMENT->value === $payload['scope']) + ) + ; + + $this->flush([ + [$assignment, ['status' => [Assignment::STATUS_PUBLISHED, Assignment::STATUS_CALLED_OFF]]], + [$this->disposition($assignment), ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]], + [$this->disposition($assignment), ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]], + ]); + } + + /** + * A teamer dropping out leaves the trip and everyone else on it running. + */ + public function testALoneCallOffIsScopedToTheDispositionOnly(): void + { + $this->recorder + ->expects($this->once()) + ->method('recordForDisposition') + ->with( + StatisticsEventName::DISPOSITION_CALLED_OFF, + $this->anything(), + $this->callback(static fn (array $payload): bool => CallOffScope::DISPOSITION->value === $payload['scope']) + ) + ; + + $this->recorder->expects($this->never())->method('recordForAssignment'); + + $this->flush([[$this->disposition(), ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]]]); + } + + /** + * The trip-level decision is its own row, carrying what the cancellation cost so that + * "how many trips did we cancel" does not have to be counted back out of the + * dispositions. + */ + public function testTheTripCancellationIsRecordedOnceWithItsCost(): void + { + $assignment = new Assignment(); + + $this->recorder + ->expects($this->once()) + ->method('recordForAssignment') + ->with( + StatisticsEventName::ASSIGNMENT_CALLED_OFF, + $assignment, + $this->callback(static fn (array $payload): bool => 2 === $payload['dispositions_affected']) + ) + ; + + $this->flush([ + [$assignment, ['status' => [Assignment::STATUS_PUBLISHED, Assignment::STATUS_CALLED_OFF]]], + [$this->disposition($assignment), ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]], + [$this->disposition($assignment), ['status' => [Disposition::STATUS_CONFIRMED, Disposition::STATUS_CALLED_OFF]]], + ]); + } + + /** + * A trip cancelled before anyone was staffed still counts as a decision - 58 of the 88 + * in the database are exactly that. + */ + public function testAnUnstaffedTripCancellationCostsNobody(): void + { + $this->recorder + ->expects($this->once()) + ->method('recordForAssignment') + ->with( + StatisticsEventName::ASSIGNMENT_CALLED_OFF, + $this->anything(), + $this->callback(static fn (array $payload): bool => 0 === $payload['dispositions_affected']) + ) + ; + + $this->flush([[new Assignment(), ['status' => [Assignment::STATUS_PUBLISHED, Assignment::STATUS_CALLED_OFF]]]]); + } + + /** + * A second flush in the same request must not replay what the first already recorded. + */ + public function testDoesNotRecordTheSameChangeTwice(): void + { + $assignment = new Assignment(); + $old = (new JobProfile())->setName('Skilehrer'); + $new = (new JobProfile())->setName('Hausleitung'); + + $this->recorder->expects($this->once())->method('recordForAssignment'); + + $listener = $this->listener(); + + $listener->onFlush($this->onFlushArgs([[$assignment, ['jobProfile' => [$old, $new]]]])); + $listener->postFlush($this->postFlushArgs()); + $listener->postFlush($this->postFlushArgs()); + } + + /** + * A flush that throws never reaches postFlush. What it collected has to die with it, + * rather than be written by whichever flush succeeds next - in a worker that is a + * different message, a different actor and a later date. + */ + public function testAFailedFlushDoesNotLeakIntoTheNextOne(): void + { + $survivor = new Assignment(); + + $this->recorder + ->expects($this->once()) + ->method('recordForAssignment') + ->with(StatisticsEventName::ASSIGNMENT_JOB_PROFILE_CHANGED, $survivor, $this->anything()) + ; + + $listener = $this->listener(); + + // Collected, and then the commit blows up: no postFlush for this one. + $listener->onFlush($this->onFlushArgs([[new Assignment(), [ + 'jobProfile' => [$this->jobProfile('Skilehrer'), $this->jobProfile('Hausleitung')], + ]]])); + + $listener->onFlush($this->onFlushArgs([[$survivor, [ + 'jobProfile' => [$this->jobProfile('Hausleitung'), $this->jobProfile('Skilehrer')], + ]]])); + $listener->postFlush($this->postFlushArgs()); + } + + /** + * Workers reset their services between messages, which is the other way a failed + * flush's leftovers get dropped rather than written by an unrelated one. + */ + public function testResettingDropsWhatAFailedFlushCollected(): void + { + $this->recorder->expects($this->never())->method('recordForAssignment'); + + $listener = $this->listener(); + + $listener->onFlush($this->onFlushArgs([[new Assignment(), [ + 'jobProfile' => [$this->jobProfile('Skilehrer'), $this->jobProfile('Hausleitung')], + ]]])); + $listener->reset(); + $listener->postFlush($this->postFlushArgs()); + } + + /** + * Folded in from a domain-event subscriber: reading the insertions catches every path + * that persists an application, not only the ones that remember to dispatch. + */ + public function testAnApplicationIsRecordedWhenItIsPersisted(): void + { + $application = new Application(new Assignment(), new Teamer()); + + $this->recorder + ->expects($this->once()) + ->method('recordForApplication') + ->with(StatisticsEventName::APPLICATION_CREATED, $application, []) + ; + + $this->flush([], [$application]); + } + + /** + * Editing an application later is not a second application. + */ + public function testAnUpdatedApplicationIsNotRecordedAgain(): void + { + $this->recorder->expects($this->never())->method('recordForApplication'); + + $this->flush([[new Application(new Assignment(), new Teamer()), ['remarks' => ['a', 'b']]]]); + } + + /** + * Other entities are inserted constantly; only applications count here. + */ + public function testOtherInsertionsAreIgnored(): void + { + $this->recorder->expects($this->never())->method('recordForApplication'); + + $this->flush([], [new Assignment(), new Teamer()]); + } + + /** + * @param array}> $changeSets + * @param array $insertions + */ + private function flush(array $changeSets, array $insertions = []): void + { + $listener = $this->listener(); + + $listener->onFlush($this->onFlushArgs($changeSets, $insertions)); + $listener->postFlush($this->postFlushArgs()); + } + + /** + * @param array}> $changeSets + * @param array $insertions + */ + private function onFlushArgs(array $changeSets, array $insertions = []): OnFlushEventArgs + { + $entities = []; + $byEntity = new \SplObjectStorage(); + + foreach ($changeSets as [$entity, $changeSet]) { + $entities[] = $entity; + $byEntity[$entity] = $changeSet; + } + + $unitOfWork = $this->createMock(UnitOfWork::class); + $unitOfWork->method('getScheduledEntityUpdates')->willReturn($entities); + $unitOfWork->method('getScheduledEntityInsertions')->willReturn($insertions); + $unitOfWork + ->method('getEntityChangeSet') + ->willReturnCallback(static fn (object $entity): array => $byEntity[$entity] ?? []) + ; + + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->method('getUnitOfWork')->willReturn($unitOfWork); + + return new OnFlushEventArgs($entityManager); + } + + private function postFlushArgs(): PostFlushEventArgs + { + return new PostFlushEventArgs($this->createMock(EntityManagerInterface::class)); + } + + /** + * Built with the real collectors, so these tests cover them as well as the timing. + */ + private function listener(): StatisticsChangeSetListener + { + return new StatisticsChangeSetListener($this->recorder, [ + new JobProfileChangeCollector(), + new CallOffCollector(), + new ApplicationCreatedCollector(), + ]); + } + + private function jobProfile(string $name): JobProfile + { + return (new JobProfile())->setName($name); + } + + private function disposition(?Assignment $assignment = null): Disposition + { + return new Disposition(new Application($assignment ?? new Assignment(), new Teamer())); + } +} diff --git a/tests/Form/AssignmentTypeTest.php b/tests/Form/AssignmentTypeTest.php new file mode 100644 index 0000000..247c8d2 --- /dev/null +++ b/tests/Form/AssignmentTypeTest.php @@ -0,0 +1,73 @@ +assertSame( + [Assignment::STATUS_DRAFT, Assignment::STATUS_PUBLISHED], + $this->statusChoices(Assignment::STATUS_DRAFT) + ); + } + + /** + * The published case is the one that mattered: a staffed trip cancelled from the edit + * form kept every disposition on it alive. + */ + public function testAPublishedAssignmentCannotBeCalledOffEither(): void + { + $this->assertNotContains(Assignment::STATUS_CALLED_OFF, $this->statusChoices(Assignment::STATUS_PUBLISHED)); + } + + /** + * Otherwise opening its edit form would render an empty status, and saving anything + * else on the page would silently resurrect the assignment. + */ + public function testAnAlreadyCalledOffAssignmentKeepsTheValue(): void + { + $this->assertContains(Assignment::STATUS_CALLED_OFF, $this->statusChoices(Assignment::STATUS_CALLED_OFF)); + } + + /** + * There is no dedicated undo for a call-off, so this form stays the way back. + */ + public function testACalledOffAssignmentCanStillBeRestored(): void + { + $this->assertContains(Assignment::STATUS_PUBLISHED, $this->statusChoices(Assignment::STATUS_CALLED_OFF)); + } + + /** + * A brand new assignment has no entity behind the form yet. + */ + public function testTheChoicesHoldWithoutAnAssignment(): void + { + $this->assertNotContains(Assignment::STATUS_CALLED_OFF, array_values(AssignmentType::statusChoices(null))); + } + + /** + * @return array + */ + private function statusChoices(string $status): array + { + return array_values(AssignmentType::statusChoices((new Assignment())->setStatus($status))); + } +} diff --git a/tests/Repository/StatisticsEventRepositoryTest.php b/tests/Repository/StatisticsEventRepositoryTest.php new file mode 100644 index 0000000..fb2e531 --- /dev/null +++ b/tests/Repository/StatisticsEventRepositoryTest.php @@ -0,0 +1,111 @@ +query(dateFrom: new \DateTimeImmutable('2026-07-01'))->getDQL(); + + $this->assertStringContainsString('assignment.dateFrom >= :dateFrom', $dql); + $this->assertStringNotContainsString('event.occurredAt', $dql); + } + + /** + * The season start is assignment.dateFrom falling back to the destination's, per + * Assignment::getEffectivePeriod(). Losing the fallback would silently drop every + * assignment that inherits its dates. + */ + public function testTheSeasonFallsBackToTheDestinationDates(): void + { + $dql = $this->query( + dateFrom: new \DateTimeImmutable('2026-07-01'), + dateTo: new \DateTimeImmutable('2027-06-30'), + )->getDQL(); + + $this->assertStringContainsString('assignment.dateFrom IS NULL', $dql); + $this->assertStringContainsString('destination.dateFrom >= :dateFrom', $dql); + $this->assertStringContainsString('assignment.dateTo IS NULL', $dql); + $this->assertStringContainsString('destination.dateTo <= :dateTo', $dql); + } + + /** + * The dimension columns are plain integers, not relations, so the assignment is only + * reachable through an arbitrary join. + */ + public function testTheSeasonReachesTheAssignmentThroughAnArbitraryJoin(): void + { + $dql = $this->query(dateFrom: new \DateTimeImmutable('2026-07-01'))->getDQL(); + + $this->assertStringContainsString('assignment.id = event.assignmentId', $dql); + } + + public function testOccurrenceAsksWhenTheEventWasRecordedAndJoinsNothing(): void + { + $dql = $this->query( + dateFrom: new \DateTimeImmutable('2026-07-01'), + dateBasis: StatisticsDateBasis::OCCURRENCE, + )->getDQL(); + + $this->assertStringContainsString('event.occurredAt >= :dateFrom', $dql); + $this->assertStringNotContainsString('JOIN', $dql); + } + + /** + * An unfiltered count spans every season there is, so there is nothing to join for. + */ + public function testNoPeriodMeansNoJoinEitherWay(): void + { + $this->assertStringNotContainsString('JOIN', $this->query()->getDQL()); + } + + public function testTheGroupingColumnStaysWhitelisted(): void + { + $this->expectException(\InvalidArgumentException::class); + + $this->query(dimension: 'payload'); + } + + private function query( + string $dimension = 'hotelCode', + ?\DateTimeImmutable $dateFrom = null, + ?\DateTimeImmutable $dateTo = null, + StatisticsDateBasis $dateBasis = StatisticsDateBasis::SEASON, + ): Query { + return $this->repository()->getCountGroupedByQuery( + StatisticsEventName::APPLICATION_CREATED, + $dimension, + $dateFrom, + $dateTo, + $dateBasis, + ); + } + + private function repository(): StatisticsEventRepository + { + /** @var EntityManagerInterface $entityManager */ + $entityManager = static::getContainer()->get(EntityManagerInterface::class); + + /** @var StatisticsEventRepository $repository */ + $repository = $entityManager->getRepository(StatisticsEvent::class); + + return $repository; + } +} diff --git a/tests/Service/Statistics/StatisticsRecorderTest.php b/tests/Service/Statistics/StatisticsRecorderTest.php new file mode 100644 index 0000000..8f5f112 --- /dev/null +++ b/tests/Service/Statistics/StatisticsRecorderTest.php @@ -0,0 +1,177 @@ +connection = $this->createMock(Connection::class); + $this->security = $this->createMock(Security::class); + $this->logger = $this->createMock(LoggerInterface::class); + } + + public function testWritesTheEnumValueAndLeavesUnusedDimensionsNull(): void + { + $written = $this->capture(); + + $this->recorder()->record(StatisticsEventName::APPLICATION_CREATED, ['application_id' => 7]); + + $this->assertSame('application.created', $written['data']['name']); + $this->assertSame('statistics_event', $written['table']); + $this->assertSame(7, $written['data']['application_id']); + $this->assertNull($written['data']['disposition_id']); + $this->assertNull($written['data']['hotel_code']); + } + + /** + * The hotel metric is asked for by hotel, not by destination, and several destinations + * share one hotel under a SER prefix. + */ + public function testFreezesTheNormalizedHotelCodeOfTheAssignment(): void + { + $written = $this->capture(); + + $this->recorder()->recordForAssignment( + StatisticsEventName::ASSIGNMENT_JOB_PROFILE_CHANGED, + $this->assignmentInHotel('SERZIL'), + ); + + $this->assertSame('ZIL', $written['data']['hotel_code']); + } + + public function testKeepsAnUnprefixedHotelCodeAsItIs(): void + { + $written = $this->capture(); + + $this->recorder()->recordForAssignment( + StatisticsEventName::ASSIGNMENT_JOB_PROFILE_CHANGED, + $this->assignmentInHotel('SBW'), + ); + + $this->assertSame('SBW', $written['data']['hotel_code']); + } + + public function testAttributesAnAdministrativeUser(): void + { + $this->givenUser(['ROLE_ADMINISTRATIVE']); + + $this->assertSame(StatisticsActorRole::ADMIN, $this->recorder()->resolveActorRole()); + } + + public function testAttributesATeamer(): void + { + $this->givenUser(['ROLE_TEAMER']); + + $this->assertSame(StatisticsActorRole::TEAMER, $this->recorder()->resolveActorRole()); + } + + /** + * The backfill and the cron run without a token. + */ + public function testAttributesAnythingWithoutATokenToTheSystem(): void + { + $this->security->method('getUser')->willReturn(null); + + $this->assertSame(StatisticsActorRole::SYSTEM, $this->recorder()->resolveActorRole()); + } + + /** + * Statistics are a side effect. Losing one must never cost the teamer their + * application or the admin their edit. + */ + public function testSwallowsAWriteFailureInsteadOfBreakingTheAction(): void + { + $this->connection + ->method('insert') + ->willThrowException(new DbalException('no such table')) + ; + + $this->logger->expects($this->once())->method('error'); + + $this->recorder()->record(StatisticsEventName::APPLICATION_CREATED); + } + + /** + * A payload that will not encode is a lost event either way. What must not happen is + * losing it to a DBAL error about the column, with nothing in the log pointing at the + * payload that caused it. + */ + public function testReportsAPayloadThatCannotBeEncodedInsteadOfWritingGarbage(): void + { + $this->connection->expects($this->never())->method('insert'); + $this->logger->expects($this->once())->method('error'); + + $this->recorder()->record( + StatisticsEventName::DISPOSITION_CALLED_OFF, + [], + ['reason' => "\xB1\x31"], + ); + } + + /** + * Hands back the row the recorder writes. An ArrayObject rather than an array, so the + * callback can fill it in after this method has already returned. + * + * @return \ArrayObject + */ + private function capture(): \ArrayObject + { + $written = new \ArrayObject(['table' => '', 'data' => []]); + + $this->connection + ->method('insert') + ->willReturnCallback(static function (string $table, array $data) use ($written): int { + $written['table'] = $table; + $written['data'] = $data; + + return 1; + }) + ; + + return $written; + } + + private function recorder(): StatisticsRecorder + { + return new StatisticsRecorder($this->connection, $this->security, $this->logger); + } + + /** + * @param array $grantedRoles + */ + private function givenUser(array $grantedRoles): void + { + $this->security->method('getUser')->willReturn(new User()); + $this->security + ->method('isGranted') + ->willReturnCallback(static fn (mixed $role): bool => in_array($role, $grantedRoles, true)) + ; + } + + private function assignmentInHotel(string $hotelCode): Assignment + { + $destination = (new Destination())->setHotelCode($hotelCode); + + return (new Assignment())->setDestination($destination); + } +}