Hotfix/speakers activity count tweaks#563
Open
smarcet wants to merge 13 commits into
Open
Conversation
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a two-phase pagination pattern (count → page IDs → hydrate entities) for speakers and summit submitters, and adds regression tests around speaker pagination/filtering/ordering as part of the “speakers activity count tweaks” hotfix work.
Changes:
- Refactor
DoctrineSpeakerRepository::getAllByPageto a two-phase approach with a shared base QueryBuilder and eager-loading of related entities. - Refactor
DoctrineMemberRepository::getSubmittersBySummitto a two-phase approach and update submitter unique-activities counting logic. - Add
SpeakerRepositoryTestregression coverage for pagination, filtering (includingnot_id), and ordering.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/SpeakerRepositoryTest.php | Adds regression tests for DoctrineSpeakerRepository::getAllByPage pagination/filter/order behavior. |
| app/Repositories/Summit/DoctrineSpeakerRepository.php | Introduces shared base QB + two-phase pagination for speakers; adjusts unique activities count query structure. |
| app/Repositories/Summit/DoctrineMemberRepository.php | Introduces shared base QB + two-phase pagination for summit submitters; changes unique activities counting implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughBoth ChangesTwo-Phase Paging Refactor
Track Group Filter Support
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getAllByPage as getAllByPage / getSubmittersBySummit
participant FastCount as getFastCount / getSubmittersFastCount
participant IDsPage as getAllIdsByPage / getSubmittersIdsByPage
participant Doctrine as Doctrine EntityManager
Caller->>getAllByPage: paging_info, filter, order
getAllByPage->>FastCount: filter, order → total (COUNT DISTINCT id)
getAllByPage->>IDsPage: paging_info, filter, order → ids[]
alt ids is empty
getAllByPage-->>Caller: PagingResponse(total=0, [])
else ids non-empty
getAllByPage->>Doctrine: SELECT e WHERE e.id IN (ids)
Doctrine-->>getAllByPage: unordered entities
getAllByPage->>getAllByPage: restore order to match ids[]
getAllByPage-->>Caller: PagingResponse(total, ordered entities)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
767-785:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
:summitparameter when applying filters that depend on it.The phase-1 query sets
:summit_id(line 777) but not:summit. However, several filter mappings (e.g.,presentations_track_id,presentations_selection_plan_id,has_accepted_presentations) contain DQL fragments like__p41_:i.summit = :summitthat expect a:summitparameter to be bound.When such filters are applied to
speakerIdsQb, the generated DQL will contain unbound:summitplaceholders, causing a runtime error.🐛 Proposed fix
->setParameter('summit_id', $summit->getId()); + // Bind :summit for filter mappings that require the entity reference + $speakerIdsQb->setParameter('summit', $summit); if (!is_null($filter)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` around lines 767 - 785, The phase-1 query in the buildSpeakerBaseQuery section sets the `:summit_id` parameter but does not set the `:summit` parameter. When filter->apply2Query() is called on speakerIdsQb, filter mappings such as presentations_track_id, presentations_selection_plan_id, and has_accepted_presentations inject DQL fragments that reference the `:summit` placeholder, which causes a runtime error because this parameter is never bound to the QueryBuilder. Add a setParameter call to bind `:summit` to the summit object or its identifier (consistent with how other summit parameters are bound in this query) before the filter->apply2Query() invocation on speakerIdsQb.
🧹 Nitpick comments (2)
app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
638-638: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUnused
$orderparameter.The
$orderparameter is declared but never used in this method. Since counting doesn't require ordering, this parameter could be removed, or documented if kept for API symmetry withgetAllIdsByPage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` at line 638, The getFastCount method in DoctrineSpeakerRepository has an unused $order parameter that is never referenced in the method body. Remove the $order parameter from the method signature since counting operations do not require ordering, or if you need to keep it for API consistency with getAllIdsByPage, add a PHPDoc comment explaining why it is retained despite not being used.Source: Linters/SAST tools
app/Repositories/Summit/DoctrineMemberRepository.php (1)
618-620: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAppend an ID tie-breaker for ordered ID pages.
When callers order by non-unique fields like
first_nameorlast_name, offset paging can duplicate or skip submitters between pages. Adde.id ASCunless the caller already ordered byid.Proposed change
- if (!is_null($order)) $order->apply2Query($qb, $this->getOrderMappings()); - else $qb->addOrderBy('e.id', 'ASC'); + if (!is_null($order)) { + $order->apply2Query($qb, $this->getOrderMappings()); + if (!$order->hasOrder('id')) { + $qb->addOrderBy('e.id', 'ASC'); + } + } else { + $qb->addOrderBy('e.id', 'ASC'); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineMemberRepository.php` around lines 618 - 620, The current code only applies a default id ordering when no custom order is provided, but when a custom order is applied via apply2Query on the order object, no tie-breaker is added for handling pagination of non-unique fields. After the if-else block that applies the order (whether from the caller via apply2Query or the default), add a secondary order by e.id ASC unless the caller has already ordered by id. Check if e.id is already present in the query builder's ordering criteria before adding it, and only append e.id ASC as a tie-breaker if it is not already included to ensure consistent pagination across pages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Repositories/Summit/DoctrineMemberRepository.php`:
- Around line 676-678: The method getSubmittersIdsBySummit in the
DoctrineMemberRepository class currently returns a raw array from
getSubmittersIdsByPage, but the interface contract IMemberRepository specifies
this method should return a PagingResponse object. Modify the return statement
in getSubmittersIdsBySummit to wrap the result in a PagingResponse object,
ensuring pagination metadata like getTotal() and toArray() are preserved for
consumers of this method.
- Around line 689-709: The current implementation materializes member IDs into a
PHP array via array_column() and passes them as parameters to the outer query,
which can exceed parameter limits on large summits. Instead, refactor to use a
subquery pattern by embedding the phase-1 DQL directly into the phase-2 query's
WHERE clause. Remove the array_column() call that populates the memberIds
variable and the empty check that returns early. Modify the andWhere() clause in
the phase-2 query to use a DQL IN subquery (reference the memberIdsQb
QueryBuilder's getDQL() output) instead of passing an array parameter. Finally,
copy all parameters from the phase-1 query to the phase-2 query using
addParameters() and the phase-1 QueryBuilder's getParameters() method to ensure
all filter conditions and summit parameter are preserved.
---
Outside diff comments:
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 767-785: The phase-1 query in the buildSpeakerBaseQuery section
sets the `:summit_id` parameter but does not set the `:summit` parameter. When
filter->apply2Query() is called on speakerIdsQb, filter mappings such as
presentations_track_id, presentations_selection_plan_id, and
has_accepted_presentations inject DQL fragments that reference the `:summit`
placeholder, which causes a runtime error because this parameter is never bound
to the QueryBuilder. Add a setParameter call to bind `:summit` to the summit
object or its identifier (consistent with how other summit parameters are bound
in this query) before the filter->apply2Query() invocation on speakerIdsQb.
---
Nitpick comments:
In `@app/Repositories/Summit/DoctrineMemberRepository.php`:
- Around line 618-620: The current code only applies a default id ordering when
no custom order is provided, but when a custom order is applied via apply2Query
on the order object, no tie-breaker is added for handling pagination of
non-unique fields. After the if-else block that applies the order (whether from
the caller via apply2Query or the default), add a secondary order by e.id ASC
unless the caller has already ordered by id. Check if e.id is already present in
the query builder's ordering criteria before adding it, and only append e.id ASC
as a tie-breaker if it is not already included to ensure consistent pagination
across pages.
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Line 638: The getFastCount method in DoctrineSpeakerRepository has an unused
$order parameter that is never referenced in the method body. Remove the $order
parameter from the method signature since counting operations do not require
ordering, or if you need to keep it for API consistency with getAllIdsByPage,
add a PHPDoc comment explaining why it is retained despite not being used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: beea6135-659b-4679-b2ac-568e34ad4398
📒 Files selected for processing (3)
app/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phptests/SpeakerRepositoryTest.php
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
4 similar comments
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php (1)
568-595: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate OpenAPI filter docs to match the new accepted field.
Line 568 and Line 595 add
presentations_track_group_idsupport, but the filterable-fields description for this endpoint (Line 526) still omits it. That creates a contract mismatch for API consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php` around lines 568 - 595, The OpenAPI documentation for the filterable fields at line 526 does not include the presentations_track_group_id field, but the code validation at lines 568 and 595 now accepts this filter parameter. Update the filterable-fields description in the OpenAPI documentation to include presentations_track_group_id so the API contract matches the actual implementation and correctly documents all supported filter parameters for API consumers.app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
605-613: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the name sort fallback order.
first_name/last_namesort by member fields first, whilefull_namesorts by speaker fields first. For linked speakers with profile overrides, the same row can sort under different names depending on the requested order. Use the same precedence as the displayed speaker name.Proposed fix
"first_name" => <<<SQL -COALESCE(LOWER(m.first_name), LOWER(e.first_name)) +COALESCE(LOWER(e.first_name), LOWER(m.first_name)) SQL, "last_name" => <<<SQL -COALESCE(LOWER(m.last_name), LOWER(e.last_name)) +COALESCE(LOWER(e.last_name), LOWER(m.last_name)) SQL, "full_name" => <<<SQL -COALESCE(LOWER(CONCAT(e.first_name, ' ', e.last_name)), LOWER(CONCAT(m.first_name, ' ', m.last_name))) +CONCAT(COALESCE(LOWER(e.first_name), LOWER(m.first_name)), ' ', COALESCE(LOWER(e.last_name), LOWER(m.last_name))) SQL,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` around lines 605 - 613, The COALESCE precedence is inconsistent across the name sort fields in the DoctrineSpeakerRepository. The first_name and last_name fields prioritize member fields first (m.first_name, m.last_name), while the full_name field prioritizes email/speaker fields first (e.first_name, e.last_name). Align all three fields to use the same fallback order by ensuring the COALESCE statements in first_name, last_name, and full_name use consistent field precedence that matches how the speaker name is actually displayed in the application.app/Repositories/Summit/DoctrineMemberRepository.php (1)
636-640: 🩺 Stability & Availability | 🟠 MajorSelect the ordered fields in the distinct ID phase.
This ID query selects only
DISTINCT e.id, then applies non-ID order mappings. Ordering by fields such asfirst_name,last_name, orfull_namewill fail in MySQL 8.0 (which enablesONLY_FULL_GROUP_BYby default), causing the submitters endpoint to error for supported order fields.🐛 Proposed fix
- $qb = $this->buildSubmitterBaseQuery($summit)->distinct(true)->select('e.id'); + $qb = $this->buildSubmitterBaseQuery($summit) + ->distinct(true) + ->select('e.id AS id') + ->addSelect('e.first_name AS HIDDEN order_first_name') + ->addSelect('e.last_name AS HIDDEN order_last_name') + ->addSelect("LOWER(CONCAT(e.first_name, ' ', e.last_name)) AS HIDDEN order_full_name") + ->addSelect('e.email AS HIDDEN order_email') + ->addSelect('e.created AS HIDDEN order_created') + ->addSelect('e.last_edited AS HIDDEN order_last_edited') + ->addSelect('e.membership_type AS HIDDEN order_membership_type'); $qb = $this->applyExtraJoins($qb, $filter, $order); if (!is_null($filter)) $filter->apply2Query($qb, $this->getFilterMappings($filter)); - if (!is_null($order)) $order->apply2Query($qb, $this->getOrderMappings()); + if (!is_null($order)) { + $order->apply2Query($qb, [ + 'id' => 'e.id', + 'first_name' => 'order_first_name', + 'last_name' => 'order_last_name', + 'full_name' => 'order_full_name', + 'email' => 'order_email', + 'created' => 'order_created', + 'last_edited' => 'order_last_edited', + 'membership_type' => 'order_membership_type', + ]); + } else $qb->addOrderBy('e.id', 'ASC');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineMemberRepository.php` around lines 636 - 640, The distinct ID query is selecting only e.id but then applying order mappings that reference fields like first_name, last_name, or full_name which aren't in the SELECT clause, causing MySQL 8.0 ONLY_FULL_GROUP_BY violations. Modify the query builder initialization to include the order fields in the SELECT statement before applying order2Query. Examine what fields getOrderMappings() will use for ordering and ensure those fields are added to the initial select() call when building the submitter base query, so that all fields referenced in the ORDER BY clause are present in the SELECT statement.
🧹 Nitpick comments (1)
tests/oauth2/OAuth2SummitSubmittersApiTest.php (1)
304-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd matching smoke tests for the other three updated endpoints.
This test validates
getAllBySummit, but this PR also updates filter validation ingetAllBySummitCSV,send, andgetSubmittersActivitiesCount. Adding parallel 200/not-422 checks for those endpoints would guard against validation-map drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitSubmittersApiTest.php` around lines 304 - 337, The test testGetSubmittersFilterByTrackGroupId validates that filtering by presentations_track_group_id returns HTTP 200 for the getAllBySummit endpoint, but three other endpoints were also updated with the same filter validation changes: getAllBySummitCSV, send, and getSubmittersActivitiesCount. Add three new parallel smoke test methods following the same pattern as testGetSubmittersFilterByTrackGroupId, each calling the corresponding controller action and verifying that the presentations_track_group_id filter returns HTTP 200 instead of HTTP 422, ensuring consistency across all four updated endpoints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`:
- Around line 568-595: The OpenAPI documentation for the filterable fields at
line 526 does not include the presentations_track_group_id field, but the code
validation at lines 568 and 595 now accepts this filter parameter. Update the
filterable-fields description in the OpenAPI documentation to include
presentations_track_group_id so the API contract matches the actual
implementation and correctly documents all supported filter parameters for API
consumers.
In `@app/Repositories/Summit/DoctrineMemberRepository.php`:
- Around line 636-640: The distinct ID query is selecting only e.id but then
applying order mappings that reference fields like first_name, last_name, or
full_name which aren't in the SELECT clause, causing MySQL 8.0
ONLY_FULL_GROUP_BY violations. Modify the query builder initialization to
include the order fields in the SELECT statement before applying order2Query.
Examine what fields getOrderMappings() will use for ordering and ensure those
fields are added to the initial select() call when building the submitter base
query, so that all fields referenced in the ORDER BY clause are present in the
SELECT statement.
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 605-613: The COALESCE precedence is inconsistent across the name
sort fields in the DoctrineSpeakerRepository. The first_name and last_name
fields prioritize member fields first (m.first_name, m.last_name), while the
full_name field prioritizes email/speaker fields first (e.first_name,
e.last_name). Align all three fields to use the same fallback order by ensuring
the COALESCE statements in first_name, last_name, and full_name use consistent
field precedence that matches how the speaker name is actually displayed in the
application.
---
Nitpick comments:
In `@tests/oauth2/OAuth2SummitSubmittersApiTest.php`:
- Around line 304-337: The test testGetSubmittersFilterByTrackGroupId validates
that filtering by presentations_track_group_id returns HTTP 200 for the
getAllBySummit endpoint, but three other endpoints were also updated with the
same filter validation changes: getAllBySummitCSV, send, and
getSubmittersActivitiesCount. Add three new parallel smoke test methods
following the same pattern as testGetSubmittersFilterByTrackGroupId, each
calling the corresponding controller action and verifying that the
presentations_track_group_id filter returns HTTP 200 instead of HTTP 422,
ensuring consistency across all four updated endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40b1998f-faa3-424f-80d9-d5af01291083
📒 Files selected for processing (6)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phptests/SpeakerRepositoryTest.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
smarcet
force-pushed
the
hotfix/speakers-activity-count-tweaks
branch
from
fbb4134 to
28d1fc0
Compare
2 tasks
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
5 similar comments
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
smarcet
force-pushed
the
hotfix/speakers-activity-count-tweaks
branch
from
8d5f141 to
2d2766b
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
…base query - Extract buildSpeakerBaseQuery() as the single place that defines the PresentationSpeaker/Member/RegistrationRequest join aliases used by all filter and order mappings. - Add getFastCount() and getAllIdsByPage() overrides following the same pattern as DoctrineSummitEventRepository. - Rewrite getAllByPage() with the two-phase approach: Phase 1 gets paged IDs via getAllIdsByPage(), Phase 2 fetches full entities WHERE id IN (:ids) with eager-loaded member and registration_request. Replaces ~130 lines of raw SQL + ResultSetMappingBuilder. - Simplify getUniqueActivitiesCountBySummit() to reuse buildSpeakerBaseQuery() instead of duplicating the from/leftJoin setup. - not_id filter was silently ignored by the old toRawSQL() mapping; now correctly applied via getFilterMappings(). - Add SpeakerRepositoryTest with 5 regression tests covering pagination, first_name filter, id filter, not_id exclusion, and first_name ordering.
- Extract buildSubmitterBaseQuery(Summit): shared base QB scoping members to those with at least one presentation in the given summit - Add getSubmittersFastCount(Summit, Filter, Order): COUNT(DISTINCT e.id) - Add getSubmittersIdsByPage(Summit, PagingInfo, Filter, Order): Phase 1 returning a paginated, ordered array of member IDs - Refactor getSubmittersBySummit() to two-phase: Phase 1 IDs + Phase 2 entity fetch by IN (:ids), restoring original order after hydration - Simplify getSubmittersIdsBySummit() to delegate to getSubmittersIdsByPage() - Refactor getUniqueActivitiesCountBySummit() to two-phase: Phase 1 gets all matching member IDs (no pagination), Phase 2 counts presentations via IN (:member_ids) - eliminates alias collision risk vs. DQL embedding Existing SubmitterRepositoryTest (3 tests, 11 assertions) all pass.
… in getUniqueActivitiesCountBySummit Both Speaker and Member repos were running a correlated EXISTS wrapping a nested IN(SELECT DISTINCT ...) subquery, resulting in O(presentations x speakers) execution — measured at 12-14s on dev data. New approach in both repos: - Phase 1: DQL QB (same filter system) executed in chunks of 1000 rows via LIMIT/OFFSET; each chunk is INSERT IGNOREd into a session-scoped MEMORY temp table (__tmp_spk_ids / __tmp_mbr_ids). PHP never holds the full ID set in memory; MySQL keeps IDs in an indexed MEMORY table. - Phase 2: raw SQL JOIN against the temp table — no correlated subqueries, no IN array passed over the wire, MySQL uses primary key index on the temp table. - try/finally guarantees DROP TEMPORARY TABLE even on exception. Speaker Phase 2: UNION of assignment-based and moderator-based counts, deduplicating across both roles in a single round-trip. Member Phase 2: single JOIN on SummitEvent.CreatedByID. 8 regression tests (5 Speaker + 3 Submitter) all pass.
MySQL forbids referencing a TEMPORARY TABLE more than once in a single SQL statement (error 1137 'Can't reopen table'). The previous UNION query touched __tmp_spk_ids in both branches. Fix: split Phase 2 into two separate INSERT IGNORE statements (one reference each) feeding a second MEMORY temp table __tmp_pres_ids, then count from that. Both temp tables are dropped in the finally block.
Filter mappings (e.g. presentations_selection_plan_id) reference the :summit parameter as a Doctrine entity in their EXISTS subqueries. Phase 1 was only binding :summit_id (integer), leaving :summit unbound when any presentation-scoped filter was applied -> HTTP 500.
…oryTest Regression tests for the MySQL 1137 fix and filter-with-no-match edge case.
Using two summit params (:summit_id integer + :summit entity) caused DoctrineFilterMapping's counter-based :value_N naming to produce a parameter the query executor counted but couldn't match. Switch to a single :summit entity parameter matching the pattern all other speaker-repo methods use and that all filter mappings expect.
Port the presentations_track_group_id filter from speakers to submitters. - OAuth2SummitSubmittersApiController: add operator ['=='] and validation rule 'sometimes|integer' in all four public methods (getAllBySummit, getAllBySummitCSV, send, getSubmittersActivitiesCount) - DoctrineMemberRepository: add preamble block that injects a category-group subquery into $extraSelectionStatusFilter, $extraSelectionPlanFilter, and $extraMediaUploadFilter; add DoctrineFilterMapping entry with alias prefix 42_ - SubmitterRepositoryTest: testGetSubmittersByTrackGroupId asserts DQL filters correctly (member2 in, member out) - OAuth2SummitSubmittersApiTest: testGetSubmittersFilterByTrackGroupId asserts HTTP 200 instead of 422
…peaker entity first
…repos Adds 11 new regression tests covering: SpeakerRepositoryTest (getUniqueActivitiesCountBySummit): - presentations_track_group_id filter (new in this PR, was zero coverage) - unknown track group returns 0 - presentations_track_id filter - presentations_selection_plan_id filter - has_accepted_presentations==true - combined has_accepted + track_group_id (exercises extraSelectionStatusFilter injection) - combined has_accepted + unknown track_group_id returns 0 (injection not a no-op) SubmitterRepositoryTest (getSubmittersBySummit): - presentations_track_id filter - presentations_selection_plan_id filter - has_accepted_presentations==true - combined has_accepted + presentations_track_id (exercises extraSelectionStatusFilter injection)
FilterParser::parse expects a flat indexed array of filter strings when multiple filters are combined. Passing them nested inside a 'filter' key caused preg_match to receive an array instead of a string.
…ter gaps - SpeakerRepositoryTest: add moderator-only presentation test to exercise the Presentation.ModeratorID INSERT in getUniqueActivitiesCountBySummit Phase 2 (previously untested code path) - SpeakerRepositoryTest: add multi-page pagination test for getAllByPage two-phase refactor (LIMIT/OFFSET for page > 1) - SubmitterRepositoryTest: add getUniqueActivitiesCountBySummit test for presentations_track_group_id filter (submitter path uses __tmp_mbr_ids; speaker path was already covered) - SubmitterRepositoryTest: add multi-page pagination test for getSubmittersBySummit - SubmitterRepositoryTest: add empty-result test covering the early-exit path when getSubmittersIdsByPage returns no IDs All 28 tests pass.
smarcet
force-pushed
the
hotfix/speakers-activity-count-tweaks
branch
from
2d2766b to
ee3b3be
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-563/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/SpeakerRepositoryTest.php (1)
357-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen pagination coverage assertion for two-page split.
The test currently checks disjointness and consistent totals, but not that both pages together cover the full result set. Add a union-size assertion to catch dropped IDs across page boundaries.
Suggested test assertion addition
$ids1 = array_map(fn($s) => $s->getId(), $page1->getItems()); $ids2 = array_map(fn($s) => $s->getId(), $page2->getItems()); $this->assertNotEmpty($ids1, 'Page 1 must not be empty'); $this->assertNotEmpty($ids2, 'Page 2 must not be empty'); $this->assertEmpty(array_intersect($ids1, $ids2), 'Pages must be disjoint'); + $this->assertEquals( + $total, + count(array_unique(array_merge($ids1, $ids2))), + 'Page 1 + Page 2 should cover the full result set' + ); $this->assertEquals($total, $page1->getTotal(), 'Total must be consistent across pages'); $this->assertEquals($total, $page2->getTotal());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/SpeakerRepositoryTest.php` around lines 357 - 364, The pagination test is missing a validation that both pages together cover the complete result set without gaps. After the existing assertions (assertNotEmpty for both pages, assertEmpty for array_intersect to verify disjointness, and assertEquals for consistent totals), add an assertion that verifies the combined count of IDs from both pages equals the total count returned by getTotal(). This will catch cases where records are dropped or missed across page boundaries during pagination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 666-670: The current code only adds e.id as a tie-breaker when no
ordering is specified, but this leaves non-unique orderings vulnerable to
pagination issues when callers order by fields like first_name or email. After
the $order->apply2Query() call in the if block (when $order is not null), add a
secondary sort on e.id ASC to serve as a deterministic tie-breaker, ensuring
this is only added if id is not already part of the caller's ordering. The else
block can remain unchanged since it already defaults to e.id ASC when no
ordering is provided.
- Around line 606-612: The COALESCE expressions in the first_name, last_name,
and full_name fields only check for NULL values but do not handle empty strings.
When speaker names (e.first_name, e.last_name) contain empty strings instead of
NULL, they will be used in the sort instead of falling back to member names
(m.first_name, m.last_name), causing inconsistency with the API serialization.
Modify each COALESCE expression to treat empty strings as NULL by wrapping each
e.first_name, e.last_name occurrence with NULLIF(field, '') so that empty
strings are converted to NULL before the COALESCE evaluation occurs.
---
Nitpick comments:
In `@tests/SpeakerRepositoryTest.php`:
- Around line 357-364: The pagination test is missing a validation that both
pages together cover the complete result set without gaps. After the existing
assertions (assertNotEmpty for both pages, assertEmpty for array_intersect to
verify disjointness, and assertEquals for consistent totals), add an assertion
that verifies the combined count of IDs from both pages equals the total count
returned by getTotal(). This will catch cases where records are dropped or
missed across page boundaries during pagination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2493d4a-66eb-43de-9131-5d7a1a09b8a6
📒 Files selected for processing (7)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phptests/SpeakerRepositoryTest.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
✅ Files skipped from review due to trivial changes (2)
- app/Models/Foundation/Main/Repositories/IMemberRepository.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/oauth2/OAuth2SummitSubmittersApiTest.php
- app/Repositories/Summit/DoctrineMemberRepository.php
- tests/SubmitterRepositoryTest.php
Comment on lines
+606
to
612
| COALESCE(LOWER(e.first_name), LOWER(m.first_name)) | ||
| SQL, | ||
| "last_name" => <<<SQL | ||
| COALESCE(LOWER(m.last_name), LOWER(e.last_name)) | ||
| COALESCE(LOWER(e.last_name), LOWER(m.last_name)) | ||
| SQL, | ||
| "full_name" => <<<SQL | ||
| COALESCE(LOWER(CONCAT(e.first_name, ' ', e.last_name)), LOWER(CONCAT(m.first_name, ' ', m.last_name))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the serialized name fallback when sorting.
PresentationSpeaker falls back to member names when speaker names are empty, but these COALESCE expressions only fall back on NULL. Speakers with '' in e.first_name/e.last_name can sort as blank while the API returns the member name.
🐛 Proposed fix
"first_name" => <<<SQL
-COALESCE(LOWER(e.first_name), LOWER(m.first_name))
+LOWER(COALESCE(NULLIF(e.first_name, ''), m.first_name))
SQL,
"last_name" => <<<SQL
-COALESCE(LOWER(e.last_name), LOWER(m.last_name))
+LOWER(COALESCE(NULLIF(e.last_name, ''), m.last_name))
SQL,
"full_name" => <<<SQL
-COALESCE(LOWER(CONCAT(e.first_name, ' ', e.last_name)), LOWER(CONCAT(m.first_name, ' ', m.last_name)))
+LOWER(CONCAT(COALESCE(NULLIF(e.first_name, ''), m.first_name, ''), ' ', COALESCE(NULLIF(e.last_name, ''), m.last_name, '')))
SQL,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| COALESCE(LOWER(e.first_name), LOWER(m.first_name)) | |
| SQL, | |
| "last_name" => <<<SQL | |
| COALESCE(LOWER(m.last_name), LOWER(e.last_name)) | |
| COALESCE(LOWER(e.last_name), LOWER(m.last_name)) | |
| SQL, | |
| "full_name" => <<<SQL | |
| COALESCE(LOWER(CONCAT(e.first_name, ' ', e.last_name)), LOWER(CONCAT(m.first_name, ' ', m.last_name))) | |
| "first_name" => <<<SQL | |
| LOWER(COALESCE(NULLIF(e.first_name, ''), m.first_name)) | |
| SQL, | |
| "last_name" => <<<SQL | |
| LOWER(COALESCE(NULLIF(e.last_name, ''), m.last_name)) | |
| SQL, | |
| "full_name" => <<<SQL | |
| LOWER(CONCAT(COALESCE(NULLIF(e.first_name, ''), m.first_name, ''), ' ', COALESCE(NULLIF(e.last_name, ''), m.last_name, ''))) | |
| SQL, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` around lines 606 -
612, The COALESCE expressions in the first_name, last_name, and full_name fields
only check for NULL values but do not handle empty strings. When speaker names
(e.first_name, e.last_name) contain empty strings instead of NULL, they will be
used in the sort instead of falling back to member names (m.first_name,
m.last_name), causing inconsistency with the API serialization. Modify each
COALESCE expression to treat empty strings as NULL by wrapping each
e.first_name, e.last_name occurrence with NULLIF(field, '') so that empty
strings are converted to NULL before the COALESCE evaluation occurs.
Comment on lines
+666
to
+670
| if (!is_null($order)) { | ||
| $order->apply2Query($query, $this->getOrderMappings()); | ||
| } else { | ||
| $query->addOrderBy('e.id', 'ASC'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a deterministic tie-breaker for ordered paging.
When callers order by non-unique fields like first_name, last_name, full_name, or email, LIMIT/OFFSET can duplicate or skip speakers across pages. Add e.id ASC after caller ordering unless id is already ordered.
🐛 Proposed fix
if (!is_null($order)) {
$order->apply2Query($query, $this->getOrderMappings());
+ if (!$order->hasOrder('id')) {
+ $query->addOrderBy('e.id', 'ASC');
+ }
} else {
$query->addOrderBy('e.id', 'ASC');
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` around lines 666 -
670, The current code only adds e.id as a tie-breaker when no ordering is
specified, but this leaves non-unique orderings vulnerable to pagination issues
when callers order by fields like first_name or email. After the
$order->apply2Query() call in the if block (when $order is not null), add a
secondary sort on e.id ASC to serve as a deterministic tie-breaker, ensuring
this is only added if id is not already part of the caller's ordering. The else
block can remain unchanged since it already defaults to e.id ASC when no
ordering is provided.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/9014802374/86b9b1qrk?comment=90140217991206
Performance improvements
getUniqueActivitiesCountBySummit— Speaker & Submitter reposBefore: correlated
EXISTSwrapping a nestedIN (SELECT DISTINCT ...)subquery — O(presentations × speakers/members) execution plan.Measured at 12–14 s on dev data.
After: two-phase temp-table pattern:
MEMORYtemp table in chunks of 1 000 viaLIMIT/OFFSET+INSERT IGNORE. PHP never holds the full set in memory.JOINagainst the indexed temp table; no correlated subqueries, no largeINarray passed over the wire.try/finallyguaranteesDROP TEMPORARY TABLEeven on exception.Result: query completes in < 1 s on the same dev dataset.
getAllByPage(speakers) andgetSubmittersBySummitBefore: single large query hydrating full entities including all associations in one pass — slow and memory-intensive on summits with many speakers/submitters.
After: two-phase pagination:
COUNT(DISTINCT e.id)for the total, thenSELECT DISTINCT e.idwithLIMIT/OFFSETfor the current page.WHERE e.id IN (:ids)to hydrate only the current page of entities.This eliminates the cost of counting and sorting full entity graphs; the DB optimises count and ID queries independently from hydration.
Summary by CodeRabbit
presentations_track_group_idacross submitter listing, CSV export, bulk email, and unique-activity counting.