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 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds two OAuth2-protected endpoints to count unique summit activities for speakers and submitters with optional filter parsing and validation; implements repository methods to compute distinct presentation counts; wires routes and seeds endpoints; adds tests and several supporting infrastructure fixes. ChangesSpeakers and Submitters Activities Count API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.54)Composer install failed: this project depends on private packages that require authentication (e.g. GitLab/GitHub, Laravel Nova, etc.). Instead, run PHPStan in a CI/CD pipeline where you can use custom packages — our pipeline remediation tool can use the PHPStan output from your CI/CD pipeline. 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. |
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
…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
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
ref: https://app.clickup.com/t/86b9b1qrk
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores