Add ability to specify if a talk should be recorded

[marcoacierno]

What

ToDo

[vercel]

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

Project	Deployment	Review	Updated (UTC)
pycon	Error		Dec 14, 2025 2:43pm

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.49%. Comparing base (13492ee) to head (e1b111b).

Additional details and impacted files

@@               Coverage Diff               @@
##           cfp-updates    #4510      +/-   ##
===============================================
- Coverage        94.49%   94.49%   -0.01%     
===============================================
  Files              352      352              
  Lines            10143    10141       -2     
  Branches           735      734       -1     
===============================================
- Hits              9585     9583       -2     
  Misses             463      463              
  Partials            95       95              

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

[marcoacierno]

@pythonitaliabot2 review this PR

[marcoacierno]

@claude review this PR

[claude]

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

---

PR Review: Add ability to specify if a talk should be recorded

Tasks

• Read and analyze all changed files
• Review backend implementation (models, migrations, API)
• Review frontend implementation (UI, GraphQL)
• Review test coverage
• Provide comprehensive feedback

---

Summary

This PR successfully implements the ability for speakers to specify whether they want their talk to be recorded. The implementation includes both backend and frontend changes with good test coverage. Overall, the code is well-structured and follows the existing patterns in the codebase.

Architecture Overview

The feature introduces two key concepts:

1. Submission.do_not_record: Boolean field on submissions indicating the speaker's preference
2. SubmissionType.is_recordable: Boolean field to control which submission types can be recorded (e.g., talks vs. workshops)

Review Findings

✅ Strengths

1. Good Test Coverage: Both test_send_submission.py and test_edit_submission.py include tests for the new field
2. Proper Field Visibility: The do_not_record field is correctly marked as a private field (backend/api/submissions/types.py:122), only visible to admins and the submitter
3. UI Conditional Display: The checkbox only shows when isRecordable is true (frontend/src/components/cfp-form/proposal-section.tsx:240)
4. Complete GraphQL Integration: The field is properly exposed through GraphQL queries and mutations
5. Database Migrations: Two clean migrations handle the schema changes
6. Admin Interface: The field is visible in the Django admin (backend/submissions/admin.py:252)
7. Good User Experience: Clear labels and descriptions in both English and Italian

🔍 Observations & Suggestions

1. Naming Inconsistency (Minor)

The field is named do_not_record (negative logic), but this creates double negatives in the UI. The checkbox label says "I confirm that I do not want my talk to be recorded" which is clear but verbose.

Consider: In a future PR, you might want to rename to allow_recording (positive logic) with default=True, which would make the UI simpler: "Allow my talk to be recorded" with an unchecked state meaning "do not record".

Current logic:

• do_not_record=False → Will be recorded (default)
• do_not_record=True → Will NOT be recorded

2. Missing Validation (Medium Priority)

Location: backend/api/submissions/mutations.py:230, 261

The API accepts do_not_record for any submission type, even if type.is_recordable=False. While the UI correctly hides the checkbox, a malicious user could send the field via the API.

Suggestion: Add validation in BaseSubmissionInput.validate():

# After line 181 in mutations.py
if input.do_not_record:
    submission_type = conference.submission_types.filter(id=input.type).first()
    if submission_type and not submission_type.is_recordable:
        errors.add_error(
            "do_not_record", 
            "This submission type cannot be recorded"
        )

3. GraphQL Schema Documentation (Low Priority)

Location: backend/api/submissions/types.py:42, 122

The GraphQL fields is_recordable and do_not_record lack descriptions. Adding strawberry.field(description="...") would improve the auto-generated GraphQL documentation.

Example:

@strawberry.type
class SubmissionType:
    id: strawberry.ID
    name: str
    is_recordable: bool = strawberry.field(
        description="Indicates if proposals of this type can be recorded"
    )

4. TypeScript Type Definitions (Minor)

Location: frontend/src/components/cfp-form/index.tsx:75, 100

The SubmissionStructure type at line 78-105 doesn't include the doNotRecord field, though it's used in the form at line 75. This might cause TypeScript issues.

Suggestion: Add to SubmissionStructure:

doNotRecord: boolean;

5. Default Value Consideration (Low Priority)

Location: backend/submissions/models.py:110-114

The field defaults to False (will be recorded). Consider if this should be configurable per conference or submission type, as recording policies might vary.

6. Missing Edge Case Test (Low Priority)

The tests cover:

• ✅ Setting do_not_record=True
• ✅ Default value (False)
• ❌ Attempting to set do_not_record=True on a non-recordable submission type

Suggestion: Add a test to verify the suggested validation in point #2.

📝 Code Quality

• Migrations: Clean and properly ordered (0029 then 0030)
• GraphQL Integration: Properly integrated with queries and mutations
• Localization: Both English and Italian translations provided
• Consistency: Follows existing code patterns and conventions
• Default Values: Sensible defaults that maintain backward compatibility

🚀 What Works Well

1. The conditional UI rendering based on isRecordable is elegant
2. The confirmation message clearly explains the implications
3. The feature integrates seamlessly with existing CFP workflow
4. Private field visibility ensures data protection

⚠️ Potential Issues

1. Medium: Missing API-level validation for non-recordable types (see Login / Authentication #2)
2. Low: Missing TypeScript type definition (see Conference #4)

🎯 Recommendations

Before Merge:

• Add validation to prevent setting do_not_record=True on non-recordable submission types

Future Improvements:

• Add GraphQL field descriptions
• Consider renaming to positive logic (allow_recording)
• Add test for the validation mentioned above

📊 Test Coverage

The PR maintains test coverage at 94.49% with comprehensive tests for both create and update operations. All modified lines are covered.

---

Conclusion

This is a solid implementation that adds a useful feature for speakers. The only concern is the missing validation on the backend to enforce the is_recordable constraint via the API. Once that's addressed, this PR should be good to merge.

Great work on the comprehensive implementation across backend, frontend, tests, and localization! 🎉

---