Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughActivity count queries now apply presentation-level filters for speakers and submitters. New raw SQL mappings provide parameter binding and status handling. Repository, unit, integration, and OAuth2 tests cover filtered counts and combined filter behavior. ChangesActivity count filtering
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to No actionable correctness or runtime issue remains. Performance measurement is still advisable but is not evidence of a current defect. Sequence Diagram(s)sequenceDiagram
participant RequestFilter
participant ActivityCountRepository
participant PresentationDatabase
RequestFilter->>ActivityCountRepository: Convert presentation filters to raw SQL
ActivityCountRepository->>PresentationDatabase: Execute named-bound count query
PresentationDatabase-->>ActivityCountRepository: Return filtered activity count
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 30.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 72 functions across 10 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-599/ This page is automatically updated on each push to this PR. |
78f9854 to
334d416
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-599/ This page is automatically updated on each push to this PR. |
334d416 to
9ab18cf
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-599/ This page is automatically updated on each push to this PR. |
…ion-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) <[email protected]>
9ab18cf to
baae826
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-599/ This page is automatically updated on each push to this PR. |
| $bindings = ['summit_id' => $summit_id]; | ||
|
|
||
| if (!is_null($filter)) { | ||
| $where = $filter->toRawSQL($this->getActivitiesCountFilterMappings()); |
There was a problem hiding this comment.
@romanetar Filter::toRawSQL keeps only the last branch of an OR group when the mappings are FilterMapping instances, so this call under-counts (down to 0) for the filter shapes summit-admin sends.
The OR-group branch of Filter::toRawSQL (app/Http/Utils/Filters/Filter.php:322-323) does $condition = $mapping->toRawSQL($e, $this->bindings) instead of appending. A group like presentations_track_id==5,presentations_type_id==7 renders as (E.TypeID = :param_2), with param_1 left as an orphan binding (DBAL 3.9 drops unused named params silently, so there is no error, just a wrong number). The other two sub-branches of the same block, for array and string mappings, do append with OR; this one is the odd one out. The bug is pre-existing, but this PR is the first path that routes OR groups with FilterMapping instances through it, and both shapes summit-admin sends hit it:
- the search term:
buildTermFilter(summit-admin/src/actions/speaker-actions.js:190-203) joinsfull_name,first_name,last_name,email,presentations_title,presentations_abstractwith,, so thepresentations_titlebranch is always dropped; - the selection-status multi-select (
speaker-actions.js:896-906): "Accepted" + "Alternate" renders only the alternate condition.
Reproduced against the SpeakerRepositoryTest fixture with a speaker owning one published presentation and one rejected one: phase 1 matches 1 speaker in every case, and the count is 0 or 1 depending only on the order of the elements inside the group (has_accepted_presentations==true,has_alternate_presentations==true → 0, swapped → 1; same for the term group with the title matching).
Suggested fix in Filter::toRawSQL, replacing the assignment:
$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;
}The empty guard is for SQLSwitchFilterMapping, which returns '' for a value without a case. The only other caller passing FilterMapping instances to toRawSQL is DoctrineSummitRegistrationPromoCodeRepository::getIdsBySummit; it gains the branches it currently drops, worth a look.
A red test for tests/ActivitiesCountFilterMappingsTest.php (uses the class's existing helpers):
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);
}On the current branch it fails with (E.TypeID = :param_2); with the fix above it passes and the existing 29 tests in the class stay green. Anchored here because Filter.php is not part of this diff.
| AND __mu.SummitMediaUploadTypeID :operator :value | ||
| )' | ||
| ), | ||
| // The == false side of every status filter must not restrict the count: a |
There was a problem hiding this comment.
@romanetar The reasoning in this comment is right for the == false side but stops there: two == true selection-status flags AND'd together make phase 2 demand mutually exclusive statuses of a single presentation, so the count is 0 while phase 1 matches people.
Phase 1 reads has_accepted_presentations==true and has_rejected_presentations==true per person: "has at least one accepted AND has at least one rejected", which can be two different presentations (DoctrineSpeakerRepository::getFilterMappings, the has_* switch mappings). Phase 2 ANDs EXISTS(selected within count) OR E.Published = 1 with E.Published = 0 AND NOT EXISTS(selected) on the same row, and no presentation satisfies both. summit-admin sends exactly that trio for the "Accepted & Rejected" option (summit-admin/src/pages/summit_speakers/summit-speakers-list-page.js:476-477 → src/actions/speaker-actions.js:881-885: has_rejected_presentations==true, has_accepted_presentations==true, has_alternate_presentations==false as three filter[] entries), and the same for "Alternate & Rejected" and "Accepted & Alternate".
Reproduced against the SpeakerRepositoryTest fixture: a speaker with one published presentation and one unpublished, unlisted one; getSpeakersBySummit with the trio returns 1 speaker, getUniqueActivitiesCountBySummit returns 0. The page shows "1 Speaker | 0 Activities".
The only reading that gives a coherent number for a combined-status person is the union: their accepted presentations plus their rejected ones. So the three selection-status flags should be combined among themselves with OR over the == true conditions, with == false staying neutral as it is now, and that group AND'd with the rest of the presentation-level filters. OR-ing all three blindly is wrong: for "Only Accepted" the two 1 = 1 from the false flags would lift the restriction entirely. A sketch in buildActivitiesCountFilter, which I ran against the branch:
$mappings = $this->getActivitiesCountFilterMappings();
$status_mappings = array_intersect_key($mappings, array_flip([
'has_accepted_presentations', 'has_alternate_presentations', 'has_rejected_presentations',
]));
$where = $filter->toRawSQL(array_diff_key($mappings, $status_mappings));
// ... append $where and its bindings exactly as today ...
$status = [];
foreach ($status_mappings as $field => $mapping) {
foreach ($filter->getFilter($field) as $element) {
$c = $mapping->toRawSQL($element); // '( ... )' or '', never binds
if ($c === '' || $c === '( ' . SQLSwitchFilterMapping::NoRestriction . ' )') continue;
$status[] = $c;
}
}
if (!empty($status)) $extra_filters .= ' AND (' . implode(' OR ', $status) . ')';The NoRestriction comparison is the quick form; dropping the 'false' cases from these three mappings and letting '' be skipped is the cleaner one. Because Filter::getFilter also returns the elements of an OR group, this makes has_accepted_presentations==true,has_alternate_presentations==true (the multi-select shape from my other comment) render as accepted OR alternate as well. With this sketch the test below passes and the 29 tests in ActivitiesCountFilterMappingsTest plus the 46 ActivitiesCount tests in both repository suites stay green.
A red test for tests/SpeakerRepositoryTest.php, using the helpers this PR adds:
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',
]));
}On the current branch it fails with 0. Worth pairing it with the "Only Accepted" shape (rejected==false, accepted==true, alternate==false on a speaker with two published presentations, expecting 2) so the union never widens past the == true flags; that one passes today and must keep passing. The same scenario applies to SubmitterRepositoryTest.
| * 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 |
There was a problem hiding this comment.
@romanetar This trade-off goes the wrong way: an OR group with a branch phase 2 cannot express must stop restricting the count, not narrow it to the branches it can express. As written, the summit-admin search term yields "N Speakers | 0 Activities" on every name search.
buildTermFilter (summit-admin/src/actions/speaker-actions.js:190-203, used for the count at :1024-1062; submitter-actions.js:113-114, :160-162) sends one OR group: full_name=@x,first_name=@x,last_name=@x,email=@x,presentations_title=@x,presentations_abstract=@x. A speaker found through their name has no title or abstract containing the term, so with this rule phase 2 counts nothing for them. Reproduced against the SpeakerRepositoryTest fixture: a speaker with three presentations matched through first_name, count 0. The PR's stated goal is that "N Speakers | M Activities" describes one same set, and for the most common path before an email blast it now describes the empty set.
Both rules are approximations, because the exact answer needs to know which branch each person matched through. But they fail in opposite directions: narrowing reads 0 when there are activities, which looks like a broken page; widening over-counts by the presentations of people matched only through a presentation branch, which is what the count did before this PR for that group. For a number whose purpose is to size a blast, the second is the one to keep. Please switch the rule to widening and drop the "inherited limitation" framing: toRawSQL has no caller that needed this before, so nothing inherits it.
The fix belongs in the same OR branch of Filter::toRawSQL as my other comment, behind an opt-in flag so the other callers keep their behaviour:
public function toRawSQL(array $mappings, int $param_idx = 1, bool $skip_partially_mapped_or_groups = false)
...
} else if (is_array($filter)) {
// an array is a OR
if ($skip_partially_mapped_or_groups) {
// a branch without a mapping cannot be expressed here, and an OR group with an
// inexpressible branch cannot restrict, so the whole group is skipped
foreach ($filter as $e) {
if ($e instanceof FilterElement && !isset($mappings[$e->getField()])) continue 2;
}
}
$condition = '';
...and in buildActivitiesCountFilter: $filter->toRawSQL($this->getActivitiesCountFilterMappings(), 1, true). It needs the append fix from my other comment underneath it, or a fully mapped OR group still loses its first branches. I ran both together on this branch: the tests below pass, the 29 tests in ActivitiesCountFilterMappingsTest stay green, and of the 46 ActivitiesCount tests in the two repository suites exactly the two that pin the current rule fail with 3 instead of 1.
Tests: flip testActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranch in both SpeakerRepositoryTest and SubmitterRepositoryTest to expect 3 (every presentation of the matched person) and rename it accordingly; it is red today with 1. Add the summit-admin shape next to it, red today with 0:
public function testActivitiesCountForATermSearchCountsEveryPresentationOfTheMatchedSpeaker(): void
{
// buildTermFilter shape: matched through first_name, no title or abstract contains 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);
}And one guard so widening never leaks into fully mapped groups: id==X AND presentations_track_id==<secondary>,has_published_presentations==true on the scenario speaker must still return 2 (P2 by track, P1 and P2 by published); it passes with the change above.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review
There was a problem hiding this comment.
🟡 Changes recommended
Filter::toRawSQL currently mishandles OR groups for FilterMapping mappings (overwriting instead of OR-ing), which can under-scope phase-2 counts for filters like presentations_track_id==X,presentations_type_id==Y.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR fixes over-counting in the speaker/submitter “activities count” endpoints by applying the same presentation-level filters to phase 2 (the raw-SQL counting step) as are used in phase 1 (the DQL “who matches” step), so the returned “Activities” number describes the same filtered presentation set targeted by the request.
Changes:
- Introduces raw-SQL filter mappings (
SQLRawFilterMapping,SQLSwitchFilterMapping) and a sharedActivitiesCountFilterMappingsTraitto translate request filters into phase-2 SQL predicates + bindings. - Updates
DoctrineSpeakerRepositoryandDoctrineMemberRepositoryphase-2 queries to append the derived predicate fragment (and switch to named parameter bindings). - Adds unit/integration/API test coverage for the corrected scoping behavior and wires the new unit test into the GitHub Actions test shard.
File summaries
| File | Description |
|---|---|
app/Repositories/Summit/Traits/ActivitiesCountFilterMappingsTrait.php |
Defines shared phase-2 presentation-level filter mappings and builds the raw-SQL WHERE fragment + bindings. |
app/Http/Utils/Filters/SQL/SQLRawFilterMapping.php |
New FilterMapping that renders :operator / :value conditions and binds values as named params. |
app/Http/Utils/Filters/SQL/SQLSwitchFilterMapping.php |
New FilterMapping that selects literal SQL conditions by filter value and ORs multi-values. |
app/Repositories/Summit/DoctrineSpeakerRepository.php |
Applies phase-2 extra filters to both speaker-role and moderator-role inserts into the temp table. |
app/Repositories/Summit/DoctrineMemberRepository.php |
Applies phase-2 extra filters to the submitter(created_by)-based count query. |
tests/ActivitiesCountFilterMappingsTest.php |
Adds unit tests for the new SQL mappings and anti-drift checks between phase-1 and phase-2 mappings. |
tests/SpeakerRepositoryTest.php |
Adds repository-level acceptance scenario tests ensuring counts are scoped by presentation filters. |
tests/SubmitterRepositoryTest.php |
Adds repository-level acceptance scenario tests ensuring counts are scoped by presentation filters. |
tests/oauth2/OAuth2SummitSpeakersApiTest.php |
Adds API-level assertions that endpoint counts respect presentation-level filters. |
tests/oauth2/OAuth2SummitSubmittersApiTest.php |
Adds API-level assertions that endpoint counts respect presentation-level filters. |
.github/workflows/push.yml |
Registers the new unit test in the existing test matrix shard so it runs in CI. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * 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 | ||
| * repository test suites. |
ref https://app.clickup.com/t/86bbv67m1
Problem
GET /api/v1/summits/{id}/speakers/all/events/countandGET /api/v1/summits/{id}/submitters/all/events/countrun in two phases (the temp-table structure from #563):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 forpresentations_track_id,presentations_type_id,has_published_presentations,has_media_upload_with_typeand for published+track combined — all five.On dev summit 73 this reads as "768 Speakers | 707 Activities" for
has_published_presentations==truewhile only 511 activities are published in total. The number on the summit-admin speakers/submitters page therefore did not describe the set of activities the email blast targets. This is what 86b9b1qrk asked for and what #543 / #563 never implemented.Solution
Phase 2 derives its
WHEREfrom the sameFilterobject throughFilter::toRawSQL, the primitive that already walks the parsed AND/OR structure, dispatches per field mapping, handles multi-value elements (&&/||) and binds the values as named parameters:Two
FilterMappingimplementations were missing for raw SQL and are added beside the existingSQLInFilterMapping/SQLNotInFilterMappinginapp/Http/Utils/Filters/SQL/:SQLRawFilterMappingDoctrineFilterMapping:operator/:value, binding each value instead of interpolating it, honouringgetSameFieldOp()for multi-value elementsSQLSwitchFilterMappingDoctrineSwitchFilterMappingActivitiesCountFilterMappingsTraitdeclares the fourteen presentation-level conditions, expressed against the physical presentation row rather than against the person, with the semantics copied from the phase-1 DQL (not re-derived), and exposesbuildActivitiesCountFilter— the single method above, so neither repository repeats the wiring. Bothuseit, since the conditions correlate to the presentation and not to the role the person plays on it:DoctrineSpeakerRepository::getUniqueActivitiesCountBySummit— in bothINSERT ... SELECTstatements (speaker role viaPresentation_Speakers, moderator role viaPresentation.ModeratorID). They stay separate because MySQL error 1137 forbids referencing the same temporary table twice in one statement. The speaker-role statement gainsINNER JOIN Presentation P, which it did not have, soP.SelectionPlanIDis reachable; the join is non-restrictive (everyPresentation_Speakersrow points at aPresentation).DoctrineMemberRepository::getUniqueActivitiesCountBySummit— in thecreated_bystatement.The selection-status semantics:
has_published_presentations==trueE.Published = 1has_accepted_presentations==truePresentationCategory.SessionCountin a Group/Session list,OR E.Published = 1has_alternate_presentations==trueSessionCountin a Group/Session listhas_rejected_presentations==trueE.Published = 0and absent from every Group/Session selected list (no order comparison, as in the mapping)has_*_presentations==false1 = 1The
== falseside does not restrict because a person matched by it has no presentation with that status among the presentations that pass the remaining presentation-level filters — which holds only because those are applied here too. It is spelled out as1 = 1rather than left absent so that a multi-value element such as==true||falsestill evaluates to true, the way the Doctrine switch mapping does.Person-level filters, and one known limitation
Person-level filters (
id,not_id,first_name,last_name,email,full_name,member_id,member_user_external_id,is_speaker) have no phase-2 mapping, sotoRawSQLskips them and the count stays unrestricted for them. For a slot joined byANDthat is exactly right — a slot that does not restrict contributes nothing to an AND chain.Inside an OR group that same skip drops a branch instead of widening it:
full_name==x,presentations_track_id==Ncounts only the track N presentations, even though phase 1 also matched people throughfull_name. Expressing the correct rule needs a per-person predicate, which no set-level condition can carry. This is the semantics every othertoRawSQLcaller in the codebase already lives with; the trait documents it andtestActivitiesCountWithAnOredPersonLevelFilterKeepsThePresentationBranchpins the resulting count in both repository suites.Filteritself is untouched. An earlier revision of this PR added accessors to it and re-implemented the traversal in a dedicated builder; that was 445 lines re-doing whattoRawSQLalready does, and it is gone.Tests
186 tests / 915 assertions, all green. Run inside the container (
docker compose exec app) — from the host theredis/db_modelhostnames do not resolve.tests/ActivitiesCountFilterMappingsTest.php(new)tests/SpeakerRepositoryTest.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.phpThe acceptance scenario — P1 (track A, type T1, published, media upload M), P2 (track B, T1, published), P3 (track A, T2, unpublished) — returns 2 / 1 / 2 / 1 / 1 at the repository layer and through both endpoints, for speaker and submitter. On
mainall five return 3.Also covered: the
== falseside of every status filter returns the same count as before, the unfiltered count is unchanged, multi-value filters, the title filter, parameter numbering continuing across mappings, and that every returned binding has its placeholder in the statement.tests/ActivitiesCountFilterMappingsTest.phpalso guards against drift in three directions: every presentation-level filter of phase 1 has a phase-2 mapping, every phase-2 mapping exists in phase 1, and no phase-2 mapping is person-level. It reads both mapping methods off an instance built without its constructor, so it needs no entity manager.It is registered in the
SpeakerSubmitterPublishedFiltershard in.github/workflows/push.yml— no job runs thetests/root, so a file added there runs nowhere unless it is listed.Two expectations from the original plan were wrong and were corrected against the real phase-1 behaviour rather than by changing the code: with
has_not_media_upload_with_type==Mthe scenario speaker does not match at all (P1 carries media M), and withhas_rejected_presentations==falsethey do not either (their unpublished presentations outside every selected list are rejected). Each became its own test with an appropriate subject.Not verified
The performance criterion is unverified. The ticket asks for under 1 second on dev summit 73 with
has_published_presentations==true(the #563 baseline). I have no access to that database, and the local fixtures are far too small for a timing to mean anything. The track / type / published / selection-plan conditions hit columns ofSummitEventandPresentation, both already joined; the media-upload and selected-list conditions add correlated subqueries, and that is where to look. This needs a measurement on dev before merging.Release note
The activities figure shown in production for the existing selection-status filters goes down, because it was over-counted. The speaker/submitter count itself does not change. The CFP admins should be told.
No summit-admin change is needed: it already consumes the endpoint at
src/actions/speaker-actions.js:946and1013andsrc/actions/submitter-actions.js:65and131, so the displayed number corrects itself. The dashboard "Published Activities" figure (Summit::getPublishedEventsCountcounts every publishedSummitEvent, not only presentations with speakers) is still not expected to equal this count.Out of scope
Which speakers/submitters match a filter (phase 1) is unchanged.
🤖 Generated with Claude Code