feat(speakers): add has_pending_presentations filter for speakers and submitters - #562
feat(speakers): add has_pending_presentations filter for speakers and submitters#562romanetar wants to merge 4 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📝 WalkthroughWalkthroughThe PR adds a Changeshas_pending_presentations filter
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 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. |
| WHERE | ||
| __p10_2.summit = :summit AND | ||
| LOWER(__cb10_2.email) :operator LOWER(:value) )"), | ||
| 'has_pending_presentations' => |
There was a problem hiding this comment.
@romanetar The has_pending_presentations definition doesn't match the ticket's stated goal. ClickUp 86badvupk is titled "Add Incomplete filter to speaker grid" and the goal is "send reminder emails to people who have not finished their submission", but this mapping puts no condition on Presentation.progress or Presentation.status — it matches any unpublished presentation with no team-list selection row. A speaker whose submission is PHASE_COMPLETE/STATUS_RECEIVED and simply awaiting selection matches pending==true (the new tests even create exactly that case and expect a match). During an open CFP, before any track-chair selection exists, this matches essentially every speaker/submitter, so a reminder email targeted with this filter would go to people who already finished their submission.
If "incomplete submission" is the intent, the condition should be based on submission state, e.g. __p41.status IS NULL OR __p41.progress != <Presentation::PHASE_COMPLETE>, instead of (or in addition to) the selection-list state. If the selection-based definition was agreed somewhere outside the ticket, can you point to it? Nothing in the ticket description, its comments, or summit-admin PR #989 redefines it. (Same applies to the mapping in DoctrineMemberRepository.php.)
There was a problem hiding this comment.
Fixed in de860ae. Added the missing check on submission state to both mappings: the filter now also requires __pX.progress != Presentation::PHASE_COMPLETE OR __pX.status IS NULL OR __pX.status != Presentation::STATUS_RECEIVED, i.e. the negation of Presentation::isSubmitted(), combined with AND alongside the existing published/selection-list conditions in both DoctrineSpeakerRepository and DoctrineMemberRepository. Kept the selection-list condition as-is (combined, not replaced) rather than guessing at a redefinition that isn't documented anywhere — happy to revisit if the selection-based semantics were intentional and should be dropped instead.
| $this->assertResponseStatus(200); | ||
| $speakers = json_decode($content); | ||
| $this->assertTrue(!is_null($speakers)); | ||
| $this->assertTrue(count($speakers->data) > 0); |
There was a problem hiding this comment.
@romanetar These three new tests pass even if the filter is a complete no-op. The two list tests assert only count($data) > 0, which also holds when the filter is silently ignored — the base query in getSpeakersBySummit already matches every speaker with summit activity. The count test's baseline + 1 looks stronger, but getUniqueActivitiesCountBySummit applies the filter only to the inner speaker-ID subquery: with the filter ignored, the count becomes "all activities", and adding one presentation still yields exactly baseline + 1. So removing the mapping entirely, or breaking its NOT EXISTS clause, fails none of the new tests. (Same issue in testGetCurrentSummitSpeakersActivitiesCountWithPendingPresentations and in OAuth2SummitSubmittersApiTest::testGetCurrentSummitSubmittersWithPendingPresentations.)
Suggested fix: in each test also create a non-pending control (a published presentation, or one with a SummitSelectedPresentation row on the Group/Session list) and assert its speaker/submitter is excluded from the pending==true results — or assert the exact set of returned IDs.
There was a problem hiding this comment.
Fixed in b0df665. Each of the three tests now also seeds a negative control (a published presentation for a different speaker/submitter) and asserts on the exact set of returned/counted IDs instead of count > 0 / baseline + 1. Confirmed locally that reverting the has_pending_presentations fix (de860ae) makes the dedicated regression tests fail, and with the fix in place the full suites (OAuth2SummitSpeakersApiTest, OAuth2SummitSubmittersApiTest) pass.
| new OA\Parameter( | ||
| name: 'filter', | ||
| description: 'Filter by 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. Operands supported: == (equal), @@ (contains), =@ (starts with).', | ||
| description: 'Filter by id, not_id, first_name, last_name, email, full_name, member_id, member_user_external_id, has_pending_presentations, 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. Operands supported: == (equal), @@ (contains), =@ (starts with).', |
There was a problem hiding this comment.
@romanetar The earlier CodeRabbit thread on this line was resolved as "Addressed in commit 1dc2b14", but the description it targeted — the protected getSpeakers filter parameter at line 181 — is unchanged: it still reads 'Filter by id, first_name, last_name, email, full_name, member_id, has_accepted_presentations, etc.' while its three sibling descriptions (lines 254, 416, 536) were updated. Suggested fix: add has_pending_presentations there too, ideally the full enumerated list matching line 254 so the generated docs advertise the same filters on both routes.
There was a problem hiding this comment.
Fixed in 2324cb9. Synced the protected getSpeakers filter description with the full enumerated list already used on the public/submitters endpoints (including has_pending_presentations and has_published_presentations).
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.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@romanetar please review
2 participants
ref https://app.clickup.com/t/86badvupk
Summary by CodeRabbit
New Features
Tests