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 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 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
… submitters Signed-off-by: romanetar <roman_ag@hotmail.com>
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.)
| $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.
| 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.
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