Add timezone support, duration inputs, hidden user filtering, main group ranking split, and UI fixes #101
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…training days 1. Change group times and training day inputs to use contest timezone instead of UTC - AddTrainingDayHandler now passes timezone_name to template - ContestHandler passes timezone info for training_day_groups fragment - All datetime inputs now display and parse in contest timezone 2. Update forms to use start time + duration instead of end date - AddTrainingDayHandler.post() parses duration_hours and duration_minutes - AddTrainingDayGroupHandler.post() uses duration instead of end_time - UpdateTrainingDayGroupsHandler.post() uses duration instead of end_time - Templates updated to show duration inputs (hours + minutes) 3. Hide hidden users from combined ranking and attendance - TrainingProgramAttendanceHandler filters out hidden students - TrainingProgramCombinedRankingHandler filters out hidden students - AddTrainingDayHandler passes hidden property when creating participations Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Author
🤖 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 full training-day and training-program admin features: training-day CRUD with main groups/durations, student/task management, main-group-aware rankings/exports, training-day-aware submission handling, and a training-day archiving subsystem with attendance and historical rankings. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
ronryv
reviewed
Jan 22, 2026
…er_params() Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
devin-ai-integration
bot
changed the title
…ports - Add main group detection for training day contests - Build student tags lookup for each participation - Organize participations by main group based on student tags - Filter tasks per group based on accessibility - Add student tags column after user column in ranking table - Create separate tables per main group with group headers - Add per-group CSV/TXT export links with group name in filename - Calculate group-specific total scores for filtered tasks - Preserve original single-table behavior for non-training-day contests Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
ronryv
force-pushed
the
devin/1769081443-training-day-timezone-duration
branch
from
db00100 to
da418ed
Compare
…roup button - Fix tag retrieval in training day ranking: use user_id join instead of participation_id since Student.participation_id refers to managing contest participation, not training day participation - Add student tags column to training program ranking tables and CSV export - Add tags column to ranking.txt export template - Fix remove main group button by using cookie-based XSRF token instead of handler.xsrf_token which returns bytes Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Add main_groups_data = None to TrainingProgramRankingHandler r_params to prevent UndefinedError when rendering ranking.html template Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Replace gradient header with simple border-bottom style matching page design - Use neutral colors for export links instead of white-on-gradient - Add 'N' indicator for inaccessible tasks in TXT export - Add 'N/A' indicator for inaccessible tasks in CSV export (trainingprogram.py) Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Remove border-bottom from group header, use inline layout with gap - Place CSV/TXT links directly after group name - Use loop.index for table IDs to avoid special character issues in jQuery selectors Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
The sorting function uses parseFloat on cell text, but task cells contain emoji indicators that make parsing fail. Adding data-value with the numeric score allows the sorting function to use the correct value. Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Rename ranking-table class to td-ranking-table to avoid picking up the combined ranking page's CSS styling which changes header appearance. Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
1. Add 'participation' column in Training Program Participations table in user page 2. Link users to training day participation pages in submissions page 3. Link user in training day participation to students page instead of users page 4. Fix submission list in user participation of training days to show correct submissions 5. Fix 404 error when clicking 'history' in training program ranking by using get_tasks() 6. Fix history link in training day ranking by using get_tasks() consistently 7. Fix profile picture not loading in combined ranking user details Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
ronryv
changed the title
For training day participations, submissions are stored with the managing contest's participation, not the training day's participation. This fix uses get_managing_participation to find the correct participation and filters submissions by training_day_id. Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Add TrainingProgramStudentDetailRedirectHandler to handle
/training_program/{id}/student/{user_id}/detail URLs by redirecting
to the managing contest's participation detail page.
Update ranking.html template to use the correct URL pattern for
training program ranking history links.
Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
ronryv
force-pushed
the
devin/1769081443-training-day-timezone-duration
branch
from
e321b43 to
d61e731
Compare
Collaborator
|
@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: 7
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/contestranking.py`:
- Around line 248-253: The current sort uses p.total_score (global total) which
misorders when tasks are filtered; change the sort to compute a group-specific
total from accessible_tasks and sort by that. Replace key=lambda p:
p.total_score with a key that sums scores for accessible_tasks (e.g., key=lambda
p: sum(p.scores.get(t.id, 0) for t in accessible_tasks) or call
p.get_score(task) if such accessor exists) so sorted_participations is ordered
by the group-specific totals using accessible_tasks, not p.total_score.
- Around line 505-521: The code chooses an arbitrary main group via
next(iter(user_main_groups)) and queries Student per participation; make
selection deterministic and use a single batch query: pick user_main_group =
sorted(user_main_groups)[0] (or another deterministic policy) and replace the
per-participation query loop over visible_participations by one query that
fetches all Student rows matching training_program.id and user_id in {p.user_id
for p in visible_participations}, then iterate those Student results to build
main_group_user_ids when the student's student_tags contains user_main_group
(references: user_tags, user_main_groups, user_main_group,
visible_participations, Student, Participation, main_group_user_ids).
- Around line 39-45: Replace the import of get_all_student_tags from
.trainingprogram with a direct import from cms.server.util to match project
convention; update the import block in contestranking.py so it imports
get_all_student_tags from cms.server.util (leave other imports unchanged) and
remove the .trainingprogram reference to avoid the unnecessary coupling.
In `@cms/server/admin/handlers/trainingprogram.py`:
- Around line 1542-1547: The duration parsing currently allows negative
hours/minutes and minutes >= 60 which can produce stop times before start;
update the block that parses duration_hours/duration_minutes (and the similar
group-duration blocks) to explicitly validate bounds: ensure duration_hours >=
0, 0 <= duration_minutes < 60, and that the computed duration
(timedelta(hours=duration_hours, minutes=duration_minutes)) is > 0 and that
contest_kwargs["start"] + duration > contest_kwargs["start"]; if any check
fails, add a clear server-side validation error (raise or append to form errors)
instead of computing contest_kwargs["stop"]. Reference the variables
duration_hours_str, duration_minutes_str, duration_hours, duration_minutes,
timedelta, and contest_kwargs["stop"] when locating and updating the code.
In `@cms/server/admin/templates/ranking.html`:
- Around line 119-129: The group total currently formats totals with hard-coded
0/1 decimals using "%.0f" / "%.1f" which ignores contest.score_precision; update
the final total cell rendering (the block that computes total_score.value and
renders the <td> for the group total) to format using contest.score_precision
(e.g., derive a format string based on contest.score_precision or use the format
filter with precision) so totals display the same number of decimal places as
contest.score_precision while preserving the "partial" class logic and existing
total_score accumulation.
In `@cms/server/admin/templates/ranking.txt`:
- Around line 17-20: The template currently iterates over contest.tasks (in the
header and per-participation rows) which is empty for training-day contests;
change those iterations to use contest.get_tasks() instead (replace both
occurrences of "for task in contest.tasks" and any other direct accesses to
contest.tasks with "for task in contest.get_tasks()" and update any dependent
expressions that rely on the loop/index like loop.index0 or task.score_precision
to still reference the same task variable) so the exported TXT uses the
training-day-aware task list.
In `@cms/server/admin/templates/training_program_combined_ranking_detail.html`:
- Line 256: The template conditionally omits the <img id="UserDetail_face">
element but the JS in cmsranking/static/UserDetail.js (where
self.face_image.replaceWith(new_face_image) is called) assumes it exists; update
the code that manipulates the DOM to first check whether self.face_image (or
document.querySelector('#UserDetail_face')) exists and if so call
replaceWith(new_face_image), otherwise insert the new_face_image into the
appropriate container (e.g., parent element via append/insertBefore) as a
fallback; alternatively you can make the template always render the <img
id="UserDetail_face"> (possibly with a placeholder or hidden) so the JS can
safely call replaceWith — pick one fix and apply it to the code paths that
reference self.face_image/new_face_image.
🧹 Nitpick comments (2)
cms/server/admin/handlers/base.py (1)
385-387: Minor: Redundant check for/detailsuffix.The check
not remaining_path.endswith("/detail")at line 387 is redundant since paths ending with/detailalready trigger an early return at line 356. This defensive check doesn't cause any issues but could be removed for clarity.cms/server/admin/handlers/contestranking.py (1)
205-217: Batch-load student tags to avoid N+1 queries.The per-participation lookup can become costly on large contests. Consider a single join query and a user_id→tags map.
♻️ Suggested refactor
training_program = training_day.training_program - for p in self.contest.participations: - # Find the student record for this participation's user - student = ( - self.sql_session.query(Student) - .join(Participation, Student.participation_id == Participation.id) - .filter(Student.training_program_id == training_program.id) - .filter(Participation.user_id == p.user_id) - .first() - ) - if student: - student_tags_by_participation[p.id] = student.student_tags or [] - else: - student_tags_by_participation[p.id] = [] + user_ids = {p.user_id for p in self.contest.participations} + rows = ( + self.sql_session.query(Student, Participation.user_id) + .join(Participation, Student.participation_id == Participation.id) + .filter(Student.training_program_id == training_program.id) + .filter(Participation.user_id.in_(user_ids)) + .all() + ) + tags_by_user_id = { + user_id: (student.student_tags or []) + for student, user_id in rows + } + for p in self.contest.participations: + student_tags_by_participation[p.id] = tags_by_user_id.get(p.user_id, [])
cms/server/admin/templates/training_program_combined_ranking_detail.html
Outdated
Show resolved
Hide resolved
Code fixes: - contestranking.py: Import get_all_student_tags from cms.server.util - contestranking.py: Sort by group-specific total using accessible_tasks - contestranking.py: Use deterministic main group selection and batch query - trainingday.py: Add duration validation for hours/minutes bounds - ranking.html: Use contest.score_precision for total formatting - ranking.txt: Use contest.get_tasks() instead of contest.tasks - training_program_combined_ranking_detail.html: Handle missing face image Refactoring: - Split trainingprogram.py (3306 lines) into logical modules: - trainingday.py: Training day management handlers - student.py: Student management handlers - archive.py: Archive, attendance, and combined ranking handlers - trainingprogram.py: Core training program handlers (965 lines) - Update __init__.py to import from new modules Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Add bidirectional validation in ContestHandler.post() to ensure training day times don't conflict with main group times (start can't be after group start, end can't be before group end) - Fix combined ranking template error by passing ArchivedStudentRanking objects instead of float scores in ranking_data dict Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
… confirmation Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
…gramCombinedRankingHandler Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
ronryv
force-pushed
the
devin/1769081443-training-day-timezone-duration
branch
from
d1f131f to
80514eb
Compare
Collaborator
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/contestranking.py`:
- Around line 418-424: The code blindly converts main_group_user_ids_param to
ints which raises ValueError on malformed input; in the handler method that
reads main_group_user_ids_param (look for main_group_user_ids_param and
main_group_user_ids in contestranking.py) wrap the parsing in validation: check
each comma-separated token is non-empty and digits (or attempt int() inside
try/except), on failure call self.set_status(400) and return a clear error
response (e.g., write/finish with "Invalid main_group_user_ids" and include the
offending token), otherwise build the set of ints as before so well-formed
requests behave unchanged.
In `@cms/server/admin/handlers/student.py`:
- Around line 582-607: Validate that the Task belongs to the same managing
contest as the student's training program before assigning: after retrieving
task with self.safe_get_item(Task, task_id) and having student available, load
the student's training program (e.g., via self.safe_get_item(TrainingProgram,
student.training_program_id) or join) and compare the contest id on the Task
(e.g., task.contest_id) to the managing_contest_id on the training program
(e.g., training_program.managing_contest_id); if they differ, raise
ValueError("Task does not belong to the student's contest") and abort the
assignment. Apply the same ownership check in both the single-assignment handler
(where StudentTask is created) and the bulk-assignment handler referenced in the
comment.
In `@cms/server/admin/handlers/trainingday.py`:
- Around line 478-485: The duration parsing in AddTrainingDayGroupHandler and
UpdateTrainingDayGroupsHandler currently allows negative hours or minutes >= 60;
before constructing timedelta and setting group_kwargs["end_time"] validate the
parsed values the same way AddTrainingDayHandler does: parse duration_hours_str
and duration_minutes_str into ints, ensure duration_hours >= 0 and 0 <=
duration_minutes < 60, and if validation fails append a validation error (or
return the same HTTP/handler error flow used elsewhere) and skip computing
end_time; apply these same checks for the other occurrence noted around the
545-556 region so both handlers enforce server-side duration bounds.
- Around line 208-215: The code accesses managing_contest.stop.year unguarded in
trainingday.py (used to build program_end_year and default_date) which will
raise if managing_contest.stop is None; update the logic in the block that
assigns program_end_year/default_date/contest_kwargs so it first checks whether
managing_contest.stop is present and uses its year, otherwise fall back to a
safe year (e.g., datetime.now().year) before computing default_date and setting
contest_kwargs["start"], contest_kwargs["analysis_start"], and
contest_kwargs["analysis_stop"]; ensure you reference managing_contest.stop,
program_end_year, and default_date when making the change.
🧹 Nitpick comments (5)
cms/server/admin/handlers/trainingprogram.py (1)
698-712: Avoid N+1 queries when building student tag lookup.
Each participation currently triggers aStudentquery. A single batch query keeps the page scalable.♻️ Suggested refactor
- student_tags_by_participation = {} - for p in self.contest.participations: - # Find the student record for this participation - student = ( - self.sql_session.query(Student) - .filter(Student.training_program_id == training_program.id) - .filter(Student.participation_id == p.id) - .first() - ) - if student: - student_tags_by_participation[p.id] = student.student_tags or [] - else: - student_tags_by_participation[p.id] = [] + student_tags_by_participation = {p.id: [] for p in self.contest.participations} + if student_tags_by_participation: + rows = ( + self.sql_session.query(Student.participation_id, Student.student_tags) + .filter(Student.training_program_id == training_program.id) + .filter( + Student.participation_id.in_( + list(student_tags_by_participation.keys()) + ) + ) + .all() + ) + for participation_id, tags in rows: + student_tags_by_participation[participation_id] = tags or []cms/server/admin/templates/ranking.html (1)
96-101: Consider moving task index computation outside the group loop.The
task_indexmap is rebuilt identically for every group iteration, but it only depends oncontest.get_tasks()which is the same across all groups. Moving this outside the{% for group_data in main_groups_data %}loop would avoid redundant computation.♻️ Suggested optimization
+{# Build task index lookup once for all groups #} +{% set all_tasks = contest.get_tasks() | list %} +{% set task_index = {} %} +{% for task in all_tasks %} + {% set _ = task_index.update({task.id: loop.index0}) %} +{% endfor %} + {% for group_data in main_groups_data %} ... <tbody> - {# Build task index lookup for task_statuses #} - {% set all_tasks = contest.get_tasks() | list %} - {% set task_index = {} %} - {% for task in all_tasks %} - {% set _ = task_index.update({task.id: loop.index0}) %} - {% endfor %} - {% for p in group_participations %}cms/server/admin/handlers/contestranking.py (1)
195-209: Consider batching student tag lookups for efficiency.The current implementation queries the
Studenttable once per participation in the loop (lines 199-205). For contests with many participations, this creates N database queries.♻️ Batch query suggestion
# For training days, always build student tags lookup if training_day: training_program = training_day.training_program + # Batch query: fetch all students for this training program's participations + participation_user_ids = {p.user_id for p in self.contest.participations} + students = ( + self.sql_session.query(Student, Participation.user_id) + .join(Participation, Student.participation_id == Participation.id) + .filter(Student.training_program_id == training_program.id) + .filter(Participation.user_id.in_(participation_user_ids)) + .all() + ) + student_by_user_id = {uid: student for student, uid in students} + for p in self.contest.participations: - # Find the student record for this participation's user - student = ( - self.sql_session.query(Student) - .join(Participation, Student.participation_id == Participation.id) - .filter(Student.training_program_id == training_program.id) - .filter(Participation.user_id == p.user_id) - .first() - ) + student = student_by_user_id.get(p.user_id) if student: student_tags_by_participation[p.id] = student.student_tags or [] else: student_tags_by_participation[p.id] = []cms/server/admin/handlers/archive.py (2)
978-985: Loop variable capture in nested function.The function
get_training_day_numcapturesarchived_tasks_databy reference. While this works correctly here since the function is used immediately within the same loop iteration, it's a latent bug that could manifest if the code is refactored (e.g., if sorting is deferred).Safer pattern using default argument
- def get_training_day_num(task_id: int) -> tuple[int, int]: - task_key = str(task_id) - if task_key in archived_tasks_data: - num = archived_tasks_data[task_key].get("training_day_num") - return (num if num is not None else 0, task_id) - return (0, task_id) + def get_training_day_num( + task_id: int, + _tasks_data: dict = archived_tasks_data + ) -> tuple[int, int]: + task_key = str(task_id) + if task_key in _tasks_data: + num = _tasks_data[task_key].get("training_day_num") + return (num if num is not None else 0, task_id) + return (0, task_id)
1001-1002: Same deprecatedQuery.get()usage.Same suggestion as earlier: use
self.sql_session.get(Task, task_id)for SQLAlchemy 2.0 compatibility.Suggested change
- task = self.sql_session.query(Task).get(task_id) + task = self.sql_session.get(Task, task_id)
- Add shared parse_and_validate_duration() function in trainingday.py - Add task ownership validation in student.py (single and bulk assignment) - Add error handling for malformed main_group_user_ids parameter - Fix N+1 query in trainingprogram.py student tag lookup (batch query) - Fix N+1 query in contestranking.py student tag lookup (batch query) - Move task_index computation outside group loop in ranking.html - Fix loop variable capture in archive.py get_training_day_num() Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Collaborator
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/student.py`:
- Around line 67-74: The NOT IN currently uses .all() which returns tuples and
can produce invalid IN bindings; replace the in-memory list with a SQL subquery
by removing .all() and calling .subquery() (or passing the Query object
directly) for the Participation.user_id selection used in the filter. Update the
expression that builds self.r_params["unassigned_users"] so it uses
self.sql_session.query(Participation.user_id).filter(Participation.contest ==
managing_contest).subquery() (or the Query itself) in the User.id.notin_ call
while keeping the username exclusion (~User.username.like(r'\_\_%',
escape='\\')) unchanged.
In `@cms/server/admin/handlers/trainingday.py`:
- Around line 335-341: The group time vs training-day bounds check currently
inside the first loop (the block comparing group_data["start_time"] to
contest_kwargs["start"] and group_data["end_time"] to contest_kwargs["stop"])
must be removed from that loop and executed after you apply the training day
defaults to contest_kwargs (i.e., after the code that defaults
contest_kwargs["start"]/["stop"] from group times). Move the same validation
logic to run once after the defaults are set, keep the same ValueError messages,
and continue to use contest_kwargs.get("start") and contest_kwargs.get("stop")
to guard the comparisons.
In `@cms/server/admin/templates/fragments/training_day_groups.html`:
- Around line 31-44: The removeMainGroup() function should stop reading the
_xsrf token from cookies and instead grab it from the hidden form input to match
other templates: replace the call to getCookie('_xsrf') with reading
document.querySelector('input[name="_xsrf"]') and using its .value (with a null
check/fallback to empty string or an error path). Update removeMainGroup() to
append that value to formData (formData.append('_xsrf', xsrfValue)) and ensure
the selector name exactly matches 'input[name="_xsrf"]'; keep getCookie()
unchanged if used elsewhere.
🧹 Nitpick comments (3)
cms/server/admin/handlers/contestranking.py (1)
426-431: Chain the exception for better debugging.The
ValueErrorfrom malformed input is caught but re-raised asHTTPErrorwithout preserving the exception chain. This makes debugging harder.♻️ Suggested fix
try: main_group_user_ids = set( int(uid) for uid in main_group_user_ids_param.split(",") if uid ) except ValueError: - raise tornado.web.HTTPError(400, "Invalid main_group_user_ids parameter") + raise tornado.web.HTTPError(400, "Invalid main_group_user_ids parameter") from Nonecms/server/admin/handlers/trainingday.py (1)
253-257: Remove unreachable duration check.The condition on line 253 ensures
duration_hours > 0 or duration_minutes > 0. Combined withparse_and_validate_durationensuring both are non-negative,durationis guaranteed positive when this block executes. The check on lines 255-256 is dead code.♻️ Suggested fix
if duration_hours > 0 or duration_minutes > 0: duration = timedelta(hours=duration_hours, minutes=duration_minutes) - if duration <= timedelta(0): - raise ValueError("Duration must be positive") contest_kwargs["stop"] = contest_kwargs["start"] + durationSame applies to lines 294-298 for group duration validation.
cms/server/admin/handlers/archive.py (1)
793-801: DeprecatedQuery.get()usage.
self.sql_session.query(Task).get(task_id)uses the legacyQuery.get()which is deprecated in SQLAlchemy 2.0. UseSession.get()instead.♻️ Suggested fix
- task = self.sql_session.query(Task).get(task_id) + task = self.sql_session.get(Task, task_id)Same applies to line 1006.
Use document.querySelector('input[name="_xsrf"]') instead of getCookie()
for consistency with other templates in the codebase.
Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Collaborator
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/student.py`:
- Around line 247-255: The loop over training_program.training_days should skip
days that have no contest to avoid querying for NULL contests; add a guard at
the start of the loop (check if training_day.contest is None and continue)
before querying Participation, so the filter uses a valid contest object; update
the block containing self.sql_session.query(Participation) /
.filter(Participation.contest == training_day.contest) accordingly to mirror the
guard used in AddTrainingProgramStudentHandler.
In `@cms/server/admin/handlers/trainingday.py`:
- Around line 234-313: The contest stop time is computed before you apply
group-based defaults to contest_kwargs["start"], so when start is later replaced
with min(group_start_times) the stop can become incorrect; after the block that
sets contest_kwargs["start"] = min(group_start_times) and contest_kwargs["stop"]
= max(group_end_times) (the group defaulting at the end of the shown diff),
recompute contest_kwargs["stop"] using the already-parsed
duration_hours/duration_minutes: if duration_hours>0 or duration_minutes>0 set
contest_kwargs["stop"] = contest_kwargs["start"] +
timedelta(hours=duration_hours, minutes=duration_minutes), otherwise if
group_end_times is non-empty keep contest_kwargs["stop"] = max(group_end_times)
else set contest_kwargs["stop"] = contest_kwargs["start"]; reference the
variables contest_kwargs, duration_hours, duration_minutes, group_start_times,
group_end_times and the parse_and_validate_duration result to locate where to
insert this recomputation.
♻️ Duplicate comments (2)
cms/server/admin/handlers/student.py (1)
591-593: Task ownership validation is now in place.The check ensures the task belongs to the student's training program's managing contest, addressing the previous review concern about cross-contest task assignment.
cms/server/admin/handlers/trainingday.py (1)
69-79: Enforce positive duration when inputs are provided.
Right now0h 0mpasses validation even if the user supplied values, which conflicts with the “duration > 0” requirement and allows zero‑length training days/groups. Consider rejecting zero duration when any input is present.🐛 Proposed fix
- hours = int(hours_str) if hours_str.strip() else 0 - minutes = int(minutes_str) if minutes_str.strip() else 0 + hours_str = hours_str.strip() + minutes_str = minutes_str.strip() + hours = int(hours_str) if hours_str else 0 + minutes = int(minutes_str) if minutes_str else 0 + provided = bool(hours_str or minutes_str) @@ if minutes < 0 or minutes >= 60: raise ValueError(f"{prefix}Duration minutes must be between 0 and 59") + if provided and hours == 0 and minutes == 0: + raise ValueError(f"{prefix}Duration must be positive")
🧹 Nitpick comments (2)
cms/server/admin/templates/fragments/training_day_groups.html (1)
35-37: Consider explicit null check for better user feedback.The XSRF token is now correctly read from the hidden input (addressing the previous review). However, the fallback to an empty string when the input is missing will cause a silent CSRF failure with a generic error message. An explicit check provides clearer feedback.
💡 Suggested improvement
var xsrfInput = document.querySelector('input[name="_xsrf"]'); + if (!xsrfInput) { + alert('Missing XSRF token'); + return; + } var formData = new FormData(); - formData.append('_xsrf', xsrfInput ? xsrfInput.value : ''); + formData.append('_xsrf', xsrfInput.value);cms/server/admin/handlers/student.py (1)
292-299: GET request has side effects: auto-creates missing Student record.Creating a
Studentrecord and committing during a GET request violates HTTP semantics (GET should be idempotent). This could:
- Mask bugs where student records are unexpectedly missing
- Cause race conditions with concurrent requests
Consider returning a 404 or showing a "create student" action instead, or move auto-creation to the POST handler only.
ronryv
force-pushed
the
devin/1769081443-training-day-timezone-duration
branch
from
a186e17 to
c3c4bca
Compare
…alidation - Refactor trainingday.py to use parse_datetime_with_timezone instead of manual UTC parsing - Update parse_datetime_with_timezone to handle HTML5 datetime-local format (YYYY-MM-DDTHH:MM) - Add validation to prevent 0-duration training and group times when inputs are provided - Fix XSRF token handling in training_day_groups.html with explicit null check - Fix GET side effect in student.py by removing auto-creation of Student record Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Collaborator
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Re-index checkbox values before form submission to match visual order, fixing the issue where deleting rows caused checkbox values to mismatch with the enumerate index in the Python backend. Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Collaborator
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/archive.py`:
- Around line 899-936: Ensure the handler verifies each Student belongs to the
current TrainingProgram and excludes hidden students when building
filtered_student_ids/users_data: in the loop that iterates
training_program.students (and the similar later loop around the
archived_training_days handling), check s.training_program_id ==
training_program.id and skip any s where s.hidden is truthy (or the model's
equivalent flag) before applying tag filters or adding to
filtered_student_ids/users_data so user_count and task visibility only reflect
non-hidden students of this program.
- Around line 868-881: TrainingProgramCombinedRankingHistoryHandler is appending
history entries from archived_training_days -> archived_student_rankings without
excluding hidden users; before iterating ranking.history (the loop that leads to
result.append) add the same hidden-user filter used by the combined ranking
table — e.g., check the student's hidden flag (match whatever model property
your code uses such as student.hidden or ranking.student.hidden or
ranking.is_hidden) or skip if the student_id is in the existing
hidden_student_ids set; apply this check alongside the existing student_tags
filter so hidden students never reach the result.append stage.
In `@cms/server/admin/handlers/contestranking.py`:
- Around line 426-431: The except block handling ValueError when parsing
main_group_user_ids_param should chain the original exception to preserve trace
context: in the try/except around main_group_user_ids (where you build
main_group_user_ids = set(int(uid) ...)), change the except ValueError handler
to capture the error (e.g., except ValueError as err) and re-raise the
tornado.web.HTTPError(400, "Invalid main_group_user_ids parameter") using "from
err" (or "from None" if you intentionally want to suppress the original
traceback) so the original ValueError is preserved or explicitly suppressed.
♻️ Duplicate comments (1)
cms/server/admin/handlers/student.py (1)
370-381: Potential issue: Missing null check fortraining_day.contest_id.When iterating over training days to apply hidden status, if a training day has no contest (
training_day.contest is None), thentraining_day.contest_idwould beNone. The query would still work but would be semantically incorrect.🐛 Proposed fix
if apply_to_existing: # Update hidden status in all existing training day participations user = participation.user for training_day in training_program.training_days: + if training_day.contest is None: + continue td_participation = self.sql_session.query(Participation)\ .filter(Participation.contest_id == training_day.contest_id)\ .filter(Participation.user_id == user.id)\ .first() if td_participation: td_participation.hidden = new_hidden
🧹 Nitpick comments (1)
cms/server/admin/handlers/student.py (1)
194-209: Potential issue with emptytraining_day_contest_idsin IN clause.If a training program has no training days,
training_day_contest_idswill be an empty list. Some database backends handleIN ()poorly or reject it. While this is unlikely to cause issues in practice (an empty list typically returns 0 results), it's worth noting.♻️ Optional defensive check
# Count submissions and participations from training days training_day_contest_ids = [td.contest_id for td in training_program.training_days] + training_day_contest_ids = [cid for cid in training_day_contest_ids if cid is not None] + + if training_day_contest_ids: - training_day_participations = ( - self.sql_session.query(Participation) - .filter(Participation.contest_id.in_(training_day_contest_ids)) - .filter(Participation.user == user) - .count() - ) - - training_day_submissions = ( - self.sql_session.query(Submission) - .join(Participation) - .filter(Participation.contest_id.in_(training_day_contest_ids)) - .filter(Participation.user == user) - .count() - ) + training_day_participations = ( + self.sql_session.query(Participation) + .filter(Participation.contest_id.in_(training_day_contest_ids)) + .filter(Participation.user == user) + .count() + ) + training_day_submissions = ( + self.sql_session.query(Submission) + .join(Participation) + .filter(Participation.contest_id.in_(training_day_contest_ids)) + .filter(Participation.user == user) + .count() + ) + else: + training_day_participations = 0 + training_day_submissions = 0
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add timezone support, duration inputs, hidden user filtering, main group ranking split, and UI fixes
Summary
This PR implements several changes to the training program feature:
Timezone support: Training day and group time inputs now use the managing contest's timezone instead of UTC. The timezone name is displayed in form labels, and datetime inputs are parsed in the contest timezone before being converted to UTC for storage.
Duration-based inputs: Replaced end time inputs with duration inputs (hours + minutes) for both training days and training day groups. This makes it quicker to configure times since users typically know the duration rather than calculating end times.
Hidden user filtering: Hidden users from the training program are now filtered from combined ranking and attendance views. Additionally, when creating a training day, the
hiddenproperty from each student's managing contest participation is passed to their training day participation.Apply hidden status to existing training days: Added a new checkbox on the student edit page that allows admins to propagate hidden status changes to all existing training day participations. A hint is also shown indicating that hidden status will automatically apply to new training days.
Split combined ranking by main groups: The combined ranking view now displays separate tables for each main group (TrainingDayGroup). Each table shows only students belonging to that group (based on historical student_tags) and only task columns for tasks that were accessible to at least one student in that group. Each table has its own CSV/TXT export links with the group name in the filename (e.g.,
_advanced).Split training day ranking by main groups: The individual training day ranking view now also displays separate tables for each main group. Students are filtered by their current student_tags matching the group's tag_name. A new "Tags" column shows each student's tags after the User column. Each group table has its own CSV/TXT export links.
UI fixes for training program/day interface: Fixed several navigation and display issues:
Refactored trainingprogram.py into logical modules: Split the 3306-line
trainingprogram.pyfile into separate modules for better maintainability:trainingday.py: Training day management handlers (TrainingProgramTrainingDaysHandler, AddTrainingDayHandler, RemoveTrainingDayHandler, AddTrainingDayGroupHandler, UpdateTrainingDayGroupsHandler, RemoveTrainingDayGroupHandler, TrainingDayTypesHandler)student.py: Student management handlers (TrainingProgramStudentsHandler, AddTrainingProgramStudentHandler, RemoveTrainingProgramStudentHandler, StudentHandler, StudentTagsHandler, StudentTasksHandler, AddStudentTaskHandler, RemoveStudentTaskHandler, BulkAssignTaskHandler)archive.py: Archive, attendance, and combined ranking handlers (ArchiveTrainingDayHandler, TrainingProgramFilterMixin, TrainingProgramAttendanceHandler, TrainingProgramCombinedRankingHandler, TrainingProgramCombinedRankingHistoryHandler, TrainingProgramCombinedRankingDetailHandler)trainingprogram.py: Core training program handlers only (965 lines)Additional code fixes:
contestranking.py: Importget_all_student_tagsfromcms.server.utilinstead of.trainingprogramcontestranking.py: Sort by group-specific total using accessible_tasks instead of global totalcontestranking.py: Use deterministic main group selection (sorted()[0]) and batch query for studentstrainingday.py: Add duration validation (hours >= 0, 0 <= minutes < 60, duration > 0)ranking.html: Usecontest.score_precisionfor total formattingranking.txt: Usecontest.get_tasks()instead ofcontest.taskstraining_program_combined_ranking_detail.html: Handle missing face image element in JSReview & Testing Checklist for Human
Combined ranking page (CRITICAL): Navigate to
/training_program/{id}/combined_rankingand verify the page loads without errors. The refactoring accidentally removed significant logic fromTrainingProgramCombinedRankingHandlerwhich has been restored - verify task columns display correctly and student filtering works.Main group checkbox behavior: On the "Add Training Day" page, add 3 main groups, check the "Alphabetical" checkbox on all 3, delete the first row, then submit. Verify the remaining two groups have their alphabetical setting preserved correctly (this tests the checkbox re-indexing fix).
Archive button on attendance page: Navigate to a training day's attendance page (
/contest/{id}/delays_and_extra_times) where all participants have finished or missed. Verify a green notification with "Archive Training Day" button appears.Student page GET behavior: Verify that navigating to a student edit page for a user who doesn't have a Student record returns 404 (this is the expected behavior after the latest fix).
Duration validation: Try creating a training day with invalid durations (negative hours, minutes >= 60, zero duration when inputs provided) and verify server-side validation errors are shown.
Recommended test plan:
Notes
RankingHandlernow detects if a contest is a training day by checkingself.contest.training_dayStudentmodel linked to each participationParticipationbyuser_idsinceStudent.participation_idrefers to the managing contest participation, not the training day participationmain_groups_data = Noneto prevent template UndefinedErrorParticipationDetailHandlerandParticipationSubmissionsHandlernow useget_tasks()instead of directtasksaccess to properly handle managing contestsget_managing_participation()to find the correct participation and filters bytraining_day_idTrainingProgramStudentDetailRedirectHandlerto handle/training_program/{id}/student/{user_id}/detailURLs by redirecting to the managing contest's participation detail pageParticipationDetailHandlerandScoreHistoryHandlernow filter by main group when viewing training day data, so rankings are computed relative to the user's main group onlyprepare()method inBaseHandlerwas adding/editto all/student/URLs - now excludes/detailpaths to fix the history redirect 404DownloadUserContestSubmissionsHandlernow usesget_managing_participation()to find the correct participation when downloading submissions for a training day participationUpdates since last revision
ContestHandler.post()to ensure training day start can't be after any main group's start time, and training day end can't be before any main group's end timeranking_datastructure inTrainingProgramCombinedRankingHandlerto passArchivedStudentRankingobjects instead of float scores, fixing the "'float object' has no attribute 'student_tags'" errortraining_day_tasks,attendance_data,active_students_per_td, andfiltered_training_daysvariables that were accidentally removed during the module refactoring, fixing the "'training_day_tasks' is undefined" error on the combined ranking pageparse_and_validate_duration()function intrainingday.pyto eliminate duplicated duration validation logic across handlersstudent.pyfor both single and bulk task assignment (validates task belongs to student's training program contest)main_group_user_idsparameter incontestranking.py(returns 400 instead of 500 for invalid input)trainingprogram.pystudent tag lookup (batch query instead of per-participation queries)contestranking.pystudent tag lookup (batch query instead of per-participation queries)task_indexcomputation outside group loop inranking.htmlto avoid redundant computationarchive.pyget_training_day_num()using default argument patternparse_datetime_with_timezone: Updatedtrainingday.pyto use the sharedparse_datetime_with_timezonefunction instead of manualdt.strptime+local_to_utccalls. Also updatedparse_datetime_with_timezoneinbase.pyto handle HTML5 datetime-local format (YYYY-MM-DDTHH:MM).parse_and_validate_duration()function now rejects 0-duration when inputs are provided (prevents creating training days/groups with zero duration when the user explicitly enters values)training_day_groups.htmlwith user-friendly alert messageadd_training_day.htmlthat re-indexes checkbox values before submission to match visual order, fixing the issue where deleting rows caused checkbox values to mismatch with the enumerate index in the Python backendLink to Devin run: https://app.devin.ai/sessions/ef0590c9cdda4b8bb4b55f7c849a821a
Additional sessions: https://app.devin.ai/sessions/2ff49dc388da4214b6c91ec4db0d2441, https://app.devin.ai/sessions/68e60529d2e84b9a8ae66e037b389166, https://app.devin.ai/sessions/d63fd204d93c42dda9958cbe5bf1bb7b, https://app.devin.ai/sessions/19405b951969465790886642c46dd6e3, https://app.devin.ai/sessions/5848dd1f20f04831b64d81368c570c6b, https://app.devin.ai/sessions/7b344f55c8bc4df19048e2aaed3a42b5, https://app.devin.ai/sessions/6c39679e7828480a8d353c61b8906e1a
Requested by: Ron Ryvchin (@ronryv)
Summary by CodeRabbit
New Features
Improvements
UX
✏️ Tip: You can customize this high-level summary in your review settings.