diff --git a/docs/statistics.md b/docs/statistics.md index 1eb897f..bd84729 100644 --- a/docs/statistics.md +++ b/docs/statistics.md @@ -26,12 +26,13 @@ Two corollaries that surprise people: ## Why the table exists -Three of the four metrics could not be answered from live data: +Most of the 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. + over time. Dispositions are hard deleted too, and leave even less behind — no `createdAt`, + no trace of the teamer they were on. - Call-offs persisted, but not *when* they happened — `updatedAt` is overwritten by any later edit — nor at what scope. @@ -81,7 +82,7 @@ hotel code does not rewrite past periods. This is the same pattern `Feedback::fr --- -## The four metrics +## The five 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. @@ -91,6 +92,7 @@ records it is the entire cost of a new metric. | `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` | +| `disposition.deleted` | a disposition is hard deleted, on any delete path | `previous_status`, `remarks`, `called_off_by` | | `application.created` | an application is persisted, by any path | — | Which dimensions each one fills: @@ -100,8 +102,14 @@ Which dimensions each one fills: | `assignment.job_profile_changed` | ✓ | ✓ | ✓ | ✓ | — | — | — | | `assignment.called_off` | ✓ | ✓ | ✓ | ✓ | — | — | — | | `disposition.called_off` | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | — | +| `disposition.deleted` | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | — | | `application.created` | ✓ | ✓ | ✓ | ✓ | ✓ | — | ✓ | +`disposition.deleted` carries `remarks` because a hard delete destroys the entity outright: +the justification an admin types on the delete form (`DeleteController`) lives nowhere else +afterwards. `called_off_by` is set only when the placement had already been called off before +it was deleted. + ### Call-offs: three different "who" and "how far" This is the part most likely to be misread when building a chart. @@ -174,6 +182,18 @@ 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`. +Collectors see **deletions** too, through `deletionsOf()`. `DispositionDeletedCollector` +reads them for the same reason: every delete path is counted, and a hard delete leaves +nothing to reconstruct afterwards. Deletions carry one wrinkle the other two do not. +Recording still happens in `postFlush`, but by then Doctrine has removed the entity from the +identity map and **nulled its generated id** (`UnitOfWork::executeDeletions()`), so +`StatisticsRecorder` can no longer resolve the dimension columns from it. A deletion +collector therefore snapshots its dimensions itself, during `onFlush`, via +`StatisticsDimensions::forDisposition()`, and passes them on the `CollectedStatisticsEvent`; +the listener writes that array verbatim instead of resolving from the subject. Anything a +deletion metric needs in its payload — status, the form `remarks` — has to be read the same +way, off the still-attached entity, before the flush commits. + > **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. @@ -217,7 +237,7 @@ before the season it belongs to. Picking the wrong one does not fail — it quie 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. +no `assignment_id`. All five metrics set it. ### Crossing with live data @@ -252,6 +272,7 @@ It writes only to `statistics_event`, so it is safe to run against production at | `disposition.called_off` | yes | `updatedAt` — **approximate** | | `assignment.called_off` | yes | `updatedAt` — **approximate** | | `assignment.job_profile_changed` | **no** | history does not exist | +| `disposition.deleted` | **no** | the disposition is gone, nothing left to seed | Every seeded row carries `"backfilled": true`; the approximate ones also carry `"approximate_date": true`. So the choice is per chart, never table-wide: @@ -322,6 +343,7 @@ and equally valid question — just not the same one. | Repository | `src/Repository/StatisticsEventRepository.php` | | Enums | `src/Enum/Statistics*.php` | | Write path | `src/Service/Statistics/StatisticsRecorder.php` | +| Dimension columns resolved from an entity | `src/Service/Statistics/StatisticsDimensions.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` | diff --git a/src/Enum/StatisticsEventName.php b/src/Enum/StatisticsEventName.php index cd9109f..108375e 100644 --- a/src/Enum/StatisticsEventName.php +++ b/src/Enum/StatisticsEventName.php @@ -17,6 +17,7 @@ enum StatisticsEventName: string case ASSIGNMENT_JOB_PROFILE_CHANGED = 'assignment.job_profile_changed'; case ASSIGNMENT_CALLED_OFF = 'assignment.called_off'; case DISPOSITION_CALLED_OFF = 'disposition.called_off'; + case DISPOSITION_DELETED = 'disposition.deleted'; case APPLICATION_CREATED = 'application.created'; public function label(): string @@ -25,6 +26,7 @@ enum StatisticsEventName: string self::ASSIGNMENT_JOB_PROFILE_CHANGED => 'Tätigkeitsprofil geändert', self::ASSIGNMENT_CALLED_OFF => 'Reise abgesagt', self::DISPOSITION_CALLED_OFF => 'Einsatz abgesagt', + self::DISPOSITION_DELETED => 'Einteilung gelöscht', self::APPLICATION_CREATED => 'Bewerbung eingegangen', }; } diff --git a/src/EventListener/StatisticsChangeSetListener.php b/src/EventListener/StatisticsChangeSetListener.php index 91bee48..32caeed 100644 --- a/src/EventListener/StatisticsChangeSetListener.php +++ b/src/EventListener/StatisticsChangeSetListener.php @@ -75,6 +75,14 @@ class StatisticsChangeSetListener implements ResetInterface foreach ($pending as $event) { $subject = $event->subject; + // Dimensions the collector froze at onFlush time - the subject is gone or + // detached by now, so there is nothing to resolve them from. + if (null !== $event->dimensions) { + $this->recorder->record($event->name, $event->dimensions, $event->payload); + + continue; + } + if ($subject instanceof Assignment) { $this->recorder->recordForAssignment($event->name, $subject, $event->payload); diff --git a/src/Service/Statistics/CollectedStatisticsEvent.php b/src/Service/Statistics/CollectedStatisticsEvent.php index 40c4700..2259660 100644 --- a/src/Service/Statistics/CollectedStatisticsEvent.php +++ b/src/Service/Statistics/CollectedStatisticsEvent.php @@ -12,16 +12,23 @@ use App\Enum\StatisticsEventName; * * Collectors run during onFlush, where the changeset still exists, but nothing may be * written until the transaction has committed. This is what travels between the two. + * + * Normally the recorder resolves the dimension columns from $subject in postFlush. A + * collector that watches deletions cannot rely on that - the entity is detached and its id + * nulled by then - so it snapshots $dimensions now, and the listener writes them verbatim. */ final class CollectedStatisticsEvent { /** - * @param array $payload + * @param array $payload + * @param array|null $dimensions pre-resolved at collect time; when + * set, written as-is instead of from $subject */ public function __construct( public readonly StatisticsEventName $name, public readonly Assignment|Disposition|Application $subject, public readonly array $payload = [], + public readonly ?array $dimensions = null, ) { } } diff --git a/src/Service/Statistics/Collector/DispositionDeletedCollector.php b/src/Service/Statistics/Collector/DispositionDeletedCollector.php new file mode 100644 index 0000000..34ae9cc --- /dev/null +++ b/src/Service/Statistics/Collector/DispositionDeletedCollector.php @@ -0,0 +1,43 @@ +deletionsOf(Disposition::class) as $disposition) { + yield new CollectedStatisticsEvent( + StatisticsEventName::DISPOSITION_DELETED, + $disposition, + [ + 'previous_status' => $disposition->getStatus(), + // The reason typed on the delete form, bound onto the entity before the + // remove(). Nowhere else survives the deletion. + 'remarks' => $disposition->getRemarks(), + // Non-null only if the placement had already been called off first. + 'called_off_by' => $disposition->getCalledOffBy(), + ], + StatisticsDimensions::forDisposition($disposition), + ); + } + } +} diff --git a/src/Service/Statistics/StatisticsDimensions.php b/src/Service/Statistics/StatisticsDimensions.php new file mode 100644 index 0000000..c4cee0c --- /dev/null +++ b/src/Service/Statistics/StatisticsDimensions.php @@ -0,0 +1,57 @@ + + */ + public static function forAssignment(?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(), + ]; + } + + /** + * @return array + */ + public static function forDisposition(Disposition $disposition): array + { + $dimensions = self::forAssignment($disposition->getAssignment()); + $dimensions['disposition_id'] = $disposition->getId(); + $dimensions['teamer_id'] = $disposition->getTeamer()?->getId(); + + return $dimensions; + } + + /** + * @return array + */ + public static function forApplication(Application $application): array + { + $dimensions = self::forAssignment($application->getAssignment()); + $dimensions['application_id'] = $application->getId(); + $dimensions['teamer_id'] = $application->getTeamer()?->getId(); + + return $dimensions; + } +} diff --git a/src/Service/Statistics/StatisticsFlush.php b/src/Service/Statistics/StatisticsFlush.php index 900747c..6b5b9db 100644 --- a/src/Service/Statistics/StatisticsFlush.php +++ b/src/Service/Statistics/StatisticsFlush.php @@ -71,6 +71,34 @@ final class StatisticsFlush )); } + /** + * @return array + */ + public function deletions(): array + { + return $this->unitOfWork->getScheduledEntityDeletions(); + } + + /** + * A scheduled-for-deletion entity still has its id and its initialised relations here, + * during onFlush. It does not in postFlush: Doctrine removes it from the identity map + * and nulls its generated id once the delete commits, so a collector that cares about a + * deletion has to read everything it needs off the entity now, not later. + * + * @template T of object + * + * @param class-string $class + * + * @return array + */ + public function deletionsOf(string $class): array + { + return array_values(array_filter( + $this->deletions(), + static fn (object $entity): bool => $entity instanceof $class, + )); + } + /** * @return array */ diff --git a/src/Service/Statistics/StatisticsRecorder.php b/src/Service/Statistics/StatisticsRecorder.php index 93a1a9e..aff7c18 100644 --- a/src/Service/Statistics/StatisticsRecorder.php +++ b/src/Service/Statistics/StatisticsRecorder.php @@ -85,7 +85,7 @@ class StatisticsRecorder array $payload = [], ?\DateTimeImmutable $occurredAt = null, ): void { - $this->record($name, $this->dimensionsFromAssignment($assignment), $payload, $occurredAt); + $this->record($name, StatisticsDimensions::forAssignment($assignment), $payload, $occurredAt); } /** @@ -97,11 +97,7 @@ class StatisticsRecorder 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); + $this->record($name, StatisticsDimensions::forDisposition($disposition), $payload, $occurredAt); } /** @@ -113,11 +109,7 @@ class StatisticsRecorder 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); + $this->record($name, StatisticsDimensions::forApplication($application), $payload, $occurredAt); } public function resolveActorRole(): StatisticsActorRole @@ -136,19 +128,4 @@ class StatisticsRecorder 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 index 80c60ea..4a4c9e0 100644 --- a/tests/EventListener/StatisticsChangeSetListenerTest.php +++ b/tests/EventListener/StatisticsChangeSetListenerTest.php @@ -14,6 +14,7 @@ use App\Enum\StatisticsEventName; use App\EventListener\StatisticsChangeSetListener; use App\Service\Statistics\Collector\ApplicationCreatedCollector; use App\Service\Statistics\Collector\CallOffCollector; +use App\Service\Statistics\Collector\DispositionDeletedCollector; use App\Service\Statistics\Collector\JobProfileChangeCollector; use App\Service\Statistics\StatisticsRecorder; use Doctrine\ORM\EntityManagerInterface; @@ -304,6 +305,46 @@ class StatisticsChangeSetListenerTest extends TestCase $this->flush([], [$application]); } + /** + * A hard delete leaves nothing behind, so this row is the only trace the placement, its + * teamer and the admin's justification ever existed. The dimensions have to be read + * while the entity is still whole - by postFlush Doctrine has nulled its id. + */ + public function testRecordsADispositionDeletion(): void + { + $disposition = $this->disposition(); + $disposition + ->setStatus(Disposition::STATUS_CONFIRMED) + ->setRemarks('Doppelt eingeteilt') + ; + + $this->recorder + ->expects($this->once()) + ->method('record') + ->with( + StatisticsEventName::DISPOSITION_DELETED, + $this->anything(), + $this->callback(static function (array $payload): bool { + return Disposition::STATUS_CONFIRMED === $payload['previous_status'] + && 'Doppelt eingeteilt' === $payload['remarks'] + && null === $payload['called_off_by']; + }) + ) + ; + + $this->flush([], [], [$disposition]); + } + + /** + * A disposition being edited is not a disposition being deleted. + */ + public function testADispositionUpdateIsNotADeletion(): void + { + $this->recorder->expects($this->never())->method('record'); + + $this->flush([[$this->disposition(), ['remarks' => ['alt', 'neu']]]]); + } + /** * Editing an application later is not a second application. */ @@ -327,20 +368,22 @@ class StatisticsChangeSetListenerTest extends TestCase /** * @param array}> $changeSets * @param array $insertions + * @param array $deletions */ - private function flush(array $changeSets, array $insertions = []): void + private function flush(array $changeSets, array $insertions = [], array $deletions = []): void { $listener = $this->listener(); - $listener->onFlush($this->onFlushArgs($changeSets, $insertions)); + $listener->onFlush($this->onFlushArgs($changeSets, $insertions, $deletions)); $listener->postFlush($this->postFlushArgs()); } /** * @param array}> $changeSets * @param array $insertions + * @param array $deletions */ - private function onFlushArgs(array $changeSets, array $insertions = []): OnFlushEventArgs + private function onFlushArgs(array $changeSets, array $insertions = [], array $deletions = []): OnFlushEventArgs { $entities = []; $byEntity = new \SplObjectStorage(); @@ -353,6 +396,7 @@ class StatisticsChangeSetListenerTest extends TestCase $unitOfWork = $this->createMock(UnitOfWork::class); $unitOfWork->method('getScheduledEntityUpdates')->willReturn($entities); $unitOfWork->method('getScheduledEntityInsertions')->willReturn($insertions); + $unitOfWork->method('getScheduledEntityDeletions')->willReturn($deletions); $unitOfWork ->method('getEntityChangeSet') ->willReturnCallback(static fn (object $entity): array => $byEntity[$entity] ?? []) @@ -377,6 +421,7 @@ class StatisticsChangeSetListenerTest extends TestCase return new StatisticsChangeSetListener($this->recorder, [ new JobProfileChangeCollector(), new CallOffCollector(), + new DispositionDeletedCollector(), new ApplicationCreatedCollector(), ]); } diff --git a/tests/Service/Statistics/StatisticsDimensionsTest.php b/tests/Service/Statistics/StatisticsDimensionsTest.php new file mode 100644 index 0000000..35d9e36 --- /dev/null +++ b/tests/Service/Statistics/StatisticsDimensionsTest.php @@ -0,0 +1,65 @@ +setDestination((new Destination())->setHotelCode('SERZIL')); + + $this->assertSame('ZIL', StatisticsDimensions::forAssignment($assignment)['hotel_code']); + } + + public function testKeepsAnUnprefixedHotelCodeAsItIs(): void + { + $assignment = (new Assignment())->setDestination((new Destination())->setHotelCode('SBW')); + + $this->assertSame('SBW', StatisticsDimensions::forAssignment($assignment)['hotel_code']); + } + + public function testADispositionCarriesItsTeamerAndTheAssignmentDimensions(): void + { + $teamer = new Teamer(); + $assignment = (new Assignment())->setDestination((new Destination())->setHotelCode('SERZIL')); + $disposition = new Disposition(new Application($assignment, $teamer)); + + $dimensions = StatisticsDimensions::forDisposition($disposition); + + $this->assertArrayHasKey('disposition_id', $dimensions); + $this->assertSame($teamer->getId(), $dimensions['teamer_id']); + $this->assertSame('ZIL', $dimensions['hotel_code']); + } + + /** + * A disposition can lose its assignment or teamer; the columns just go null, they do not + * blow up the flush that is trying to record the event. + */ + public function testDegradesToNullWithoutAnAssignmentOrTeamer(): void + { + $disposition = new Disposition(new Application(new Assignment(), new Teamer())); + $disposition->setAssignment(null); + $disposition->setTeamer(null); + + $dimensions = StatisticsDimensions::forDisposition($disposition); + + $this->assertNull($dimensions['assignment_id']); + $this->assertNull($dimensions['destination_id']); + $this->assertNull($dimensions['hotel_code']); + $this->assertNull($dimensions['teamer_id']); + } +}