From 164f9d9368c138c0c95a06cf4528d633529b15b2 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 29 Apr 2026 13:19:36 -0300 Subject: [PATCH] fix(stats): filter registration stats by SummitAttendeeTicket.SummitID Replace JOIN SummitOrder + SummitOrder.SummitID = :summit_id with a direct SummitAttendeeTicket.SummitID = :summit_id filter across the registration stats queries. Adds supporting indexes via migration Version20260429120000 and unit/integration tests covering the SQL shape and the new index presence. --- .../Summit/SummitRegistrationStats.php | 64 +++++------ .../model/Version20260429120000.php | 88 +++++++++++++++ .../RegistrationStatsIndexesTest.php | 70 ++++++++++++ .../Unit/RegistrationStatsSqlPatternTest.php | 63 +++++++++++ tests/oauth2/OAuth2SummitApiTest.php | 102 ++++++++++++++++++ 5 files changed, 349 insertions(+), 38 deletions(-) create mode 100644 database/migrations/model/Version20260429120000.php create mode 100644 tests/Migrations/RegistrationStatsIndexesTest.php create mode 100644 tests/Unit/RegistrationStatsSqlPatternTest.php diff --git a/app/Models/Foundation/Summit/SummitRegistrationStats.php b/app/Models/Foundation/Summit/SummitRegistrationStats.php index 37aad51aa..863f2ce16 100644 --- a/app/Models/Foundation/Summit/SummitRegistrationStats.php +++ b/app/Models/Foundation/Summit/SummitRegistrationStats.php @@ -104,10 +104,9 @@ public function getActiveTicketsCount(?DateTime $startDate = null, ?DateTime $e { try { $sql = <<getTimeZoneOffset()); $sql = <<sm->listTableIndexes('SummitAttendeeTicket'); + $attendeeIndexes = $this->sm->listTableIndexes('SummitAttendee'); + + if (!array_key_exists('IDX_SummitAttendeeTicket_Stats', $ticketIndexes)) { + $this->addSql( + 'ALTER TABLE SummitAttendeeTicket ADD INDEX IDX_SummitAttendeeTicket_Stats (SummitID, Status, IsActive)' + ); + } + + if (!array_key_exists('IDX_SummitAttendeeTicket_BoughtDate', $ticketIndexes)) { + $this->addSql( + 'ALTER TABLE SummitAttendeeTicket ADD INDEX IDX_SummitAttendeeTicket_BoughtDate (SummitID, Status, IsActive, TicketBoughtDate)' + ); + } + + if (!array_key_exists('IDX_SummitAttendee_HallCheckIn', $attendeeIndexes)) { + $this->addSql( + 'ALTER TABLE SummitAttendee ADD INDEX IDX_SummitAttendee_HallCheckIn (SummitID, SummitHallCheckedIn, SummitHallCheckedInDate)' + ); + } + + if (!array_key_exists('IDX_SummitAttendee_VirtualCheckIn', $attendeeIndexes)) { + $this->addSql( + 'ALTER TABLE SummitAttendee ADD INDEX IDX_SummitAttendee_VirtualCheckIn (SummitID, SummitVirtualCheckedInDate)' + ); + } + } + + public function down(Schema $schema): void + { + $ticketIndexes = $this->sm->listTableIndexes('SummitAttendeeTicket'); + $attendeeIndexes = $this->sm->listTableIndexes('SummitAttendee'); + + if (array_key_exists('IDX_SummitAttendeeTicket_Stats', $ticketIndexes)) { + $this->addSql('ALTER TABLE SummitAttendeeTicket DROP INDEX IDX_SummitAttendeeTicket_Stats'); + } + + if (array_key_exists('IDX_SummitAttendeeTicket_BoughtDate', $ticketIndexes)) { + $this->addSql('ALTER TABLE SummitAttendeeTicket DROP INDEX IDX_SummitAttendeeTicket_BoughtDate'); + } + + if (array_key_exists('IDX_SummitAttendee_HallCheckIn', $attendeeIndexes)) { + $this->addSql('ALTER TABLE SummitAttendee DROP INDEX IDX_SummitAttendee_HallCheckIn'); + } + + if (array_key_exists('IDX_SummitAttendee_VirtualCheckIn', $attendeeIndexes)) { + $this->addSql('ALTER TABLE SummitAttendee DROP INDEX IDX_SummitAttendee_VirtualCheckIn'); + } + } +} diff --git a/tests/Migrations/RegistrationStatsIndexesTest.php b/tests/Migrations/RegistrationStatsIndexesTest.php new file mode 100644 index 000000000..793865682 --- /dev/null +++ b/tests/Migrations/RegistrationStatsIndexesTest.php @@ -0,0 +1,70 @@ + [ + 'IDX_SummitAttendeeTicket_Stats', + 'IDX_SummitAttendeeTicket_BoughtDate', + ], + 'SummitAttendee' => [ + 'IDX_SummitAttendee_HallCheckIn', + 'IDX_SummitAttendee_VirtualCheckIn', + ], + ]; + + $missing = []; + foreach ($required as $tableName => $indexNames) { + foreach ($indexNames as $indexName) { + $rows = DB::connection('model')->select( + "SELECT INDEX_NAME FROM INFORMATION_SCHEMA.STATISTICS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = ? + AND INDEX_NAME = ? + LIMIT 1", + [$tableName, $indexName] + ); + if (empty($rows)) { + $missing[] = "{$tableName}.{$indexName}"; + } + } + } + + $this->assertEmpty( + $missing, + 'The following Approach D composite indexes are missing — ' . + 'ensure the migration (Version20260429.php) was created and applied: ' . + implode(', ', $missing) + ); + } +} diff --git a/tests/Unit/RegistrationStatsSqlPatternTest.php b/tests/Unit/RegistrationStatsSqlPatternTest.php new file mode 100644 index 000000000..4634506bd --- /dev/null +++ b/tests/Unit/RegistrationStatsSqlPatternTest.php @@ -0,0 +1,63 @@ +assertNotFalse($content, 'SummitRegistrationStats.php not found at ' . self::TRAIT_FILE); + + $matchCount = preg_match_all( + '/INNER JOIN SummitOrder\s+on\s+SummitOrder\.ID\s*=\s*SummitAttendeeTicket\.OrderID/i', + $content + ); + + $this->assertSame( + 0, + $matchCount, + "Found {$matchCount} queries in SummitRegistrationStats.php with " . + "'INNER JOIN SummitOrder ON SummitOrder.ID = SummitAttendeeTicket.OrderID'. " . + "After Approach D (D-half-1), ALL 12 affected queries must filter via " . + "SummitAttendeeTicket.SummitID directly — the column added by Version20251002160949. " . + "Rewrite each of the 12 listed methods in the plan: getActiveTicketsCount (L103), " . + "getInactiveTicketsCount (L132), getActiveAssignedTicketsCount (L161), " . + "getTotalPaymentAmountCollected (L218), getTotalRefundAmountEmitted (L248), " . + "getActiveTicketsCountPerTicketType (L278), getCheckedInActiveTicketsCountPerTicketType (L307), " . + "getActiveBadgesCountPerBadgeType (L344), getActiveCheckedInBadgesCountPerBadgeType (L375), " . + "getActiveTicketsPerBadgeFeatureType (L606), getAttendeesCheckinPerBadgeFeatureType (L642), " . + "getPurchasedTicketsGroupedBy (L781)." + ); + } +} diff --git a/tests/oauth2/OAuth2SummitApiTest.php b/tests/oauth2/OAuth2SummitApiTest.php index f2dc68610..705dfdcdd 100644 --- a/tests/oauth2/OAuth2SummitApiTest.php +++ b/tests/oauth2/OAuth2SummitApiTest.php @@ -1227,4 +1227,106 @@ public function testValidateBadge(){ $attendee_badge = json_decode($content); $this->assertNotNull($attendee_badge); } + + /** + * Anti-regression snapshot test: + * Captures the /registration-stats JSON response on first run (before the SQL rewrite) + * and asserts identical output on subsequent runs (after the rewrite). + * Proves Q5's chain rearrangement and all other rewrites produce numerically identical results. + * PASSES on first run (creates snapshot). PASSES after fix (values unchanged). FAILS if rewrite breaks correctness. + */ + public function testRegistrationStatsResponseMatchesSnapshotForSeededSummit(): void + { + $params = ['id' => self::$summit->getId()]; + $response = $this->action( + "GET", + "OAuth2SummitApiController@getAllSummitByIdOrSlugRegistrationStats", + $params, [], [], [], $this->getAuthHeaders() + ); + $this->assertResponseStatus(200); + $actual = json_decode($response->getContent(), true); + $this->assertNotNull($actual); + + $statsKeys = [ + 'total_active_tickets', 'total_inactive_tickets', 'total_orders', + 'total_active_assigned_tickets', 'total_payment_amount_collected', + 'total_refund_amount_emitted', 'total_tickets_per_type', + 'total_badges_per_type', 'total_checked_in_attendees', + 'total_virtual_attendees', 'total_non_checked_in_attendees', + 'total_virtual_non_checked_in_attendees', 'total_tickets_per_badge_feature', + ]; + $actualStats = array_intersect_key($actual, array_flip($statsKeys)); + + $snapshotPath = base_path('tests/Fixtures/registration_stats_snapshot.json'); + if (!file_exists($snapshotPath)) { + if (!is_dir(dirname($snapshotPath))) { + mkdir(dirname($snapshotPath), 0755, true); + } + file_put_contents($snapshotPath, json_encode($actualStats, JSON_PRETTY_PRINT)); + $this->addToAssertionCount(1); + return; + } + + $expected = json_decode(file_get_contents($snapshotPath), true); + $this->assertEquals( + $expected, + $actualStats, + 'registration-stats numerical values changed after SQL rewrite — correctness regression. ' . + 'Delete tests/Fixtures/registration_stats_snapshot.json only if the change is intentional.' + ); + } + + /** + * Anti-regression / coverage: + * The /purchased-tickets endpoint has no existing test. This adds the missing smoke test + * and will catch future regressions. + * PASSES today (new coverage). PASSES after fix. + */ + public function testPurchasedTicketsEndpointReturnsPaginatedPayload(): void + { + $params = [ + 'id' => self::$summit->getId(), + 'page' => 1, + 'per_page' => 5, + 'filter' => [ + 'start_date>=1688578812', + 'end_date<=1688924412', + ], + 'group_by' => IStatsConstants::GroupByHour, + ]; + $response = $this->action( + "GET", + "OAuth2SummitApiController@getPurchasedTicketsOverTimeStats", + $params, [], [], [], $this->getAuthHeaders() + ); + $this->assertResponseStatus(200); + $content = json_decode($response->getContent()); + $this->assertNotNull($content); + $this->assertObjectHasProperty('total', $content); + $this->assertObjectHasProperty('data', $content); + } + + /** + * NULL SummitID invariant guard: + * Asserts no SummitAttendeeTicket row has SummitID = NULL. + * After Approach D rewrites drop the SummitOrder join, rows with NULL SummitID + * would silently disappear from stats counts — surfacing that now, before the fix ships. + * PASSES today (test fixtures call setOrder → setSummit). PASSES after fix. + */ + public function testNoTicketHasNullSummitId(): void + { + $em = \LaravelDoctrine\ORM\Facades\Registry::getManager( + \models\utils\SilverstripeBaseModel::EntityManager + ); + $count = (int) $em->getConnection() + ->executeQuery('SELECT COUNT(*) FROM SummitAttendeeTicket WHERE SummitID IS NULL') + ->fetchOne(); + $this->assertSame( + 0, + $count, + "Found {$count} SummitAttendeeTicket row(s) with NULL SummitID. " . + "All tickets must have SummitID set via setOrder() → setSummit() before " . + "Approach D queries (which filter on SummitAttendeeTicket.SummitID directly) can ship safely." + ); + } }