From 0a52edbacda94da59aa73c0f337638041e1034a1 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 8 Sep 2026 18:44:29 +0200 Subject: [PATCH 1/5] fix(speakers/submitters): scope the activities count by the presentation-level filters The activities count endpoints run in two phases: phase 1 resolves which speakers/submitters match the filter, phase 2 counts their presentations. Phase 2 had E.SummitID as its only predicate, so it counted every presentation of a matched person whether or not the presentation itself satisfied the filter. A speaker with three presentations returned 3 for presentations_track_id, presentations_type_id, has_published_presentations and has_media_upload_with_type alike. Phase 2 now derives its WHERE from the same Filter object through Filter::toRawSQL, which already walks the parsed AND/OR structure, dispatches per field mapping and binds the values. Two mappings were missing for raw SQL and are added next to SQLInFilterMapping: SQLRawFilterMapping renders a condition carrying :operator and :value (the counterpart of DoctrineFilterMapping) and SQLSwitchFilterMapping picks a condition per value (the counterpart of DoctrineSwitchFilterMapping). ActivitiesCountFilterMappingsTrait declares the fourteen presentation-level conditions, expressed against the physical presentation row instead of against the person, with the semantics copied from the phase-1 DQL, and exposes buildActivitiesCountFilter to turn a Filter into the WHERE fragment and its bindings. Both repositories call that one method - the speaker repo for both INSERT statements (speaker and moderator roles, kept separate because of MySQL error 1137), the member repo for the created_by statement. Person-level filters have no phase-2 mapping, so toRawSQL skips them and the count stays unrestricted for them. Inside an OR group that skip drops a branch rather than widening it; the trait documents that limitation and a test in each repository suite pins the resulting count. Repository and endpoint tests assert exact counts for the scenario in the ticket, per filter and per combination, for both roles. A unit test compares the phase-1 mapping keys of both repositories against the phase-2 ones in both directions, so the two phases cannot drift. The activities figure shown in production for the selection-status filters goes down, because it was over-counted. The speaker count does not change. Co-Authored-By: Claude Opus 5 (1M context) --- .../Utils/Filters/SQL/SQLRawFilterMapping.php | 90 ++++ .../Filters/SQL/SQLSwitchFilterMapping.php | 72 +++ .../Summit/DoctrineMemberRepository.php | 12 +- .../Summit/DoctrineSpeakerRepository.php | 17 +- .../ActivitiesCountFilterMappingsTrait.php | 219 +++++++++ tests/ActivitiesCountFilterMappingsTest.php | 435 ++++++++++++++++++ tests/SpeakerRepositoryTest.php | 269 +++++++++++ tests/SubmitterRepositoryTest.php | 261 +++++++++++ tests/oauth2/OAuth2SummitSpeakersApiTest.php | 130 ++++++ .../oauth2/OAuth2SummitSubmittersApiTest.php | 126 +++++ 10 files changed, 1625 insertions(+), 6 deletions(-) create mode 100644 app/Http/Utils/Filters/SQL/SQLRawFilterMapping.php create mode 100644 app/Http/Utils/Filters/SQL/SQLSwitchFilterMapping.php create mode 100644 app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php create mode 100644 tests/ActivitiesCountFilterMappingsTest.php diff --git a/app/Http/Utils/Filters/SQL/SQLRawFilterMapping.php b/app/Http/Utils/Filters/SQL/SQLRawFilterMapping.php new file mode 100644 index 000000000..efeb8c7c2 --- /dev/null +++ b/app/Http/Utils/Filters/SQL/SQLRawFilterMapping.php @@ -0,0 +1,90 @@ +bindings = []; + $param_idx = count($bindings) + 1; + + $value = $filter->getValue(); + $operator = $filter->getOperator(); + + if (!is_array($value)) { + return $this->renderCondition( + $value, + is_array($operator) ? $operator[0] : $operator, + $param_idx + ); + } + + $conditions = []; + foreach ($value as $idx => $v) { + $conditions[] = $this->renderCondition( + $v, + is_array($operator) ? $operator[$idx] : $operator, + $param_idx++ + ); + } + + $same_field_op = $filter->getSameFieldOp() ?? Filter::MainOperatorOr; + + return '( ' . implode(sprintf(' %s ', $same_field_op), $conditions) . ' )'; + } + + /** + * @param mixed $value + * @param string $operator + * @param int $param_idx + * @return string + */ + private function renderCondition($value, string $operator, int $param_idx): string + { + $param = sprintf(Filter::ParamPrefix, $param_idx); + $this->bindings[$param] = $value; + + return str_replace( + [Filter::OperatorPlaceholder, Filter::ValuePlaceholder], + [$operator, ':' . $param], + $this->where + ); + } +} diff --git a/app/Http/Utils/Filters/SQL/SQLSwitchFilterMapping.php b/app/Http/Utils/Filters/SQL/SQLSwitchFilterMapping.php new file mode 100644 index 000000000..46c906fb1 --- /dev/null +++ b/app/Http/Utils/Filters/SQL/SQLSwitchFilterMapping.php @@ -0,0 +1,72 @@ + SQL condition + */ + private $case_statements; + + /** + * @param array $case_statements + */ + public function __construct(array $case_statements = []) + { + parent::__construct('', ''); + $this->case_statements = $case_statements; + } + + /** + * @param FilterElement $filter + * @param array $bindings + * @return string + */ + public function toRawSQL(FilterElement $filter, array $bindings = []): string + { + $this->bindings = []; + + $value = $filter->getValue(); + if (!is_array($value)) $value = [$value]; + + $conditions = []; + foreach ($value as $v) { + if (!isset($this->case_statements[$v])) continue; + $conditions[] = '( ' . $this->case_statements[$v] . ' )'; + } + + if (empty($conditions)) return ''; + + return implode(' OR ', $conditions); + } +} diff --git a/app/Repositories/Summit/DoctrineMemberRepository.php b/app/Repositories/Summit/DoctrineMemberRepository.php index b5846ec8c..988899922 100644 --- a/app/Repositories/Summit/DoctrineMemberRepository.php +++ b/app/Repositories/Summit/DoctrineMemberRepository.php @@ -14,6 +14,7 @@ use App\Http\Utils\Filters\DoctrineInFilterMapping; use App\Http\Utils\Filters\DoctrineNotInFilterMapping; +use App\Repositories\Summit\Traits\ActivitiesCountFilterMappingsTrait; use App\Http\Utils\Filters\SQL\SQLInFilterMapping; use App\Http\Utils\Filters\SQL\SQLNotInFilterMapping; use App\libs\Utils\PunnyCodeHelper; @@ -44,6 +45,8 @@ final class DoctrineMemberRepository extends SilverStripeDoctrineRepository implements IMemberRepository { + use ActivitiesCountFilterMappingsTrait; + /** * @return string */ @@ -831,15 +834,20 @@ public function getUniqueActivitiesCountBySummit(Summit $summit, Filter $filter } while (count($chunk) === $chunkSize); // Phase 2: count distinct presentations whose creator is in the matched set. + // The presentation-level filters of the request scope phase 2 too: we count + // only the presentations that both belong to a matched submitter and satisfy + // the filter, so that "N Submitters | M Activities" describes one same set. + [$extra_filters, $bindings] = $this->buildActivitiesCountFilter($filter, $summit->getId()); + $sql = <<fetchOne($sql, [$summit->getId()]); + return (int) $conn->fetchOne($sql, $bindings); } finally { $conn->executeStatement('DROP TEMPORARY TABLE IF EXISTS `__tmp_mbr_ids`'); } diff --git a/app/Repositories/Summit/DoctrineSpeakerRepository.php b/app/Repositories/Summit/DoctrineSpeakerRepository.php index a533cf229..4f6352873 100644 --- a/app/Repositories/Summit/DoctrineSpeakerRepository.php +++ b/app/Repositories/Summit/DoctrineSpeakerRepository.php @@ -14,6 +14,7 @@ use App\Http\Utils\Filters\DoctrineInFilterMapping; use App\Http\Utils\Filters\DoctrineNotInFilterMapping; +use App\Repositories\Summit\Traits\ActivitiesCountFilterMappingsTrait; use App\libs\Utils\PunnyCodeHelper; use App\Repositories\SilverStripeDoctrineRepository; use Doctrine\ORM\Query\ResultSetMappingBuilder; @@ -42,6 +43,8 @@ final class DoctrineSpeakerRepository extends SilverStripeDoctrineRepository implements ISpeakerRepository { + use ActivitiesCountFilterMappingsTrait; + /** * @return array */ @@ -968,14 +971,20 @@ public function getUniqueActivitiesCountBySummit(Summit $summit, Filter $filter ); $conn->executeStatement('TRUNCATE TABLE `__tmp_pres_ids`'); + // The presentation-level filters of the request scope phase 2 too: we count + // only the presentations that both belong to a matched speaker and satisfy + // the filter, so that "N Speakers | M Activities" describes one same set. + [$extra_filters, $bindings] = $this->buildActivitiesCountFilter($filter, $summit->getId()); + $conn->executeStatement( 'INSERT IGNORE INTO `__tmp_pres_ids` (id) SELECT DISTINCT E.ID FROM SummitEvent E + INNER JOIN Presentation P ON P.ID = E.ID INNER JOIN Presentation_Speakers PS ON PS.PresentationID = E.ID INNER JOIN `__tmp_spk_ids` T ON T.id = PS.PresentationSpeakerID - WHERE E.SummitID = ?', - [$summit->getId()] + WHERE E.SummitID = :summit_id' . $extra_filters, + $bindings ); $conn->executeStatement( @@ -984,8 +993,8 @@ public function getUniqueActivitiesCountBySummit(Summit $summit, Filter $filter FROM SummitEvent E INNER JOIN Presentation P ON P.ID = E.ID INNER JOIN `__tmp_spk_ids` T ON T.id = P.ModeratorID - WHERE E.SummitID = ?', - [$summit->getId()] + WHERE E.SummitID = :summit_id' . $extra_filters, + $bindings ); return (int) $conn->fetchOne('SELECT COUNT(*) FROM `__tmp_pres_ids`'); diff --git a/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php b/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php new file mode 100644 index 000000000..9902cae0a --- /dev/null +++ b/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php @@ -0,0 +1,219 @@ + $summit_id]; + + if (!is_null($filter)) { + $where = $filter->toRawSQL($this->getActivitiesCountFilterMappings()); + if (!empty($where)) { + $extra_filters = ' AND (' . $where . ')'; + $bindings = array_merge($bindings, $filter->getSQLBindings()); + } + } + + return [$extra_filters, $bindings]; + } + + /** + * Aliases the conditions correlate to: SummitEvent E, Presentation P. + * + * The selection-status semantics are copied from the phase-1 DQL mappings, not + * re-derived. PresentationMediaUpload is a JOINED subclass of PresentationMaterial, + * so its PresentationID column lives on the parent table and the type on the child. + * + * @return array + */ + private function getActivitiesCountFilterMappings(): array + { + return [ + 'presentations_track_id' => new SQLRawFilterMapping( + 'E.CategoryID :operator :value' + ), + 'presentations_track_group_id' => new SQLRawFilterMapping( + 'EXISTS ( + SELECT 1 + FROM PresentationCategoryGroup_Categories __cg + WHERE __cg.PresentationCategoryID = E.CategoryID + AND __cg.PresentationCategoryGroupID :operator :value + )' + ), + 'presentations_selection_plan_id' => new SQLRawFilterMapping( + 'P.SelectionPlanID :operator :value' + ), + 'presentations_type_id' => new SQLRawFilterMapping( + 'E.TypeID :operator :value' + ), + 'presentations_title' => new SQLRawFilterMapping( + 'LOWER(E.Title) :operator LOWER(:value)' + ), + 'presentations_abstract' => new SQLRawFilterMapping( + 'LOWER(E.Abstract) :operator LOWER(:value)' + ), + 'presentations_submitter_full_name' => new SQLRawFilterMapping( + "EXISTS ( + SELECT 1 + FROM `Member` __sub + WHERE __sub.ID = E.CreatedByID + AND CONCAT(LOWER(__sub.FirstName), ' ', LOWER(__sub.Surname)) :operator LOWER(:value) + )" + ), + 'presentations_submitter_email' => new SQLRawFilterMapping( + 'EXISTS ( + SELECT 1 + FROM `Member` __sub + WHERE __sub.ID = E.CreatedByID + AND LOWER(__sub.Email) :operator LOWER(:value) + )' + ), + 'has_media_upload_with_type' => new SQLRawFilterMapping( + 'EXISTS ( + SELECT 1 + FROM PresentationMediaUpload __mu + INNER JOIN PresentationMaterial __mat ON __mat.ID = __mu.ID + WHERE __mat.PresentationID = E.ID + AND __mu.SummitMediaUploadTypeID :operator :value + )' + ), + 'has_not_media_upload_with_type' => new SQLRawFilterMapping( + 'NOT EXISTS ( + SELECT 1 + FROM PresentationMediaUpload __mu + INNER JOIN PresentationMaterial __mat ON __mat.ID = __mu.ID + WHERE __mat.PresentationID = E.ID + AND __mu.SummitMediaUploadTypeID :operator :value + )' + ), + // The == false side of every status filter must not restrict the count: a + // person matched by it has no presentation with that status among the ones + // that pass the remaining presentation-level filters, so all of them qualify. + 'has_published_presentations' => new SQLSwitchFilterMapping([ + 'true' => 'E.Published = 1', + 'false' => SQLSwitchFilterMapping::NoRestriction, + ]), + 'has_accepted_presentations' => new SQLSwitchFilterMapping([ + // accepted = selected within the track session count, or published + 'true' => sprintf( + 'EXISTS ( + SELECT 1 + FROM SummitSelectedPresentation __sp + INNER JOIN SummitSelectedPresentationList __spl ON __spl.ID = __sp.SummitSelectedPresentationListID + INNER JOIN PresentationCategory __cat ON __cat.ID = E.CategoryID + WHERE __sp.PresentationID = E.ID + AND __sp.Collection = \'%1$s\' + AND __spl.ListType = \'%2$s\' + AND __spl.ListClass = \'%3$s\' + AND __sp.`Order` IS NOT NULL + AND __sp.`Order` <= __cat.SessionCount + ) OR E.Published = 1', + SummitSelectedPresentation::CollectionSelected, + SummitSelectedPresentationList::Group, + SummitSelectedPresentationList::Session + ), + 'false' => SQLSwitchFilterMapping::NoRestriction, + ]), + 'has_alternate_presentations' => new SQLSwitchFilterMapping([ + // alternate = selected beyond the track session count + 'true' => sprintf( + 'EXISTS ( + SELECT 1 + FROM SummitSelectedPresentation __sp + INNER JOIN SummitSelectedPresentationList __spl ON __spl.ID = __sp.SummitSelectedPresentationListID + INNER JOIN PresentationCategory __cat ON __cat.ID = E.CategoryID + WHERE __sp.PresentationID = E.ID + AND __sp.Collection = \'%1$s\' + AND __spl.ListType = \'%2$s\' + AND __spl.ListClass = \'%3$s\' + AND __sp.`Order` IS NOT NULL + AND __sp.`Order` > __cat.SessionCount + )', + SummitSelectedPresentation::CollectionSelected, + SummitSelectedPresentationList::Group, + SummitSelectedPresentationList::Session + ), + 'false' => SQLSwitchFilterMapping::NoRestriction, + ]), + 'has_rejected_presentations' => new SQLSwitchFilterMapping([ + // rejected = not published and absent from every Group/Session list, + // the order playing no part, as in the phase-1 mapping + 'true' => sprintf( + 'E.Published = 0 AND NOT EXISTS ( + SELECT 1 + FROM SummitSelectedPresentation __sp + INNER JOIN SummitSelectedPresentationList __spl ON __spl.ID = __sp.SummitSelectedPresentationListID + WHERE __sp.PresentationID = E.ID + AND __sp.Collection = \'%1$s\' + AND __spl.ListType = \'%2$s\' + AND __spl.ListClass = \'%3$s\' + )', + SummitSelectedPresentation::CollectionSelected, + SummitSelectedPresentationList::Group, + SummitSelectedPresentationList::Session + ), + 'false' => SQLSwitchFilterMapping::NoRestriction, + ]), + ]; + } +} diff --git a/tests/ActivitiesCountFilterMappingsTest.php b/tests/ActivitiesCountFilterMappingsTest.php new file mode 100644 index 000000000..35e83decd --- /dev/null +++ b/tests/ActivitiesCountFilterMappingsTest.php @@ -0,0 +1,435 @@ +toRawSQL(FilterElement::makeEqual('presentations_track_id', '5')); + + $this->assertEquals('E.CategoryID = :param_1', $sql); + $this->assertEquals(['param_1' => '5'], $mapping->getBindings()); + // the value must never be interpolated into the statement + $this->assertStringNotContainsString('= 5', $sql); + } + + public function testRawMappingContinuesTheCallersParameterNumbering(): void + { + $mapping = new SQLRawFilterMapping('E.TypeID :operator :value'); + + $sql = $mapping->toRawSQL( + FilterElement::makeEqual('presentations_type_id', '7'), + ['param_1' => 'already taken', 'param_2' => 'also taken'] + ); + + $this->assertEquals('E.TypeID = :param_3', $sql); + $this->assertEquals(['param_3' => '7'], $mapping->getBindings()); + } + + public function testRawMappingRendersEachValueOfAMultiValueElement(): void + { + $mapping = new SQLRawFilterMapping('E.CategoryID :operator :value'); + + $sql = $mapping->toRawSQL( + FilterElement::makeEqual('presentations_track_id', ['5', '6'], 'OR') + ); + + $this->assertEquals('( E.CategoryID = :param_1 OR E.CategoryID = :param_2 )', $sql); + $this->assertEquals(['param_1' => '5', 'param_2' => '6'], $mapping->getBindings()); + } + + public function testRawMappingHonoursTheAndSameFieldOperator(): void + { + $mapping = new SQLRawFilterMapping('X :operator :value'); + + $sql = $mapping->toRawSQL( + FilterElement::makeEqual('has_media_upload_with_type', ['1', '2'], 'AND') + ); + + $this->assertStringContainsString(' AND ', $sql); + $this->assertStringNotContainsString(' OR ', $sql); + } + + public function testRawMappingResetsItsBindingsBetweenUses(): void + { + $mapping = new SQLRawFilterMapping('E.CategoryID :operator :value'); + + $mapping->toRawSQL(FilterElement::makeEqual('presentations_track_id', '5')); + $mapping->toRawSQL(FilterElement::makeEqual('presentations_track_id', '9'), ['param_1' => '5']); + + $this->assertEquals(['param_2' => '9'], $mapping->getBindings()); + } + + public function testRawMappingLowersBothSidesOfATextCondition(): void + { + $mapping = new SQLRawFilterMapping('LOWER(E.Title) :operator LOWER(:value)'); + + $sql = $mapping->toRawSQL(FilterElement::makeLike('presentations_title', 'keynote')); + + $this->assertEquals('LOWER(E.Title) like LOWER(:param_1)', $sql); + // makeLike wraps the value in wildcards + $this->assertEquals(['param_1' => '%keynote%'], $mapping->getBindings()); + } + + // ----------------------------------------------------------------- + // SQLSwitchFilterMapping + // ----------------------------------------------------------------- + + public function testSwitchMappingPicksTheConditionOfTheValue(): void + { + $mapping = new SQLSwitchFilterMapping([ + 'true' => 'E.Published = 1', + 'false' => SQLSwitchFilterMapping::NoRestriction, + ]); + + $this->assertEquals( + '( E.Published = 1 )', + $mapping->toRawSQL(FilterElement::makeEqual('has_published_presentations', 'true')) + ); + $this->assertEquals( + '( 1 = 1 )', + $mapping->toRawSQL(FilterElement::makeEqual('has_published_presentations', 'false')) + ); + $this->assertEmpty($mapping->getBindings()); + } + + public function testSwitchMappingOrsEveryValueOfAMultiValueElement(): void + { + $mapping = new SQLSwitchFilterMapping([ + 'true' => 'E.Published = 1', + 'false' => SQLSwitchFilterMapping::NoRestriction, + ]); + + // true OR no-restriction evaluates to no restriction, as in the Doctrine mapping + $sql = $mapping->toRawSQL( + FilterElement::makeEqual('has_published_presentations', ['true', 'false'], 'OR') + ); + + $this->assertEquals('( E.Published = 1 ) OR ( 1 = 1 )', $sql); + } + + public function testSwitchMappingYieldsNothingForAnUnknownValue(): void + { + $mapping = new SQLSwitchFilterMapping(['true' => 'E.Published = 1']); + + $this->assertEquals( + '', + $mapping->toRawSQL(FilterElement::makeEqual('has_published_presentations', 'maybe')) + ); + } + + // ----------------------------------------------------------------- + // Filter::toRawSQL over the activities count mappings + // ----------------------------------------------------------------- + + private function activitiesCountSQL(Filter $filter, string $repository_class = DoctrineSpeakerRepository::class): array + { + $sql = $filter->toRawSQL($this->getMappings($repository_class, 'getActivitiesCountFilterMappings')); + + return [$sql, $filter->getSQLBindings()]; + } + + private function filterOf(...$conditions): Filter + { + $filter = new Filter(); + foreach ($conditions as $condition) { + $filter->addFilterCondition($condition); + } + return $filter; + } + + public function testAPersonLevelFilterYieldsNoPresentationCondition(): void + { + [$sql, $bindings] = $this->activitiesCountSQL( + $this->filterOf(FilterElement::makeEqual('id', '123')) + ); + + $this->assertEmpty($sql, 'id has no phase-2 mapping, so it must not restrict the count'); + $this->assertEmpty($bindings); + } + + public function testCombinedFiltersAreAnded(): void + { + [$sql, $bindings] = $this->activitiesCountSQL( + $this->filterOf( + FilterElement::makeEqual('has_published_presentations', 'true'), + FilterElement::makeEqual('presentations_track_id', '5') + ) + ); + + $this->assertStringContainsString('E.Published = 1', $sql); + $this->assertStringContainsString('E.CategoryID = :param_1', $sql); + $this->assertStringContainsString(') AND (', $sql); + $this->assertEquals(['param_1' => '5'], $bindings); + } + + public function testAPersonLevelFilterDoesNotStopTheOthersFromScoping(): void + { + [$sql] = $this->activitiesCountSQL( + $this->filterOf( + FilterElement::makeEqual('id', '123'), + FilterElement::makeEqual('presentations_track_id', '5') + ) + ); + + $this->assertStringContainsString('E.CategoryID = :param_1', $sql); + } + + public function testEveryReturnedBindingHasItsPlaceholderInTheStatement(): void + { + [$sql, $bindings] = $this->activitiesCountSQL( + $this->filterOf( + FilterElement::makeEqual( + 'presentations_track_id', + ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11'], + 'OR' + ), + FilterElement::makeEqual('presentations_type_id', '7') + ) + ); + + $this->assertCount(12, $bindings); + foreach (array_keys($bindings) as $name) { + $this->assertMatchesRegularExpression( + '/:' . preg_quote($name, '/') . '\b/', + $sql, + sprintf('binding "%s" has no placeholder in the statement', $name) + ); + } + } + + public function testSelectionStatusConditionsUseTheSelectedListSemantics(): void + { + [$accepted] = $this->activitiesCountSQL( + $this->filterOf(FilterElement::makeEqual('has_accepted_presentations', 'true')) + ); + $this->assertStringContainsString('__sp.`Order` <= __cat.SessionCount', $accepted); + $this->assertStringContainsString("__sp.Collection = 'selected'", $accepted); + $this->assertStringContainsString("__spl.ListType = 'Group'", $accepted); + $this->assertStringContainsString("__spl.ListClass = 'Session'", $accepted); + $this->assertStringContainsString('OR E.Published = 1', $accepted); + + [$alternate] = $this->activitiesCountSQL( + $this->filterOf(FilterElement::makeEqual('has_alternate_presentations', 'true')) + ); + $this->assertStringContainsString('__sp.`Order` > __cat.SessionCount', $alternate); + $this->assertStringNotContainsString('Published', $alternate); + + [$rejected] = $this->activitiesCountSQL( + $this->filterOf(FilterElement::makeEqual('has_rejected_presentations', 'true')) + ); + $this->assertStringContainsString('E.Published = 0', $rejected); + $this->assertStringContainsString('AND NOT EXISTS', $rejected); + // the rejected mapping ignores the order, it only asks for absence from the list + $this->assertStringNotContainsString('SessionCount', $rejected); + } + + public function testMediaUploadConditionJoinsTheParentMaterialTable(): void + { + [$sql] = $this->activitiesCountSQL( + $this->filterOf(FilterElement::makeEqual('has_media_upload_with_type', '11')) + ); + + // PresentationMediaUpload is a JOINED subclass: PresentationID lives on the parent + $this->assertStringContainsString('INNER JOIN PresentationMaterial __mat ON __mat.ID = __mu.ID', $sql); + $this->assertStringContainsString('__mat.PresentationID = E.ID', $sql); + $this->assertStringContainsString('__mu.SummitMediaUploadTypeID = :param_1', $sql); + } + + // ----------------------------------------------------------------- + // buildActivitiesCountFilter - the entry point both repositories share + // ----------------------------------------------------------------- + + private function buildFor(?Filter $filter, string $repository_class = DoctrineSpeakerRepository::class): array + { + return $this->invokeOnRepository($repository_class, 'buildActivitiesCountFilter', $filter, 73); + } + + #[DataProvider('repositoryProvider')] + public function testBuildWithoutAFilterOnlyBindsTheSummit(string $repository_class): void + { + [$extra_filters, $bindings] = $this->buildFor(null, $repository_class); + + $this->assertEmpty($extra_filters); + $this->assertEquals(['summit_id' => 73], $bindings); + } + + #[DataProvider('repositoryProvider')] + public function testBuildWithOnlyPersonLevelFiltersAddsNothing(string $repository_class): void + { + [$extra_filters, $bindings] = $this->buildFor( + $this->filterOf( + FilterElement::makeEqual('id', '123'), + FilterElement::makeEqual('first_name', 'Sebastian') + ), + $repository_class + ); + + $this->assertEmpty($extra_filters); + $this->assertEquals(['summit_id' => 73], $bindings); + } + + #[DataProvider('repositoryProvider')] + public function testBuildAppendsTheFragmentAndKeepsTheSummitBinding(string $repository_class): void + { + [$extra_filters, $bindings] = $this->buildFor( + $this->filterOf( + FilterElement::makeEqual('presentations_track_id', '5'), + FilterElement::makeEqual('has_published_presentations', 'true') + ), + $repository_class + ); + + $this->assertStringStartsWith(' AND (', $extra_filters); + $this->assertStringContainsString('E.CategoryID = :param_1', $extra_filters); + $this->assertStringContainsString('E.Published = 1', $extra_filters); + $this->assertEquals(['summit_id' => 73, 'param_1' => '5'], $bindings); + } + + #[DataProvider('repositoryProvider')] + public function testBuildProducesTheSameFragmentForBothRoles(string $repository_class): void + { + // the conditions correlate to the presentation, so both repositories must agree + [$speakers] = $this->buildFor( + $this->filterOf(FilterElement::makeEqual('presentations_type_id', '7')), + DoctrineSpeakerRepository::class + ); + [$submitters] = $this->buildFor( + $this->filterOf(FilterElement::makeEqual('presentations_type_id', '7')), + DoctrineMemberRepository::class + ); + + $this->assertEquals($speakers, $submitters); + } + + // ----------------------------------------------------------------- + // anti drift guard: phase 1 and phase 2 must know the same filters + // ----------------------------------------------------------------- + + public static function repositoryProvider(): array + { + return [ + 'speakers' => [DoctrineSpeakerRepository::class], + 'submitters' => [DoctrineMemberRepository::class], + ]; + } + + #[DataProvider('repositoryProvider')] + public function testPhaseTwoKnowsEveryPresentationLevelFilterOfPhaseOne(string $repository_class): void + { + $phase_one = array_keys($this->getMappings($repository_class, 'getFilterMappings')); + $phase_two = array_keys($this->getMappings($repository_class, 'getActivitiesCountFilterMappings')); + + $this->assertNotEmpty($phase_one); + + foreach ($phase_one as $key) { + if (!$this->looksPresentationLevel($key)) continue; + + $this->assertContains( + $key, + $phase_two, + sprintf( + '%s exposes the presentation-level filter "%s" in phase 1 but the ' . + 'activities count does not scope phase 2 with it, so the count would over-count.', + $repository_class, + $key + ) + ); + } + } + + #[DataProvider('repositoryProvider')] + public function testEveryPhaseTwoFilterExistsInPhaseOne(string $repository_class): void + { + $phase_one = array_keys($this->getMappings($repository_class, 'getFilterMappings')); + $phase_two = array_keys($this->getMappings($repository_class, 'getActivitiesCountFilterMappings')); + + foreach ($phase_two as $key) { + $this->assertContains( + $key, + $phase_one, + sprintf('%s has no phase-1 mapping for "%s".', $repository_class, $key) + ); + } + } + + #[DataProvider('repositoryProvider')] + public function testPhaseTwoCarriesNoPersonLevelFilter(string $repository_class): void + { + $phase_two = array_keys($this->getMappings($repository_class, 'getActivitiesCountFilterMappings')); + + foreach ($phase_two as $key) { + $this->assertTrue( + $this->looksPresentationLevel($key), + sprintf( + '"%s" is not a presentation-level filter: scoping phase 2 with it would ' . + 'count fewer presentations than the people phase 1 matched.', + $key + ) + ); + } + } + + /** + * A filter name that phase 1 resolves through a presentation, and that therefore has + * to scope phase 2 as well. + */ + private function looksPresentationLevel(string $key): bool + { + if (str_starts_with($key, 'presentations_')) return true; + if (str_starts_with($key, 'has_') && str_ends_with($key, '_presentations')) return true; + if (str_contains($key, 'media_upload')) return true; + return false; + } + + /** + * The trait's methods touch no $this beyond each other, so they can be called on an + * instance built without its constructor -- no entity manager needed. + */ + private function invokeOnRepository(string $repository_class, string $method, ...$args): array + { + $reflection = new \ReflectionClass($repository_class); + $instance = $reflection->newInstanceWithoutConstructor(); + $m = $reflection->getMethod($method); + $m->setAccessible(true); + + return $m->invoke($instance, ...$args); + } + + private function getMappings(string $repository_class, string $method): array + { + return $this->invokeOnRepository($repository_class, $method, null); + } +} diff --git a/tests/SpeakerRepositoryTest.php b/tests/SpeakerRepositoryTest.php index 1f9cd3133..0c4df87fa 100644 --- a/tests/SpeakerRepositoryTest.php +++ b/tests/SpeakerRepositoryTest.php @@ -2,6 +2,7 @@ use LaravelDoctrine\ORM\Facades\EntityManager; use models\summit\Presentation; +use models\summit\PresentationMediaUpload; use models\summit\PresentationSpeaker; use utils\FilterParser; use utils\Order; @@ -689,4 +690,272 @@ public function testGetAllByPagePaginatesCorrectlyAcrossPages(): void $this->assertEquals($total, $page1->getTotal(), 'Total must be consistent across pages'); $this->assertEquals($total, $page2->getTotal()); } + // ----------------------------------------------------------------- + // getUniqueActivitiesCountBySummit - the count is scoped by the + // presentation-level filters, not only by who matches them. + // + // Phase 1 resolves WHICH speakers match; phase 2 used to count every + // presentation of those speakers, so any presentation-level filter + // over-counted (a speaker with 3 presentations returned 3 for all of them). + // ----------------------------------------------------------------- + + /** + * Seeds the acceptance scenario: one speaker owning three presentations that + * differ in track, type, published state and media upload. + * + * P1 - defaultTrack, defaultPresentationType, published, media upload of type M + * P2 - secondaryTrack, defaultPresentationType, published, no media upload + * P3 - defaultTrack, allow2VotePresentationType, unpublished, no media upload + */ + private function seedActivitiesCountScenario(string $first_name): PresentationSpeaker + { + $speaker = new PresentationSpeaker(); + $speaker->setFirstName($first_name); + $speaker->setLastName('ActivitiesScenario'); + self::$em->persist($speaker); + + $p1 = $this->seedPresentation($speaker, self::$defaultTrack, 'P1 Published Default Track', true); + $media_upload = new PresentationMediaUpload(); + $media_upload->setName('P1 Media Upload'); + $media_upload->setDescription('P1 Media Upload Description'); + $media_upload->setFilename('p1.pdf'); + $media_upload->setMediaUploadType(self::$media_uploads_types[0]); + $p1->addMediaUpload($media_upload); + + $this->seedPresentation($speaker, self::$secondaryTrack, 'P2 Published Secondary Track', true); + + // allow2VotePresentationType does not allow a publishing period, so this one + // must not get start/end dates or SummitEvent rejects it. + $p3 = new Presentation(); + self::$summit->addEvent($p3); + $p3->setTitle('P3 Unpublished Default Track Other Type'); + $p3->setAbstract('Abstract'); + $p3->setCategory(self::$defaultTrack); + $p3->setType(self::$allow2VotePresentationType); + $p3->setProgress(Presentation::PHASE_COMPLETE); + $p3->setStatus(Presentation::STATUS_RECEIVED); + $p3->addSpeaker($speaker); + + self::$em->flush(); + + return $speaker; + } + + /** + * Counts the activities of one speaker under the given presentation-level conditions. + */ + private function countActivitiesOf(PresentationSpeaker $speaker, array $conditions = []): int + { + $expressions = ['id==' . $speaker->getId()]; + $rules = ['id' => ['==']]; + + foreach ($conditions as $field => $value) { + $expressions[] = $field . '==' . $value; + $rules[$field] = ['==']; + } + + return $this->repo()->getUniqueActivitiesCountBySummit( + self::$summit, + FilterParser::parse($expressions, $rules) + ); + } + + public function testActivitiesCountWithoutPresentationFilterCountsEveryPresentationOfTheSpeaker(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioAll'); + + $this->assertEquals(3, $this->countActivitiesOf($speaker)); + } + + public function testActivitiesCountIsScopedByTrackFilter(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioTrack'); + + // P1 and P3 are in defaultTrack, P2 is in secondaryTrack. + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'presentations_track_id' => self::$defaultTrack->getId(), + ])); + $this->assertEquals(1, $this->countActivitiesOf($speaker, [ + 'presentations_track_id' => self::$secondaryTrack->getId(), + ])); + } + + public function testActivitiesCountIsScopedByTrackGroupFilter(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioTrackGroup'); + + // defaultTrackGroup contains defaultTrack only: P1 and P3. + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'presentations_track_group_id' => self::$defaultTrackGroup->getId(), + ])); + } + + public function testActivitiesCountIsScopedByTypeFilter(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioType'); + + // only P3 uses allow2VotePresentationType + $this->assertEquals(1, $this->countActivitiesOf($speaker, [ + 'presentations_type_id' => self::$allow2VotePresentationType->getId(), + ])); + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'presentations_type_id' => self::$defaultPresentationType->getId(), + ])); + } + + public function testActivitiesCountIsScopedByPublishedFilter(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioPublished'); + + // P1 and P2 are published, P3 is not. + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'has_published_presentations' => 'true', + ])); + } + + public function testActivitiesCountIsScopedByMediaUploadFilter(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioMediaUpload'); + + // only P1 carries a media upload of that type + $this->assertEquals(1, $this->countActivitiesOf($speaker, [ + 'has_media_upload_with_type' => self::$media_uploads_types[0]->getId(), + ])); + } + + public function testActivitiesCountForNotMediaUploadFilterCountsEveryPresentationOfTheMatchedSpeaker(): void + { + // has_not_media_upload_with_type matches a speaker only when NONE of their + // presentations carries a media upload of that type, so every presentation + // phase 2 reaches already qualifies. + $speaker = new PresentationSpeaker(); + $speaker->setFirstName('ScenarioNoMediaUpload'); + $speaker->setLastName('ActivitiesScenario'); + self::$em->persist($speaker); + + $this->seedPresentation($speaker, self::$defaultTrack, 'No Media A', true); + $this->seedPresentation($speaker, self::$secondaryTrack, 'No Media B', true); + self::$em->flush(); + + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'has_not_media_upload_with_type' => self::$media_uploads_types[0]->getId(), + ])); + + // the scenario speaker owns P1 with that media upload, so they do not match at all + $with_media = $this->seedActivitiesCountScenario('ScenarioHasMediaUpload'); + $this->assertEquals(0, $this->countActivitiesOf($with_media, [ + 'has_not_media_upload_with_type' => self::$media_uploads_types[0]->getId(), + ])); + } + + public function testActivitiesCountIsScopedByPublishedAndTrackCombined(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioCombined'); + + // only P1 is both published and in defaultTrack + $this->assertEquals(1, $this->countActivitiesOf($speaker, [ + 'has_published_presentations' => 'true', + 'presentations_track_id' => self::$defaultTrack->getId(), + ])); + + // P2 is published but in secondaryTrack + $this->assertEquals(1, $this->countActivitiesOf($speaker, [ + 'has_published_presentations' => 'true', + 'presentations_track_id' => self::$secondaryTrack->getId(), + ])); + + // P3 is in defaultTrack with that type but unpublished + $this->assertEquals(0, $this->countActivitiesOf($speaker, [ + 'has_published_presentations' => 'true', + 'presentations_type_id' => self::$allow2VotePresentationType->getId(), + ])); + } + + public function testActivitiesCountIsScopedByTitleFilter(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioTitle'); + + $expressions = [ + 'id==' . $speaker->getId(), + 'presentations_title=@P1 Published', + ]; + $count = $this->repo()->getUniqueActivitiesCountBySummit( + self::$summit, + FilterParser::parse($expressions, ['id' => ['=='], 'presentations_title' => ['=@']]) + ); + + $this->assertEquals(1, $count); + } + + public function testActivitiesCountForPublishedFalseCountsEveryPresentationOfTheMatchedSpeaker(): void + { + // A speaker with no published and no selected presentation at all: the == false + // side adds no presentation predicate, so every presentation of theirs is counted. + $speaker = new PresentationSpeaker(); + $speaker->setFirstName('ScenarioUnpublishedOnly'); + $speaker->setLastName('ActivitiesScenario'); + self::$em->persist($speaker); + + $this->seedPresentation($speaker, self::$defaultTrack, 'Unpublished A', false); + $this->seedPresentation($speaker, self::$secondaryTrack, 'Unpublished B', false); + self::$em->flush(); + + foreach (['has_published_presentations', 'has_accepted_presentations', 'has_alternate_presentations'] as $field) { + $this->assertEquals( + 2, + $this->countActivitiesOf($speaker, [$field => 'false']), + $field . '==false must not restrict which presentations are counted' + ); + } + } + + public function testActivitiesCountForRejectedFalseCountsEveryPresentationOfTheMatchedSpeaker(): void + { + // rejected == false needs a speaker with no rejected presentation: an unpublished + // presentation outside every selected list is rejected, so this one publishes both. + $speaker = new PresentationSpeaker(); + $speaker->setFirstName('ScenarioNoRejected'); + $speaker->setLastName('ActivitiesScenario'); + self::$em->persist($speaker); + + $this->seedPresentation($speaker, self::$defaultTrack, 'Published A', true); + $this->seedPresentation($speaker, self::$secondaryTrack, 'Published B', true); + self::$em->flush(); + + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'has_rejected_presentations' => 'false', + ])); + } + + public function testActivitiesCountWithNoFilterIsUnaffectedByTheScoping(): void + { + $before = $this->repo()->getUniqueActivitiesCountBySummit(self::$summit); + + $this->seedActivitiesCountScenario('ScenarioUnfiltered'); + + $after = $this->repo()->getUniqueActivitiesCountBySummit(self::$summit); + + $this->assertEquals($before + 3, $after); + } + + public function testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranch(): void + { + $speaker = $this->seedActivitiesCountScenario('ScenarioOrGroup'); + + // "id== OR presentations_track_id==". Phase 1 matches + // the speaker through either branch, but phase 2 sees only the presentation-level + // branch: Filter::toRawSQL skips the fields it has no mapping for, which is the + // same semantics every other toRawSQL caller lives with. The count is therefore + // the secondaryTrack presentations of the matched speakers -- P2 alone -- and not + // all three. Pinned here because it is the one case where phase 2 ends up + // narrower than the set phase 1 matched. + $filter = FilterParser::parse( + ['id==' . $speaker->getId() . ',presentations_track_id==' . self::$secondaryTrack->getId()], + ['id' => ['=='], 'presentations_track_id' => ['==']] + ); + + $count = $this->repo()->getUniqueActivitiesCountBySummit(self::$summit, $filter); + + $this->assertEquals(1, $count); + } } diff --git a/tests/SubmitterRepositoryTest.php b/tests/SubmitterRepositoryTest.php index cc3fbc9b8..dea3ce3e0 100644 --- a/tests/SubmitterRepositoryTest.php +++ b/tests/SubmitterRepositoryTest.php @@ -4,6 +4,7 @@ use models\main\Member; use models\summit\Presentation; use models\summit\PresentationCategory; +use models\summit\PresentationMediaUpload; use models\summit\PresentationSpeaker; use ModelSerializers\SerializerRegistry; use utils\FilterParser; @@ -833,4 +834,264 @@ public function testGetSubmittersBySummitReturnsEmptyPageForNonMatchingFilter(): $this->assertEquals(0, $page->getTotal()); $this->assertEmpty($page->getItems()); } + // ----------------------------------------------------------------- + // getUniqueActivitiesCountBySummit - the count is scoped by the + // presentation-level filters, not only by who matches them. + // + // Phase 1 resolves WHICH submitters match; phase 2 used to count every + // presentation of those submitters, so any presentation-level filter + // over-counted (a submitter with 3 presentations returned 3 for all of them). + // ----------------------------------------------------------------- + + /** + * Seeds the acceptance scenario: one submitter owning three presentations that + * differ in track, type, published state and media upload. InsertSummitTestData + * never sets created_by, so these are the only presentations of this member. + * + * P1 - defaultTrack, defaultPresentationType, published, media upload of type M + * P2 - secondaryTrack, defaultPresentationType, published, no media upload + * P3 - defaultTrack, allow2VotePresentationType, unpublished, no media upload + */ + private function seedActivitiesCountScenario(): Member + { + $submitter = self::$em->find(Member::class, self::$member2->getId()); + + $p1 = $this->seedPresentation($submitter, self::$defaultTrack, 'P1 Published Default Track', true); + $media_upload = new PresentationMediaUpload(); + $media_upload->setName('P1 Media Upload'); + $media_upload->setDescription('P1 Media Upload Description'); + $media_upload->setFilename('p1.pdf'); + $media_upload->setMediaUploadType(self::$media_uploads_types[0]); + $p1->addMediaUpload($media_upload); + + $this->seedPresentation($submitter, self::$secondaryTrack, 'P2 Published Secondary Track', true); + + // allow2VotePresentationType does not allow a publishing period, so this one + // must not get start/end dates or SummitEvent rejects it. + $p3 = new Presentation(); + self::$summit->addEvent($p3); + $p3->setTitle('P3 Unpublished Default Track Other Type'); + $p3->setAbstract('Abstract'); + $p3->setCategory(self::$defaultTrack); + $p3->setType(self::$allow2VotePresentationType); + $p3->setProgress(Presentation::PHASE_COMPLETE); + $p3->setStatus(Presentation::STATUS_RECEIVED); + $p3->setCreatedBy($submitter); + + self::$em->flush(); + + return $submitter; + } + + /** + * Counts the activities of one submitter under the given presentation-level conditions. + */ + private function countActivitiesOf(Member $submitter, array $conditions = []): int + { + $expressions = ['id==' . $submitter->getId()]; + $rules = ['id' => ['==']]; + + foreach ($conditions as $field => $value) { + $expressions[] = $field . '==' . $value; + $rules[$field] = ['==']; + } + + return EntityManager::getRepository(Member::class)->getUniqueActivitiesCountBySummit( + self::$summit, + FilterParser::parse($expressions, $rules) + ); + } + + public function testActivitiesCountWithoutPresentationFilterCountsEveryPresentationOfTheSubmitter(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + $this->assertEquals(3, $this->countActivitiesOf($submitter)); + } + + public function testActivitiesCountIsScopedByTrackFilter(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + // P1 and P3 are in defaultTrack, P2 is in secondaryTrack. + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'presentations_track_id' => self::$defaultTrack->getId(), + ])); + $this->assertEquals(1, $this->countActivitiesOf($submitter, [ + 'presentations_track_id' => self::$secondaryTrack->getId(), + ])); + } + + public function testActivitiesCountIsScopedByTrackGroupFilter(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + // defaultTrackGroup contains defaultTrack only: P1 and P3. + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'presentations_track_group_id' => self::$defaultTrackGroup->getId(), + ])); + } + + public function testActivitiesCountIsScopedByTypeFilter(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + // only P3 uses allow2VotePresentationType + $this->assertEquals(1, $this->countActivitiesOf($submitter, [ + 'presentations_type_id' => self::$allow2VotePresentationType->getId(), + ])); + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'presentations_type_id' => self::$defaultPresentationType->getId(), + ])); + } + + public function testActivitiesCountIsScopedByPublishedFilter(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + // P1 and P2 are published, P3 is not. + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'has_published_presentations' => 'true', + ])); + } + + public function testActivitiesCountIsScopedByMediaUploadFilter(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + // only P1 carries a media upload of that type + $this->assertEquals(1, $this->countActivitiesOf($submitter, [ + 'has_media_upload_with_type' => self::$media_uploads_types[0]->getId(), + ])); + } + + public function testActivitiesCountForNotMediaUploadFilterCountsEveryPresentationOfTheMatchedSubmitter(): void + { + // has_not_media_upload_with_type matches a submitter only when NONE of their + // presentations carries a media upload of that type, so every presentation + // phase 2 reaches already qualifies. + $submitter = self::$em->find(Member::class, self::$member2->getId()); + + $this->seedPresentation($submitter, self::$defaultTrack, 'No Media A', true); + $this->seedPresentation($submitter, self::$secondaryTrack, 'No Media B', true); + self::$em->flush(); + + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'has_not_media_upload_with_type' => self::$media_uploads_types[0]->getId(), + ])); + } + + public function testActivitiesCountIsZeroWhenTheSubmitterOwnsAMediaUploadOfThatType(): void + { + // the scenario submitter owns P1 with that media upload, so they do not match at all + $submitter = $this->seedActivitiesCountScenario(); + + $this->assertEquals(0, $this->countActivitiesOf($submitter, [ + 'has_not_media_upload_with_type' => self::$media_uploads_types[0]->getId(), + ])); + } + + public function testActivitiesCountIsScopedByPublishedAndTrackCombined(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + // only P1 is both published and in defaultTrack + $this->assertEquals(1, $this->countActivitiesOf($submitter, [ + 'has_published_presentations' => 'true', + 'presentations_track_id' => self::$defaultTrack->getId(), + ])); + + // P2 is published but in secondaryTrack + $this->assertEquals(1, $this->countActivitiesOf($submitter, [ + 'has_published_presentations' => 'true', + 'presentations_track_id' => self::$secondaryTrack->getId(), + ])); + + // P3 is in defaultTrack with that type but unpublished + $this->assertEquals(0, $this->countActivitiesOf($submitter, [ + 'has_published_presentations' => 'true', + 'presentations_type_id' => self::$allow2VotePresentationType->getId(), + ])); + } + + public function testActivitiesCountIsScopedByTitleFilter(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + $count = EntityManager::getRepository(Member::class)->getUniqueActivitiesCountBySummit( + self::$summit, + FilterParser::parse( + ['id==' . $submitter->getId(), 'presentations_title=@P1 Published'], + ['id' => ['=='], 'presentations_title' => ['=@']] + ) + ); + + $this->assertEquals(1, $count); + } + + public function testActivitiesCountForPublishedFalseCountsEveryPresentationOfTheMatchedSubmitter(): void + { + // A submitter with no published and no selected presentation at all: the == false + // side adds no presentation predicate, so every presentation of theirs is counted. + $submitter = self::$em->find(Member::class, self::$member2->getId()); + + $this->seedPresentation($submitter, self::$defaultTrack, 'Unpublished A', false); + $this->seedPresentation($submitter, self::$secondaryTrack, 'Unpublished B', false); + self::$em->flush(); + + foreach (['has_published_presentations', 'has_accepted_presentations', 'has_alternate_presentations'] as $field) { + $this->assertEquals( + 2, + $this->countActivitiesOf($submitter, [$field => 'false']), + $field . '==false must not restrict which presentations are counted' + ); + } + } + + public function testActivitiesCountForRejectedFalseCountsEveryPresentationOfTheMatchedSubmitter(): void + { + // rejected == false needs a submitter with no rejected presentation: an unpublished + // presentation outside every selected list is rejected, so this one publishes both. + $submitter = self::$em->find(Member::class, self::$member2->getId()); + + $this->seedPresentation($submitter, self::$defaultTrack, 'Published A', true); + $this->seedPresentation($submitter, self::$secondaryTrack, 'Published B', true); + self::$em->flush(); + + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'has_rejected_presentations' => 'false', + ])); + } + + public function testActivitiesCountWithNoFilterIsUnaffectedByTheScoping(): void + { + $repo = EntityManager::getRepository(Member::class); + $before = $repo->getUniqueActivitiesCountBySummit(self::$summit); + + $this->seedActivitiesCountScenario(); + + $this->assertEquals($before + 3, $repo->getUniqueActivitiesCountBySummit(self::$summit)); + } + + public function testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranch(): void + { + $submitter = $this->seedActivitiesCountScenario(); + + // "id== OR presentations_track_id==". Phase 1 matches + // the submitter through either branch, but phase 2 sees only the + // presentation-level branch: Filter::toRawSQL skips the fields it has no mapping + // for, which is the same semantics every other toRawSQL caller lives with. The + // count is therefore the secondaryTrack presentations of the matched submitters -- + // P2 alone -- and not all three. Pinned here because it is the one case where + // phase 2 ends up narrower than the set phase 1 matched. + $filter = FilterParser::parse( + ['id==' . $submitter->getId() . ',presentations_track_id==' . self::$secondaryTrack->getId()], + ['id' => ['=='], 'presentations_track_id' => ['==']] + ); + + $count = EntityManager::getRepository(Member::class) + ->getUniqueActivitiesCountBySummit(self::$summit, $filter); + + $this->assertEquals(1, $count); + } } diff --git a/tests/oauth2/OAuth2SummitSpeakersApiTest.php b/tests/oauth2/OAuth2SummitSpeakersApiTest.php index 4f709cd30..73d4b8dab 100644 --- a/tests/oauth2/OAuth2SummitSpeakersApiTest.php +++ b/tests/oauth2/OAuth2SummitSpeakersApiTest.php @@ -22,6 +22,7 @@ use Illuminate\Support\Facades\Queue; use LaravelDoctrine\ORM\Facades\EntityManager; use models\summit\Presentation; +use models\summit\PresentationMediaUpload; use models\summit\PresentationSpeaker; use utils\FilterParser; use models\summit\SpeakersSummitRegistrationPromoCode; @@ -3506,4 +3507,133 @@ public function testGetCurrentSummitSpeakersActivitiesCountWithPublishedPresenta $this->assertGreaterThan(0, $data->count); } + // ----------------------------------------------------------------- + // GET /api/v1/summits/{id}/speakers/all/events/count + // The count must describe the presentations that satisfy the request + // filter, not every presentation of the matched speakers. + // ----------------------------------------------------------------- + + /** + * P1 - defaultTrack, defaultPresentationType, published, media upload of type M + * P2 - secondaryTrack, defaultPresentationType, published, no media upload + * P3 - defaultTrack, allow2VotePresentationType, unpublished, no media upload + * + * The fixture speaker owns 40 other presentations, so the assertions below filter + * by this speaker's id to get exact counts. + */ + private function seedActivitiesCountScenario(): PresentationSpeaker + { + $speaker = new PresentationSpeaker(); + $speaker->setFirstName('ApiActivitiesScenario'); + $speaker->setLastName('TestSpeaker'); + self::$em->persist($speaker); + + $start = new \DateTime('now', new \DateTimeZone('UTC')); + + $p1 = new Presentation(); + self::$summit->addEvent($p1); + $p1->setTitle('Api Count P1 Published Default Track'); + $p1->setAbstract('Abstract'); + $p1->setCategory(self::$defaultTrack); + $p1->setType(self::$defaultPresentationType); + $p1->setProgress(Presentation::PHASE_COMPLETE); + $p1->setStatus(Presentation::STATUS_RECEIVED); + $p1->setStartDate($start); + $p1->setEndDate((clone $start)->add(new \DateInterval('PT2H'))); + $p1->addSpeaker($speaker); + $p1->publish(); + + $media_upload = new PresentationMediaUpload(); + $media_upload->setName('Api Count P1 Media Upload'); + $media_upload->setDescription('Api Count P1 Media Upload Description'); + $media_upload->setFilename('p1.pdf'); + $media_upload->setMediaUploadType(self::$media_uploads_types[0]); + $p1->addMediaUpload($media_upload); + + $p2 = new Presentation(); + self::$summit->addEvent($p2); + $p2->setTitle('Api Count P2 Published Secondary Track'); + $p2->setAbstract('Abstract'); + $p2->setCategory(self::$secondaryTrack); + $p2->setType(self::$defaultPresentationType); + $p2->setProgress(Presentation::PHASE_COMPLETE); + $p2->setStatus(Presentation::STATUS_RECEIVED); + $p2->setStartDate($start); + $p2->setEndDate((clone $start)->add(new \DateInterval('PT2H'))); + $p2->addSpeaker($speaker); + $p2->publish(); + + // allow2VotePresentationType does not allow a publishing period: no start/end dates. + $p3 = new Presentation(); + self::$summit->addEvent($p3); + $p3->setTitle('Api Count P3 Unpublished Default Track Other Type'); + $p3->setAbstract('Abstract'); + $p3->setCategory(self::$defaultTrack); + $p3->setType(self::$allow2VotePresentationType); + $p3->setProgress(Presentation::PHASE_COMPLETE); + $p3->setStatus(Presentation::STATUS_RECEIVED); + $p3->addSpeaker($speaker); + + self::$em->flush(); + + return $speaker; + } + + private function getActivitiesCount(array $filter): int + { + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + + $response = $this->action( + "GET", + "OAuth2SummitSpeakersApiController@getSpeakersActivitiesCount", + ['id' => self::$summit->getId(), 'filter' => $filter], + [], [], [], $headers + ); + + $this->assertResponseStatus(200); + $data = json_decode($response->getContent()); + $this->assertNotNull($data); + $this->assertTrue(isset($data->count)); + + return (int) $data->count; + } + + public function testGetSpeakersActivitiesCountIsScopedByThePresentationFilters() + { + $speaker = $this->seedActivitiesCountScenario(); + $id = 'id==' . $speaker->getId(); + + // no presentation-level filter: every presentation of the speaker + $this->assertEquals(3, $this->getActivitiesCount([$id])); + + // P1 and P3 are in defaultTrack + $this->assertEquals(2, $this->getActivitiesCount([ + $id, 'presentations_track_id==' . self::$defaultTrack->getId(), + ])); + + // only P3 uses allow2VotePresentationType + $this->assertEquals(1, $this->getActivitiesCount([ + $id, 'presentations_type_id==' . self::$allow2VotePresentationType->getId(), + ])); + + // P1 and P2 are published + $this->assertEquals(2, $this->getActivitiesCount([ + $id, 'has_published_presentations==true', + ])); + + // only P1 carries a media upload of that type + $this->assertEquals(1, $this->getActivitiesCount([ + $id, 'has_media_upload_with_type==' . self::$media_uploads_types[0]->getId(), + ])); + + // only P1 is both published and in defaultTrack + $this->assertEquals(1, $this->getActivitiesCount([ + $id, + 'has_published_presentations==true', + 'presentations_track_id==' . self::$defaultTrack->getId(), + ])); + } } diff --git a/tests/oauth2/OAuth2SummitSubmittersApiTest.php b/tests/oauth2/OAuth2SummitSubmittersApiTest.php index b06dc2381..a1b6a8f5f 100644 --- a/tests/oauth2/OAuth2SummitSubmittersApiTest.php +++ b/tests/oauth2/OAuth2SummitSubmittersApiTest.php @@ -4,6 +4,7 @@ use LaravelDoctrine\ORM\Facades\EntityManager; use models\main\Member; use models\summit\Presentation; +use models\summit\PresentationMediaUpload; use utils\FilterParser; /** @@ -794,4 +795,129 @@ public function testGetCurrentSummitSubmittersActivitiesCountWithPublishedPresen $this->assertEquals(1, $data->count, 'exactly one published presentation was seeded; count must be 1'); } + // ----------------------------------------------------------------- + // GET /api/v1/summits/{id}/submitters/all/events/count + // The count must describe the presentations that satisfy the request + // filter, not every presentation of the matched submitters. + // ----------------------------------------------------------------- + + /** + * P1 - defaultTrack, defaultPresentationType, published, media upload of type M + * P2 - secondaryTrack, defaultPresentationType, published, no media upload + * P3 - defaultTrack, allow2VotePresentationType, unpublished, no media upload + * + * InsertSummitTestData never sets created_by, so these are the only presentations + * of this submitter in the summit. + */ + private function seedActivitiesCountScenario(): Member + { + $submitter = self::$em->find(Member::class, self::$member2->getId()); + $start = new \DateTime('now', new \DateTimeZone('UTC')); + + $p1 = new Presentation(); + self::$summit->addEvent($p1); + $p1->setTitle('Api Count P1 Published Default Track'); + $p1->setAbstract('Abstract'); + $p1->setCategory(self::$defaultTrack); + $p1->setType(self::$defaultPresentationType); + $p1->setProgress(Presentation::PHASE_COMPLETE); + $p1->setStatus(Presentation::STATUS_RECEIVED); + $p1->setStartDate($start); + $p1->setEndDate((clone $start)->add(new \DateInterval('PT2H'))); + $p1->setCreatedBy($submitter); + $p1->publish(); + + $media_upload = new PresentationMediaUpload(); + $media_upload->setName('Api Count P1 Media Upload'); + $media_upload->setDescription('Api Count P1 Media Upload Description'); + $media_upload->setFilename('p1.pdf'); + $media_upload->setMediaUploadType(self::$media_uploads_types[0]); + $p1->addMediaUpload($media_upload); + + $p2 = new Presentation(); + self::$summit->addEvent($p2); + $p2->setTitle('Api Count P2 Published Secondary Track'); + $p2->setAbstract('Abstract'); + $p2->setCategory(self::$secondaryTrack); + $p2->setType(self::$defaultPresentationType); + $p2->setProgress(Presentation::PHASE_COMPLETE); + $p2->setStatus(Presentation::STATUS_RECEIVED); + $p2->setStartDate($start); + $p2->setEndDate((clone $start)->add(new \DateInterval('PT2H'))); + $p2->setCreatedBy($submitter); + $p2->publish(); + + // allow2VotePresentationType does not allow a publishing period: no start/end dates. + $p3 = new Presentation(); + self::$summit->addEvent($p3); + $p3->setTitle('Api Count P3 Unpublished Default Track Other Type'); + $p3->setAbstract('Abstract'); + $p3->setCategory(self::$defaultTrack); + $p3->setType(self::$allow2VotePresentationType); + $p3->setProgress(Presentation::PHASE_COMPLETE); + $p3->setStatus(Presentation::STATUS_RECEIVED); + $p3->setCreatedBy($submitter); + + self::$em->flush(); + + return $submitter; + } + + private function getActivitiesCount(array $filter): int + { + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + + $response = $this->action( + "GET", + "OAuth2SummitSubmittersApiController@getSubmittersActivitiesCount", + ['id' => self::$summit->getId(), 'filter' => $filter], + [], [], [], $headers + ); + + $this->assertResponseStatus(200); + $data = json_decode($response->getContent()); + $this->assertNotNull($data); + $this->assertTrue(isset($data->count)); + + return (int) $data->count; + } + + public function testGetSubmittersActivitiesCountIsScopedByThePresentationFilters() + { + $submitter = $this->seedActivitiesCountScenario(); + $id = 'id==' . $submitter->getId(); + + // no presentation-level filter: every presentation of the submitter + $this->assertEquals(3, $this->getActivitiesCount([$id])); + + // P1 and P3 are in defaultTrack + $this->assertEquals(2, $this->getActivitiesCount([ + $id, 'presentations_track_id==' . self::$defaultTrack->getId(), + ])); + + // only P3 uses allow2VotePresentationType + $this->assertEquals(1, $this->getActivitiesCount([ + $id, 'presentations_type_id==' . self::$allow2VotePresentationType->getId(), + ])); + + // P1 and P2 are published + $this->assertEquals(2, $this->getActivitiesCount([ + $id, 'has_published_presentations==true', + ])); + + // only P1 carries a media upload of that type + $this->assertEquals(1, $this->getActivitiesCount([ + $id, 'has_media_upload_with_type==' . self::$media_uploads_types[0]->getId(), + ])); + + // only P1 is both published and in defaultTrack + $this->assertEquals(1, $this->getActivitiesCount([ + $id, + 'has_published_presentations==true', + 'presentations_track_id==' . self::$defaultTrack->getId(), + ])); + } } From 9773af7aac54306eae45b38f8c1bfa699dc5d4e8 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 22 Sep 2026 16:38:49 +0200 Subject: [PATCH 2/5] fix(filters): keep every branch of an OR group when mappings are FilterMapping instances Filter::toRawSQL's OR-group branch assigned each mapping's rendered condition instead of appending it, so only the last FilterMapping in a group survived and earlier bindings were left orphaned. The two other mapping shapes in the same block (array and plain) already appended with OR; this made the FilterMapping case consistent with them. ActivitiesCountFilterMappingsTrait is the first caller routing OR groups through FilterMapping instances, which is what exposed this on the speakers/submitters activities count (e.g. the term search and the accepted/alternate multi-select from summit-admin). Reported by @smarcet on #599: https://github.com/OpenStackweb/summit-api/pull/599#discussion_r4032584690 Co-Authored-By: Claude Sonnet 5 --- app/Http/Utils/Filters/Filter.php | 6 +++++- tests/ActivitiesCountFilterMappingsTest.php | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/Http/Utils/Filters/Filter.php b/app/Http/Utils/Filters/Filter.php index cc4b3ecc4..c2f5c1007 100644 --- a/app/Http/Utils/Filters/Filter.php +++ b/app/Http/Utils/Filters/Filter.php @@ -320,12 +320,16 @@ public function toRawSQL(array $mappings, int $param_idx = 1) if ($e instanceof FilterElement && isset($mappings[$e->getField()])) { $mapping = $mappings[$e->getField()]; if ($mapping instanceof FilterMapping) { - $condition = $mapping->toRawSQL($e, $this->bindings); + $c = $mapping->toRawSQL($e, $this->bindings); $local_bindings = $mapping->getBindings(); if(count($local_bindings) > 0 ){ $this->bindings = array_merge($this->bindings, $local_bindings); $param_idx = count($this->bindings) + 1; } + if (!empty($c)) { + if (!empty($condition)) $condition .= ' OR '; + $condition .= $c; + } } else if (is_array($mapping)) { foreach ($mapping as $mapping_or) { diff --git a/tests/ActivitiesCountFilterMappingsTest.php b/tests/ActivitiesCountFilterMappingsTest.php index 35e83decd..841dfa249 100644 --- a/tests/ActivitiesCountFilterMappingsTest.php +++ b/tests/ActivitiesCountFilterMappingsTest.php @@ -207,6 +207,19 @@ public function testAPersonLevelFilterDoesNotStopTheOthersFromScoping(): void $this->assertStringContainsString('E.CategoryID = :param_1', $sql); } + public function testAnOrGroupOfTwoMappedFieldsKeepsEveryBranch(): void + { + [$sql, $bindings] = $this->activitiesCountSQL( + $this->filterOf([ + FilterElement::makeEqual('presentations_track_id', '5'), + FilterElement::makeEqual('presentations_type_id', '7'), + ]) + ); + + $this->assertEquals('(E.CategoryID = :param_1 OR E.TypeID = :param_2)', $sql); + $this->assertEquals(['param_1' => '5', 'param_2' => '7'], $bindings); + } + public function testEveryReturnedBindingHasItsPlaceholderInTheStatement(): void { [$sql, $bindings] = $this->activitiesCountSQL( From b446825ede62104415458e586e9ae064fb5c9413 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 22 Sep 2026 16:54:35 +0200 Subject: [PATCH 3/5] fix(speakers/submitters): OR the selection-status flags instead of ANDing them in the activities count Phase 1 reads has_accepted_presentations/has_alternate_presentations/ has_rejected_presentations per person ("has at least one presentation with that status"), which can be satisfied by different presentations. buildActivitiesCountFilter ANDed all presentation-level filters on the same physical row, so combining two of these flags (e.g. summit-admin's "Accepted & Rejected" status option) asked one presentation to be two mutually exclusive statuses at once and the count came back 0 even though phase 1 matched people. The three selection-status mappings are now pulled out and combined with OR among themselves (their == true conditions only, == false stays neutral as before), then AND'd with the rest of the presentation-level filters same as before. Added a red/green SQL-shape test to ActivitiesCountFilterMappingsTest (runs without a DB, same pattern already used in that file) plus the DB-backed regression tests in SpeakerRepositoryTest and SubmitterRepositoryTest. Could not execute the DB-backed tests in this sandbox: the host PHP lacks the apcu extension Doctrine's cache needs to boot the app outside its Docker container, so only the DB-free suite (ActivitiesCountFilterMappingsTest) could actually be run here. Reported by @smarcet on #599: https://github.com/OpenStackweb/summit-api/pull/599#discussion_r4032651374 Co-Authored-By: Claude Sonnet 5 --- .../ActivitiesCountFilterMappingsTrait.php | 41 +++++++++++++++++- tests/ActivitiesCountFilterMappingsTest.php | 42 +++++++++++++++++++ tests/SpeakerRepositoryTest.php | 35 ++++++++++++++++ tests/SubmitterRepositoryTest.php | 32 ++++++++++++++ 4 files changed, 148 insertions(+), 2 deletions(-) diff --git a/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php b/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php index 9902cae0a..d0d82cf34 100644 --- a/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php +++ b/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php @@ -49,6 +49,17 @@ */ trait ActivitiesCountFilterMappingsTrait { + /** + * The three selection-status filters share a single presentation-level check + * (has_published_presentations does not, its published check is compatible with + * being AND'd with any of them). + */ + private const SELECTION_STATUS_FILTERS = [ + 'has_accepted_presentations', + 'has_alternate_presentations', + 'has_rejected_presentations', + ]; + /** * Turns the request filter into the phase-2 WHERE fragment and the bindings the * statement needs. @@ -57,6 +68,16 @@ trait ActivitiesCountFilterMappingsTrait * `Presentation P`, binds the summit as `:summit_id`, and appends the fragment to its * own WHERE. The fragment is empty when no presentation-level filter applies. * + * The three selection-status filters (SELECTION_STATUS_FILTERS) are handled apart from + * the rest: phase 1 reads them per person ("has at least one accepted presentation"), + * so a request combining two of them with AND (e.g. the "Accepted & Rejected" option in + * summit-admin) asks for a person who has one presentation of each status, not for a + * single presentation that is both -- no row can satisfy that literally. The `== true` + * conditions among them are therefore OR'd together (the union of the matched + * statuses), while `== false` stays neutral exactly as it already is for every other + * filter, and the resulting group is AND'd with the rest of the presentation-level + * filters same as before. + * * @param Filter|null $filter * @param int $summit_id * @return array [string $extra_filters, array $bindings] @@ -67,11 +88,27 @@ protected function buildActivitiesCountFilter(?Filter $filter, int $summit_id): $bindings = ['summit_id' => $summit_id]; if (!is_null($filter)) { - $where = $filter->toRawSQL($this->getActivitiesCountFilterMappings()); + $mappings = $this->getActivitiesCountFilterMappings(); + $status_mappings = array_intersect_key($mappings, array_flip(self::SELECTION_STATUS_FILTERS)); + $other_mappings = array_diff_key($mappings, $status_mappings); + + $where = $filter->toRawSQL($other_mappings); if (!empty($where)) { - $extra_filters = ' AND (' . $where . ')'; + $extra_filters .= ' AND (' . $where . ')'; $bindings = array_merge($bindings, $filter->getSQLBindings()); } + + $status_conditions = []; + foreach ($status_mappings as $field => $mapping) { + foreach ($filter->getFilter($field) as $element) { + $condition = $mapping->toRawSQL($element); + if ($condition === '' || $condition === '( ' . SQLSwitchFilterMapping::NoRestriction . ' )') continue; + $status_conditions[] = $condition; + } + } + if (!empty($status_conditions)) { + $extra_filters .= ' AND (' . implode(' OR ', $status_conditions) . ')'; + } } return [$extra_filters, $bindings]; diff --git a/tests/ActivitiesCountFilterMappingsTest.php b/tests/ActivitiesCountFilterMappingsTest.php index 841dfa249..3d02840f0 100644 --- a/tests/ActivitiesCountFilterMappingsTest.php +++ b/tests/ActivitiesCountFilterMappingsTest.php @@ -331,6 +331,48 @@ public function testBuildAppendsTheFragmentAndKeepsTheSummitBinding(string $repo $this->assertEquals(['summit_id' => 73, 'param_1' => '5'], $bindings); } + #[DataProvider('repositoryProvider')] + public function testBuildOrsTheTrueSelectionStatusFlagsAndKeepsFalseNeutral(string $repository_class): void + { + // The three selection-status filters are read per person in phase 1 ("has at + // least one accepted presentation"), so AND-ing e.g. accepted==true and + // rejected==true in phase 2 would demand a single row with both statuses at + // once -- impossible. They must be OR'd among themselves instead, with the + // == false side staying neutral (dropped, not turned into an OR branch). + [$extra_filters] = $this->buildFor( + $this->filterOf( + FilterElement::makeEqual('has_rejected_presentations', 'true'), + FilterElement::makeEqual('has_accepted_presentations', 'true'), + FilterElement::makeEqual('has_alternate_presentations', 'false') + ), + $repository_class + ); + + $this->assertStringContainsString('E.Published = 0 AND NOT EXISTS', $extra_filters); + $this->assertStringContainsString('E.Published = 1', $extra_filters); + $this->assertStringContainsString(') OR (', $extra_filters, 'the true flags must be OR\'d together'); + // the alternate==false branch must not appear as a bare "1 = 1" clause of its own + $this->assertStringNotContainsString('OR (1 = 1)', $extra_filters); + $this->assertStringNotContainsString('(1 = 1) OR', $extra_filters); + } + + #[DataProvider('repositoryProvider')] + public function testBuildKeepsTheStatusGroupAndedWithTheRestOfTheFilters(string $repository_class): void + { + [$extra_filters, $bindings] = $this->buildFor( + $this->filterOf( + FilterElement::makeEqual('has_accepted_presentations', 'true'), + FilterElement::makeEqual('presentations_track_id', '5') + ), + $repository_class + ); + + $this->assertStringContainsString('E.CategoryID = :param_1', $extra_filters); + $this->assertStringContainsString('E.Published = 1', $extra_filters); + $this->assertMatchesRegularExpression('/\)\s*AND\s*\(/', $extra_filters); + $this->assertEquals(['summit_id' => 73, 'param_1' => '5'], $bindings); + } + #[DataProvider('repositoryProvider')] public function testBuildProducesTheSameFragmentForBothRoles(string $repository_class): void { diff --git a/tests/SpeakerRepositoryTest.php b/tests/SpeakerRepositoryTest.php index 0c4df87fa..69d14a7b3 100644 --- a/tests/SpeakerRepositoryTest.php +++ b/tests/SpeakerRepositoryTest.php @@ -927,6 +927,41 @@ public function testActivitiesCountForRejectedFalseCountsEveryPresentationOfTheM ])); } + public function testActivitiesCountForCombinedStatusFlagsIsTheUnionOfTheStatuses(): void + { + // summit-admin "Accepted & Rejected": three AND'd flags. Phase 1 reads them per + // person (one accepted AND one rejected presentation, possibly different ones), + // so phase 2 must not demand both statuses of a single presentation. + $speaker = new PresentationSpeaker(); + $speaker->setFirstName('ScenarioAcceptedRejected'); + $speaker->setLastName('ActivitiesScenario'); + self::$em->persist($speaker); + + $this->seedPresentation($speaker, self::$defaultTrack, 'Accepted (published)', true); + $this->seedPresentation($speaker, self::$secondaryTrack, 'Rejected (unpublished, unlisted)', false); + self::$em->flush(); + + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'has_rejected_presentations' => 'true', + 'has_accepted_presentations' => 'true', + 'has_alternate_presentations' => 'false', + ])); + } + + public function testActivitiesCountForOnlyAcceptedStatusDoesNotWidenPastTheTrueFlags(): void + { + // Guards the OR-ing of the status group from over-widening: the two == false + // companions must stay neutral, not turn into an unrestricted OR branch. + $speaker = $this->seedActivitiesCountScenario('ScenarioOnlyAccepted'); + + // P1 and P2 are published (accepted); P3 is not. + $this->assertEquals(2, $this->countActivitiesOf($speaker, [ + 'has_rejected_presentations' => 'false', + 'has_accepted_presentations' => 'true', + 'has_alternate_presentations' => 'false', + ])); + } + public function testActivitiesCountWithNoFilterIsUnaffectedByTheScoping(): void { $before = $this->repo()->getUniqueActivitiesCountBySummit(self::$summit); diff --git a/tests/SubmitterRepositoryTest.php b/tests/SubmitterRepositoryTest.php index dea3ce3e0..97c8cd312 100644 --- a/tests/SubmitterRepositoryTest.php +++ b/tests/SubmitterRepositoryTest.php @@ -1063,6 +1063,38 @@ public function testActivitiesCountForRejectedFalseCountsEveryPresentationOfTheM ])); } + public function testActivitiesCountForCombinedStatusFlagsIsTheUnionOfTheStatuses(): void + { + // summit-admin "Accepted & Rejected": three AND'd flags. Phase 1 reads them per + // person (one accepted AND one rejected presentation, possibly different ones), + // so phase 2 must not demand both statuses of a single presentation. + $submitter = self::$em->find(Member::class, self::$member2->getId()); + + $this->seedPresentation($submitter, self::$defaultTrack, 'Accepted (published)', true); + $this->seedPresentation($submitter, self::$secondaryTrack, 'Rejected (unpublished, unlisted)', false); + self::$em->flush(); + + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'has_rejected_presentations' => 'true', + 'has_accepted_presentations' => 'true', + 'has_alternate_presentations' => 'false', + ])); + } + + public function testActivitiesCountForOnlyAcceptedStatusDoesNotWidenPastTheTrueFlags(): void + { + // Guards the OR-ing of the status group from over-widening: the two == false + // companions must stay neutral, not turn into an unrestricted OR branch. + $submitter = $this->seedActivitiesCountScenario(); + + // P1 and P2 are published (accepted); P3 is not. + $this->assertEquals(2, $this->countActivitiesOf($submitter, [ + 'has_rejected_presentations' => 'false', + 'has_accepted_presentations' => 'true', + 'has_alternate_presentations' => 'false', + ])); + } + public function testActivitiesCountWithNoFilterIsUnaffectedByTheScoping(): void { $repo = EntityManager::getRepository(Member::class); From 5406927d920a5590d0942f085977e5c14cb5dcd8 Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 22 Sep 2026 17:01:01 +0200 Subject: [PATCH 4/5] fix(filters): widen instead of narrow an OR group phase 2 cannot fully express Filter::toRawSQL naturally narrows an OR group to the branches that have a mapping, which is the right call for every existing caller. The activities count needed the opposite: summit-admin's term search sends full_name/first_name/last_name/email (person-level, unmapped in phase 2) OR'd with presentations_title/presentations_abstract (mapped). Narrowing that group meant a speaker matched through their name alone contributed nothing to the count, so a plain name search showed "N Speakers | 0 Activities". Added an opt-in $skip_partially_mapped_or_groups flag to Filter::toRawSQL (default false, existing callers unaffected): when set, an OR group containing any field absent from the mappings is dropped in full rather than narrowed, so it stops restricting the count instead of restricting it to what phase 2 can express. buildActivitiesCountFilter now passes it for the non-status mappings. This over-counts for people matched only through such a group, which is the acceptable direction for a number sizing an email blast. Flipped testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranch (now .../CountsEveryPresentation) in both repository test suites to the new expected count, and added the term-search regression plus a guard that a fully-mapped OR group still restricts as before -- both in the repository suites and as a DB-free SQL-shape check in ActivitiesCountFilterMappingsTest. Reported by @smarcet on #599: https://github.com/OpenStackweb/summit-api/pull/599#discussion_r4032715141 Co-Authored-By: Claude Sonnet 5 --- app/Http/Utils/Filters/Filter.php | 15 ++++- .../ActivitiesCountFilterMappingsTrait.php | 19 +++--- tests/ActivitiesCountFilterMappingsTest.php | 38 +++++++++++ tests/SpeakerRepositoryTest.php | 64 +++++++++++++++--- tests/SubmitterRepositoryTest.php | 67 ++++++++++++++++--- 5 files changed, 176 insertions(+), 27 deletions(-) diff --git a/app/Http/Utils/Filters/Filter.php b/app/Http/Utils/Filters/Filter.php index c2f5c1007..30cc80e3d 100644 --- a/app/Http/Utils/Filters/Filter.php +++ b/app/Http/Utils/Filters/Filter.php @@ -282,9 +282,15 @@ private function applyCondition(FilterElement $filter, string $mapping, int &$pa /** * @param array $mappings + * @param int $param_idx + * @param bool $skip_partially_mapped_or_groups When true, an OR group containing a + * field absent from $mappings is dropped in full instead of being narrowed to + * the branches that do have a mapping: a branch this call cannot express must + * stop the group from restricting the result, not shrink it to what it can + * express. Off by default so existing callers keep the narrowing behaviour. * @return string */ - public function toRawSQL(array $mappings, int $param_idx = 1) + public function toRawSQL(array $mappings, int $param_idx = 1, bool $skip_partially_mapped_or_groups = false) { $sql = ''; $this->bindings = []; @@ -315,6 +321,13 @@ public function toRawSQL(array $mappings, int $param_idx = 1) if(!empty($condition)) $sql .= '('. $condition. ')'; } else if (is_array($filter)) { // an array is a OR + if ($skip_partially_mapped_or_groups) { + foreach ($filter as $e) { + if ($e instanceof FilterElement && !isset($mappings[$e->getField()])) { + continue 2; + } + } + } $condition = ''; foreach ($filter as $e) { if ($e instanceof FilterElement && isset($mappings[$e->getField()])) { diff --git a/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php b/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php index d0d82cf34..6b58f93c3 100644 --- a/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php +++ b/app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php @@ -36,13 +36,16 @@ * unrestricted for them, and both repositories share this list because the conditions * correlate to the presentation, not to the role the person plays on it. * - * Known limitation, inherited from Filter::toRawSQL and shared with every other caller of - * it: skipping an unmapped field is right for a slot joined by AND, but inside an OR group - * it drops a branch instead of widening it, so `full_name==x,presentations_track_id==N` - * counts only the track N presentations even though phase 1 also matched people through - * full_name. Expressing the correct rule needs a per-person predicate, which no set-level - * condition can carry. The behaviour is pinned by - * testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranch in both + * An unmapped field inside an OR group is a different case: skipping it there would + * narrow the group to only the branches phase 2 can express, e.g. summit-admin's term + * search ORs full_name/first_name/last_name/email (unmapped) with presentations_title/ + * presentations_abstract (mapped) -- narrowing would count nothing for a person matched + * through their name alone. `Filter::toRawSQL`'s $skip_partially_mapped_or_groups flag + * drops such a group entirely instead, so it does not restrict the count at all; every + * presentation of the matched person is counted, which over-counts for people matched + * through a presentation branch but never reads as "0 Activities" for a match phase 2 + * cannot express. The behaviour is pinned by + * testActivitiesCountWithAnOredPersonLevelFilterCountsEveryPresentation in both * repository test suites. * * @package App\Repositories\Summit\Traits @@ -92,7 +95,7 @@ protected function buildActivitiesCountFilter(?Filter $filter, int $summit_id): $status_mappings = array_intersect_key($mappings, array_flip(self::SELECTION_STATUS_FILTERS)); $other_mappings = array_diff_key($mappings, $status_mappings); - $where = $filter->toRawSQL($other_mappings); + $where = $filter->toRawSQL($other_mappings, 1, true); if (!empty($where)) { $extra_filters .= ' AND (' . $where . ')'; $bindings = array_merge($bindings, $filter->getSQLBindings()); diff --git a/tests/ActivitiesCountFilterMappingsTest.php b/tests/ActivitiesCountFilterMappingsTest.php index 3d02840f0..9c225a43a 100644 --- a/tests/ActivitiesCountFilterMappingsTest.php +++ b/tests/ActivitiesCountFilterMappingsTest.php @@ -373,6 +373,44 @@ public function testBuildKeepsTheStatusGroupAndedWithTheRestOfTheFilters(string $this->assertEquals(['summit_id' => 73, 'param_1' => '5'], $bindings); } + #[DataProvider('repositoryProvider')] + public function testBuildDropsAnOrGroupWithAnUnmappedBranchEntirely(string $repository_class): void + { + // "id" has no phase-2 mapping. A branch phase 2 cannot express must stop the + // whole OR group from restricting the count, not narrow it to the branches it + // can express -- narrowing would read as "0 Activities" for anyone matched only + // through the unmapped branch (summit-admin's term search does this on every + // name match). + [$extra_filters, $bindings] = $this->buildFor( + $this->filterOf([ + FilterElement::makeEqual('id', '123'), + FilterElement::makeEqual('presentations_track_id', '5'), + ]), + $repository_class + ); + + $this->assertEmpty($extra_filters); + $this->assertEquals(['summit_id' => 73], $bindings); + } + + #[DataProvider('repositoryProvider')] + public function testBuildStillRestrictsAFullyMappedOrGroup(string $repository_class): void + { + // Guards the widening from leaking into a group phase 2 CAN fully express. + [$extra_filters, $bindings] = $this->buildFor( + $this->filterOf([ + FilterElement::makeEqual('presentations_track_id', '5'), + FilterElement::makeEqual('presentations_type_id', '7'), + ]), + $repository_class + ); + + $this->assertStringContainsString('E.CategoryID = :param_1', $extra_filters); + $this->assertStringContainsString('E.TypeID = :param_2', $extra_filters); + $this->assertStringContainsString(' OR ', $extra_filters); + $this->assertEquals(['summit_id' => 73, 'param_1' => '5', 'param_2' => '7'], $bindings); + } + #[DataProvider('repositoryProvider')] public function testBuildProducesTheSameFragmentForBothRoles(string $repository_class): void { diff --git a/tests/SpeakerRepositoryTest.php b/tests/SpeakerRepositoryTest.php index 69d14a7b3..496d50123 100644 --- a/tests/SpeakerRepositoryTest.php +++ b/tests/SpeakerRepositoryTest.php @@ -973,17 +973,17 @@ public function testActivitiesCountWithNoFilterIsUnaffectedByTheScoping(): void $this->assertEquals($before + 3, $after); } - public function testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranch(): void + public function testActivitiesCountWithAnOredPersonLevelFilterCountsEveryPresentation(): void { + // "id== OR presentations_track_id==". Phase 1 matches the + // speaker through either branch. Phase 2 cannot express the id branch, and an OR + // group with a branch it cannot express must stop restricting the count rather + // than narrow to the branches it can express -- narrowing would read as "0 + // Activities" for anyone matched only through the unmapped branch, which is what + // summit-admin's term search does on every name match. So the whole group is + // dropped and every presentation of the matched speaker is counted. $speaker = $this->seedActivitiesCountScenario('ScenarioOrGroup'); - // "id== OR presentations_track_id==". Phase 1 matches - // the speaker through either branch, but phase 2 sees only the presentation-level - // branch: Filter::toRawSQL skips the fields it has no mapping for, which is the - // same semantics every other toRawSQL caller lives with. The count is therefore - // the secondaryTrack presentations of the matched speakers -- P2 alone -- and not - // all three. Pinned here because it is the one case where phase 2 ends up - // narrower than the set phase 1 matched. $filter = FilterParser::parse( ['id==' . $speaker->getId() . ',presentations_track_id==' . self::$secondaryTrack->getId()], ['id' => ['=='], 'presentations_track_id' => ['==']] @@ -991,6 +991,52 @@ public function testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentati $count = $this->repo()->getUniqueActivitiesCountBySummit(self::$summit, $filter); - $this->assertEquals(1, $count); + $this->assertEquals(3, $count); + } + + public function testActivitiesCountForATermSearchCountsEveryPresentationOfTheMatchedSpeaker(): void + { + // buildTermFilter shape (summit-admin): one OR group of full_name, first_name, + // last_name, email (all unmapped in phase 2) plus presentations_title and + // presentations_abstract (mapped). Matched here through first_name alone, with no + // title or abstract containing the term. + $speaker = $this->seedActivitiesCountScenario('Zzterm'); + + $count = $this->repo()->getUniqueActivitiesCountBySummit( + self::$summit, + FilterParser::parse( + ['full_name=@zzterm,first_name=@zzterm,last_name=@zzterm,email=@zzterm,presentations_title=@zzterm,presentations_abstract=@zzterm'], + [ + 'full_name' => ['=@'], + 'first_name' => ['=@'], + 'last_name' => ['=@'], + 'email' => ['=@'], + 'presentations_title' => ['=@'], + 'presentations_abstract' => ['=@'], + ] + ) + ); + + $this->assertEquals(3, $count); + } + + public function testActivitiesCountWithAFullyMappedOrGroupStillRestricts(): void + { + // Guards the widening from leaking into a group phase 2 CAN fully express: every + // branch here has a phase-2 mapping, so it must still narrow the count as before. + $speaker = $this->seedActivitiesCountScenario('ScenarioFullyMappedOrGroup'); + + $filter = FilterParser::parse( + [ + 'id==' . $speaker->getId(), + 'presentations_track_id==' . self::$secondaryTrack->getId() . ',has_published_presentations==true', + ], + ['id' => ['=='], 'presentations_track_id' => ['=='], 'has_published_presentations' => ['==']] + ); + + $count = $this->repo()->getUniqueActivitiesCountBySummit(self::$summit, $filter); + + // P2 matches by track, P1 and P2 match by published; P3 matches neither. + $this->assertEquals(2, $count); } } diff --git a/tests/SubmitterRepositoryTest.php b/tests/SubmitterRepositoryTest.php index 97c8cd312..ccc91588f 100644 --- a/tests/SubmitterRepositoryTest.php +++ b/tests/SubmitterRepositoryTest.php @@ -1105,17 +1105,17 @@ public function testActivitiesCountWithNoFilterIsUnaffectedByTheScoping(): void $this->assertEquals($before + 3, $repo->getUniqueActivitiesCountBySummit(self::$summit)); } - public function testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranch(): void + public function testActivitiesCountWithAnOredPersonLevelFilterCountsEveryPresentation(): void { + // "id== OR presentations_track_id==". Phase 1 matches + // the submitter through either branch. Phase 2 cannot express the id branch, and + // an OR group with a branch it cannot express must stop restricting the count + // rather than narrow to the branches it can express -- narrowing would read as "0 + // Activities" for anyone matched only through the unmapped branch, which is what + // summit-admin's term search does on every name match. So the whole group is + // dropped and every presentation of the matched submitter is counted. $submitter = $this->seedActivitiesCountScenario(); - // "id== OR presentations_track_id==". Phase 1 matches - // the submitter through either branch, but phase 2 sees only the - // presentation-level branch: Filter::toRawSQL skips the fields it has no mapping - // for, which is the same semantics every other toRawSQL caller lives with. The - // count is therefore the secondaryTrack presentations of the matched submitters -- - // P2 alone -- and not all three. Pinned here because it is the one case where - // phase 2 ends up narrower than the set phase 1 matched. $filter = FilterParser::parse( ['id==' . $submitter->getId() . ',presentations_track_id==' . self::$secondaryTrack->getId()], ['id' => ['=='], 'presentations_track_id' => ['==']] @@ -1124,6 +1124,55 @@ public function testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentati $count = EntityManager::getRepository(Member::class) ->getUniqueActivitiesCountBySummit(self::$summit, $filter); - $this->assertEquals(1, $count); + $this->assertEquals(3, $count); + } + + public function testActivitiesCountForATermSearchCountsEveryPresentationOfTheMatchedSubmitter(): void + { + // buildTermFilter shape (summit-admin submitter-actions.js): one OR group of + // full_name, first_name, last_name, email (all unmapped in phase 2) plus + // presentations_title and presentations_abstract (mapped). Matched here through + // first_name alone, with no title or abstract containing the term. + $submitter = $this->seedActivitiesCountScenario(); + $submitter->setFirstName('Zzterm'); + self::$em->flush(); + + $count = EntityManager::getRepository(Member::class)->getUniqueActivitiesCountBySummit( + self::$summit, + FilterParser::parse( + ['full_name=@zzterm,first_name=@zzterm,last_name=@zzterm,email=@zzterm,presentations_title=@zzterm,presentations_abstract=@zzterm'], + [ + 'full_name' => ['=@'], + 'first_name' => ['=@'], + 'last_name' => ['=@'], + 'email' => ['=@'], + 'presentations_title' => ['=@'], + 'presentations_abstract' => ['=@'], + ] + ) + ); + + $this->assertEquals(3, $count); + } + + public function testActivitiesCountWithAFullyMappedOrGroupStillRestricts(): void + { + // Guards the widening from leaking into a group phase 2 CAN fully express: every + // branch here has a phase-2 mapping, so it must still narrow the count as before. + $submitter = $this->seedActivitiesCountScenario(); + + $filter = FilterParser::parse( + [ + 'id==' . $submitter->getId(), + 'presentations_track_id==' . self::$secondaryTrack->getId() . ',has_published_presentations==true', + ], + ['id' => ['=='], 'presentations_track_id' => ['=='], 'has_published_presentations' => ['==']] + ); + + $count = EntityManager::getRepository(Member::class) + ->getUniqueActivitiesCountBySummit(self::$summit, $filter); + + // P2 matches by track, P1 and P2 match by published; P3 matches neither. + $this->assertEquals(2, $count); } } From e163eff3997d72a8e3cb2922edda9d1e8f81ebbd Mon Sep 17 00:00:00 2001 From: romanetar Date: Tue, 22 Sep 2026 17:25:58 +0200 Subject: [PATCH 5/5] fix(tests): stop reusing the activities-count fixture for the "only accepted" guard testActivitiesCountForOnlyAcceptedStatusDoesNotWidenPastTheTrueFlags seeded its speaker/submitter via seedActivitiesCountScenario(), whose P3 is unpublished and absent from every list -- which is exactly what has_rejected_presentations considers "rejected". Phase 1's has_rejected_presentations==false requires NOT EXISTS a rejected presentation, so that fixture never matches the filter this test sends, and the count came back 0 regardless of the fix under test. CI caught it (2 failures, both this test in the Speaker and Submitter suites): https://github.com/OpenStackweb/summit-api/actions/runs/35744375018/job/106802031954 Switched both to the same two-published-presentations fixture already used by testActivitiesCountForRejectedFalseCountsEveryPresentationOfTheMatchedSpeaker/Submitter, which has no rejected presentation and is known to satisfy phase 1. Co-Authored-By: Claude Sonnet 5 --- tests/SpeakerRepositoryTest.php | 16 ++++++++++++++-- tests/SubmitterRepositoryTest.php | 13 +++++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/SpeakerRepositoryTest.php b/tests/SpeakerRepositoryTest.php index 496d50123..d0bb3ae75 100644 --- a/tests/SpeakerRepositoryTest.php +++ b/tests/SpeakerRepositoryTest.php @@ -952,9 +952,21 @@ public function testActivitiesCountForOnlyAcceptedStatusDoesNotWidenPastTheTrueF { // Guards the OR-ing of the status group from over-widening: the two == false // companions must stay neutral, not turn into an unrestricted OR branch. - $speaker = $this->seedActivitiesCountScenario('ScenarioOnlyAccepted'); + // + // Can't reuse seedActivitiesCountScenario here: its P3 is unpublished and absent + // from every list, which makes it "rejected" -- a speaker owning it never + // satisfies phase 1's has_rejected_presentations==false (NOT EXISTS a rejected + // presentation of theirs). Needs a speaker with zero rejected presentations, same + // fixture shape as testActivitiesCountForRejectedFalseCountsEveryPresentationOfTheMatchedSpeaker. + $speaker = new PresentationSpeaker(); + $speaker->setFirstName('ScenarioOnlyAccepted'); + $speaker->setLastName('ActivitiesScenario'); + self::$em->persist($speaker); + + $this->seedPresentation($speaker, self::$defaultTrack, 'Accepted A', true); + $this->seedPresentation($speaker, self::$secondaryTrack, 'Accepted B', true); + self::$em->flush(); - // P1 and P2 are published (accepted); P3 is not. $this->assertEquals(2, $this->countActivitiesOf($speaker, [ 'has_rejected_presentations' => 'false', 'has_accepted_presentations' => 'true', diff --git a/tests/SubmitterRepositoryTest.php b/tests/SubmitterRepositoryTest.php index ccc91588f..66ddaac0f 100644 --- a/tests/SubmitterRepositoryTest.php +++ b/tests/SubmitterRepositoryTest.php @@ -1085,9 +1085,18 @@ public function testActivitiesCountForOnlyAcceptedStatusDoesNotWidenPastTheTrueF { // Guards the OR-ing of the status group from over-widening: the two == false // companions must stay neutral, not turn into an unrestricted OR branch. - $submitter = $this->seedActivitiesCountScenario(); + // + // Can't reuse seedActivitiesCountScenario here: its P3 is unpublished and absent + // from every list, which makes it "rejected" -- a submitter owning it never + // satisfies phase 1's has_rejected_presentations==false (NOT EXISTS a rejected + // presentation of theirs). Needs a submitter with zero rejected presentations, + // same fixture shape as testActivitiesCountForRejectedFalseCountsEveryPresentationOfTheMatchedSubmitter. + $submitter = self::$em->find(Member::class, self::$member2->getId()); + + $this->seedPresentation($submitter, self::$defaultTrack, 'Accepted A', true); + $this->seedPresentation($submitter, self::$secondaryTrack, 'Accepted B', true); + self::$em->flush(); - // P1 and P2 are published (accepted); P3 is not. $this->assertEquals(2, $this->countActivitiesOf($submitter, [ 'has_rejected_presentations' => 'false', 'has_accepted_presentations' => 'true',