Skip to content

Conversation

@marcoacierno
Copy link
Member

Summary

This PR makes the proposal submission limit customizable per conference instead of being hardcoded to 3.

Changes

  • 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

Generated with Claude Code

- 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>
@vercel
Copy link

vercel bot commented Jan 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
pycon Ready Ready Preview Jan 10, 2026 7:20am

@marcoacierno
Copy link
Member Author

@claude

  • Rename the field to max_proposals_per_user
  • Expose it in the Conference admin

@claude
Copy link
Contributor

claude bot commented Jan 7, 2026

Claude finished @marcoacierno's task in 2m 42s —— View job


Task completed successfully!

  • Gather context - read the changed files
  • Rename field from max_proposals to max_proposals_per_user in Conference model
  • Update the migration file
  • Update API mutations code
  • Update tests
  • Expose field in Conference admin
  • Commit and push changes

Summary

I've successfully completed both requested changes:

  1. Renamed the field from max_proposals to max_proposals_per_user across:

    • Conference model (backend/conferences/models/conference.py)
    • Migration file (backend/conferences/migrations/0056_conference_max_proposals.py)
    • API mutations (backend/api/submissions/mutations.py)
    • Tests (backend/api/submissions/tests/test_send_submission.py)
  2. Exposed the field in Conference admin by adding max_proposals_per_user to the "Conference" fieldset in backend/conferences/admin/conference.py

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
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.42%. Comparing base (d69e0a2) to head (b695f58).
⚠️ Report is 1 commits behind head on main.

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:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- 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>
@marcoacierno
Copy link
Member Author

@claude do a code-review

@claude
Copy link
Contributor

claude bot commented Jan 7, 2026

Claude finished @marcoacierno's task in 1m 10s —— View job


Code Review

  • Read all changed files
  • Analyze implementation
  • Post review feedback

Summary

Overall, 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 Found

1. 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 title(language: "en") which returns a string, not an object. The assertion should be:

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.).

Fix this →

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:

  • test_cannot_submit_more_than_3_proposals tests when max_proposals_per_user=3 (the if branch)
  • test_can_submit_unlimited_proposals_when_max_proposals_is_none tests when max_proposals_per_user=None (the else branch)

However, the Codecov report indicates line 415 is missing coverage. This is likely because:

  1. Most existing tests don't set max_proposals_per_user at all, so it defaults to None
  2. The coverage report may be flagging that the condition itself isn't fully tested

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 Review

Model (backend/conferences/models/conference.py:120-125) ✅

Good:

  • ✅ Uses PositiveIntegerField (correct - can't have negative proposals)
  • ✅ Proper null=True, blank=True for optional field
  • ✅ Clear help text explaining the behavior
  • ✅ Good field name: max_proposals_per_user is descriptive

Migration (backend/conferences/migrations/0056_conference_max_proposals.py) ✅

Good:

  • ✅ Correctly uses null=True, blank=True
  • ✅ Proper field configuration matches model
  • ✅ Safe migration (adds nullable field, no data migration needed)

API Logic (backend/api/submissions/mutations.py:415-426) ✅

Good:

  • ✅ Proper null check using is not None (important for distinguishing None from 0)
  • ✅ Uses .non_cancelled() to only count active submissions
  • ✅ Correct error message that includes the actual limit
  • ✅ Validation happens before creating the submission

Consideration: The logic correctly uses >= comparison (line 422), which means if max_proposals_per_user=3, users can submit exactly 3 proposals (0, 1, 2 are allowed; 3 is blocked). This is the expected behavior based on the test.

Tests (backend/api/submissions/tests/test_send_submission.py) ✅

Good:

  • ✅ Test for enforcing the limit (test_cannot_submit_more_than_3_proposals)
  • ✅ Test for unlimited submissions (test_can_submit_unlimited_proposals_when_max_proposals_is_none)
  • ✅ Both tests create exactly 3 existing submissions and try to add a 4th
  • ✅ Good coverage of edge cases

Issue: The assertion error mentioned in #1 above.

Admin (backend/conferences/admin/conference.py:184) ✅

Good:

  • ✅ Placed in the "Conference" fieldset (logical grouping)
  • ✅ Position after other proposal-related fields (proposal_tags)
  • ✅ Will display with the help text from the model

Minor Suggestions

  1. Consider adding validation for max_proposals_per_user = 0: Currently, setting it to 0 would block all submissions. Consider adding a MinValueValidator(1) to the model field if 0 shouldn't be a valid value, or add a note in the help text.

  2. Documentation: Consider updating any user-facing documentation about CFP limits (though this may be out of scope for this PR).


Security & Performance

  • ✅ No SQL injection risks (using Django ORM properly)
  • ✅ No N+1 query issues (single count query)
  • ✅ Proper authentication check (uses IsAuthenticated permission)
  • ✅ No XSS risks (error messages don't include user input)

Conclusion

This 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


@marcoacierno
Copy link
Member Author

@claude the test test_can_submit_unlimited_proposals_when_max_proposals_is_none is failing

>       assert resp["data"]["sendSubmission"]["__typename"] == "Submission"
E       AssertionError: assert 'SendSubmissionErrors' == 'Submission'
E         - Submission
E         + SendSubmissionErrors

@claude
Copy link
Contributor

claude bot commented Jan 8, 2026

Claude finished @marcoacierno's task in 4m 36s —— View job


Task completed

  • Read the failing test to understand what it's doing
  • Read the mutation code to check validation logic
  • Identify the bug
  • Implement the fix
  • Commit and push changes

Summary

I've fixed the failing test test_can_submit_unlimited_proposals_when_max_proposals_is_none. There were two issues:

  1. Incorrect assertion on line 1309: The test was checking resp["data"]["sendSubmission"]["title"]["en"], but the GraphQL query title(language: "en") returns a string, not an object. Fixed to resp["data"]["sendSubmission"]["title"].

  2. Explicit None parameter: Removed the explicit max_proposals_per_user=None parameter from the ConferenceFactory call. Instead, let it default to None naturally. Some factory implementations can be sensitive to explicitly passed None values vs. omitted parameters.

Changes Made

  • Fixed the title assertion to match the actual GraphQL response structure
  • Updated the test to rely on the default None value for max_proposals_per_user rather than explicitly passing it

The test should now pass. The mutation logic correctly skips the proposal limit check when max_proposals_per_user is None, allowing unlimited submissions.


| View commit

- 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>
@marcoacierno marcoacierno merged commit 2e96a70 into main Jan 10, 2026
7 checks passed
@marcoacierno marcoacierno deleted the claude/issue-4523-20260107-0109 branch January 10, 2026 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make proposal limit customisable by conference

2 participants