feat(speakers): add unique activities count endpoints for speakers and submitters#543
feat(speakers): add unique activities count endpoints for speakers and submitters#543mulldug wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds two OAuth2-protected GET endpoints to return unique activity counts for summit speakers and submitters, implements repository methods to compute COUNT(DISTINCT presentations) with optional filters, wires routes, seeds and migrates endpoints, updates tests, and includes small infrastructure/formatting fixes. ChangesActivities Count Endpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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 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-543/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/oauth2/OAuth2SummitSubmittersApiTest.php (1)
264-301: ⚡ Quick winStrengthen count assertions to catch regressions earlier.
These tests only assert
count >= 0. Add an integer-type assertion and a filtered-vs-unfiltered comparison (filtered <= unfiltered) to validate behavior, not just shape.🤖 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 264 - 301, Update the assertions in testGetCurrentSummitSubmittersActivitiesCountWithAcceptedPresentations (and the sibling unfiltered test if present) to validate type and relational behavior: assert the returned $data->count is an integer (use assertIsInt or assertTrue(is_int(...))) and then perform an additional request to get the unfiltered count (call OAuth2SummitSubmittersApiController@getSubmittersActivitiesCount with the same $params but without the 'filter' entry), decode that response and assert that the filtered count is less than or equal to the unfiltered count ($filteredCount <= $unfilteredCount); use the existing $response/$content/json_decode flow and $this->action helper to obtain the unfiltered result.app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php (1)
432-497: ⚖️ Poor tradeoffConsider extracting the duplicated filter definitions.
The filter field definitions (lines 442-464) and validation rules (lines 468-490) are duplicated across
getSpeakers,getSpeakersCSV,send, and this newgetSpeakersActivitiesCountmethod. Extracting these to private methods or class constants would improve maintainability and ensure consistency when filter fields are added or modified.♻️ Example refactor
private function getSpeakerFilterDefinition(): array { return [ 'id' => ['=='], 'not_id' => ['=='], 'first_name' => ['=@', '@@', '=='], 'last_name' => ['=@', '@@', '=='], 'email' => ['=@', '@@', '=='], 'full_name' => ['=@', '@@', '=='], 'member_id' => ['=='], 'member_user_external_id' => ['=='], 'has_accepted_presentations' => ['=='], 'has_alternate_presentations' => ['=='], 'has_rejected_presentations' => ['=='], 'presentations_track_id' => ['=='], 'presentations_track_group_id' => ['=='], 'presentations_selection_plan_id' => ['=='], 'presentations_type_id' => ['=='], 'presentations_title' => ['=@', '@@', '=='], 'presentations_abstract' => ['=@', '@@', '=='], 'presentations_submitter_full_name' => ['=@', '@@', '=='], 'presentations_submitter_email' => ['=@', '@@', '=='], 'has_media_upload_with_type' => ['=='], 'has_not_media_upload_with_type' => ['=='], ]; } private function getSpeakerFilterValidationRules(): array { return [ 'id' => 'sometimes|integer', 'not_id' => 'sometimes|integer', 'first_name' => 'sometimes|string', 'last_name' => 'sometimes|string', 'email' => 'sometimes|string', 'full_name' => 'sometimes|string', 'member_id' => 'sometimes|integer', 'member_user_external_id' => 'sometimes|integer', 'has_accepted_presentations' => 'sometimes|required|string|in:true,false', 'has_alternate_presentations' => 'sometimes|required|string|in:true,false', 'has_rejected_presentations' => 'sometimes|required|string|in:true,false', 'presentations_track_id' => 'sometimes|integer', 'presentations_track_group_id' => 'sometimes|integer', 'presentations_selection_plan_id' => 'sometimes|integer', 'presentations_type_id' => 'sometimes|integer', 'presentations_title' => 'sometimes|string', 'presentations_abstract' => 'sometimes|string', 'presentations_submitter_full_name' => 'sometimes|string', 'presentations_submitter_email' => 'sometimes|string', 'has_media_upload_with_type' => 'sometimes|integer', 'has_not_media_upload_with_type' => 'sometimes|integer', ]; }Then use:
$filter = FilterParser::parse(Request::input('filter'), $this->getSpeakerFilterDefinition()); // ... if (!is_null($filter)) { $filter->validate($this->getSpeakerFilterValidationRules()); }🤖 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/OAuth2SummitSpeakersApiController.php` around lines 432 - 497, The filter field definitions and validation rules duplicated in getSpeakersActivitiesCount should be extracted into reusable helpers (e.g., private methods or class constants) and reused across getSpeakers, getSpeakersCSV, send and getSpeakersActivitiesCount; create methods like getSpeakerFilterDefinition() and getSpeakerFilterValidationRules() returning the arrays shown in the comment, then replace the inline arrays in getSpeakersActivitiesCount (the FilterParser::parse call and the $filter->validate call) and the same spots in getSpeakers, getSpeakersCSV and send to call those helper methods instead.tests/oauth2/OAuth2SummitSpeakersApiTest.php (1)
1938-1968: 💤 Low valueConsider comparing filtered vs unfiltered count for consistency.
The test
testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan(lines 1903-1936) establishes a useful pattern: it fetches both unfiltered and filtered counts, then asserts the filtered count is less than or equal to the unfiltered count. This test only fetches the filtered count and asserts it's greater than zero. For consistency and stronger validation, consider adding an unfiltered baseline call and comparing the results.🤖 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/OAuth2SummitSpeakersApiTest.php` around lines 1938 - 1968, The test method testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations should also fetch an unfiltered baseline using the same controller action getSpeakersActivitiesCount and same summit id (but without the filter param), parse its JSON count, then assert the filtered count is > 0 and that filtered_count <= unfiltered_count to mirror the pattern in testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan; locate and update the test method testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations to perform the extra request, decode the response, and add the additional assertion comparing counts.app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php (1)
539-604: ⚖️ Poor tradeoffConsider extracting the duplicated filter definitions.
The filter field definitions (lines 549-571) and validation rules (lines 575-597) are duplicated across
getAllBySummit,getAllBySummitCSV,send, and this newgetSubmittersActivitiesCountmethod. Extracting these to a private method or class constant would improve maintainability and ensure consistency when filter fields are added or modified.♻️ Example refactor
private function getSubmitterFilterDefinition(): array { return [ 'id' => ['=='], 'not_id' => ['=='], 'first_name' => ['=@', '@@', '=='], 'last_name' => ['=@', '@@', '=='], 'email' => ['=@', '@@', '=='], 'full_name' => ['=@', '@@', '=='], 'member_id' => ['=='], 'member_user_external_id' => ['=='], 'has_accepted_presentations' => ['=='], 'has_alternate_presentations' => ['=='], 'has_rejected_presentations' => ['=='], 'presentations_track_id' => ['=='], 'presentations_selection_plan_id' => ['=='], 'presentations_type_id' => ['=='], 'presentations_title' => ['=@', '@@', '=='], 'presentations_abstract' => ['=@', '@@', '=='], 'presentations_submitter_full_name' => ['=@', '@@', '=='], 'presentations_submitter_email' => ['=@', '@@', '=='], 'is_speaker' => ['=='], 'has_media_upload_with_type' => ['=='], 'has_not_media_upload_with_type' => ['=='], ]; } private function getSubmitterFilterValidationRules(): array { return [ 'id' => 'sometimes|integer', 'not_id' => 'sometimes|integer', 'first_name' => 'sometimes|string', 'last_name' => 'sometimes|string', 'email' => 'sometimes|string', 'full_name' => 'sometimes|string', 'member_id' => 'sometimes|integer', 'member_user_external_id' => 'sometimes|integer', 'has_accepted_presentations' => 'sometimes|string|in:true,false', 'has_alternate_presentations' => 'sometimes|string|in:true,false', 'has_rejected_presentations' => 'sometimes|string|in:true,false', 'presentations_track_id' => 'sometimes|integer', 'presentations_selection_plan_id' => 'sometimes|integer', 'presentations_type_id' => 'sometimes|integer', 'presentations_title' => 'sometimes|string', 'presentations_abstract' => 'sometimes|string', 'presentations_submitter_full_name' => 'sometimes|string', 'presentations_submitter_email' => 'sometimes|string', 'is_speaker' => 'sometimes|string|in:true,false', 'has_media_upload_with_type' => 'sometimes|integer', 'has_not_media_upload_with_type' => 'sometimes|integer', ]; }Then use:
$filter = FilterParser::parse(Request::input('filter'), $this->getSubmitterFilterDefinition()); // ... $filter->validate($this->getSubmitterFilterValidationRules());🤖 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 539 - 604, The duplicated submitter filter definitions and validation rules used in getSubmittersActivitiesCount (and also in getAllBySummit, getAllBySummitCSV, send) should be extracted into reusable methods or constants; add private methods like getSubmitterFilterDefinition() and getSubmitterFilterValidationRules() that return the arrays shown in the review, then replace the inline arrays in getSubmittersActivitiesCount with FilterParser::parse(Request::input('filter'), $this->getSubmitterFilterDefinition()) and $filter->validate($this->getSubmitterFilterValidationRules()); update the other methods (getAllBySummit, getAllBySummitCSV, send) to call these new methods to remove duplication and keep behavior identical.
🤖 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/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php`:
- Around line 175-177: The origin check in
OAuth2BearerAccessTokenRequestValidator uses str_contains on
token_info->getAllowedOrigins(), which allows partial matches; update the logic
to perform exact origin matching: normalize the incoming $origin (trim trailing
slash), split token_info->getAllowedOrigins() into discrete origins (e.g., by
comma) and check for strict equality (or use in_array) against the normalized
origin when getApplicationType() === 'JS_CLIENT', ensuring null/empty $origin
still fails.
In `@app/Repositories/Summit/DoctrineMemberRepository.php`:
- Around line 669-686: The current code in DoctrineMemberRepository builds
$memberIds by materializing $qb->getQuery()->getArrayResult() and then passes
that array into the COUNT query (member_ids), which can blow up for large
summits; replace this two-step approach with a single SQL that counts distinct
presentations for submitters from the summit using a subquery or JOIN instead of
an IN-list (e.g. COUNT(DISTINCT p.ID) FROM Presentation p INNER JOIN SummitEvent
se ON se.ID = p.ID WHERE se.SummitID = :summit_id AND p.CreatedByID IN (SELECT
DISTINCT m.ID FROM Member m INNER JOIN <relevant_table> ... WHERE SummitID =
:summit_id) or use EXISTS: WHERE EXISTS (SELECT 1 FROM <submitter_relation> s
WHERE s.MemberID = p.CreatedByID AND s.SummitID = :summit_id)); update the call
to executeQuery to only bind the single summit_id parameter (remove member_ids
and ArrayParameterType usage) and keep the query execution within
DoctrineMemberRepository (target symbols: $qb, getArrayResult, $memberIds,
Presentation p, SummitEvent se, executeQuery).
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 731-755: The code in DoctrineSpeakerRepository materializes all
speaker IDs via $qb->getQuery()->getArrayResult() into $speakerIds and then uses
IN (:speaker_ids) in a raw SQL count, which is brittle for large sets; instead
modify the count SQL to embed the speaker selection as a SQL subquery (or reuse
the QueryBuilder as a subselect) so you never pass a large PHP array — replace
the IN (:speaker_ids) clauses with EXISTS/subselects that reference the same
speaker-selection logic (derived from $qb) and execute the count with a single
query using bound scalar parameters (e.g., :summit_id) rather than an
ArrayParameterType for speaker ids.
In `@database/seeders/ConfigSeeder.php`:
- Around line 38-42: The seeder currently calls non-existent methods
evictEntityRegions() and evictQueryRegions() on $l2Cache; replace them with the
proper Doctrine Cache API calls by invoking $l2Cache->evictQueryCache() for
query cache, and for entities either call
$l2Cache->evictEntityRegion($entityClass) in a loop over your entity class names
(e.g., from metadata) or call $l2Cache->evictEntityRegion(null) to evict all
entity regions; update the code in ConfigSeeder where $l2Cache is used to
perform these correct calls.
---
Nitpick comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php`:
- Around line 432-497: The filter field definitions and validation rules
duplicated in getSpeakersActivitiesCount should be extracted into reusable
helpers (e.g., private methods or class constants) and reused across
getSpeakers, getSpeakersCSV, send and getSpeakersActivitiesCount; create methods
like getSpeakerFilterDefinition() and getSpeakerFilterValidationRules()
returning the arrays shown in the comment, then replace the inline arrays in
getSpeakersActivitiesCount (the FilterParser::parse call and the
$filter->validate call) and the same spots in getSpeakers, getSpeakersCSV and
send to call those helper methods instead.
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`:
- Around line 539-604: The duplicated submitter filter definitions and
validation rules used in getSubmittersActivitiesCount (and also in
getAllBySummit, getAllBySummitCSV, send) should be extracted into reusable
methods or constants; add private methods like getSubmitterFilterDefinition()
and getSubmitterFilterValidationRules() that return the arrays shown in the
review, then replace the inline arrays in getSubmittersActivitiesCount with
FilterParser::parse(Request::input('filter'),
$this->getSubmitterFilterDefinition()) and
$filter->validate($this->getSubmitterFilterValidationRules()); update the other
methods (getAllBySummit, getAllBySummitCSV, send) to call these new methods to
remove duplication and keep behavior identical.
In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php`:
- Around line 1938-1968: The test method
testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations should also
fetch an unfiltered baseline using the same controller action
getSpeakersActivitiesCount and same summit id (but without the filter param),
parse its JSON count, then assert the filtered count is > 0 and that
filtered_count <= unfiltered_count to mirror the pattern in
testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan; locate and update
the test method
testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations to perform
the extra request, decode the response, and add the additional assertion
comparing counts.
In `@tests/oauth2/OAuth2SummitSubmittersApiTest.php`:
- Around line 264-301: Update the assertions in
testGetCurrentSummitSubmittersActivitiesCountWithAcceptedPresentations (and the
sibling unfiltered test if present) to validate type and relational behavior:
assert the returned $data->count is an integer (use assertIsInt or
assertTrue(is_int(...))) and then perform an additional request to get the
unfiltered count (call
OAuth2SummitSubmittersApiController@getSubmittersActivitiesCount with the same
$params but without the 'filter' entry), decode that response and assert that
the filtered count is less than or equal to the unfiltered count ($filteredCount
<= $unfilteredCount); use the existing $response/$content/json_decode flow and
$this->action helper to obtain the unfiltered result.
🪄 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: 36745d1c-f076-4af9-b8a0-83be313aee9a
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phpdocker-compose.override.testing.ymlroutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
mulldug
force-pushed
the
feature/speakers-submitters-activities-count
branch
from
ca6dbae to
bd000e6
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
711-713: ⚡ Quick winPush presentation de-duplication into SQL for the speaker branch.
speakerQbcan return duplicatep.idrows due to thep.speakersjoin; deduping earlier reduces result size and PHP work with no behavior change.Proposed change
- $speakerQb = $this->getEntityManager()->createQueryBuilder() - ->select("p.id") + $speakerQb = $this->getEntityManager()->createQueryBuilder() + ->select("DISTINCT p.id") ->from('models\summit\Presentation', 'p') ->join('p.speakers', '__cnt_assign') ->join('__cnt_assign.speaker', 'e')Also applies to: 741-744
🤖 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 711 - 713, The query builder in DoctrineSpeakerRepository producing $speakerQb can return duplicate p.id rows because of the p.speakers join; modify the query to deduplicate at the SQL level (e.g., change the select to request unique ids or add a GROUP BY) so it returns distinct presentation ids (use DISTINCT p.id or group by p.id) to reduce result size and PHP-side work; apply the same fix to the other occurrence of $speakerQb-like code around the later block (the similar select at lines ~741-744).
🤖 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.
Nitpick comments:
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 711-713: The query builder in DoctrineSpeakerRepository producing
$speakerQb can return duplicate p.id rows because of the p.speakers join; modify
the query to deduplicate at the SQL level (e.g., change the select to request
unique ids or add a GROUP BY) so it returns distinct presentation ids (use
DISTINCT p.id or group by p.id) to reduce result size and PHP-side work; apply
the same fix to the other occurrence of $speakerQb-like code around the later
block (the similar select at lines ~741-744).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fac9124-d47b-4e92-be67-726afb96df68
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phpdocker-compose.override.testing.ymlroutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
✅ Files skipped from review due to trivial changes (4)
- .env.example
- docker-compose.override.testing.yml
- .gitignore
- app/Models/Foundation/Main/Repositories/IMemberRepository.php
🚧 Files skipped from review as they are similar to previous changes (10)
- app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php
- database/seeders/ConfigSeeder.php
- app/ModelSerializers/Summit/SummitSerializer.php
- routes/api_v1.php
- tests/oauth2/OAuth2SummitSubmittersApiTest.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
- tests/oauth2/OAuth2SummitSpeakersApiTest.php
- database/seeders/ApiEndpointsSeeder.php
- tests/SubmitterRepositoryTest.php
Test coverage for the new activities-count endpointsThe new count endpoints are exercised, but the assertions are happy-path only and one repository test is effectively a no-op in CI. Worth tightening before merge:
See the inline comments for the specific spots. |
| $summit_repository = EntityManager::getRepository(Summit::class); | ||
|
|
||
| $summit = $summit_repository->find(3401); | ||
| if (is_null($summit)) $this->markTestSkipped('Summit 3401 not in test DB'); |
There was a problem hiding this comment.
This test no-ops on a clean CI database: summit 3401 won't exist, so it hits markTestSkipped and provides zero coverage for getUniqueActivitiesCountBySummit. Please drive it off the seeded test summit (as the API tests do) so the repository method is actually exercised in CI rather than skipped.
| $data = json_decode($content); | ||
| $this->assertNotNull($data); | ||
| $this->assertTrue(isset($data->count)); | ||
| $this->assertGreaterThan(0, $data->count); |
There was a problem hiding this comment.
Happy-path only: count > 0 would still pass if moderators are double-counted, the array_unique dedup is broken, or the join is wrong. Please assert an exact expected count against a known fixture — ideally a presentation with multiple assigned speakers plus a moderator — so the speaker/moderator union + dedup is actually verified, not just "non-zero".
|
|
||
| // Take the union of both result sets and count distinct presentation IDs. | ||
| // Presentation counts per summit are small (<1000) so PHP-side deduplication is fine. | ||
| $speakerIds = array_column($speakerQb->getQuery()->getScalarResult(), 'id'); |
There was a problem hiding this comment.
This count path materializes every (presentation, speaker) and (presentation, moderator) row into PHP via getScalarResult() and dedups with array_unique. The "presentation counts per summit are small (<1000)" assumption is unenforced and degrades for large summits, while still executing two full filtered queries.
This is now also inconsistent with DoctrineMemberRepository::getUniqueActivitiesCountBySummit, which was already moved to a SQL subquery. Please do the same here — take the union of the two presentation-id sets and COUNT(DISTINCT p.id) in DQL/SQL instead of pulling all ids into PHP.
mulldug
force-pushed
the
feature/speakers-submitters-activities-count
branch
from
bd000e6 to
621283c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ 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)
app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
712-724: ⚡ Quick winConsider replacing cross-join with explicit JOINs or UNION for clarity.
The two
FROMclauses (lines 714-715) create a cartesian product of Presentation × PresentationSpeaker, then filter via theEXISTSsubquery andOR p.moderator = econdition. While functionally correct and properly deduplicated viaCOUNT(DISTINCT p.id), this cross-join approach processes N×M intermediate rows before filtering.For better readability and potentially improved query plan, consider using:
- Option 1: Explicit INNER JOIN via PresentationSpeakerAssignment + LEFT JOIN for moderators
- Option 2: UNION of two simpler queries (one for assignments, one for moderators)
However, if the current approach performs acceptably in production and the query plan shows adequate filtering, this optimization can be deferred.
🤖 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 712 - 724, The query in DoctrineSpeakerRepository building $countQb uses two FROM clauses (Presentation p and PresentationSpeaker e) producing a cross-join; replace this with an explicit join-based or union-based approach to avoid the cartesian product: update the QueryBuilder logic that constructs the COUNT query (the block creating $countQb selecting COUNT(DISTINCT p.id)) to either 1) JOIN Presentation to PresentationSpeakerAssignment (or PresentationSpeaker via a proper JOIN) and LEFT JOIN the moderator relationship so the WHERE clause can reference those joins without a second FROM, or 2) build two simpler queries (one for presentations matched via PresentationSpeakerAssignment and one for p.moderator) and UNION them before counting distinct p.id; ensure you keep the same parameter binding for :summit and preserve the EXISTS/p.moderator semantics while removing the cross-join between Presentation and PresentationSpeaker.
🤖 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 `@tests/SubmitterRepositoryTest.php`:
- Line 85: The assertion assertIsArray($submitterIds) is too weak; update the
test in SubmitterRepositoryTest.php to assert that $submitterIds actually
contains data from the seeded summit fixture — for example replace or supplement
assertIsArray($submitterIds) with assertNotEmpty($submitterIds) or
assertGreaterThan(0, count($submitterIds)), and optionally assert expected
values (e.g., assertContains/ assertEquals on a known seeded submitter ID)
against the $submitterIds array so the repository call (the test that calls the
repository method returning $submitterIds) fails when no IDs are returned.
---
Nitpick comments:
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 712-724: The query in DoctrineSpeakerRepository building $countQb
uses two FROM clauses (Presentation p and PresentationSpeaker e) producing a
cross-join; replace this with an explicit join-based or union-based approach to
avoid the cartesian product: update the QueryBuilder logic that constructs the
COUNT query (the block creating $countQb selecting COUNT(DISTINCT p.id)) to
either 1) JOIN Presentation to PresentationSpeakerAssignment (or
PresentationSpeaker via a proper JOIN) and LEFT JOIN the moderator relationship
so the WHERE clause can reference those joins without a second FROM, or 2) build
two simpler queries (one for presentations matched via
PresentationSpeakerAssignment and one for p.moderator) and UNION them before
counting distinct p.id; ensure you keep the same parameter binding for :summit
and preserve the EXISTS/p.moderator semantics while removing the cross-join
between Presentation and PresentationSpeaker.
🪄 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: d83e26c5-ffda-47b8-bda0-54ccc5fab375
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/migrations/config/Version20260603000000.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phproutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- .env.example
🚧 Files skipped from review as they are similar to previous changes (8)
- app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php
- app/ModelSerializers/Summit/SummitSerializer.php
- database/seeders/ConfigSeeder.php
- app/Repositories/Summit/DoctrineMemberRepository.php
- app/Models/Foundation/Summit/Repositories/ISpeakerRepository.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
- database/seeders/ApiEndpointsSeeder.php
- tests/oauth2/OAuth2SummitSubmittersApiTest.php
…d submitters
- Add GET /summits/{id}/speakers/all/events/count and GET /summits/{id}/submitters/all/events/count controller methods with OpenAPI attributes and route registrations
- Implement getUniqueActivitiesCountBySummit in DoctrineSpeakerRepository and DoctrineMemberRepository using two-stage DQL→raw SQL COUNT(DISTINCT)
- Add interface declarations to ISpeakerRepository and IMemberRepository
- Register both endpoints in ApiEndpointsSeeder with ReadSummitData scopes
- Add PHPUnit tests for both HTTP endpoints and repository-level method
- Fix null-guard bug in DoctrineMemberRepository::applyExtraJoins
mulldug
force-pushed
the
feature/speakers-submitters-activities-count
branch
from
621283c to
d9cf735
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/SubmitterRepositoryTest.php`:
- Around line 91-103: The test currently only checks types and bounds for
$totalCount and $filteredCount, which lets join fan-out or filter bugs pass;
replace the loose assertions with exact expected values for the seeded summit
fixture by asserting $totalCount === <EXPECTED_TOTAL> and $filteredCount ===
<EXPECTED_FILTERED> (use the concrete integers from the fixture), keeping the
same call sites getUniqueActivitiesCountBySummit(self::$summit, null) and
getUniqueActivitiesCountBySummit(self::$summit, $filter) and the same
FilterParser::parse([... "is_speaker" => ['=='] ...]) setup so the
distinct-count and is_speaker filter behavior is validated precisely.
🪄 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: 4fea2d3c-6f09-4517-b98c-e8de695a1cbe
📒 Files selected for processing (16)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/migrations/config/Version20260603000000.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phproutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- .env.example
- app/ModelSerializers/Summit/SummitSerializer.php
🚧 Files skipped from review as they are similar to previous changes (11)
- app/Models/Foundation/Summit/Repositories/ISpeakerRepository.php
- routes/api_v1.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php
- tests/oauth2/OAuth2SummitSubmittersApiTest.php
- database/seeders/ApiEndpointsSeeder.php
- app/Models/Foundation/Main/Repositories/IMemberRepository.php
- tests/oauth2/OAuth2SummitSpeakersApiTest.php
- database/seeders/ConfigSeeder.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
- app/Repositories/Summit/DoctrineSpeakerRepository.php
- app/Repositories/Summit/DoctrineMemberRepository.php
| $countQb = $this->getEntityManager()->createQueryBuilder() | ||
| ->select('COUNT(DISTINCT p.id)') | ||
| ->from('models\summit\Presentation', 'p') | ||
| ->from('models\summit\PresentationSpeaker', 'e') |
There was a problem hiding this comment.
@mulldug
This FROM clause creates an unbounded cross-join: PresentationSpeaker is not scoped to the summit, so the query starts from a product of (presentations in summit) × (every speaker in the entire database) before the EXISTS/moderator clause narrows it down. On a large deployment — say 1K summit presentations and 10K total speakers — the engine evaluates 10M intermediate rows per call.
The member repository avoids this with a two-stage subquery: first collect matching member IDs, then count presentations for those IDs. The same pattern works here — collect matching speaker IDs in a subquery scoped to this summit's assignments, then feed that as an IN subquery into the count.
There was a problem hiding this comment.
The feedback is based on an earlier version of the query. The current implementation does not perform an unbounded scan of PresentationSpeaker — the inner query's WHERE clause constrains it to speakers who belong to the target summit via an EXISTS subquery against PresentationSpeakerAssignment or a moderator join, both filtered by summit_id. Only the matching speaker IDs are returned, and the outer query then counts distinct presentations for those speakers.
Restructuring to start from Presentation (as the member repository does) is not straightforward in the speaker case. The member repository has a single speaker-role relationship to navigate; this method must cover two distinct paths — assigned speaker (PresentationSpeakerAssignment) and moderator. All caller-supplied filters (e.g. has_accepted_presentations, has_alternate_presentations, has_rejected_presentations) are expressed against the e alias on PresentationSpeaker. A Presentation-first structure would require either duplicating every filter mapping for both paths or reintroducing the same inner subquery, which gains nothing over the current design and significantly increases complexity.
If there is a specific query plan concern (e.g. a missing index on PresentationSpeakerAssignment.speaker or Presentation.moderator), that would be worth addressing at the database level. But the query structure itself is not doing a full table scan — the summit predicate is applied in the innermost subquery before any join expansion occurs.
| ->select("COUNT(DISTINCT p.id)") | ||
| ->from('models\summit\Presentation', 'p') | ||
| ->where('p.summit = :summit_outer') | ||
| ->andWhere("p.created_by IN ({$qb->getDQL()})"); |
There was a problem hiding this comment.
@mulldug
Embedding $qb->getDQL() via string interpolation is fragile in two ways.
First, the filter mappings for this repository contain hardcoded :summit parameter references inside their EXISTS subqueries (presentations_track_id, has_accepted_presentations, presentations_title, etc.). Those parameters are propagated by the copy loop below, but if any future filter mapping introduces a parameter named :summit_outer it will silently overwrite the outer query's summit binding and return incorrect counts.
Second, getDQL() is an internal Doctrine QueryBuilder API with no stability guarantee on its output format.
Consider using Expr\In instead of string interpolation ($countQb->expr()->in('p.created_by', $qb->getDQL())) and renaming the inner query's :summit parameter to something unambiguous (e.g. :submitter_summit) to make the parameter boundary explicit.
There was a problem hiding this comment.
This feedback is based on a previous version.
| { | ||
| return $this->processRequest(function () use ($summit_id) { | ||
|
|
||
| $summit = SummitFinderStrategyFactory::build($this->getRepository(), $this->getResourceServerContext())->find($summit_id); |
| { | ||
| return $this->processRequest(function () use ($summit_id) { | ||
|
|
||
| $summit = SummitFinderStrategyFactory::build($this->summit_repository, $this->getResourceServerContext())->find(intval($summit_id)); |
There was a problem hiding this comment.
| 'full_name' => 'sometimes|string', | ||
| 'member_id' => 'sometimes|integer', | ||
| 'member_user_external_id' => 'sometimes|integer', | ||
| 'has_accepted_presentations' => 'sometimes|string|in:true,false', |
There was a problem hiding this comment.
@mulldug The getSpeakersActivitiesCount validation rules for has_accepted_presentations, has_alternate_presentations, and has_rejected_presentations use sometimes|required|string|in:true,false,
while the sibling getSubmittersActivitiesCount uses sometimes|string|in:true,false (no required). Since both endpoints accept the same filter semantics, the validation should be consistent
— pick one form and apply it to both.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
717-718: 💤 Low valueConsider making LEFT JOINs conditional on filter presence.
Lines 717-718 add
registration_requestandmemberjoins unconditionally, but they're only needed when filter mappings referencerrorm. When$filteris null, these joins add unnecessary overhead.Wrap them in
if (!is_null($filter))to optimize the unfiltered path.⚡ Proposed optimization
->from('models\summit\PresentationSpeaker', 'e') - ->leftJoin('e.registration_request', 'rr') - ->leftJoin('e.member', 'm') ->where( 'EXISTS (SELECT 1 FROM App\Models\Foundation\Summit\Speakers\PresentationSpeakerAssignment __a' . ' JOIN __a.presentation __ap WHERE IDENTITY(__ap.summit) = :summit_id AND __a.speaker = e)' . ' OR EXISTS (SELECT 1 FROM models\summit\Presentation __mp WHERE IDENTITY(__mp.summit) = :summit_id AND __mp.moderator = e)' ) ->setParameter('summit_id', $summit->getId()); if (!is_null($filter)) { + $innerQb + ->leftJoin('e.registration_request', 'rr') + ->leftJoin('e.member', 'm'); $filter->apply2Query($innerQb, $this->getFilterMappings($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 717 - 718, The unconditional leftJoin calls for 'e.registration_request' and 'e.member' add overhead when no filters are applied; in DoctrineSpeakerRepository, wrap the ->leftJoin('e.registration_request', 'rr') and ->leftJoin('e.member', 'm') calls in a conditional that checks the incoming $filter (e.g., if ($filter !== null) or if (!is_null($filter))) so these joins are only added when filter mappings reference rr or m; ensure the surrounding query builder variable (the one used to chain leftJoin) remains in scope and that any later code that references rr or m is also guarded by the same $filter check.
🤖 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.
Nitpick comments:
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 717-718: The unconditional leftJoin calls for
'e.registration_request' and 'e.member' add overhead when no filters are
applied; in DoctrineSpeakerRepository, wrap the
->leftJoin('e.registration_request', 'rr') and ->leftJoin('e.member', 'm') calls
in a conditional that checks the incoming $filter (e.g., if ($filter !== null)
or if (!is_null($filter))) so these joins are only added when filter mappings
reference rr or m; ensure the surrounding query builder variable (the one used
to chain leftJoin) remains in scope and that any later code that references rr
or m is also guarded by the same $filter check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 307e66f7-df52-4201-b441-8d9e3e7b101c
📒 Files selected for processing (4)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phptests/SubmitterRepositoryTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
…ints
Address review feedback:
- Improve queries in repositories
- Cast $summit_id to int in getSpeakersActivitiesCount (consistent with
getSubmittersActivitiesCount which already used intval())
- Drop redundant `required` from boolean filter validation rules in
OAuth2SummitSpeakersApiController (redundant given in:true,false; aligns
with OAuth2SummitSubmittersApiController)
- Strengthen SubmitterRepositoryTest with exact seeded counts instead of
type/bounds checks that pass vacuously when the method returns 0
mulldug
force-pushed
the
feature/speakers-submitters-activities-count
branch
from
677cfba to
a224da8
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
Revert two spurious whitespace changes introduced by the previous commit: - SummitSerializer.php: restore blank lines around the payment_profiles block (trailing-whitespace line replaced with true blank; blank line after serialize() call restored) - ConfigSeeder.php: remove blank line added between em->clear() and the connection assignment Strengthen testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations: the previous assertGreaterThan(0, count) would pass even if the has_accepted_presentations filter was broken and returned all results. Replace with a baseline-plus-one pattern: capture the filtered count via the repository before seeding, add one published presentation for a new speaker, then assert the HTTP endpoint returns exactly baseline + 1.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
3 participants
ref: https://app.clickup.com/t/86b9b1qrk
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores