Refactor training day submissions to use managing contest participation #86
Conversation
This refactoring changes how submissions are handled in training programs: 1. Add training_day_id field to Submission model to track which training day a submission was made via (or None for task archive/regular contest submissions) 2. Update SubmitHandler to use managing contest participation for training day submissions instead of training day participation, while recording the training_day_id on the submission 3. Update score cache logic to update both managing contest participation cache AND training day participation cache when a training day submission is scored 4. Update CWS TaskSubmissionsHandler to: - Filter submissions by training_day_id for training day context - Group submissions by source (task archive vs training day) for task archive view 5. Update CWS task_submissions.html template to display submissions grouped by source in task archive view 6. Update AWS ContestSubmissionsHandler to: - Filter submissions by training_day_id for training day contests - Pass is_training_program flag to template 7. Update AWS submission macro and contest_submissions.html to show a 'Source' column for training program submissions indicating which training day the submission came from (or 'Task Archive') 8. Add database migration (update_from_1.5.sql and update_50.py) to add training_day_id column to submissions table Co-Authored-By: Ron Ryvchin <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds nullable Changes
Sequence DiagramsequenceDiagram
participant User
participant ContestHandler as Contest Handler
participant DB as Database
participant Scoring as ScoringService
User->>ContestHandler: Submit solution (optionally via TrainingDay)
ContestHandler->>DB: Create Submission (set training_day_id when applicable)
DB-->>ContestHandler: Submission saved
ContestHandler->>Scoring: Request scoring for Submission
activate Scoring
Scoring->>DB: Load Submission, Dataset
DB-->>Scoring: Submission + Dataset
alt Submission.training_day_id present
Scoring->>DB: Lookup managing Participation and training-day Participation
DB-->>Scoring: Participation(s)
Scoring->>DB: Update/invalidate score cache for managing & training-day Participation
DB-->>Scoring: Acknowledged
else no training_day
Scoring->>DB: Update/invalidate score cache for participation only
DB-->>Scoring: Acknowledged
end
deactivate Scoring
Scoring-->>User: Scoring complete / results available
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Per code review feedback, moved the training day participation cache handling from scorecache.py to the callers in ScoringService.py: 1. In ScoringExecutor.execute: After update_score_cache, if the submission has a training_day_id, invalidate the training day participation's cache so it gets rebuilt with only training day submissions. 2. In invalidate_submission: When collecting affected (participation, task) pairs, also add the training day participation pairs for submissions that have training_day_id set. This keeps scorecache.py focused on its core responsibility and makes the training day logic explicit at the call sites. Co-Authored-By: Ron Ryvchin <[email protected]>
The TrainingProgramSubmissionsHandler was rendering contest_submissions.html without passing is_training_program, which is needed by the macro to show the training day source column. Co-Authored-By: Ron Ryvchin <[email protected]>
1. DownloadContestSubmissionsHandler: For training day contests, now filters by training_day_id to only download submissions made via that training day. 2. DownloadUserContestSubmissionsHandler: Same fix for per-user downloads. 3. New DownloadTrainingProgramSubmissionsHandler: Downloads all submissions for a training program with folder structure: user/task/source/official/ where source is either 'task_archive' or the training day description. 4. New DownloadTrainingProgramStudentSubmissionsHandler: Same for per-student downloads in a training program. 5. Updated templates to use the correct download URLs for training programs. Co-Authored-By: Ron Ryvchin <[email protected]>
ronryv
force-pushed
the
devin/1768049815-training-day-submission-refactor
branch
from
44c8fce to
fb0919e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cms/service/ScoringService.py (1)
109-129: Useone_or_none()instead offirst()for the unique (contest_id, user_id) lookup; compare by IDs to avoid unnecessary relationship loads.The Participation table has a UNIQUE constraint on (contest_id, user_id), so the query is guaranteed to return at most one row. However, using
.first()is semantically incorrect—it suggests no uniqueness guarantee and masks the intent. Use.one_or_none()to enforce the uniqueness expectation. Additionally, comparing by IDs rather than relationship objects eliminates unnecessary relationship loads:Recommended change
- training_day_participation = session.query(Participation).filter( - Participation.contest == training_day.contest, - Participation.user == submission.participation.user, - ).first() + training_day_participation = ( + session.query(Participation) + .filter( + Participation.contest_id == training_day.contest_id, + Participation.user_id == submission.participation.user_id, + ) + .one_or_none() + )
🤖 Fix all issues with AI agents
In @cms/server/admin/handlers/contestsubmission.py:
- Around line 42-60: In ContestSubmissionsHandler where is_training_program is
computed, set self.r_params["training_program"] when contest.training_program is
not None so the template contest_submissions.html sees training_program and
routes the "Download all submissions" link to the training program endpoint;
update the block around is_training_program (used before calling
render_params_for_submissions and assigning
self.r_params["is_training_program"]) to assign
self.r_params["training_program"] = contest.training_program when true.
🧹 Nitpick comments (9)
cms/service/ScoringService.py (1)
244-266: De-duplicate training-day participation lookup (execute vs invalidate_submission) + memoize inside the loop.
This repeats the same “training day participation for (contest,user)” lookup pattern as Line 118-123, and ininvalidate_submissionit can run many times for the same(training_day.contest_id, user_id)during bulk invalidations.Suggestion: extract a small helper (module-level or private staticmethod) and in
invalidate_submissionmemoize by(training_day.contest_id, user_id)to avoid N queries.cmscontrib/updaters/update_from_1.5.sql (1)
514-523: Schema change looks consistent (nullable FK + SET NULL + index).
Only minor nit: elsewhere this file sometimes omitspublic.(e.g., earlierALTER TABLE submissions ...); consider standardizing schema qualification for consistency.cmscontrib/updaters/update_50.py (1)
30-43: Good: updater only transforms objects and doesn’t mutate_version. (Based on learnings, updaters shouldn’t setself.objs["_version"]inrun().)
Consider hardening against malformed dumps and re-runs by usingget()+setdefault().Proposed defensive tweak
for k, v in self.objs.items(): if k.startswith("_"): continue - if v["_class"] == "Submission": - v["training_day_id"] = None + if v.get("_class") == "Submission": + v.setdefault("training_day_id", None)cms/server/admin/templates/student.html (1)
37-37: Use a consistent student id in URLs (selected_user.idvsparticipation.user_id).This link uses
participation.user_idwhile other links on the page useselected_user.id. If those ever diverge, this would route incorrectly; using one consistently avoids the risk.Proposed tweak
- <a href="{{ url("training_program", training_program.id, "student", participation.user_id, "submissions", "download") }}">Download all submissions</a> + <a href="{{ url("training_program", training_program.id, "student", selected_user.id, "submissions", "download") }}">Download all submissions</a>cms/db/training_day.py (1)
91-95: TrainingDay↔Submission relationship wiring looks correct (back_populates).Optional: if you rely on DB-level
ON DELETE SET NULLand want to avoid ORM-side UPDATEs when deleting a TrainingDay, considerpassive_deletes=Truehere (only if consistent with existing relationship patterns).cms/server/admin/templates/contest_submissions.html (1)
25-32: Guard against missingis_training_programin template context (StrictUndefined risk).If any render path forgets to set
is_training_program, this can blow up; a|default(false)makes the template resilient.Proposed tweak
@@ - show_training_day=is_training_program) }} + show_training_day=is_training_program|default(false)) }}cms/server/admin/handlers/submissiondownload.py (3)
228-269: New training program download handler looks correct.The handler properly:
- Fetches the training program and its managing contest
- Queries all submissions for tasks in the managing contest
- Uses a
base_path_builderthat includes the source folder based on training day associationConsider eager loading to avoid N+1 queries: Accessing
submission.training_day.contest.descriptioninside the loop (line 252) could trigger lazy loads for each submission. Consider adding.options(joinedload(Submission.training_day).joinedload(TrainingDay.contest))to the query.♻️ Suggested optimization
+from sqlalchemy.orm import joinedload +from cms.db import TrainingDay + # Get all submissions for the managing contest submissions = ( self.sql_session.query(Submission) .join(Task) .filter(Task.contest_id == managing_contest.id) + .options(joinedload(Submission.training_day).joinedload(TrainingDay.contest)) .all() )
295-309: Same N+1 query concern and duplicated source folder logic.The
source_folderdetermination logic (lines 303-308) duplicates the logic inDownloadTrainingProgramSubmissionsHandler(lines 249-254). Consider extracting a helper function and adding eager loading to the query.♻️ Extract helper and add eager loading
def get_source_folder(submission): """Get the source folder name for a submission.""" if submission.training_day_id is not None: return submission.training_day.contest.description return "task_archive"And add to the query:
submissions = ( self.sql_session.query(Submission) .filter(Submission.participation_id == participation.id) + .options(joinedload(Submission.training_day).joinedload(TrainingDay.contest)) .all() )
75-80: Optional: Use list unpacking for cleaner syntax.Per static analysis, consider using list unpacking instead of concatenation for slightly cleaner code.
♻️ Suggested change
- path_parts = base_path_parts + [official_folder] + path_parts = [*base_path_parts, official_folder] for filename, file_obj in submission.files.items(): real_filename = safe_get_lang_filename(submission.language, filename) prefixed_filename = f"{timestamp}_{status}_{real_filename}" - file_path = "/".join(path_parts + [prefixed_filename]) + file_path = "/".join([*path_parts, prefixed_filename])
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cms/db/submission.pycms/db/training_day.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/submissiondownload.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/templates/contest_submissions.htmlcms/server/admin/templates/macro/submission.htmlcms/server/admin/templates/student.htmlcms/server/contest/handlers/base.pycms/server/contest/handlers/tasksubmission.pycms/server/contest/templates/task_submissions.htmlcms/service/ScoringService.pycmscontrib/updaters/update_50.pycmscontrib/updaters/update_from_1.5.sql
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-09T19:33:59.218Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cms/server/admin/handlers/dataset.py:1323-1387
Timestamp: 2026-01-09T19:33:59.218Z
Learning: In cms/server/admin/handlers/**/*.py, follow the existing pattern: do not emit explicit error notifications when try_commit() fails. Rely on centralized error handling and logging instead. Apply this consistently to all new and updated handlers to maintain uniform behavior and maintainability.
Applied to files:
cms/server/admin/handlers/trainingprogram.pycms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/submissiondownload.pycms/server/admin/handlers/__init__.py
📚 Learning: 2026-01-02T19:12:00.691Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cmscontrib/updaters/update_49.py:36-37
Timestamp: 2026-01-02T19:12:00.691Z
Learning: In CMS codebase, updater modules matching cmscontrib/updaters/update_*.py should not set self.objs["_version"] in their run() methods. The schema version is updated via the VERSION constant in cms/db/__init__.py instead. Updaters should only assert the incoming version and transform data.
Applied to files:
cmscontrib/updaters/update_50.py
🧬 Code graph analysis (6)
cms/db/submission.py (1)
cms/db/training_day.py (1)
TrainingDay(36-95)
cms/server/contest/handlers/tasksubmission.py (3)
cms/db/user.py (1)
Participation(179-345)cms/server/contest/submission/workflow.py (1)
accept_submission(170-322)cms/db/submission.py (1)
Submission(45-222)
cms/server/contest/handlers/base.py (2)
cms/server/util.py (1)
exclude_internal_contests(54-61)cms/db/contest.py (1)
Contest(47-384)
cms/server/admin/handlers/contestsubmission.py (4)
cms/db/submission.py (1)
Submission(45-222)cms/log.py (1)
filter(340-354)cms/db/task.py (1)
Task(55-358)cms/server/admin/handlers/base.py (1)
render_params_for_submissions(713-749)
cms/server/admin/handlers/submissiondownload.py (2)
cms/db/training_program.py (1)
TrainingProgram(36-81)cms/grading/languagemanager.py (1)
safe_get_lang_filename(56-72)
cms/server/admin/handlers/__init__.py (1)
cms/server/admin/handlers/submissiondownload.py (3)
DownloadContestSubmissionsHandler(187-225)DownloadTrainingProgramSubmissionsHandler(228-269)DownloadTrainingProgramStudentSubmissionsHandler(272-317)
🪛 Ruff (0.14.10)
cms/server/admin/handlers/submissiondownload.py
75-75: Consider [*base_path_parts, official_folder] instead of concatenation
Replace with [*base_path_parts, official_folder]
(RUF005)
80-80: Consider [*path_parts, prefixed_filename] instead of concatenation
Replace with [*path_parts, prefixed_filename]
(RUF005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (ubuntu:noble)
🔇 Additional comments (16)
cms/service/ScoringService.py (1)
31-32: Import change is fine; consider keeping the cms.db import list stable.
No functional concerns here.cms/server/admin/handlers/__init__.py (2)
342-345: Routes look correct and non-overlapping.
Nice addition alongside the existing training program submissions view.
105-110: Handler symbols are properly defined and importable. BothDownloadTrainingProgramSubmissionsHandler(line 228) andDownloadTrainingProgramStudentSubmissionsHandler(line 272) exist insubmissiondownload.pyand are reachable for import.cms/server/admin/handlers/trainingprogram.py (1)
1122-1130: LGTM. Templates and macros are already safe.The macro parameters have explicit defaults (
show_training_day=false), and all template conditionals safely check the flag with{% if %}rather than direct indexing. Both handlers (ContestSubmissionsHandlerand the training program handler) explicitly setis_training_programbefore rendering, so no KeyError risk exists.cms/db/training_day.py (1)
32-34: TYPE_CHECKING import update looks correct.cms/server/contest/handlers/base.py (3)
266-271:~Contest.training_day.has()exclusion is correct here.
340-346: Root-level contests query correctly excludes training-day contests.
355-361: Folder-level contests query correctly excludes training-day contests.cms/server/admin/templates/contest_submissions.html (1)
18-23: Download link branching is clear; ensuretraining_programis always set in training-program contexts.cms/db/submission.py (1)
88-101: LGTM! Well-structured training day association.The new
training_day_idcolumn and relationship follow established patterns in this file. Theondelete="SET NULL"behavior is appropriate since submissions should remain valid even if the training day is deleted. The index on the FK column will support efficient filtering by training day.cms/server/contest/templates/task_submissions.html (1)
370-443: LGTM! Clear separation of submissions by source.The template correctly branches on
is_task_archiveto provide a grouped view distinguishing task archive submissions from training day submissions. The pattern of rendering unofficial submissions first (with header) followed by all submissions for the official section is consistent.cms/server/admin/templates/macro/submission.html (1)
97-105: LGTM! Clean conditional Source column rendering.The Source column correctly displays a link to the training day's contest when
s.training_dayis present, and falls back to "Task Archive" text otherwise. The conditional column header and body cells are properly synchronized via theshow_training_dayparameter.cms/server/contest/handlers/tasksubmission.py (2)
105-133: Training day submission flow looks correct.The logic properly:
- Identifies training day context from
self.contest.training_day- Fetches the managing contest participation for the current user
- Uses that participation for
accept_submissionto ensure submissions are stored under the managing contest- Sets
training_dayreference after submission creation (appropriate sinceaccept_submissiondoesn't know about training days)The 403 error for missing managing participation is appropriate.
176-230: Submission filtering and grouping logic is well-structured.The branching correctly handles three scenarios:
- Training day context: Filters by managing participation + specific
training_day_id- Task archive context: Fetches all submissions for participation, then groups by source
- Regular contest: Standard participation-based filtering
The grouping at lines 218-230 cleanly separates archive submissions (no training day) from training-day-sourced submissions.
cms/server/admin/handlers/submissiondownload.py (2)
155-168: LGTM! Training day filtering for user contest downloads.The handler correctly distinguishes between training day contests (filtering by
training_day_id) and regular contests (downloading all submissions for the participation).
198-212: LGTM! Training day filtering for contest downloads.The handler properly filters by
training_day_idfor training day contests and falls back to task-based filtering for regular/managing contests.
1. Pass training_program to template in ContestSubmissionsHandler so the download link routes correctly for managing contest submissions. 2. Extract _get_training_day_participation helper function to de-duplicate the training day participation lookup pattern. 3. Add memoization in invalidate_submission to avoid N queries during bulk invalidations when multiple submissions share the same (training_day_contest_id, user_id) pair. Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cms/server/admin/templates/contest_submissions.html (1)
18-33: Useis_training_program(nottraining_program is defined) to choose download route.Relying on
training_program is defined(Line 18) can pick the wrong route if some other handler providestraining_programincidentally; you already haveis_training_programfor the macro, so it’s better to use the same flag for the link.Proposed fix
- {% if training_program is defined %} + {% if is_training_program|default(false) %} <a href="{{ url("training_program", training_program.id, "submissions", "download") }}">Download all submissions</a> {% else %} <a href="{{ url("contest", contest.id, "submissions", "download") }}">Download all submissions</a> {% endif %}
🧹 Nitpick comments (3)
cms/server/contest/templates/task_submissions.html (1)
370-481: Make training-day sections deterministic + fix “Official submissions” header condition.
training_day_submissions.items()(Line 408) will render in whatever order the dict was populated; consider sorting bytraining_day.position(or description) here for stable UI.- “Official submissions” heading is only rendered when unofficial submissions exist (Line 412-428), but the official rows render unconditionally (Line 430-442). This leads to an unlabeled “official” block when there are no unofficial submissions.
Proposed tweak (stable ordering + consistent headings)
-{% for td_id, td_data in training_day_submissions.items() %} +{% for td_id, td_data in training_day_submissions.items()|sort(attribute="1.0.position") %} {% set training_day = td_data[0] %} {% set td_submissions = td_data[1] %} <h3>{% trans td_desc=training_day.contest.description %}Submissions from training day: {{ td_desc }}{% endtrans %}</h3> -{% if td_submissions|rejectattr("official")|list|length > 0 %} +{% set td_has_unofficial = td_submissions|rejectattr("official")|list|length > 0 %} +{% if td_has_unofficial %} <h4>{% trans %}Unofficial submissions{% endtrans %}</h4> {{ macro_submission.rows( url, contest_url, translation, xsrf_form_html, actual_phase, task, td_submissions, can_use_tokens, can_play_token, can_play_token_now, submissions_download_allowed, false) }} -<h4>{% trans %}Official submissions{% endtrans %}</h4> {% endif %} + +<h4>{% trans %}Official submissions{% endtrans %}</h4> {{ macro_submission.rows( url, contest_url, translation, xsrf_form_html, actual_phase, task, td_submissions, can_use_tokens, can_play_token, can_play_token_now, submissions_download_allowed, true) }} {% endfor %}cms/server/contest/handlers/tasksubmission.py (1)
176-216: Verify query efficiency with eager loading.The submission queries correctly use
joinedloadfortokenandresults, which is good for avoiding N+1 queries. However, whentraining_dayis not None, the submissions are filtered bytraining_day_id, and later in the template,s.training_daymay be accessed. Consider addingjoinedload(Submission.training_day)to the query at lines 198-206 if the training day relationship is accessed in the template.♻️ Optional: Add joinedload for training_day relationship
submissions: list[Submission] = ( self.sql_session.query(Submission) .filter(Submission.participation == managing_participation) .filter(Submission.task == task) .filter(Submission.training_day_id == training_day.id) .options(joinedload(Submission.token)) .options(joinedload(Submission.results)) + .options(joinedload(Submission.training_day)) .all() )cms/server/admin/handlers/submissiondownload.py (1)
44-48: Potential path traversal/sanitization concern with contest description.The
get_source_folderfunction usessubmission.training_day.contest.descriptiondirectly in file paths. If the contest description contains characters like/,\, or other special path characters, this could create unexpected directory structures or potentially malformed zip entries.Consider sanitizing the description to replace problematic characters:
♻️ Proposed fix for path sanitization
def get_source_folder(submission): """Get the source folder name for a submission.""" if submission.training_day_id is not None: - return submission.training_day.contest.description + # Sanitize description for use as folder name + description = submission.training_day.contest.description + # Replace path separators and other problematic characters + return description.replace("/", "_").replace("\\", "_").replace(":", "_") return "task_archive"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cms/db/submission.pycms/db/training_day.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/submissiondownload.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/templates/contest_submissions.htmlcms/server/admin/templates/macro/submission.htmlcms/server/admin/templates/student.htmlcms/server/contest/handlers/base.pycms/server/contest/handlers/tasksubmission.pycms/server/contest/templates/task_submissions.htmlcms/service/ScoringService.pycmscontrib/updaters/update_49.pycmscontrib/updaters/update_from_1.5.sql
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-09T19:33:59.218Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cms/server/admin/handlers/dataset.py:1323-1387
Timestamp: 2026-01-09T19:33:59.218Z
Learning: In cms/server/admin/handlers/**/*.py, follow the existing pattern: do not emit explicit error notifications when try_commit() fails. Rely on centralized error handling and logging instead. Apply this consistently to all new and updated handlers to maintain uniform behavior and maintainability.
Applied to files:
cms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/submissiondownload.py
📚 Learning: 2026-01-02T19:12:00.691Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cmscontrib/updaters/update_49.py:36-37
Timestamp: 2026-01-02T19:12:00.691Z
Learning: In CMS codebase, updater modules matching cmscontrib/updaters/update_*.py should not set self.objs["_version"] in their run() methods. The schema version is updated via the VERSION constant in cms/db/__init__.py instead. Updaters should only assert the incoming version and transform data.
Applied to files:
cmscontrib/updaters/update_49.py
🧬 Code graph analysis (5)
cms/server/contest/handlers/tasksubmission.py (3)
cms/db/user.py (1)
Participation(179-345)cms/server/contest/submission/workflow.py (1)
accept_submission(170-322)cms/db/submission.py (1)
Submission(45-222)
cmscontrib/updaters/update_49.py (1)
cms/server/admin/handlers/submissiondownload.py (5)
get(129-147)get(158-199)get(210-240)get(251-283)get(294-330)
cms/db/submission.py (1)
cms/db/training_day.py (1)
TrainingDay(36-96)
cms/server/contest/handlers/base.py (2)
cms/server/util.py (1)
exclude_internal_contests(54-61)cms/db/contest.py (1)
Contest(47-384)
cms/server/admin/handlers/__init__.py (1)
cms/server/admin/handlers/submissiondownload.py (3)
DownloadContestSubmissionsHandler(202-240)DownloadTrainingProgramSubmissionsHandler(243-283)DownloadTrainingProgramStudentSubmissionsHandler(286-330)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (debian:bookworm)
🔇 Additional comments (28)
cms/server/admin/handlers/trainingprogram.py (1)
1127-1129: LGTM!The
is_training_programflag is correctly set to enable training-day column rendering in the submissions template. The placement is appropriate—after query construction and before rendering.cmscontrib/updaters/update_49.py (1)
43-47: LGTM!The migration logic correctly initializes the new
training_day_idfield on all Submission objects with a default ofNone. The loop properly skips internal keys and usessetdefaultto avoid overwriting existing values.cms/server/contest/handlers/base.py (3)
266-271: LGTM! Idiomatic SQLAlchemy relationship filtering.The change from
Contest.training_day is Noneto~Contest.training_day.has()is the correct SQLAlchemy pattern for filtering on relationship existence. This properly excludes training-day contests from the folder tree.
340-346: LGTM! Consistent relationship filtering at root level.Correctly applies the
~Contest.training_day.has()pattern to exclude training-day contests when browsing the root folder level.
355-361: LGTM! Consistent relationship filtering for specific folders.Correctly applies the
~Contest.training_day.has()pattern to exclude training-day contests when browsing a specific folder.cmscontrib/updaters/update_from_1.5.sql (1)
514-523: LGTM! Proper schema addition for training_day_id.The migration correctly adds the
training_day_idcolumn with:
- Nullable integer type (appropriate since not all submissions are from training days)
- FK constraint with
ON DELETE SET NULL(preserves submissions when training day is deleted)- Btree index for efficient filtering/joining
cms/db/training_day.py (2)
33-33: LGTM! Proper TYPE_CHECKING import.Adding
Submissionto the TYPE_CHECKING imports correctly avoids circular import issues while maintaining type hints.
92-96: LGTM! Correct bidirectional relationship.The
submissionsrelationship is properly configured:
back_populates="training_day"establishes the bidirectional linkpassive_deletes=Truelets the database handle theON DELETE SET NULLFK constraintcms/db/submission.py (1)
41-102: Import structure is safe; no circular dependency risk.
training_day_id+training_dayrelationship (Lines 88–101) matches the intended model, and the FK semantics (SET NULL) are appropriate.The import of
TrainingDayinsubmission.pyis safe becausetraining_day.pyuses aTYPE_CHECKINGguard for itsSubmissionimport (line 33 oftraining_day.py). This eliminates runtime circular dependency risk, so direct imports are fine and no string-based escape hatch is needed.cms/server/admin/templates/student.html (1)
31-38: Routes are correctly parameterized.The
selected_user.idused in lines 34 and 37 is always identical toparticipation.user_id. BothStudentHandler.get()andDownloadTrainingProgramStudentSubmissionsHandler.get()query the participation directly byuser_idfrom the URL parameter, andselected_useris set asparticipation.userin the render context. No context-switching issues exist.cms/server/admin/handlers/contestsubmission.py (1)
42-62: Verify: Training day submission filtering design choice clarification needed.The code filters training day contest submissions by
Submission.training_day_id == contest.training_day.id, which means pre-existing submissions (withtraining_day_id = NULL) will not appear. However, the migration intentionally sets all legacy submissions totraining_day_id = Nonewith no backfill—this appears to be by design, not an oversight. The training_day_id field specifically tracks submissions made via the training day interface, not all submissions for the contest.Clarify in comments or documentation whether admins should see only submissions made through the training day interface (current behavior), or all submissions for tasks assigned to that training day. If the current filtering is intentional, add explicit documentation to prevent future confusion.
cms/service/ScoringService.py (4)
31-32: LGTM!The import of
Participationis correctly added alongside other database models and is needed for the new training day participation lookup functionality.
43-67: LGTM!The helper function is well-documented and correctly queries for a user's participation in a training day's contest. Using
one_or_none()appropriately handles the case where no participation exists.
140-155: Potential issue: cache invalidation occurs beforesession.commit().The score cache for the training day participation is invalidated at lines 151-155, but the
session.commit()happens later at line 158. If the commit fails, the cache will have been invalidated but the score update won't persist, potentially leaving the cache in an inconsistent state (though it would be rebuilt lazily on next access, so the impact is minimal).Additionally, consider whether
update_score_cache(line 138) andinvalidate_score_cache(line 151) should both be called within the same transaction flow, or if there's a reason for the asymmetry (update for managing contest participation, invalidate for training day participation).
272-299: LGTM! Good use of memoization for bulk operations.The
training_day_participation_cachedictionary correctly avoids redundant database queries when processing multiple submissions from the same training day/user combination during bulk invalidations. The pattern follows the existing affected_pairs collection logic.cms/server/admin/templates/macro/submission.html (3)
5-5: LGTM!The
show_training_dayparameter is properly added with a sensible default offalse, and the documentation clearly explains its purpose for training program submissions view.Also applies to: 18-19
44-46: LGTM!The conditional header column is correctly rendered only when
show_training_dayis true, and the parameter is properly propagated to therowmacro.Also applies to: 58-58
97-105: LGTM!The Source cell logic correctly distinguishes between training day submissions (linking to the training day's contest) and task archive submissions. The conditional rendering is properly guarded by
show_training_day.cms/server/contest/handlers/tasksubmission.py (3)
105-133: Logic is correct; submission correctly linked to training day.The code properly:
- Determines the training day context from
self.contest.training_day- Looks up the managing contest participation for training day submissions
- Returns 403 if the user lacks managing contest participation
- Uses the managing participation for the submission while recording the training day reference
One minor note: the
training_dayassignment at line 133 occurs beforesql_session.commit(), which is correct since it needs to be part of the same transaction.
218-230: LGTM! Grouping logic for task archive view.The code correctly separates submissions into:
archive_submissions: submissions without a training day (made directly in task archive)training_day_submissions: grouped bytraining_day_idwith the training day objectThe dictionary structure
{training_day_id: (training_day, [submissions])}allows the template to access both the training day metadata and associated submissions efficiently.
268-270: LGTM!The new template context variables (
archive_submissions,training_day_submissions,is_task_archive) are correctly passed to enable the template to render grouped submissions and distinguish task archive context.cms/server/admin/handlers/__init__.py (2)
107-109: LGTM!The new download handlers are correctly imported from the submissiondownload module.
343-344: LGTM!The route patterns follow the existing conventions:
/training_program/([0-9]+)/submissions/downloadfor all submissions in a training program/training_program/([0-9]+)/student/([0-9]+)/submissions/downloadfor a specific student's submissionsThese are consistent with the existing training program routes structure (e.g., lines 332-342).
cms/server/admin/handlers/submissiondownload.py (5)
88-95: LGTM!The path construction correctly uses list expansion (
[*base_path_parts, official_folder]) for clean path assembly, and theofficial/unofficialfolder is properly included in the path hierarchy.
170-183: LGTM! Training day filtering correctly implemented.For training day contests, submissions are properly filtered by both
participation_idandtraining_day_idto ensure only submissions made via that specific training day are included. The else branch handles regular contests correctly.
213-227: Minor inconsistency: training day contest query doesn't filter by participation.For training day contests (line 215-218), submissions are filtered only by
training_day_id, which would include all users' submissions. This differs from the non-training-day path which joins onTask.contest_id. Both paths produce correct results for their use cases (downloading all contest submissions), but the training day path could potentially include submissions from users not in this contest if the training day is somehow shared.This is likely fine given the data model constraints, but worth verifying that a training day is always associated with exactly one contest.
257-264: Good use of eager loading.The
joinedloadforSubmission.training_daychained withTrainingDay.contestensures theget_source_folderfunction can accesssubmission.training_day.contest.descriptionwithout triggering additional queries.
286-329: LGTM! Student-specific download handler is well implemented.The handler:
- Correctly validates that the user has a participation in the managing contest (404 if not)
- Properly eager-loads the training day relationship for efficient source folder lookup
- Uses consistent path structure (
task/source) as documentedThe overall implementation follows the pattern established by the other download handlers.
1. Use is_training_program|default(false) instead of 'training_program is defined' in contest_submissions.html template for consistent download URL routing. 2. Add joinedload(Submission.training_day) to queries in tasksubmission.py to avoid N+1 queries when accessing training_day relationship. 3. Add sanitize_path_component() function in submissiondownload.py to sanitize contest descriptions for safe use in zip file paths, preventing path traversal issues from special characters like / \ : * ? etc. Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cms/server/contest/handlers/tasksubmission.py (1)
105-123: Avoid repeating “managing participation lookup” and consider ordering submissions.
Both SubmitHandler and TaskSubmissionsHandler re-implement the same lookup; also, the submissions query doesn’t specify an order (UI/grouping can jitter).Proposed refactor (local helper + stable ordering)
@@ class SubmitHandler(ContestHandler): @@ def post(self, task_name): participation = self.current_user @@ - training_day = self.contest.training_day - submission_participation = participation - if training_day is not None: - # This is a training day submission - use managing contest participation - managing_contest = training_day.training_program.managing_contest - managing_participation = ( - self.sql_session.query(Participation) - .filter(Participation.contest == managing_contest) - .filter(Participation.user == participation.user) - .first() - ) - if managing_participation is None: - # User doesn't have a participation in the managing contest - raise tornado.web.HTTPError(403) - submission_participation = managing_participation + def _get_managing_participation(training_day, user): + managing_contest = training_day.training_program.managing_contest + return (self.sql_session.query(Participation) + .filter(Participation.contest == managing_contest) + .filter(Participation.user == user) + .first()) + + training_day = self.contest.training_day + submission_participation = participation + if training_day is not None: + submission_participation = _get_managing_participation( + training_day, participation.user + ) + if submission_participation is None: + raise tornado.web.HTTPError(403) @@ submission = accept_submission( self.sql_session, self.service.file_cacher, submission_participation, task, self.timestamp, self.request.files, self.get_argument("language", None), official) # Set the training day reference if submitting via a training day if training_day is not None: submission.training_day = training_day@@ class TaskSubmissionsHandler(ContestHandler): @@ if training_day is not None: @@ submissions: list[Submission] = ( self.sql_session.query(Submission) .filter(Submission.participation == managing_participation) .filter(Submission.task == task) .filter(Submission.training_day_id == training_day.id) + .order_by(Submission.timestamp.desc()) .options(joinedload(Submission.token)) .options(joinedload(Submission.results)) .options(joinedload(Submission.training_day)) .all() ) else: @@ submissions: list[Submission] = ( self.sql_session.query(Submission) .filter(Submission.participation == participation) .filter(Submission.task == task) + .order_by(Submission.timestamp.desc()) .options(joinedload(Submission.token)) .options(joinedload(Submission.results)) .options(joinedload(Submission.training_day)) .all() )Also applies to: 127-134
cms/server/contest/templates/task_submissions.html (1)
370-481: Missing "Official submissions" header when no unofficial submissions exist.The three submission sections (archive, training days, and regular) conditionally display the "Official submissions" header only when unofficial submissions exist. When a section has exclusively official submissions, users see a submissions table with no heading. Move the "Official submissions" header outside the conditional check so it always appears:
{% if archive_submissions|rejectattr("official")|list|length > 0 %} <h4>{% trans %}Unofficial submissions{% endtrans %}</h4> {{ macro_submission.rows(..., official=false) }} {% endif %} <h4>{% trans %}Official submissions{% endtrans %}</h4> {{ macro_submission.rows(..., official=true) }}The macro gracefully handles the case where no submissions match the filter, displaying "no submissions" in the table.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cms/db/submission.pycms/db/training_day.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/submissiondownload.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/templates/contest_submissions.htmlcms/server/admin/templates/macro/submission.htmlcms/server/admin/templates/student.htmlcms/server/contest/handlers/base.pycms/server/contest/handlers/tasksubmission.pycms/server/contest/templates/task_submissions.htmlcms/service/ScoringService.pycmscontrib/updaters/update_49.pycmscontrib/updaters/update_from_1.5.sql
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-09T19:33:59.218Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cms/server/admin/handlers/dataset.py:1323-1387
Timestamp: 2026-01-09T19:33:59.218Z
Learning: In cms/server/admin/handlers/**/*.py, follow the existing pattern: do not emit explicit error notifications when try_commit() fails. Rely on centralized error handling and logging instead. Apply this consistently to all new and updated handlers to maintain uniform behavior and maintainability.
Applied to files:
cms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/submissiondownload.py
📚 Learning: 2026-01-02T19:12:00.691Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cmscontrib/updaters/update_49.py:36-37
Timestamp: 2026-01-02T19:12:00.691Z
Learning: In CMS codebase, updater modules matching cmscontrib/updaters/update_*.py should not set self.objs["_version"] in their run() methods. The schema version is updated via the VERSION constant in cms/db/__init__.py instead. Updaters should only assert the incoming version and transform data.
Applied to files:
cmscontrib/updaters/update_49.py
🧬 Code graph analysis (4)
cms/db/submission.py (1)
cms/db/training_day.py (1)
TrainingDay(36-96)
cms/server/admin/handlers/__init__.py (1)
cms/server/admin/handlers/submissiondownload.py (3)
DownloadContestSubmissionsHandler(225-263)DownloadTrainingProgramSubmissionsHandler(266-306)DownloadTrainingProgramStudentSubmissionsHandler(309-353)
cms/server/admin/handlers/submissiondownload.py (4)
cms/db/training_program.py (1)
TrainingProgram(36-81)cms/db/training_day.py (1)
TrainingDay(36-96)cms/grading/languagemanager.py (1)
safe_get_lang_filename(56-72)cms/server/contest/handlers/tasksubmission.py (4)
get(161-280)get(335-395)get(405-446)get(456-502)
cms/server/contest/handlers/tasksubmission.py (3)
cms/db/user.py (1)
Participation(179-345)cms/server/contest/submission/workflow.py (1)
accept_submission(170-322)cms/db/submission.py (1)
Submission(45-222)
🔇 Additional comments (27)
cms/server/contest/handlers/tasksubmission.py (1)
268-280: Render params additions look good.
archive_submissions,training_day_submissions, andis_task_archiveare cleanly passed through for the template branching.cms/server/admin/handlers/trainingprogram.py (1)
1127-1129: LGTM: explicitis_training_programflag is a clear template signal.cms/db/training_day.py (1)
32-33: TrainingDay ↔ Submission relationship is correctly configured.The
passive_deletes=Truesetting onTrainingDay.submissionscorrectly matches theondelete="SET NULL"constraint onSubmission.training_day_id. This tells SQLAlchemy to rely on the database to null the FK when a TrainingDay is deleted, rather than attempting ORM-side cleanup. The TYPE_CHECKING import and bidirectionalback_populatesare properly configured.cms/server/contest/handlers/base.py (1)
266-271: The pattern~Contest.training_day.has()is the correct SQLAlchemy 1.3 idiom for scalar relationships.has()without a criterion compiles toEXISTS (...)on the related table, and the~operator correctly negates it toNOT EXISTS (...), which filters for contests with no related TrainingDay row. No changes needed.cmscontrib/updaters/update_49.py (1)
42-48: LGTM!The updater correctly:
- Skips internal keys (prefixed with
_)- Safely checks
_classwith.get()- Uses
setdefaultto addtraining_day_idonly when missing- Does not modify
_version(as per learnings, schema version is updated via theVERSIONconstant incms/db/__init__.py)cms/server/admin/templates/student.html (1)
34-37: LGTM!The URL updates correctly align with the training program context:
- Line 34: Uses
selected_user.idconsistently with the template's context- Line 37: Download URL matches the new
DownloadTrainingProgramStudentSubmissionsHandlerroute patterncms/server/admin/handlers/contestsubmission.py (1)
42-61: LGTM!The handler correctly distinguishes between:
- Training day contests: Filters submissions by
training_day_idto show only submissions made via that specific training day- Regular/managing contests: Filters by task's contest association
The template context is properly extended with
is_training_programandtraining_programfor conditional rendering.cms/db/submission.py (1)
88-101: LGTM!The new
training_day_idcolumn and relationship are well-designed:
- Nullable: Correct, as submissions can be made without a training day context
- Indexed: Good for query performance when filtering by training day
ondelete SET NULL: Appropriate behavior - preserves submissions when training day is deletedback_populates: Correctly matchesTrainingDay.submissionsrelationshipcms/server/admin/templates/contest_submissions.html (1)
18-32: LGTM!The template correctly:
- Uses
is_training_program|default(false)for safe fallback behavior- Routes downloads to the appropriate endpoint (training program vs contest)
- Passes
show_training_dayto the macro for conditional "Source" column renderingcmscontrib/updaters/update_from_1.5.sql (1)
514-522: LGTM!The SQL migration correctly mirrors the ORM model definition:
- Nullable
INTEGERcolumn- Foreign key with
ON UPDATE CASCADE ON DELETE SET NULLmatching the SQLAlchemy definition- B-tree index for query performance
- Follows existing naming conventions for constraints and indexes
cms/server/admin/handlers/__init__.py (2)
107-109: LGTM!New handler imports follow the existing pattern and include both training program submission download handlers.
343-344: LGTM!Routes are correctly placed in the training program section and follow the established URL pattern conventions:
- Program-level:
/training_program/{id}/submissions/download- Student-level:
/training_program/{id}/student/{user_id}/submissions/downloadcms/service/ScoringService.py (4)
31-32: LGTM!The import of
Participationis correctly added to support the new training day participation lookups.
43-67: LGTM!The helper function is well-documented and correctly uses
one_or_none()for the unique participation lookup.
140-155: LGTM!The training day cache invalidation logic is correctly placed after the main score cache update and before the commit. The relationship navigation (
submission.training_day.contest_id) will work within the session context.
272-299: LGTM!Good optimization using memoization to avoid repeated participation lookups during bulk invalidations. The cache key correctly captures the unique combination of training day contest and user.
cms/server/admin/templates/macro/submission.html (3)
5-19: LGTM!The macro signature and documentation are correctly updated to include the new
show_training_dayparameter with a sensible default offalse.
44-46: LGTM!The conditional Source header column is correctly placed and aligned with the row rendering logic.
97-105: LGTM!The Source cell correctly handles both cases: training day submissions link to the training day's contest, while task archive submissions display static text. The null check on
s.training_dayensures safe property access.cms/server/admin/handlers/submissiondownload.py (8)
27-36: LGTM!The imports are correctly organized to include the new models (
TrainingProgram,TrainingDay) andjoinedloadfor eager loading optimization.
44-64: LGTM!Good security practice sanitizing path components before use in zip file paths. The function correctly handles path traversal characters, null bytes, and edge cases like empty results.
67-71: LGTM!The function correctly applies path sanitization to the user-controlled training day contest description.
193-206: LGTM!The conditional filtering correctly differentiates between training day contests (filtering by
training_day_id) and regular contests (all participation submissions).
236-250: LGTM!The training day filtering correctly retrieves all submissions made via that specific training day, while regular contests use the task-based join to get submissions within the contest scope.
279-288: Good use of eager loading to avoid N+1 queries.The
joinedloadchain correctly preloads thetraining_dayrelationship and its nestedcontestfor efficient path construction inget_source_folder().
332-339: LGTM!Consistent pattern with the program-level handler, correctly using eager loading and filtering by participation.
112-118: LGTM!The path construction correctly builds the hierarchy using list unpacking, appending the official/unofficial folder before the filename.
- Extract _get_managing_participation helper to avoid code duplication - Use managing_participation for task_score, tokens_available, and get_submission_count in training-day context (fixes incorrect scores, token availability, and submission counts) - Add order_by(Submission.timestamp.desc()) to submission queries for stable UI ordering - Always show 'Official submissions' header in task_submissions.html template (not just when unofficial submissions exist) Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
_get_sorted_official_submissions now correctly handles training day participations by querying submissions from the managing contest's participation (where they are stored) and filtering by training_day_id. This fixes the issue where score cache rebuilds for training day participations would return no submissions because they were looking at the wrong participation. Co-Authored-By: Ron Ryvchin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cms/server/contest/handlers/tasksubmission.py (1)
189-299: Contest and task submission limits can be inconsistent in training-day context.In training-day mode:
accept_submission()validates limits against the managing contest (viasubmission_participation.contest), but- the UI computes
submissions_left_contestagainstself.contest(the training-day contest), which may have differentmax_submission_number, and- the UI computes
submissions_left_taskfromlen(submissions)(filtered bytraining_day_id), while the backend counts all submissions for the task without that filter.This shows users incorrect "you can submit X more" numbers that don't match what the backend will actually accept.
The submission counts also don't align: scores/tokens are computed from
score_participation(all task submissions), but only training-day-filtered submissions are displayed to the user, creating a mismatch between displayed submissions and computed scores.To fix: compute
submissions_left_contestagainstscore_participation.contest(the managing contest) andsubmissions_left_taskusingget_submission_count()on the unfiltered task submissions, so the UI displays the same limits the backend enforces.
🤖 Fix all issues with AI agents
In @cms/server/contest/handlers/tasksubmission.py:
- Around line 122-147: Add an explicit pre-check before calling
accept_submission to ensure the task actually belongs to the contest used for
the submission and make GET/POST consistent about missing managing
participation: after resolving submission_participation (and after calling
_get_managing_participation when training_day is not None) call the same
validation used by workflow (e.g. contest.task_belongs_here(task) against
submission_participation.contest or self.contest) and raise HTTPError(403) if it
fails; also update TaskSubmissionsHandler.get() to perform the same
_get_managing_participation check and either raise HTTPError(403) or
hide/disable the submit form when managing_participation is None so GET and POST
behave consistently.
🧹 Nitpick comments (2)
cms/server/contest/templates/task_submissions.html (1)
370-443: Make training-day sections deterministic (and optionally safer).Right now
training_day_submissions.items()(Line 408) will follow whatever insertion order the dict had in the handler, which can be surprising (e.g., order changes based on most-recent submission). Consider sorting before rendering (e.g., by training day id/description) to keep the UI stable.Also, the template assumes
td_data[0]is always non-null and hascontest.description(Line 411); a defensiveif training_day is not noneguard would prevent a template hard-fail if any unexpected DB state slips in.cms/server/contest/handlers/tasksubmission.py (1)
72-87: Helper looks fine; consider filtering by ids to avoid relationship-compare surprises.Using
Participation.contest == managing_contest/Participation.user == user(Line 83-84) is usually OK, but filtering by*_idis less ambiguous and can be easier for the ORM to optimize.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cms/server/contest/handlers/tasksubmission.pycms/server/contest/templates/task_submissions.html
🧰 Additional context used
🧬 Code graph analysis (1)
cms/server/contest/handlers/tasksubmission.py (2)
cms/server/contest/submission/workflow.py (1)
accept_submission(170-322)cms/db/submission.py (1)
Submission(45-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (ubuntu:noble)
🔇 Additional comments (1)
cms/server/contest/templates/task_submissions.html (1)
445-481: Archive vs regular view branching looks consistent with existing macro usage.The new
is_task_archivesplit cleanly falls back to the existing “unofficial/official” rendering path, and continues usingmacro_submission.rows(..., false/true)the same way as before.
- Move get_managing_participation to cms/db/training_day.py as shared helper - Update tasksubmission.py and scorecache.py to use the shared helper - Fix GET/POST inconsistency: TaskSubmissionsHandler.get() now raises 403 when managing participation is missing, consistent with SubmitHandler.post() Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cms/server/contest/handlers/tasksubmission.py (2)
234-270: Task submission limits use filtered submission count instead of database query, causing inconsistency in training day contexts.On line 257,
task.max_submission_number - len(submissions)counts only the submissions in the filtered list. In a training day context (lines 195-204), this list is filtered bytraining_day_id, so the task limit only counts submissions from that specific training day.In contrast, contest limits (line 249) use
get_submission_count()which queries the database across all submissions for the participation, regardless of training day filtering.Since
get_submission_count()supports ataskparameter (seecms/server/contest/submission/check.py:84), task limits should be calculated consistently using a database query:submissions_left_task = \ get_submission_count(self.sql_session, score_participation, task=task)This ensures task submission limits count all submissions for the task, not just those from the current training day or context.
340-400:get_submission()does not handle training day submissions—four handlers will return 404 for training day users.Training day submissions are stored with
participation = managing_participation(not the training day participation), butget_submission()filters byself.current_user, which in training day context is the training day participation. This causesget_submission()to returnNonefor submissions made via training day, resulting in 404 errors.
TaskSubmissionsHandler.get()works around this with explicit logic to query viamanaging_participationwhenself.contest.training_day is not None. However, four handlers lack this handling and will fail to retrieve submissions for training day users:
SubmissionStatusHandler.get()(line 355)SubmissionDetailsHandler.get()(line 425)SubmissionFileHandler.get()(line 479)UseTokenHandler.post()(line 532)These handlers must apply the same training day resolution logic as
TaskSubmissionsHandler, orget_submission()must be updated to handle training day context automatically.
🤖 Fix all issues with AI agents
In @cms/grading/scorecache.py:
- Around line 743-759: The current code in scorecache (where you call
get_managing_participation(session, training_day, participation.user)) silently
returns an empty list when managing_participation is None; instead make this
behavior consistent with cms/server/contest/handlers/tasksubmission.py by
raising a tornado.web.HTTPError(403) (or at minimum logging a warning then
raising) when managing_participation is None; update the function that builds
the Submission query to import and raise tornado.web.HTTPError(403) rather than
returning [] so missing managing participation is handled uniformly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cms/db/training_day.pycms/grading/scorecache.pycms/server/contest/handlers/tasksubmission.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cms/db/training_day.py
🧰 Additional context used
🧬 Code graph analysis (2)
cms/server/contest/handlers/tasksubmission.py (2)
cms/db/training_day.py (1)
get_managing_participation(36-60)cms/db/submission.py (1)
Submission(45-222)
cms/grading/scorecache.py (2)
cms/db/training_day.py (1)
get_managing_participation(36-60)cms/db/submission.py (1)
Submission(45-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (debian:bookworm)
🔇 Additional comments (5)
cms/grading/scorecache.py (2)
735-740: Clear documentation of training-day-aware behavior.The updated docstring accurately describes the new logic for training day participations, making the function's dual behavior easy to understand.
761-766: Regular contest logic preserved correctly.The existing query logic for regular contests is unchanged and continues to work as expected for non-training-day participations.
cms/server/contest/handlers/tasksubmission.py (3)
106-130: Training day submission handling looks correct.The logic properly resolves the managing participation for training day submissions and consistently raises 403 when the managing participation is missing. Setting
training_dayon the submission afteraccept_submissioncreates it with the correct participation is the right approach.
173-216: Query logic for training day context is well-implemented.The submission filtering correctly handles training day context by using managing participation and training_day_id. The 403 error check is consistent with
SubmitHandler.post(), and ordering by timestamp descending ensures stable UI presentation.
218-230: LGTM: Task archive grouping is correctly implemented.The logic properly separates archive submissions from training day submissions, building a clear data structure for the template to distinguish sources.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cms/grading/scorecache.py (1)
735-775: Don’t hard-fail score-cache rebuild on missing managing participation (prefer graceful fallback or handled error).
raise ValueError(...)in a low-level cache utility risks breaking ranking/admin pages due to a single data inconsistency. If this state is possible in production, consider returning[](score 0) and logging, or raising a domain-specific exception that callers explicitly handle.Also, consider adding a tiebreaker to ensure deterministic ordering when timestamps collide.
Proposed adjustment (graceful fallback + deterministic ordering)
@@ - if managing_participation is None: - # User doesn't have a participation in the managing contest - # This indicates a configuration or data integrity issue - raise ValueError( - f"User {participation.user_id} does not have participation in managing contest " - f"{training_day.training_program.managing_contest_id} for training day {training_day.id}" - ) + if managing_participation is None: + # Data integrity issue; avoid taking down ranking/cache rebuild paths. + # Prefer logging at the call site if you need visibility. + return [] @@ - ).order_by(Submission.timestamp.asc()).all() + ).order_by(Submission.timestamp.asc(), Submission.id.asc()).all() @@ - ).order_by(Submission.timestamp.asc()).all() + ).order_by(Submission.timestamp.asc(), Submission.id.asc()).all()
🤖 Fix all issues with AI agents
In @cms/db/training_day.py:
- Around line 36-60: get_managing_participation is currently defined inside an
if typing.TYPE_CHECKING: block so it is not present at runtime and imports from
cms/server/contest/handlers/tasksubmission.py will fail; move the entire
get_managing_participation function definition out of the TYPE_CHECKING block
into module scope in cms/db/training_day.py so it is available at runtime,
leaving only type-only imports (Session, TrainingDay, User, etc.) inside
TYPE_CHECKING if needed and keeping the runtime import of Participation inside
the function as currently implemented.
In @cms/server/admin/templates/macro/submission.html:
- Around line 97-105: The template accesses s.training_day.contest.description
directly which can be None when the contest was deleted; update the conditional
to check both s.training_day and s.training_day.contest (e.g., {% if
s.training_day is not none and s.training_day.contest is not none %}) before
rendering the link, and fall back to the existing "Task Archive" (or a distinct
message) when the contest is missing.
In @cms/server/contest/handlers/tasksubmission.py:
- Around line 173-231: The Submission queries that currently use
joinedload(Submission.training_day) should eagerly load the training day’s
contest to avoid N+1 DB queries; update both query calls that build submissions
(the block that sets submissions when training_day is not None and the else
block for regular/archive) to replace the simple
joinedload(Submission.training_day) with a nested joinedload that also
joinedloads TrainingDay.contest (i.e. chain a .joinedload(TrainingDay.contest)
off the Submission.training_day joinedload) so templates accessing
training_day.contest.description do not trigger extra queries.
In @cms/service/ScoringService.py:
- Around line 285-299: The code assumes sr.submission.training_day has a
non-null contest and directly uses training_day.contest_id, which can raise
AttributeError if the training day's contest was deleted; update the block that
populates training_day_participation_cache (using sr.submission.training_day,
training_day.contest_id, training_day_participation_cache,
_get_training_day_participation, and affected_pairs) to first verify
training_day is not None and training_day.contest is not None (or contest_id
exists) and only then compute cache_key, call _get_training_day_participation
and add to affected_pairs; otherwise skip that training-day branch.
🧹 Nitpick comments (6)
cms/server/contest/handlers/contest.py (1)
463-481: Logic correctly handles training-day managing participations.The implementation properly resolves the managing contest participation for training-day submissions, ensuring queries retrieve submissions stored on the managing contest. The None check at line 472 prevents issues when
get_managing_participationreturns None.Optional refactor: Consider moving the import of
get_managing_participationto the module level (line 56) for consistency with other imports, rather than importing at the method level.cmscontrib/updaters/update_from_1.5.sql (1)
514-523: Consider migration/runtime impact ofCREATE INDEXonpublic.submissions.
CREATE INDEX ix_submissions_training_day_id ...can block writes on large tables; if this updater runs online, consider a “create index concurrently” strategy (which would require splitting out of the transaction) or ensure this runs in a maintenance window.cms/db/submission.py (1)
41-43: FK target viaTrainingDay.idmay create import-order coupling; consider string-based FK.If
cms/db/__init__.pyimport/export ordering ever changes,ForeignKey(TrainingDay.id)can become fragile. Using the table-name string avoids that coupling and matches other models’ style.Proposed tweak (string-based FK)
@@ - training_day_id: int | None = Column( - Integer, - ForeignKey(TrainingDay.id, - onupdate="CASCADE", ondelete="SET NULL"), + training_day_id: int | None = Column( + Integer, + ForeignKey("training_days.id", + onupdate="CASCADE", ondelete="SET NULL"), nullable=True, index=True)Also applies to: 88-102
cms/server/admin/handlers/contestsubmission.py (1)
42-62: Watch for N+1 when rendering “Source” (training day) in submissions templates.If templates read
submission.training_day(ortraining_day.description) per row,render_params_for_submissions()currently eager-loads several relationships but notSubmission.training_day(per the shared helper snippet). Consider eager-loading it to avoid per-submission queries.cms/server/admin/handlers/submissiondownload.py (2)
193-206: Consider eager loading training_day for consistency.While the current code works correctly (the base_path_builder doesn't call
get_source_folderfor this handler), for consistency and to prevent future issues if the code is modified, consider eager loading the training_day relationship when filtering by training_day_id.♻️ Proposed refactor for consistency
# For training day contests, only download submissions made via that training day if self.contest.training_day is not None: submissions = ( self.sql_session.query(Submission) .filter(Submission.participation_id == participation.id) .filter(Submission.training_day_id == self.contest.training_day.id) + .options(joinedload(Submission.training_day)) .all() )
236-250: Consider eager loading training_day for consistency.Similar to the previous handler, while the current code works correctly, consider eager loading the training_day relationship when filtering by training_day_id for consistency and future-proofing.
♻️ Proposed refactor for consistency
# For training day contests, only download submissions made via that training day if self.contest.training_day is not None: submissions = ( self.sql_session.query(Submission) .filter(Submission.training_day_id == self.contest.training_day.id) + .options(joinedload(Submission.training_day)) .all() )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cms/db/submission.pycms/db/training_day.pycms/grading/scorecache.pycms/grading/scoring.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/submissiondownload.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/templates/contest_submissions.htmlcms/server/admin/templates/macro/submission.htmlcms/server/admin/templates/student.htmlcms/server/contest/handlers/base.pycms/server/contest/handlers/contest.pycms/server/contest/handlers/tasksubmission.pycms/server/contest/templates/task_submissions.htmlcms/service/ScoringService.pycmscontrib/updaters/update_49.pycmscontrib/updaters/update_from_1.5.sql
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-09T19:33:59.218Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cms/server/admin/handlers/dataset.py:1323-1387
Timestamp: 2026-01-09T19:33:59.218Z
Learning: In cms/server/admin/handlers/**/*.py, follow the existing pattern: do not emit explicit error notifications when try_commit() fails. Rely on centralized error handling and logging instead. Apply this consistently to all new and updated handlers to maintain uniform behavior and maintainability.
Applied to files:
cms/server/admin/handlers/trainingprogram.pycms/server/admin/handlers/contestsubmission.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/submissiondownload.py
📚 Learning: 2026-01-02T19:12:00.691Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cmscontrib/updaters/update_49.py:36-37
Timestamp: 2026-01-02T19:12:00.691Z
Learning: In CMS codebase, updater modules matching cmscontrib/updaters/update_*.py should not set self.objs["_version"] in their run() methods. The schema version is updated via the VERSION constant in cms/db/__init__.py instead. Updaters should only assert the incoming version and transform data.
Applied to files:
cmscontrib/updaters/update_49.py
🧬 Code graph analysis (9)
cms/db/submission.py (1)
cms/db/training_day.py (1)
TrainingDay(63-123)
cms/grading/scorecache.py (2)
cms/db/training_day.py (1)
get_managing_participation(36-60)cms/db/submission.py (1)
Submission(45-222)
cms/service/ScoringService.py (4)
cms/db/submission.py (1)
Submission(45-222)cms/db/user.py (1)
Participation(179-345)cms/db/util.py (1)
get_submission_results(228-301)cms/grading/scorecache.py (1)
invalidate_score_cache(291-361)
cms/grading/scoring.py (2)
cms/db/task.py (1)
Task(55-358)cms/db/training_day.py (1)
TrainingDay(63-123)
cmscontrib/updaters/update_49.py (1)
cms/server/admin/handlers/submissiondownload.py (5)
get(152-170)get(181-222)get(233-263)get(274-306)get(317-353)
cms/server/admin/handlers/contestsubmission.py (3)
cms/db/submission.py (1)
Submission(45-222)cms/db/task.py (1)
Task(55-358)cms/server/admin/handlers/base.py (1)
render_params_for_submissions(713-749)
cms/server/admin/handlers/__init__.py (1)
cms/server/admin/handlers/submissiondownload.py (3)
DownloadContestSubmissionsHandler(225-263)DownloadTrainingProgramSubmissionsHandler(266-306)DownloadTrainingProgramStudentSubmissionsHandler(309-353)
cms/server/admin/handlers/submissiondownload.py (4)
cms/db/training_program.py (1)
TrainingProgram(36-81)cms/db/training_day.py (1)
TrainingDay(63-123)cms/grading/languagemanager.py (1)
safe_get_lang_filename(56-72)cms/server/contest/handlers/tasksubmission.py (4)
get(158-297)get(352-412)get(422-463)get(473-519)
cms/server/contest/handlers/tasksubmission.py (5)
cms/db/training_day.py (1)
get_managing_participation(36-60)cms/server/contest/submission/workflow.py (1)
accept_submission(170-322)cms/db/submission.py (1)
Submission(45-222)cms/grading/scoring.py (1)
task_score(106-187)cms/server/contest/tokening.py (1)
tokens_available(179-316)
🪛 Ruff (0.14.10)
cms/grading/scorecache.py
757-760: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (ubuntu:noble)
- GitHub Check: test (debian:bookworm)
🔇 Additional comments (28)
cms/server/admin/handlers/trainingprogram.py (1)
1127-1128: LGTM! Fixes undefined variable issue.The addition of
is_training_program = Truein the rendering context resolves the undefined variable issue mentioned in the PR objectives and enables the training-day source column to display correctly in the template.cmscontrib/updaters/update_49.py (1)
43-47: LGTM! Migration logic is correct.The updater correctly ensures the
training_day_idfield exists on all Submission objects, defaulting toNonefor existing submissions. The implementation properly skips internal keys and usessetdefaultto avoid overwriting existing values.cms/server/admin/templates/student.html (1)
34-37: LGTM! URLs updated for training-program context.Both changes correctly align with the new training-program-scoped download handlers:
- Line 34: Uses
selected_user.idfor consistency in the reevaluation URL- Line 37: Updates to the new training-program-based download endpoint structure
These changes support the PR's goal of providing program-level and per-student downloads with proper folder structure.
cms/server/contest/handlers/base.py (3)
266-271: LGTM! Correct use of SQLAlchemy relationship filtering.The change from direct
Nonecomparison to~Contest.training_day.has()is the proper SQLAlchemy pattern for filtering based on relationship existence. This correctly excludes training-day contests from the folder browser, maintaining separation between training-day and regular contest contexts.
340-346: LGTM! Consistent relationship filtering.Correctly applies the same
~Contest.training_day.has()pattern for root-level contests, maintaining consistency with the filtering approach used in_build_folder_tree.
355-361: LGTM! Consistent relationship filtering.Correctly applies the same
~Contest.training_day.has()pattern for folder-level contests, ensuring training-day contests are excluded from all folder browsing contexts.cms/grading/scoring.py (1)
30-31: The implementation correctly uses the managing Participation when callingtask_score()withtraining_day. Incms/server/contest/handlers/tasksubmission.py, the only two call sites that passtraining_day(lines 239–244 and 246–251) both usescore_participation, which is properly set tomanaging_participationwhen available (lines 55–58). The managing participation is retrieved viaget_managing_participation()before these calls (lines 185–186), and the code rejects early if it is unavailable. No other call sites passtraining_daytotask_score().cms/server/admin/handlers/__init__.py (2)
107-109: LGTM!The imports are correctly structured and align with the new download handlers defined in
submissiondownload.py.
343-344: LGTM!The routes follow the existing pattern and correctly map to the new download handlers for training programs and per-student downloads.
cms/server/admin/templates/macro/submission.html (2)
5-5: LGTM!The
show_training_dayparameter is correctly added to the macro signature, documented, and propagated through to therowmacro.Also applies to: 18-19, 31-32
44-46: LGTM!The conditional rendering of the Source column header is correct, and the parameter is properly passed to each row invocation.
Also applies to: 58-58
cms/service/ScoringService.py (4)
31-32: LGTM!The import of
Participationis necessary for the new training day participation lookup logic.
43-67: LGTM!The helper function correctly queries for a training day participation by contest_id and user_id, using
one_or_none()to handle cases where no participation exists.
272-274: LGTM!The memoization cache is a good optimization to avoid redundant database queries during bulk invalidations.
140-155: This concern is incorrect.The code accesses
submission.training_day.contest_id, which is a database column (not the relationship). Thecontest_idcolumn is defined asnullable=Falsein the TrainingDay model atcms/db/training_day.py:84-90. Whensubmission.training_day_id is not None, the TrainingDay exists with a guaranteed validcontest_idvalue. The foreign key withondelete="CASCADE"ensures that if a contest is deleted, its associated TrainingDay is also deleted. Therefore, accessing the column is safe, and no defensive check is needed.Likely an incorrect or invalid review comment.
cms/server/contest/templates/task_submissions.html (3)
408-443: Verify structure and availability oftraining_day_submissions.The template expects
training_day_submissionsto be a dictionary where each value is a tuple(training_day, td_submissions)(line 409-410). If the handler doesn't provide this variable with the correct structure whenis_task_archiveis true, the template will fail.Additionally, line 411 accesses
training_day.contest.description, which could fail if the training day's contest relationship is broken.The verification script in the previous comment will also check for
training_day_submissions. Additionally, consider adding defensive checks in the template:🛡️ Suggested defensive template code
{% for td_id, td_data in training_day_submissions.items() %} {% set training_day = td_data[0] %} {% set td_submissions = td_data[1] %} {% if training_day.contest is not none %} <h3>{% trans td_desc=training_day.contest.description %}Submissions from training day: {{ td_desc }}{% endtrans %}</h3> {# ... rest of the block ... #} {% endif %} {% endfor %}
465-465: LGTM!Changing from
<h4>to<h3>maintains consistency with the task archive branch (line 392) and properly structures the heading hierarchy.
370-406: No issue found.archive_submissionsis always initialized before the conditional check (line 219), so it's guaranteed to exist in the template context regardless of whetheris_task_archiveis true. The handler correctly passes both variables on every render call.Likely an incorrect or invalid review comment.
cms/server/admin/templates/contest_submissions.html (1)
18-22: The code correctly handles this case. Both handlers that rendercontest_submissions.htmlproperly provide thetraining_programcontext variable whenis_training_programis true:
- ContestSubmissionsHandler (line 60-61): Conditionally sets
training_programonly whenis_training_programis true- TrainingProgramSubmissionsHandler (line 1113): Always explicitly sets
training_programbefore settingis_training_program = True(line 1128)No template rendering error will occur.
cms/db/training_day.py (1)
119-123: LGTM!The
submissionsrelationship is correctly configured withpassive_deletes=True, which ensures proper cascade behavior when a TrainingDay is deleted.cms/server/contest/handlers/tasksubmission.py (4)
106-120: LGTM! Correct use of managing participation for training day submissions.The logic correctly determines the managing contest participation for training day submissions and raises a 403 error if the user doesn't have a participation in the managing contest. This ensures submissions are stored consistently with the managing contest.
128-130: LGTM! Correct placement of training day reference.Setting the training_day reference after
accept_submissionis correct, as the submission object must exist first. The reference is properly set before the commit.
234-282: LGTM! Correct use of score_participation for calculations.The logic correctly uses
score_participation(which ismanaging_participationfor training days, otherwise the regular participation) for all score, token, and submission count calculations. This ensures consistency with where the submissions are stored and avoids showing incorrect scores (0.0) or token counts.
285-297: Template variables are correctly used and empty collections are properly handled.The template uses
archive_submissionsat lines 375 and 384-405 with appropriate checks for empty submissions (checking length after filtering by official status). Thetraining_day_submissionsdict is correctly iterated using.items()at line 408, which gracefully handles empty collections. Theis_task_archiveboolean properly gates the conditional rendering at line 370.cms/server/admin/handlers/submissiondownload.py (4)
44-64: LGTM! Robust path sanitization.The
sanitize_path_componentfunction properly sanitizes strings for use in zip paths by replacing unsafe characters and handling edge cases like empty strings. The default "unnamed" return value prevents empty path components.
266-306: LGTM! Proper eager loading of relationships.The handler correctly uses
joinedloadto load the training_day and contest relationships (lines 285-286), which prevents N+1 queries whenget_source_folderaccessessubmission.training_day.contest.description.
309-353: LGTM! Proper eager loading of relationships.The handler correctly uses
joinedloadto load the training_day and contest relationships (lines 336-337), which prevents N+1 queries whenget_source_folderaccessessubmission.training_day.contest.description.
67-71: No action needed — eager loading is already properly implemented at all call sites.Both callers of
get_source_folderalready load the required relationships using.options(joinedload(Submission.training_day).joinedload(TrainingDay.contest)), which prevents N+1 queries. The training_day and contest relationships are eagerly loaded before the function is called.
ronryv
force-pushed
the
devin/1768049815-training-day-submission-refactor
branch
from
20e229d to
ff5f7af
Compare
Summary
This PR refactors how submissions are handled in training programs. Previously, submissions made via a training day were stored with the training day's participation. Now, they are stored with the managing contest's participation while tracking which training day they came from via a new
training_day_idfield.Key changes:
training_day_idfield to Submission model to track submission sourceScoringService.py)Updates since last revision
Score cache architecture fix: Per code review feedback, moved the training day participation cache handling from
scorecache.pyto the callers inScoringService.py. This keepsscorecache.pyfocused on its core responsibility and makes the training day logic explicit at the call sites. The approach now usesinvalidate_score_cacheto mark training day participation caches as stale (triggering lazy rebuild) rather than attempting incremental updates.Fixed
is_training_programundefined error: Added missingis_training_program = TrueinTrainingProgramSubmissionsHandler.Fixed "Download all submissions" for training days and training programs:
DownloadContestSubmissionsHandlerandDownloadUserContestSubmissionsHandlernow filter bytraining_day_idfor training day contestsDownloadTrainingProgramSubmissionsHandlerdownloads all submissions with folder structure:user/task/source/official/where source is "task_archive" or the training day descriptionDownloadTrainingProgramStudentSubmissionsHandlerfor per-student downloadsFixed training-day context helper functions (CodeRabbit review):
_get_managing_participationhelper to avoid code duplication between SubmitHandler and TaskSubmissionsHandlertask_score,tokens_available, andget_submission_countnow correctly usemanaging_participationin training-day context (previously used wrong participation, causing scores to appear as 0.0, incorrect token availability, and miscounted submission limits).order_by(Submission.timestamp.desc())to submission queries for stable UI orderingReview & Testing Checklist for Human
Critical: Score/token/count calculations in training-day context (
cms/server/contest/handlers/tasksubmission.pylines 246-284): Verify thatscore_participationcorrectly resolves tomanaging_participationwhen in training-day context. This fix addresses the bug where scores appeared as 0.0 because the wrong participation was used.Download handlers for training programs (
cms/server/admin/handlers/submissiondownload.pylines 211-290): Verify the folder structure logic is correct. The source folder usessubmission.training_day.description- confirm this is always a valid folder name (no special characters that would break zip paths).Score cache invalidation in ScoringService (
cms/service/ScoringService.pylines 137-155 and 282-298): Verify the training day participation lookup and cache invalidation logic is correct.Template variable passing: Verify
is_task_archive,archive_submissions,training_day_submissions, andis_training_programare always defined when rendering their respective templates.Recommended test plan:
Notes
Requested by: Ron Ryvchin (@ronryv)
Devin sessions:
Summary by CodeRabbit
New Features
UI
Bug Fixes / UX
Chores
✏️ Tip: You can customize this high-level summary in your review settings.