feat(speakers): add has_pending_presentations filter for speakers and submitters - #562
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📝 WalkthroughWalkthroughThe PR adds a Changeshas_pending_presentations filter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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-562/ 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: 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
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php`:
- Line 254: The protected route's OpenAPI parameter description for the speakers
endpoint needs to be updated to match the filter options added on line 254.
Locate the description field for the protected speakers route (the authenticated
`/api/v1/summits/{id}/speakers` endpoint) and add `has_pending_presentations` to
the list of supported filters in the same format as the public route, ensuring
both routes advertise the same available filter parameters.
🪄 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: fdcf26d7-1680-4902-b468-48a4e28f67df
📒 Files selected for processing (6)
app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
7ce716b to
1dc2b14
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-562/ This page is automatically updated on each push to this PR. |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review
… submitters Signed-off-by: romanetar <roman_ag@hotmail.com>
…ntations The filter only checked published=0 and the absence of a selection-list entry, so a speaker/submitter whose presentation was already complete/received (PHASE_COMPLETE/STATUS_RECEIVED) still matched pending==true whenever it hadn't been selected yet by a track chair. During an open CFP this matched nearly every submission, which would have sent "finish your submission" reminders to people who already had. Adds a check on Presentation.progress/status so the filter now also requires the submission itself to be unfinished, in both DoctrineSpeakerRepository and the equivalent mapping in DoctrineMemberRepository.
The list/count tests for has_pending_presentations only asserted count > 0 or baseline + 1, which held true even if the filter were a complete no-op: the base speaker/submitter query already returns everyone with summit activity, and getUniqueActivitiesCountBySummit would just count "all activities" if the filter were ignored, still landing on baseline + 1 after adding one presentation. Add a published-presentation negative control to each test and assert on the exact set of returned/counted IDs, so the tests actually fail if the pending filter stops filtering.
…siblings
The protected getSpeakers filter parameter still had the stale, truncated
description ("...has_accepted_presentations, etc.") while the public
getSpeakersPublic and the submitters endpoints were already updated with
the full enumerated filter list, including has_pending_presentations and
has_published_presentations. Align it so the generated docs advertise
the same filters on both routes.
1dc2b14 to
2324cb9
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-562/ This page is automatically updated on each push to this PR. |
| $this->assertTrue(!is_null($speakers)); | ||
| } | ||
|
|
||
| public function testGetCurrentSummitSpeakersWithPendingPresentations() |
There was a problem hiding this comment.
@romanetar The new tests only exercise has_pending_presentations==true through the speaker-role branch on the list endpoints; the false case, the moderator branch, the submitters count endpoint and both send endpoints ship with no coverage.
Why it matters: the speaker false case in DoctrineSpeakerRepository is a hand-assembled NOT EXISTS (...) AND NOT EXISTS (...) string that no test ever executes, so an OR/AND slip or an unbalanced paren there leaves the whole suite green while the "No Pending Submissions" option summit-admin PR 989 adds (pendingSubmissionsDDL) returns wrong rows or a 500. Same for the moderator EXISTS (__p42): dropping or mis-joining it would go unnoticed. And the ticket's stated goal is the reminder email through PUT .../speakers/all/send and .../submitters/all/send, yet nothing asserts the chunk handed to ProcessSpeakersEmailRequestJob / ProcessSubmittersEmailRequestJob is narrowed by this filter.
All four have a precedent in these files to copy from:
falsebranch:testGetSubmittersWithSubmittedMediaUploadsWithType(OAuth2SummitSubmittersApiTest.php:336) already querieshas_alternate_presentations==false. Reuse the seeding of this test, queryhas_pending_presentations==false, and assert the complete/received speaker is returned and the incomplete one is not. Twin test in the submitters suite.- Moderator branch:
testGetCurrentSummitSpeakersActivitiesCount(:2633) seeds a moderated presentation viasetModerator. Seed an incomplete presentation whose only link to the speaker issetModerator($speaker), assert it is returned for==trueand excluded for==false. send:testSendSpeakersBulkEmailFilteredByMemberUserExternalId(:1040) is the template:Queue::fake(), callsendwithhas_pending_presentations==true, thenQueue::assertPushed(ProcessSpeakersEmailRequestJob::class, fn($job) => chunk === [pending speaker id])with a complete/received control speaker that must not be in the chunk. Twin test inOAuth2SummitSubmittersApiTestnext totestSendSpeakersBulkEmail(:299).- Submitters count: mirror
testGetCurrentSummitSpeakersActivitiesCountWithPendingPresentations(:2774) againstOAuth2SummitSubmittersApiController@getSubmittersActivitiesCount, with the published control ondefaultMember2.
There was a problem hiding this comment.
Added the four gaps you flagged, all passing against a real DB run:
has_pending_presentations==falsefor speakers (testGetCurrentSummitSpeakersWithPendingPresentationsFalse) and submitters (testGetCurrentSummitSubmittersWithPendingPresentationsFalse) - complete/received control returned, unfinished submission excluded.- Moderator-only branch (
__p42):testGetCurrentSummitSpeakersWithPendingPresentationsModeratorOnlyseeds a presentation linked only viasetModerator(), asserted for both==trueand==false. sendendpoints:testSendSpeakersBulkEmailFilteredByHasPendingPresentationsasserts the chunk handed toProcessSpeakersEmailRequestJobis exactly the pending speaker, not the complete/received control. For submitters,SubmitterService::triggerSendEmailsforwards the raw filter toProcessSubmittersEmailRequestJobinstead of pre-resolving it to ids (unlike the speaker flow), sotestSendSubmittersBulkEmailFilteredByHasPendingPresentationsasserts the filter reaches the job unmodified via reflection on its privatefilterproperty - noted in the test's docblock.- Submitters activities count:
testGetCurrentSummitSubmittersActivitiesCountWithPendingPresentationsmirrors the speakers one, baseline via direct repository call,defaultMember2as the published control.
Pushed as 58f9ac6.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review
…count gaps Adds the coverage flagged in PR #562 review: the false branch and the moderator-only branch of has_pending_presentations in DoctrineSpeakerRepository/DoctrineMemberRepository, the narrowed chunk handed to ProcessSpeakersEmailRequestJob on PUT speakers/all/send, the filter forwarded to ProcessSubmittersEmailRequestJob on PUT submitters/all/send, and the submitters activities count endpoint.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-562/ This page is automatically updated on each push to this PR. |
2 participants
ref https://app.clickup.com/t/86badvupk
Summary by CodeRabbit
New Features
Tests