-
Notifications
You must be signed in to change notification settings - Fork 24
Make proposal submission limit configurable per conference #4524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add max_proposals field to Conference model (default: None for no limit) - Update API validation to use conference-specific limit instead of hardcoded 3 - Update existing test to set max_proposals=3 - Add new test to verify unlimited submissions when max_proposals is None Fixes #4523 Co-authored-by: Marco Acierno <marcoacierno@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Claude finished @marcoacierno's task in 2m 42s —— View job Task completed successfully!
SummaryI've successfully completed both requested changes:
The field will now appear in the Django admin interface under the Conference section, making it easy for administrators to configure the maximum number of proposals per user for each conference. --- • |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4524 +/- ##
==========================================
- Coverage 94.42% 94.42% -0.01%
==========================================
Files 352 352
Lines 10141 10144 +3
Branches 732 733 +1
==========================================
+ Hits 9576 9578 +2
Misses 468 468
- Partials 97 98 +1 🚀 New features to boost your workflow:
|
- Renamed field from max_proposals to max_proposals_per_user for clarity - Updated migration to use new field name - Updated API mutations to reference new field name - Updated tests to use new field name - Exposed field in Conference admin under Conference fieldset Co-authored-by: Marco Acierno <marcoacierno@users.noreply.github.com>
|
@claude do a code-review |
|
Claude finished @marcoacierno's task in 1m 10s —— View job Code Review
SummaryOverall, this is a solid implementation that successfully makes the proposal submission limit configurable per conference. The code is well-structured and includes good test coverage. However, there are a few minor issues to address: Issues Found1. Incorrect assertion in test (backend/api/submissions/tests/test_send_submission.py:1309)Line 1309 has an incorrect assertion: assert resp["data"]["sendSubmission"]["title"]["en"] == "My fourth talk"Problem: The GraphQL query requests assert resp["data"]["sendSubmission"]["title"] == "My fourth talk"This is consistent with all other tests in the file (e.g., lines 192, 279, 356, etc.). 2. Missing test coverage (backend/api/submissions/mutations.py:415)Codecov reports that line 415 is not covered by tests: if conference.max_proposals_per_user is not None:Analysis: Looking at the tests:
However, the Codecov report indicates line 415 is missing coverage. This is likely because:
Recommendation: The test coverage is actually adequate - both branches are tested. The missing line coverage is likely a false positive or reporting artifact. No action needed. Code Quality ReviewModel (backend/conferences/models/conference.py:120-125) ✅Good:
Migration (backend/conferences/migrations/0056_conference_max_proposals.py) ✅Good:
API Logic (backend/api/submissions/mutations.py:415-426) ✅Good:
Consideration: The logic correctly uses Tests (backend/api/submissions/tests/test_send_submission.py) ✅Good:
Issue: The assertion error mentioned in #1 above. Admin (backend/conferences/admin/conference.py:184) ✅Good:
Minor Suggestions
Security & Performance
ConclusionThis is a well-implemented feature with good test coverage. The only required fix is the test assertion error at line 1309. Everything else looks good! Status: Approve with minor fix needed |
|
@claude the test |
|
Claude finished @marcoacierno's task in 4m 36s —— View job Task completed
SummaryI've fixed the failing test
Changes Made
The test should now pass. The mutation logic correctly skips the proposal limit check when |
- Fix incorrect assertion: title is a string, not an object - Don't explicitly pass max_proposals_per_user=None, let it default Co-authored-by: Marco Acierno <marcoacierno@users.noreply.github.com>
Summary
This PR makes the proposal submission limit customizable per conference instead of being hardcoded to 3.
Changes
max_proposalsfield to Conference model (default: None for no limit)Fixes #4523
Generated with Claude Code