From 92ea10feb8c0233c2c38b5c9b74b058c8ea048b4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 24 Sep 2026 00:52:53 -0300 Subject: [PATCH 1/2] fix(audit): log each track chair score change once and as a single entry Every score is added to both Presentation::track_chairs_scores and SummitTrackChair::scores, and the OTLP strategy audited both collections, so each score was logged twice. Changing a score is persisted as remove old + add new, which the generic collection formatter joined into "scored ...|Score removed ...". - OTLP strategy skips Presentation::track_chairs_scores; the track chair side is the one that carries removals - child formatters can format the whole collection diff; the score formatter pairs delete + insert for the same presentation and rating type into one "changed score from A to B" entry, taking the chair from the collection owner since removeScore() nulls the removed score's reviewer - disable entity-level audit for PresentationTrackChairScore, already covered by the collection entry --- app/Audit/AuditLogOtlpStrategy.php | 26 +++ ...ChildEntityCollectionAuditLogFormatter.php | 33 ++++ ...tationTrackChairScoreAuditLogFormatter.php | 86 ++++++++- ...ntityCollectionUpdateAuditLogFormatter.php | 7 + config/audit_log.php | 5 + ...itLogOtlpStrategySkippedCollectionTest.php | 101 ++++++++++ ...onTrackChairScoreAuditLogFormatterTest.php | 176 ++++++++++++++++++ 7 files changed, 433 insertions(+), 1 deletion(-) create mode 100644 app/Audit/ConcreteFormatters/ChildEntityFormatters/IChildEntityCollectionAuditLogFormatter.php create mode 100644 tests/OpenTelemetry/AuditLogOtlpStrategySkippedCollectionTest.php create mode 100644 tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php diff --git a/app/Audit/AuditLogOtlpStrategy.php b/app/Audit/AuditLogOtlpStrategy.php index 171a9fa8e..eff0467ac 100644 --- a/app/Audit/AuditLogOtlpStrategy.php +++ b/app/Audit/AuditLogOtlpStrategy.php @@ -21,6 +21,15 @@ */ class AuditLogOtlpStrategy implements IAuditStrategy { + /** + * Bidirectional collections whose changes are already audited from the other side. + * A track chair score is added to both Presentation::track_chairs_scores and + * SummitTrackChair::scores, and only the track chair side carries score removals, + * so auditing both emits every score twice. + */ + private const SKIPPED_COLLECTIONS = [ + \models\summit\Presentation::class => ['track_chairs_scores'], + ]; private bool $enabled; private string $elasticIndex; @@ -51,6 +60,9 @@ public function audit($subject, array $change_set, string $event_type, AuditCon } Log::debug("AuditLogOtlpStrategy::audit", ['subject' => $subject, 'change_set' => $change_set, 'event_type' => $event_type]); try { + if ($this->isSkippedCollection($subject)) { + return; + } $entity = $this->resolveAuditableEntity($subject); if (is_null($entity)) { Log::warning("AuditLogOtlpStrategy::audit subject not found"); @@ -89,6 +101,20 @@ public function audit($subject, array $change_set, string $event_type, AuditCon } } + private function isSkippedCollection($subject): bool + { + if (!$subject instanceof PersistentCollection) { + return false; + } + $owner = $subject->getOwner(); + foreach (self::SKIPPED_COLLECTIONS as $class => $fields) { + if ($owner instanceof $class && in_array($subject->getMapping()->fieldName, $fields, true)) { + return true; + } + } + return false; + } + private function resolveAuditableEntity($subject) { // 1) special cases first diff --git a/app/Audit/ConcreteFormatters/ChildEntityFormatters/IChildEntityCollectionAuditLogFormatter.php b/app/Audit/ConcreteFormatters/ChildEntityFormatters/IChildEntityCollectionAuditLogFormatter.php new file mode 100644 index 000000000..75b88edbe --- /dev/null +++ b/app/Audit/ConcreteFormatters/ChildEntityFormatters/IChildEntityCollectionAuditLogFormatter.php @@ -0,0 +1,33 @@ +getOwner(); + + $removed = []; + foreach ($collection->getDeleteDiff() as $score) { + if (!$score instanceof PresentationTrackChairScore) continue; + $removed[$this->getReplacementKey($score, $owner)] = $score; + } + + $lines = []; + foreach ($collection->getInsertDiff() as $score) { + if (!$score instanceof PresentationTrackChairScore) continue; + $key = $this->getReplacementKey($score, $owner); + $old = $removed[$key] ?? null; + unset($removed[$key]); + + $lines[] = is_null($old) + ? sprintf( + "Track Chair '%s' scored '%s' on presentation '%s'", + $this->getChairName($score, $owner), + $score->getType()->getName(), + $this->getPresentationTitle($score, $owner) + ) + : sprintf( + "Track Chair '%s' changed score from '%s' to '%s' on presentation '%s'", + $this->getChairName($score, $owner), + $old->getType()->getName(), + $score->getType()->getName(), + $this->getPresentationTitle($score, $owner) + ); + } + + foreach ($removed as $score) { + $lines[] = sprintf( + "Track Chair '%s' removed score '%s' from presentation '%s'", + $this->getChairName($score, $owner), + $score->getType()->getName(), + $this->getPresentationTitle($score, $owner) + ); + } + + return empty($lines) ? null : implode(' | ', $lines); + } catch (\Exception $ex) { + Log::warning("PresentationTrackChairScoreAuditLogFormatter::formatCollection error: " . $ex->getMessage()); + } + + return null; + } + + private function getReplacementKey(PresentationTrackChairScore $score, $owner): string + { + $presentation = $owner instanceof Presentation ? $owner : ($score->hasPresentation() ? $score->getPresentation() : null); + return sprintf("%s:%s", $presentation?->getId() ?? 0, $score->getType()->getType()->getId()); + } + + /** + * removeScore() / removeTrackChairScore() null the back reference on the removed score, + * so the side that owns the collection is taken from the collection owner. + */ + private function getChairName(PresentationTrackChairScore $score, $owner): string + { + $chair = $owner instanceof SummitTrackChair ? $owner : ($score->hasReviewer() ? $score->getReviewer() : null); + return is_null($chair) ? 'Unknown Chair' : ($chair->getMember()->getFullName() ?? 'Unknown Chair'); + } + + private function getPresentationTitle(PresentationTrackChairScore $score, $owner): string + { + $presentation = $owner instanceof Presentation ? $owner : ($score->hasPresentation() ? $score->getPresentation() : null); + return is_null($presentation) ? 'Unknown Presentation' : $presentation->getTitle(); + } + public function format($subject, string $child_entity_action_type, ?string $additional_info = ""): ?string { if (!$subject instanceof PresentationTrackChairScore) { diff --git a/app/Audit/ConcreteFormatters/EntityCollectionUpdateAuditLogFormatter.php b/app/Audit/ConcreteFormatters/EntityCollectionUpdateAuditLogFormatter.php index dd99c55be..ffa70ff46 100644 --- a/app/Audit/ConcreteFormatters/EntityCollectionUpdateAuditLogFormatter.php +++ b/app/Audit/ConcreteFormatters/EntityCollectionUpdateAuditLogFormatter.php @@ -3,8 +3,10 @@ namespace App\Audit\ConcreteFormatters; use App\Audit\ConcreteFormatters\ChildEntityFormatters\IChildEntityAuditLogFormatter; +use App\Audit\ConcreteFormatters\ChildEntityFormatters\IChildEntityCollectionAuditLogFormatter; use App\Audit\AbstractAuditLogFormatter; use App\Audit\Interfaces\IAuditStrategy; +use Doctrine\ORM\PersistentCollection; use Illuminate\Support\Facades\Log; use ReflectionException; @@ -44,6 +46,11 @@ public function __construct(?IChildEntityAuditLogFormatter $child_entity_formatt */ public function format($subject, $change_set): ?string { try { + if ($this->child_entity_formatter instanceof IChildEntityCollectionAuditLogFormatter + && $subject instanceof PersistentCollection) { + return $this->child_entity_formatter->formatCollection($subject); + } + if ($this->child_entity_formatter != null) { $changes = []; diff --git a/config/audit_log.php b/config/audit_log.php index 1d30bd73f..485cfe260 100644 --- a/config/audit_log.php +++ b/config/audit_log.php @@ -60,6 +60,11 @@ 'enabled' => true, 'strategy' => \App\Audit\ConcreteFormatters\PresentationFormatters\PresentationTrackChairRatingTypeAuditLogFormatter::class, ], + // scores are audited through the SummitTrackChair::scores collection; the entity-level + // insert/delete events would log each score again with the generic formatters + \App\Models\Foundation\Summit\Events\Presentations\TrackChairs\PresentationTrackChairScore::class => [ + 'enabled' => false, + ], \App\Models\Foundation\Summit\Events\Presentations\TrackChairs\PresentationTrackChairScoreType::class => [ 'enabled' => true, 'strategy' => \App\Audit\ConcreteFormatters\PresentationFormatters\PresentationTrackChairScoreTypeAuditLogFormatter::class, diff --git a/tests/OpenTelemetry/AuditLogOtlpStrategySkippedCollectionTest.php b/tests/OpenTelemetry/AuditLogOtlpStrategySkippedCollectionTest.php new file mode 100644 index 000000000..f20d35854 --- /dev/null +++ b/tests/OpenTelemetry/AuditLogOtlpStrategySkippedCollectionTest.php @@ -0,0 +1,101 @@ +getProperty('enabled'); + $enabled->setAccessible(true); + $enabled->setValue($strategy, true); + return $strategy; + } + + private function buildCollection(object $owner, string $field, string $mapped_by): PersistentCollection + { + $collection = new PersistentCollection( + $this->createMock(EntityManagerInterface::class), + new ClassMetadata(PresentationTrackChairScore::class), + new ArrayCollection() + ); + $collection->setOwner($owner, OneToManyAssociationMapping::fromMappingArray([ + 'fieldName' => $field, + 'sourceEntity' => get_class($owner), + 'targetEntity' => PresentationTrackChairScore::class, + 'mappedBy' => $mapped_by, + 'isOwningSide' => false, + ])); + return $collection; + } + + public function testPresentationTrackChairScoresCollectionIsNotAudited(): void + { + $factory = $this->createMock(IAuditLogFormatterFactory::class); + $factory->expects($this->never())->method('make'); + + Queue::fake(); + + $this->buildStrategy($factory)->audit( + $this->buildCollection(Mockery::mock(Presentation::class), 'track_chairs_scores', 'presentation'), + [], + IAuditStrategy::EVENT_COLLECTION_UPDATE, + new AuditContext() + ); + + Queue::assertNothingPushed(); + } + + public function testTrackChairScoresCollectionIsAudited(): void + { + $factory = $this->createMock(IAuditLogFormatterFactory::class); + $factory->expects($this->once())->method('make')->willReturn(null); + + $this->buildStrategy($factory)->audit( + $this->buildCollection(Mockery::mock(SummitTrackChair::class), 'scores', 'reviewer'), + [], + IAuditStrategy::EVENT_COLLECTION_UPDATE, + new AuditContext() + ); + } +} diff --git a/tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php b/tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php new file mode 100644 index 000000000..020e7d56d --- /dev/null +++ b/tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php @@ -0,0 +1,176 @@ +shouldReceive('getFullName')->andReturn(self::ChairName); + + $this->chair = Mockery::mock(SummitTrackChair::class); + $this->chair->shouldReceive('getId')->andReturn(7); + $this->chair->shouldReceive('getMember')->andReturn($member); + + $this->presentation = Mockery::mock(Presentation::class); + $this->presentation->shouldReceive('getId')->andReturn(1063); + $this->presentation->shouldReceive('getTitle')->andReturn(self::PresentationTitle); + + $this->rating_type = Mockery::mock(PresentationTrackChairRatingType::class); + $this->rating_type->shouldReceive('getId')->andReturn(3); + } + + protected function tearDown(): void + { + Mockery::close(); + parent::tearDown(); + } + + private function buildScore(string $label): PresentationTrackChairScore + { + $type = Mockery::mock(PresentationTrackChairScoreType::class); + $type->shouldReceive('getName')->andReturn($label); + $type->shouldReceive('getType')->andReturn($this->rating_type); + + $score = new PresentationTrackChairScore(); + $score->setType($type); + $score->setReviewer($this->chair); + $score->setPresentation($this->presentation); + return $score; + } + + private function buildCollection(object $owner, string $field, array $initial): PersistentCollection + { + $mapping = OneToManyAssociationMapping::fromMappingArray([ + 'fieldName' => $field, + 'sourceEntity' => get_class($owner), + 'targetEntity' => PresentationTrackChairScore::class, + 'mappedBy' => $field === 'scores' ? 'reviewer' : 'presentation', + 'isOwningSide' => false, + ]); + + $collection = new PersistentCollection( + $this->createMock(EntityManagerInterface::class), + new ClassMetadata(PresentationTrackChairScore::class), + new ArrayCollection($initial) + ); + $collection->setOwner($owner, $mapping); + $collection->takeSnapshot(); + return $collection; + } + + public function testNewScoreNamesChairAndScore(): void + { + $collection = $this->buildCollection($this->chair, 'scores', []); + $collection->add($this->buildScore('Great')); + + $this->assertSame( + "Track Chair 'Ada Lovelace' scored 'Great' on presentation 'Liquid Cooling Filtration'", + (new PresentationTrackChairScoreAuditLogFormatter())->formatCollection($collection) + ); + } + + public function testReplacedScoreIsOneChangeEntry(): void + { + $old = $this->buildScore('Good'); + $collection = $this->buildCollection($this->chair, 'scores', [$old]); + + // same steps as SummitTrackChair::removeScore + addScore + $collection->removeElement($old); + $old->clearReviewer(); + $collection->add($this->buildScore('Great')); + + $this->assertSame( + "Track Chair 'Ada Lovelace' changed score from 'Good' to 'Great' on presentation 'Liquid Cooling Filtration'", + (new PresentationTrackChairScoreAuditLogFormatter())->formatCollection($collection) + ); + } + + public function testRemovedScoreStillNamesChair(): void + { + $old = $this->buildScore('Good'); + $collection = $this->buildCollection($this->chair, 'scores', [$old]); + + $collection->removeElement($old); + $old->clearReviewer(); + + $this->assertSame( + "Track Chair 'Ada Lovelace' removed score 'Good' from presentation 'Liquid Cooling Filtration'", + (new PresentationTrackChairScoreAuditLogFormatter())->formatCollection($collection) + ); + } + + public function testPresentationSideCollectionResolvesChairFromScore(): void + { + // the database audit strategy only sees the presentation side + $collection = $this->buildCollection($this->presentation, 'track_chairs_scores', []); + $collection->add($this->buildScore('Great')); + + $this->assertSame( + "Track Chair 'Ada Lovelace' scored 'Great' on presentation 'Liquid Cooling Filtration'", + (new PresentationTrackChairScoreAuditLogFormatter())->formatCollection($collection) + ); + } + + public function testCollectionUpdateFormatterDelegatesToCollectionFormatter(): void + { + $old = $this->buildScore('Good'); + $collection = $this->buildCollection($this->chair, 'scores', [$old]); + $collection->removeElement($old); + $old->clearReviewer(); + $collection->add($this->buildScore('Great')); + + $formatter = new EntityCollectionUpdateAuditLogFormatter(new PresentationTrackChairScoreAuditLogFormatter()); + + $this->assertSame( + "Track Chair 'Ada Lovelace' changed score from 'Good' to 'Great' on presentation 'Liquid Cooling Filtration'", + $formatter->format($collection, []) + ); + } +} From 5db89b950a15ce71a5223a2c7cd517c026dfecac Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 24 Sep 2026 00:53:59 -0300 Subject: [PATCH 2/2] fix(audit): resolve chair and score in track chair score entries PresentationTrackChairScoreAuditLogFormatter::format called getScoreType(), getLabel() and getCreatedBy(), which do not exist on the score entity. The magic __call returned null, so every entry read "Track Chair 'Unknown Chair' scored 'Unknown Score'". Use getType()->getName() for the score and the reviewer's member name for the chair. Reviewer and presentation go through hasReviewer()/hasPresentation(), since removeScore() nulls the reviewer and the typed getter would throw a TypeError that no catch(\Exception) stops, failing the flush. --- ...tationTrackChairScoreAuditLogFormatter.php | 18 +++++------ ...onTrackChairScoreAuditLogFormatterTest.php | 31 +++++++++++++++++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/app/Audit/ConcreteFormatters/ChildEntityFormatters/PresentationTrackChairScoreAuditLogFormatter.php b/app/Audit/ConcreteFormatters/ChildEntityFormatters/PresentationTrackChairScoreAuditLogFormatter.php index b5fbc1a0f..e722b55bc 100644 --- a/app/Audit/ConcreteFormatters/ChildEntityFormatters/PresentationTrackChairScoreAuditLogFormatter.php +++ b/app/Audit/ConcreteFormatters/ChildEntityFormatters/PresentationTrackChairScoreAuditLogFormatter.php @@ -96,16 +96,11 @@ public function format($subject, string $child_entity_action_type, ?string $addi } try { - $score_type = $subject->getScoreType(); - $score_label = $score_type ? $score_type->getLabel() : 'Unknown Score'; - - $presentation = $subject->getPresentation(); - $presentation_title = $presentation ? $presentation->getTitle() : 'Unknown Presentation'; - - $created_by = $subject->getCreatedBy(); - $chair_name = $created_by - ? sprintf("%s %s", $created_by->getFirstName(), $created_by->getLastName()) - : 'Unknown Chair'; + $score_label = $subject->getType()->getName(); + // reviewer / presentation are resolved through hasReviewer() / hasPresentation(): + // removeScore() / removeTrackChairScore() null them and the typed getters would throw + $presentation_title = $this->getPresentationTitle($subject, null); + $chair_name = $this->getChairName($subject, null); switch ($child_entity_action_type) { case self::CHILD_ENTITY_CREATION: @@ -117,8 +112,9 @@ public function format($subject, string $child_entity_action_type, ?string $addi ); case self::CHILD_ENTITY_DELETION: return sprintf( - "Score removed for Track Chair '%s' from presentation '%s'", + "Track Chair '%s' removed score '%s' from presentation '%s'", $chair_name, + $score_label, $presentation_title ); case self::CHILD_ENTITY_UPDATE: diff --git a/tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php b/tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php index 020e7d56d..b37527c3b 100644 --- a/tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php +++ b/tests/OpenTelemetry/Formatters/PresentationTrackChairScoreAuditLogFormatterTest.php @@ -158,6 +158,37 @@ public function testPresentationSideCollectionResolvesChairFromScore(): void ); } + public function testFormatResolvesChairAndScoreForEveryAction(): void + { + $formatter = new PresentationTrackChairScoreAuditLogFormatter(); + $score = $this->buildScore('Great'); + + $this->assertSame( + "Track Chair 'Ada Lovelace' scored 'Great' on presentation 'Liquid Cooling Filtration'", + $formatter->format($score, PresentationTrackChairScoreAuditLogFormatter::CHILD_ENTITY_CREATION) + ); + $this->assertSame( + "Track Chair 'Ada Lovelace' score updated to 'Great' on presentation 'Liquid Cooling Filtration'", + $formatter->format($score, PresentationTrackChairScoreAuditLogFormatter::CHILD_ENTITY_UPDATE) + ); + $this->assertSame( + "Track Chair 'Ada Lovelace' removed score 'Great' from presentation 'Liquid Cooling Filtration'", + $formatter->format($score, PresentationTrackChairScoreAuditLogFormatter::CHILD_ENTITY_DELETION) + ); + } + + public function testFormatDoesNotThrowOnRemovedScoreWithoutReviewer(): void + { + $score = $this->buildScore('Good'); + $score->clearReviewer(); + + $this->assertSame( + "Track Chair 'Unknown Chair' removed score 'Good' from presentation 'Liquid Cooling Filtration'", + (new PresentationTrackChairScoreAuditLogFormatter()) + ->format($score, PresentationTrackChairScoreAuditLogFormatter::CHILD_ENTITY_DELETION) + ); + } + public function testCollectionUpdateFormatterDelegatesToCollectionFormatter(): void { $old = $this->buildScore('Good');