Training program

[ronryv]

Summary

This PR adds comprehensive training program functionality to CMS, enabling structured training activities with organized participant groups, scheduled training days, student management, attendance tracking, and combined rankings.

Key Features:

• Training Programs: Create and manage structured training activities with organized participant groups
• Training Days: Schedule individual training sessions within programs with customizable groups and timing
• Student Management: Add participants to programs, assign tasks, and manage student tags for categorization
• Attendance Tracking: Record attendance with status, location, delays, and justification
• Targeted Announcements: Direct announcements to specific student groups within training
• Combined Rankings: View aggregated performance across multiple training sessions
• Excel Export: Download attendance and ranking reports in Excel format

Updates since last revision:

• Added setup_contest_or_training_program helper method to BaseHandler to reduce code duplication
• Consolidated TrainingProgramSubmissionsHandler, TrainingProgramAnnouncementsHandler, and TrainingProgramQuestionsHandler with their contest counterparts using (contest|training_program) URL patterns
• Fixed build_user_to_student_map usage in trainingprogramtask.py to use the utility function instead of inline query
• Removed duplicate handler classes and cleaned up unused imports

Review & Testing Checklist for Human

• Database migration (v48→v49): Review cms/db/scripts/db_v49.sql for correctness. Verify all new tables, constraints, and indexes are properly defined
• Handler consolidation: Test that both /contest/{id}/submissions and /training_program/{id}/submissions routes work correctly (same for announcements and questions)
• setup_contest_or_training_program helper: Verify the helper correctly sets self.contest and self.r_params for both entity types, and returns the training_program when applicable
• Training day task visibility: Confirm contest.get_tasks() returns training day tasks (not managing contest tasks) when accessed from a training day contest
• End-to-end test: Create a training program, add students, create training days with tasks, verify attendance tracking and ranking display work correctly

Notes

Link to Devin run: https://app.devin.ai/sessions/ab6b4117aa0a4d10a3d40ee24019eae7
Requested by: Ron Ryvchin (@ronryv)

Summary by CodeRabbit

• New Features

  • Added training program management for organizing structured training sessions
  • Student enrollment and task assignment within programs
  • Training day archiving with historical attendance and ranking data preservation
  • Tag-based student visibility controls for tasks and announcements
  • Excel export for attendance and combined rankings
  • Score distribution histograms with tag-based filtering

• Enhancements

  • Delay request management with per-group time windows
  • Combined ranking analytics across training days
  • Task submission tracking by source (training day vs. archive)

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

• 🔍 Trigger a full review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch training_program

Important

Action Needed: IP Allowlist Update

If your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:

• ✨ 136.113.208.247/32 (new)
• 34.170.211.100/32
• 35.222.179.152/32

Failure to add the new IP will result in interrupted reviews.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

cms/db/submission.py (1)

40-101: ⚠️ Potential issue | 🔴 Critical

Create update_49.sql to add the training_day_id column and index to the submissions table.

The ORM model in cms/db/submission.py adds training_day_id as a nullable foreign key column with an index, but there is no corresponding SQL migration file. Fresh database installations will get this column via metadata.create_all(), but existing databases upgrading through the migration system will lack it. Create cmscontrib/updaters/update_49.sql with an ALTER TABLE statement to add both the column and index to maintain schema consistency across all deployment paths.

cms/server/admin/templates/announcements.html (1)

109-132: ⚠️ Potential issue | 🟠 Major

Preserve visibility tags when editing announcements.

The edit input isn’t pre-populated, so submitting without re-adding tags clears visible_to_tags (and may unintentionally broaden visibility). Populate the input with the current tag list (or ensure your edit-toggle JS does so before submit).

🔧 Proposed fix

-                <input type="text" id="edit_visible_to_tags_{{ msg.id }}" name="visible_to_tags" class="announcement-visibility-tags" style="width: 100%">
+                <input type="text" id="edit_visible_to_tags_{{ msg.id }}" name="visible_to_tags" class="announcement-visibility-tags" style="width: 100%" value="{{ msg.visible_to_tags|join(', ')|e }}">

cms/server/admin/templates/base.html (1)

55-73: ⚠️ Potential issue | 🔴 Critical

Move jQuery before aws_table_utils.js and aws_form_utils.js.
Both files execute jQuery code at load time (e.g., $(selector) on lines 26–27 in aws_table_utils.js and line 30 in aws_form_utils.js), but are currently loaded before jQuery. This causes immediate runtime errors. Load jquery-3.6.0.min.js before these two scripts.

🤖 Fix all issues with AI agents

In `@cms/db/base.py`:
- Around line 276-280: The callable default invocation in cms/db/base.py
currently always calls col.default.arg(None) when col.default.is_callable, which
fails for zero-arg callables (e.g., the password lambda); change the logic in
the block handling col.default (used when fill_with_defaults=True) to detect
callable arity (e.g., via inspect.signature or try/except) and call
col.default.arg() when it accepts zero parameters, otherwise call
col.default.arg(None); update the branch around col.default.is_callable and use
prp.key to set the attribute after selecting the correct call form.

In `@cms/server/admin/handlers/contesttask.py`:
- Around line 119-194: The block guarded by training_day is not None performs
reordering (handlers using REMOVE_FROM_TRAINING_DAY, MOVE_UP, MOVE_DOWN,
MOVE_TOP, MOVE_BOTTOM) without verifying the passed task actually belongs to
that training_day; add an explicit ownership check at the start of that branch
(e.g. confirm task.training_day == training_day or task.training_day_id ==
training_day.id) and raise/return an error or no-op if it doesn't match, so
MOVE_UP/MOVE_DOWN/MOVE_TOP/MOVE_BOTTOM and the REMOVE_FROM_TRAINING_DAY path
only operate on tasks that belong to the specified training_day; reference the
existing symbols task, training_day, REMOVE_FROM_TRAINING_DAY, MOVE_UP,
MOVE_DOWN, MOVE_TOP, MOVE_BOTTOM and use the same sql_session/Task query flow
after the validation.

In `@cms/server/admin/handlers/trainingday.py`:
- Around line 440-446: In the get method, remove the unused local variable
managing_contest assigned from training_program.managing_contest; update the
get(self, training_program_id: str, training_day_id: str) function to not create
managing_contest (leave training_program = self.safe_get_item(TrainingProgram,
training_program_id) and training_day = self.safe_get_item(TrainingDay,
training_day_id) as-is) so there is no unused assignment; ensure no other
references to managing_contest remain in this method.

In `@cms/server/admin/handlers/user.py`:
- Around line 41-42: There are duplicate imports of exclude_internal_contests
(imported both with validate_date_of_birth and again on its own); remove the
redundant import line and ensure exclude_internal_contests and
validate_date_of_birth are imported only once (keep the combined import
statement that includes validate_date_of_birth and exclude_internal_contests and
delete the standalone exclude_internal_contests import) so the module no longer
triggers the F811 duplicate-import error.

In `@cms/server/admin/static/aws_style.css`:
- Around line 1-3: The CSS file begins with a BOM (U+FEFF) immediately before
the leading "html {" selector which causes parsing/analysis errors; remove the
invisible BOM and re-save the file using UTF-8 without BOM so the first
character is the plain "h" of "html {"; you can strip the BOM in your editor or
via a file-encoding conversion tool and verify the file now starts directly with
the "html {" token.

In `@cms/server/admin/templates/fragments/user_detail_scripts.html`:
- Around line 72-80: The AJAX error handler for fetchSubmissionsViaAjax
currently logs and updates UserDetail.submission_table but does not advance
UserDetail.data_fetched or trigger UserDetail.do_show(), which blocks rendering;
update the error function in the $.ajax call (where success uses
UserDetail.submissions_callback) to treat the failure as a completed fetch by
incrementing UserDetail.data_fetched and calling UserDetail.do_show() after
updating the DOM so the rest of the page (navigator) renders.

In `@cms/server/admin/templates/student_tasks.html`:
- Around line 48-53: The label's for attribute ("start_date") doesn't match the
select's id ("task_selector"), breaking the association; update the label's for
to "task_selector" (or alternatively change the select id to "start_date") so
the <label> targets the <select> used in the manual task assignment form (look
for the label with class "tp-filter-label" and the select element with id
"task_selector").

🧹 Nitpick comments (24)

cms/server/admin/static/js/admin_table_utils.js (2)

60-70: Drag cancel (Escape key) still triggers saveNewOrder.

When a user cancels a drag operation (e.g., pressing Escape), the dragend event still fires and calls saveNewOrder(). While the order comparison at lines 108-113 prevents unnecessary saves when the order hasn't changed, if the user moved rows around before pressing Escape, the visual order may have changed from placeholder insertions, potentially triggering an unintended save.

Consider tracking whether a successful drop event occurred before calling saveNewOrder:

♻️ Proposed fix to track drop completion

     var draggedRow = null;
     var placeholder = null;
     var originalOrder = null;
+    var dropCompleted = false;
 
     // ... in drop handler:
     tbody.addEventListener('drop', function(e) {
         e.preventDefault();
         if (placeholder && placeholder.parentNode && draggedRow) {
             placeholder.parentNode.insertBefore(draggedRow, placeholder);
             placeholder.parentNode.removeChild(placeholder);
+            dropCompleted = true;
         }
     });
 
     // ... in dragend handler:
     tbody.addEventListener('dragend', function(e) {
         if (draggedRow) {
             draggedRow.classList.remove('dragging');
             if (placeholder && placeholder.parentNode) {
                 placeholder.parentNode.removeChild(placeholder);
             }
+            if (dropCompleted) {
+                saveNewOrder();
+            } else {
+                restoreOriginalOrder();
+            }
             draggedRow = null;
             placeholder = null;
-            saveNewOrder();
+            dropCompleted = false;
         }
     });

---

157-173: Consider storing Tagify instances for cleanup.

The Tagify instances created here are not stored, which means:

1. They cannot be destroyed if the filter needs to be reinitialized
2. Calling initFilterTagify multiple times on the same elements may create duplicate instances (though Tagify may handle this internally)

If reinitialization is not a concern for this use case, this is fine as-is.

cms/server/admin/templates/training_program_student_remove.html (1)

21-21: Anchor tag missing href attribute affects accessibility.

The "Yes, remove" link lacks an href attribute, which can prevent keyboard users from focusing and activating it in some browsers. Screen readers may also not properly announce it as interactive.

♻️ Proposed fix

-<a onclick="CMS.AWSUtils.ajax_delete('{{ url("training_program", training_program.id, "student", user.id, "remove") }}');" class="button-link">Yes, remove</a>
+<a href="#" onclick="CMS.AWSUtils.ajax_delete('{{ url("training_program", training_program.id, "student", user.id, "remove") }}'); return false;" class="button-link">Yes, remove</a>

cms/server/admin/templates/student_tasks.html (1)

49-49: Unnecessary enctype="multipart/form-data" on form without file upload.

This form only submits a select value. The multipart encoding is typically used for file uploads and adds overhead. Consider removing it.

♻️ Proposed fix

-        <form enctype="multipart/form-data" action="{{ url("training_program", training_program.id, "student", selected_user.id, "tasks", "add") }}" method="POST" style="display: flex; gap: 12px;">
+        <form action="{{ url("training_program", training_program.id, "student", selected_user.id, "tasks", "add") }}" method="POST" style="display: flex; gap: 12px;">

cms/server/admin/static/aws_form_utils.js (1)

271-304: Consider documenting the 200ms timing assumption.

The userRemovalTriggeredAt timing detection relies on a 200ms window to distinguish user-initiated removals from automatic ones (e.g., duplicate detection). This works but is inherently timing-dependent.

Consider adding a comment explaining why 200ms was chosen and under what conditions it might need adjustment.

cms/server/admin/static/aws_table_utils.js (1)

103-109: Double-sort uses same numeric setting for both columns.

The sort applies getRowComparator twice: first for initial_column_idx, then for data_column_idx. Both use the same numeric flag derived from the clicked column's settings. If the initial sort column has a different data type (e.g., string names vs. numeric scores), this could produce unexpected ordering for the secondary sort.

This is likely acceptable if the initial column is typically the same type, but worth noting.

cms/server/admin/templates/macro/delay_request.html (1)

33-38: Consider extracting inline styles to a CSS class.

The warning block uses inline styles for background, border, padding, margin, and border-radius. While functional, extracting these to a reusable CSS class (e.g., .warning-box) would improve maintainability and ensure consistent styling across the admin interface.

cms/server/admin/templates/student_task_submissions.html (1)

1-4: Consider reordering template directives.

The {% import %} statement before {% extends %} is unconventional. While Jinja2 processes inheritance correctly regardless of order, the idiomatic pattern places {% extends %} first.

♻️ Suggested reordering

-{% import "macro/reevaluation_buttons.html" as macro_reevaluation_buttons %}
-
 {% extends "base.html" %}
+{% import "macro/reevaluation_buttons.html" as macro_reevaluation_buttons %}
 {% import 'macro/submission.html' as macro_submission %}

cms/server/admin/templates/training_program.html (1)

39-43: Consider adding confirmation for the destructive "Remove" action.

The Remove button directly submits the form without user confirmation. For destructive operations, a confirmation dialog helps prevent accidental data loss.

♻️ Suggested confirmation

 <form action="{{ url("training_programs") }}" method="POST" style="display:inline;">
   {{ xsrf_form_html|safe }}
   <input type="hidden" name="training_program_id" value="{{ training_program.id }}"/>
-  <input type="submit" name="operation" value="Remove" style="float: right;"/>
+  <input type="submit" name="operation" value="Remove" style="float: right;"
+         onclick="return confirm('Are you sure you want to remove this training program? This action cannot be undone.');"/>
 </form>

cms/server/admin/templates/fragments/user_detail_layout.html (1)

12-19: Consider moving inline close-button styles into the shared stylesheet.

Keeps presentation centralized and easier to override.

cms/server/admin/handlers/task.py (1)

723-734: Use exception chaining for better traceability.

When re-raising an exception as a different type, use raise ... from None to suppress the original exception context or raise ... from err to chain it. This improves stack traces and debugging.

♻️ Proposed fix

         try:
             task.set_default_output_only_submission_format()
-        except Exception:
+        except Exception as err:
             logger.error(
                 "Couldn't create default submission format for task %s "
                 "(dataset %s, type %s)",
                 task.id,
                 task.active_dataset.id,
                 task.active_dataset.task_type,
                 exc_info=True
             )
             raise tornado.web.HTTPError(
                 500, f"Couldn't create default submission format for task {task.id}"
-            )
+            ) from err

cms/server/admin/templates/fragments/training_day_groups.html (1)

78-91: Inconsistent form field naming for alphabetical_task_order.

The checkbox uses alphabetical_{{ group.id }} naming while other fields use array notation (group_id[], start_time[], duration_hours[]). This requires the server handler to use different parsing logic for this field. Consider using consistent array notation or ensure the handler correctly processes the mixed naming patterns.

♻️ Suggestion for consistent naming

       <td style="text-align: center;">
-        <input type="checkbox" name="alphabetical_{{ group.id }}" {{ "checked" if group.alphabetical_task_order else "" }}/>
+        <input type="hidden" name="alphabetical_task_order[]" value="false"/>
+        <input type="checkbox" name="alphabetical_task_order[]" value="true" {{ "checked" if group.alphabetical_task_order else "" }}/>
       </td>

Note: The hidden input ensures a value is always submitted even when unchecked. The server would need to handle pairs of values per group.

cms/db/student_task.py (1)

78-82: Consider adding a default for assigned_at.

The assigned_at column is non-nullable but has no default value. If a StudentTask is created without explicitly providing assigned_at, it will fail. Consider adding a default using datetime.utcnow or func.now() to match common patterns:

Proposed fix

+from sqlalchemy.sql import func
+
 # When the task was assigned to the student.
-assigned_at: datetime = Column(
+assigned_at: datetime = Column(
     DateTime,
     nullable=False,
+    default=func.now(),
 )

cms/db/training_day.py (1)

40-64: Consider defensive check for missing training_program.

The get_managing_participation function accesses training_day.training_program.managing_contest without checking if training_program is None. While the FK constraint makes this unlikely in normal operation, a defensive check could prevent cryptic AttributeError if called with inconsistent data.

Proposed defensive check

 def get_managing_participation(
     session: Session,
     training_day: "TrainingDay",
     user: "User",
 ) -> "Participation | None":
     ...
     from . import Participation
+    if training_day.training_program is None:
+        return None
     managing_contest = training_day.training_program.managing_contest
     return (
         session.query(Participation)
         .filter(Participation.contest_id == managing_contest.id)
         .filter(Participation.user_id == user.id)
         .first()
     )

cms/server/admin/static/aws_tp_styles.css (1)

966-977: Consolidate duplicate .tp-btn-secondary definitions.
The class is defined twice with slightly different properties; merging reduces style drift.

Also applies to: 2464-2485

cms/server/admin/handlers/contestdelayrequest.py (1)

145-156: Avoid double-scanning students for main group.
check_training_day_eligibility already walks the roster and can return the main group. Calling get_participation_main_group again repeats that scan and can drift if logic diverges. Consider reusing the main_group from the eligibility call (and caching it for warnings/CSV) to reduce per-request cost.

♻️ Suggested tweak

-        for participation in participations:
-            # For training day contests, check eligibility and skip ineligible students
-            if self.contest.training_day is not None:
-                is_eligible, _, _ = check_training_day_eligibility(
-                    self.sql_session, participation, self.contest.training_day
-                )
+        for participation in participations:
+            main_group = None
+            if self.contest.training_day is not None:
+                is_eligible, main_group, _ = check_training_day_eligibility(
+                    self.sql_session, participation, self.contest.training_day
+                )
                 if not is_eligible:
                     continue
-
-            main_group = get_participation_main_group(self.contest, participation)

cms/db/task.py (1)

64-121: Consider enforcing training_day_num when training_day_id is set.
Postgres treats NULL as distinct in unique constraints, so multiple tasks assigned to the same training day can still share a NULL order. If every assigned task must have a position, add a check constraint (or enforce at the model layer) to prevent training_day_id with training_day_num IS NULL.

🔧 Possible constraint

 __table_args__ = (
     UniqueConstraint('contest_id', 'num'),
     UniqueConstraint('contest_id', 'name'),
     UniqueConstraint('training_day_id', 'training_day_num'),
+    CheckConstraint("training_day_id IS NULL OR training_day_num IS NOT NULL"),
     ForeignKeyConstraint(

cms/server/admin/handlers/contest.py (1)

235-253: Consider using a custom exception class for validation errors.

The static analysis hints suggest abstracting these ValueError raises to avoid long messages in raises. While this is a minor style concern, the validation logic itself is correct - ensuring training day times don't conflict with main group times.

♻️ Optional: Extract validation to helper

+def _validate_training_day_times(training_day, new_start, new_stop):
+    """Validate training day times against main group times."""
+    for group in training_day.groups:
+        if group.start_time is not None and new_start is not None:
+            if new_start > group.start_time:
+                raise ValueError(
+                    f"Training day start cannot be after main group "
+                    f"'{group.tag_name}' start time"
+                )
+        if group.end_time is not None and new_stop is not None:
+            if new_stop < group.end_time:
+                raise ValueError(
+                    f"Training day end cannot be before main group "
+                    f"'{group.tag_name}' end time"
+                )
+
 # In ContestHandler.post():
-            for group in training_day.groups:
-                if group.start_time is not None and new_start is not None:
-                    # ... validation logic ...
+            _validate_training_day_times(training_day, new_start, new_stop)

cms/server/admin/templates/training_program_attendance.html (2)

69-75: Export URL construction could be simplified with urlencode.

The manual URL construction with conditional ampersands is error-prone. Consider using a template helper or building the query string programmatically.

---

135-135: Consider using data attributes instead of inline onclick.

The onclick="openAttendanceModal(this)" works but separating JavaScript from HTML is generally preferred for maintainability. However, this is a minor stylistic point.

cms/server/admin/handlers/trainingprogramtask.py (1)

333-442: Consider batching task-archive progress calculation.
calculate_task_archive_progress(...) runs once per participation, which can lead to N+1 StudentTask queries on large programs. Prefetch StudentTask rows (and any cached scores) in bulk and compute progress in-memory, or extend the helper to accept preloaded data.

cms/server/admin/handlers/student.py (1)

167-266: Batch user/participation lookups to avoid N+1 queries.
The per-username user lookup and per-user participation check can be batched to reduce DB round-trips on large files.

♻️ Proposed refactor (batch lookups)

-            for username in usernames:
-                user = self.sql_session.query(User).filter(
-                    User.username == username).first()
+            users = (
+                self.sql_session.query(User)
+                .filter(User.username.in_(usernames))
+                .all()
+            )
+            user_by_username = {u.username: u for u in users}
+            existing_user_ids = {
+                uid for (uid,) in self.sql_session.query(Participation.user_id)
+                .filter(Participation.contest == managing_contest)
+                .filter(Participation.user_id.in_([u.id for u in users]))
+                .all()
+            }
+
+            for username in usernames:
+                user = user_by_username.get(username)
@@
-                    existing_participation = (
-                        self.sql_session.query(Participation)
-                        .filter(Participation.contest == managing_contest)
-                        .filter(Participation.user == user)
-                        .first()
-                    )
-
-                    if existing_participation is not None:
+                    if user.id in existing_user_ids:

cms/server/admin/handlers/base.py (2)

355-414: Consider narrowing the exception handling scope.

The broad except Exception: pass at line 413-414 could silently hide unexpected errors. While the intent is to gracefully fall back to normal request handling if the redirect logic fails, catching all exceptions makes debugging difficult.

♻️ Proposed refinement

-            except Exception:
-                pass
+            except (ValueError, AttributeError, KeyError):
+                # Redirect logic failed - continue with normal request handling
+                pass

---

991-994: Use exception chaining for cleaner stack traces.

The static analysis correctly identifies that the exception should be raised with explicit chaining to clarify the relationship between the ValueError and the HTTPError.

♻️ Proposed fix

         try:
             user_id_int = int(user_id)
         except ValueError:
-            raise tornado.web.HTTPError(404)
+            raise tornado.web.HTTPError(404) from None

[coderabbitai]

Actionable comments posted: 14

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

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/macro/submission.html (1)

201-201: ⚠️ Potential issue | 🟡 Minor

Minor: Extra quote in id attribute.

There's a typo with an extra double-quote in id="latency"".

🐛 Fix typo

-  <td id="latency"">
+  <td id="latency">

🤖 Fix all issues with AI agents

In `@cms/server/admin/handlers/archive.py`:
- Around line 155-215: The post handler can leave the SQLAlchemy session dirty
on exception; in the except block inside post (method post, symbols:
self.sql_session, training_day, contest, self.try_commit) call
self.sql_session.rollback() before adding the failure notification and
redirecting so the session is reset, then return as before; ensure rollback
happens before any other session-using calls in that error path.

In `@cms/server/admin/handlers/contestdelayrequest.py`:
- Around line 49-76: get_participation_main_group duplicates eligibility logic
and does case-sensitive tag matching; replace its manual scan with a call into
the shared check_training_day_eligibility utility (or whichever canonical
function computes a student's main group) to ensure consistent, case-insensitive
rules and reuse any precomputed results. Locate get_participation_main_group and
replace the loop over training_program.students and the
main_group_tags/student_tags matching with a delegation to
check_training_day_eligibility (or the shared helper that returns the student's
eligible group/tag), passing the contest/training_day and participation.user_id
(or the student object) as required, and return the resulting TrainingDayGroup
if present. Ensure tag comparisons follow the shared function's case
normalization and remove the ad-hoc set/intersection logic so behavior remains
canonical and DRY.

In `@cms/server/admin/handlers/contestsubmission.py`:
- Line 28: The handler references Contest in ContestUserTestsHandler.get via
self.safe_get_item(Contest, contest_id) but Contest is not imported; update the
import statement that currently reads "from cms.db import Submission, UserTest,
Task" to also import Contest so the symbol is available to
ContestUserTestsHandler and avoid NameError at runtime.

In `@cms/server/admin/handlers/excel.py`:
- Around line 81-85: The excel_safe function only checks the very first
character and misses values where the first non-space character is a formula
token (e.g., " =1"). Update excel_safe to detect the first non-whitespace
character (use str.lstrip or iterate to find the index of the first non-space)
and, if that character is one of "=", "+", "-", "@", return the original string
prefixed with an apostrophe; ensure you handle empty or all-whitespace strings
by returning them unchanged. This change should be made inside the excel_safe
function so it preserves the original value except for the added leading
apostrophe when a formula token appears after any leading whitespace.

In `@cms/server/admin/handlers/submissiondownload.py`:
- Around line 295-304: The current query joins Task and filters Task.contest_id
== managing_contest.id which excludes training-day submissions (they use
Task.training_day_id); instead query Submission for the managing contest via the
submission's participation relationship so training-day submissions are
included. Update the submissions query (the block using Submission, Task,
joinedload(Submission.training_day)) to remove the join/filter on
Task.contest_id and replace it with a filter that matches the managing contest
on Submission.participation (e.g. Participation.contest_id ==
managing_contest.id), keeping the joinedload(Submission.training_day) and
allowing get_source_folder() to handle training-day submissions.

In `@cms/server/admin/handlers/training_analytics.py`:
- Around line 582-593: The handler currently writes an explicit error JSON on
try_commit() failure; remove the write_error_json(500, ...) call and instead
return early so centralized error handling/logging can handle the failure;
locate the block around try_commit() in training_analytics.py (the code that
calls self.try_commit(), self.write(...), and self.write_error_json(...)) and
replace the else branch with an early return (no explicit response), preserving
the successful self.write(...) branch and any local variables like
att.justified/att.comment/att.recorded.
- Around line 127-132: Remove the runtime filter that excludes archived students
based on current participation.hidden: in the loop over
archived_student_rankings (symbol: archived_student_rankings, variable: rank)
delete the check that inspects rank.student.participation.hidden so historical
visibility is preserved; likewise update get_student_detail_data to stop
filtering historical student lists by current participation.hidden (remove that
conditional), while leaving the access-control check that uses current hidden
status for detail pages unchanged.

In `@cms/server/admin/handlers/trainingprogram.py`:
- Around line 379-449: The loop that deletes training days assumes
training_day.contest is always present; if it's None (archived day) deleting
None will raise. In the delete method, update the loop over
training_program.training_days (and the local td_contest variable) to check for
a non-None contest before calling self.sql_session.delete(td_contest) — always
delete the training_day, but only delete td_contest when td_contest is truthy;
reference the delete method, training_program.training_days,
training_day.contest and td_contest to locate the change.

In `@cms/server/admin/handlers/trainingprogramtask.py`:
- Around line 290-334: Ensure the handler validates task ownership before
detaching it: in the delete method (the function decorated with
`@require_permission` and using safe_get_item for TrainingProgram and Task), check
that task.contest equals the training_program.managing_contest (or is
None/expected) and abort (raise a permission/validation error or return 404/403)
if it does not match; only proceed with clearing training_day, contest, num,
shifting task numbers, committing and reinitializing when the task is confirmed
to belong to that managing_contest.

In `@cms/server/admin/static/aws_table_utils.js`:
- Around line 24-35: The comparator in CMS.AWSTableUtils.getRowComparator reads
only "td" cells, so columns using <th> (row headers) are ignored; update the
selectors in that function (the variables cellA and cellB where you call
.children("td").eq(column_idx)) to select both cell types by using "td,th"
instead of "td" so the comparator reads data-value/text from either <td> or <th>
when determining valA and valB.

In `@cms/server/admin/static/training_program.js`:
- Line 10: The bootstrap reads CMS before its declaration; change the
initialization to reference the global explicitly by replacing the current var
CMS = CMS || {}; with a safe global access that uses window.CMS (e.g., var CMS =
window.CMS || {}), so the symbol CMS is not referenced before declaration and
the global namespace is preserved — update the CMS bootstrap initialization in
training_program.js accordingly.

In `@cms/server/admin/templates/fragments/user_detail_scripts.html`:
- Around line 92-125: UserDetail.do_show builds HTML by concatenating
contest['name'] and task['name'] directly, which allows HTML/script injection;
update UserDetail.do_show to escape these values before inserting into strings
(or preferably create DOM nodes and set textContent/.text() instead of string
concatenation). Locate UserDetail.do_show and replace occurrences of
contest['name'] and task['name'] (and any similar direct insertions like in the
global row if needed) with a safe-escaped variant (e.g., use an existing escape
helper or add one like escapeHtml and call it where contest['name'] and
task['name'] are used) or construct the row via document.createElement and
element.textContent to ensure names are safely encoded.

In `@cms/server/admin/templates/student_tasks.html`:
- Around line 128-153: The template uses a fixed threshold (>= 100) to mark
green/amber which is incorrect because training_scores and home_scores are raw
values; update the color logic to compare each score to the task's max score
(use st.task.max_score or whatever the task property is) or compute a normalized
percentage (score / st.task.max_score * 100) and then set green when score >=
st.task.max_score (or percent >= 100), amber when score > 0, else grey; change
the color expression in both the training_scores and home_scores blocks (the
spans that currently use '{{ '#059669' if score >= 100 else '#d97706' if score >
0 else '#cbd5e1' }}') to use the task max-based comparison or normalized
percentage.

In `@cms/server/admin/templates/training_program_combined_ranking_detail.html`:
- Around line 6-16: The "Edit Student" anchor should be rendered only when
student.participation exists to avoid a 500; wrap the <a href="{{
url("training_program", training_program.id, "student",
student.participation.user.id, "edit") }}">Edit Student</a> output in a
conditional like {% if student.participation %}...{% endif %} (or check
student.participation.user) so the template won't try to access
student.participation.user.id when participation is missing.

🟡 Minor comments (14)

cms/server/admin/static/aws_utils.js-832-845 (1)

832-845: ⚠️ Potential issue | 🟡 Minor

Ensure tags are cleared when no raw values are present.

If .announcement_raw_visible_to_tags is absent, the previous Tagify state can linger across edits. Clearing the input/tagify when the raw field is missing prevents accidental tag carry-over.

🧹 Suggested fix

-    const visibleToTagsInput = form.querySelector('input[name="visible_to_tags"]');
-    const rawVisibleToTags = notification.querySelector('.announcement_raw_visible_to_tags');
-    if (visibleToTagsInput && rawVisibleToTags) {
-        const rawValue = rawVisibleToTags.value;
-        const tagify = visibleToTagsInput._tagify;
-        if (tagify) {
-            tagify.removeAllTags();
-            const tags = rawValue.split(",").map(t => t.trim()).filter(Boolean);
-            if (tags.length) tagify.addTags(tags);
-        } else {
-            visibleToTagsInput.value = rawValue;
-        }
-    }
+    const visibleToTagsInput = form.querySelector('input[name="visible_to_tags"]');
+    const rawVisibleToTags = notification.querySelector('.announcement_raw_visible_to_tags');
+    if (visibleToTagsInput) {
+        const rawValue = rawVisibleToTags ? rawVisibleToTags.value : "";
+        const tagify = visibleToTagsInput._tagify;
+        if (tagify) {
+            tagify.removeAllTags();
+            const tags = rawValue.split(",").map(t => t.trim()).filter(Boolean);
+            if (tags.length) tagify.addTags(tags);
+        } else {
+            visibleToTagsInput.value = rawValue;
+        }
+    }

cms/server/admin/static/aws_form_utils.js-211-218 (1)

211-218: ⚠️ Potential issue | 🟡 Minor

Add guard for Tagify in initTagify to match initReadOnlyTagify.

initTagify instantiates Tagify at line 306 without checking if it's defined, unlike initReadOnlyTagify which guards on line 171. If the Tagify script fails to load, this will throw and break other page functionality. Add the same guard used elsewhere in the codebase:

♻️ Suggested guard

 CMS.AWSFormUtils.initTagify = function(config) {
+    if (typeof Tagify === 'undefined') {
+        return;
+    }
     var inputs = document.querySelectorAll(config.inputSelector);
     if (!inputs.length) return;

cms/server/admin/handlers/task.py-723-734 (1)

723-734: ⚠️ Potential issue | 🟡 Minor

Add from None to the raised exception to satisfy exception chaining best practices.

The static analysis tool (Ruff B904) correctly flags that exceptions raised within an except block should use raise ... from err or raise ... from None to distinguish intentional exception transformations from errors in exception handling.

Since the original exception is already logged with exc_info=True, using from None is appropriate here.

🐛 Proposed fix

             raise tornado.web.HTTPError(
                 500, f"Couldn't create default submission format for task {task.id}"
-            )
+            ) from None

cms/server/admin/templates/contest_tasks.html-122-122 (1)

122-122: ⚠️ Potential issue | 🟡 Minor

Potential error if visible_to_tags is None.

If t.visible_to_tags is None (rather than an empty list), the |join(', ') filter will fail. Consider adding a default:

🛡️ Suggested fix

-          <input type="text" class="task-visibility-tags" data-task-id="{{ t.id }}" value="{{ t.visible_to_tags|join(', ') }}"/>
+          <input type="text" class="task-visibility-tags" data-task-id="{{ t.id }}" value="{{ (t.visible_to_tags or [])|join(', ') }}"/>

cms/server/admin/templates/delays_and_extra_times.html-246-286 (1)

246-286: ⚠️ Potential issue | 🟡 Minor

Potential undefined variable ineligible_training_program.

Line 246 checks ineligible_students is defined, but Line 285 uses ineligible_training_program.id without verifying it's defined. If the server fails to provide this variable, the template will error.

Consider adding a guard or ensuring the server always provides both variables together:

🛡️ Suggested fix

-{% if ineligible_students is defined and ineligible_students|length > 0 %}
+{% if ineligible_students is defined and ineligible_students|length > 0 and ineligible_training_program is defined %}

cms/server/admin/handlers/contestannouncement.py-91-121 (1)

91-121: ⚠️ Potential issue | 🟡 Minor

Training-program announcements currently require non-empty text.
Contests allow empty text, so this is inconsistent and can block valid announcements. If empty text is acceptable, gate on subject only and keep the same notification as contests.

✅ Align with contest behavior

-        if subject and text:
+        if subject:
             if announcement_id is not None:
                 # Edit existing announcement
                 announcement = self.safe_get_item(Announcement, announcement_id)
                 if announcement.contest_id != managing_contest.id:
                     raise tornado.web.HTTPError(404)
                 announcement.subject = subject
                 announcement.text = text
                 announcement.visible_to_tags = visible_to_tags
             else:
                 # Add new announcement
                 announcement = Announcement(
                     timestamp=make_datetime(),
                     subject=subject,
                     text=text,
                     contest=managing_contest,
                     admin=self.current_user,
                     visible_to_tags=visible_to_tags,
                 )
                 self.sql_session.add(announcement)
             self.try_commit()
+        else:
+            self.service.add_notification(
+                make_datetime(), "Subject is mandatory.", "")

cms/server/admin/templates/student.html-210-214 (1)

210-214: ⚠️ Potential issue | 🟡 Minor

Add |format_datetime filter to timestamp display.

msg.timestamp is displayed without formatting in this template, while identical message displays in participation.html (line 196) and announcements.html (line 92) use the |format_datetime filter. Update to {{ msg.timestamp|format_datetime }} for consistency and readable output.

cms/db/task.py-126-129 (1)

126-129: ⚠️ Potential issue | 🟡 Minor

Use default=list instead of default=[] for ARRAY columns.

While SQLAlchemy copies mutable defaults safely, the best practice is to pass a callable (list) rather than a mutable object ([]). This pattern is already used elsewhere in the codebase (e.g., cms/db/student.py:71, cms/db/training_day.py:144), but several ARRAY columns inconsistently use default=[] instead. Standardize to default=list for consistency and to follow SQLAlchemy conventions. The same issue also appears in primary_statements (line 152 of this file) and visible_to_tags in cms/db/contest.py (line 443).

cms/server/admin/templates/training_program_students.html-185-196 (1)

185-196: ⚠️ Potential issue | 🟡 Minor

Confirm prompt has unescaped username - potential XSS vector.

Line 187 interpolates {{ u.username }} directly into a JavaScript string within an onclick attribute. If a username contains a single quote, it would break out of the string and could enable XSS.

🔧 Suggested fix using tojson for proper escaping

                                 <button type="submit" name="operation" value="remove_{{ u.id }}" class="btn-icon-only"
                                     {% if not admin.permission_all %}disabled{% endif %}
-                                    onclick="return confirm('Are you sure you want to remove {{ u.username }} from the training program?');"
+                                    onclick='return confirm("Are you sure you want to remove " + {{ u.username|tojson }} + " from the training program?");'
                                     title="Remove from program">

cms/server/admin/handlers/contestdelayrequest.py-210-229 (1)

210-229: ⚠️ Potential issue | 🟡 Minor

Case-insensitive tag matching for ineligible students.
Eligibility uses lowercase comparisons in check_training_day_eligibility; the ineligible-student list should match that behavior to avoid false “no/multiple main group” flags.

🛠️ Suggested fix

-            main_group_tags = {g.tag_name for g in training_day.groups}
+            main_group_tags = {g.tag_name.lower() for g in training_day.groups}
 ...
-                student_tags = set(student.student_tags or [])
+                student_tags = {t.lower() for t in (student.student_tags or [])}
                 matching_tags = student_tags & main_group_tags

cms/server/admin/handlers/student.py-118-164 (1)

118-164: ⚠️ Potential issue | 🟡 Minor

Add-student path should guard against existing membership.
The bulk-add flow checks for existing participation, but the single-add path doesn’t. That can trigger an IntegrityError or duplicate enrollment if the UI sends a stale user_id.

🛠️ Suggested fix

         user = self.safe_get_item(User, user_id)

+        existing_participation = (
+            self.sql_session.query(Participation)
+            .filter(Participation.contest == managing_contest)
+            .filter(Participation.user == user)
+            .first()
+        )
+        if existing_participation is not None:
+            self.service.add_notification(
+                make_datetime(),
+                "Invalid field(s)",
+                "User is already a student in this program",
+            )
+            self.redirect(fallback_page)
+            return
+
         # Set starting_time to now so the student can see everything immediately

cms/server/admin/handlers/utils.py-218-245 (1)

218-245: ⚠️ Potential issue | 🟡 Minor

Default score_precision when unset.
task.score_precision can be None in other parts of the codebase; returning None here risks downstream formatting/rounding issues.

🛠️ Suggested fix

-    score_precision = task.score_precision
+    score_precision = task.score_precision if task.score_precision is not None else 0

cms/server/admin/handlers/trainingday.py-757-769 (1)

757-769: ⚠️ Potential issue | 🟡 Minor

Normalize scoreboard-sharing tag keys to lowercase.
Student tags are normalized to lowercase elsewhere (e.g., parse_tags). Leaving mixed-case keys here can cause mismatches when resolving sharing rules.

📝 Suggested tweak

-                    if tag == "__everyone__":
-                        normalized_tag = tag
-                    else:
-                        normalized_tag = tag.strip()
+                    if tag == "__everyone__":
+                        normalized_tag = tag
+                    else:
+                        normalized_tag = tag.strip().lower()

cms/server/admin/handlers/contestranking.py-504-509 (1)

504-509: ⚠️ Potential issue | 🟡 Minor

Chain the parsing error to preserve debugging context.
This aligns with Ruff B904 (raise-without-from-inside-except), which flags exception raises inside except handlers that don't use exception chaining.

🔧 Suggested fix

-            except ValueError:
-                raise tornado.web.HTTPError(400, "Invalid main_group_user_ids parameter")
+            except ValueError as err:
+                raise tornado.web.HTTPError(
+                    400, "Invalid main_group_user_ids parameter"
+                ) from err

🧹 Nitpick comments (20)

cms/locale/cms.pot (1)

1267-1411: Consider de‑ambiguating single‑letter labels (“H:”, “T:”) for translators.
These are hard to localize without context; prefer full labels or add translator comments/context in the source so the POT carries that metadata.

cms/server/admin/static/aws_style.css (1)

1448-1478: Consolidate duplicate CSS rule definitions.

Several selectors are defined twice in this file, which creates maintenance burden and potential for inconsistencies:

Selector	First definition	Duplicate here
.menu_entry	Lines 500-505	Lines 1448-1453
.menu_link	Lines 514-522	Lines 1455-1458
.sidebar-section-title	Lines 222-237	Lines 1461-1466
.sidebar-contest, .sidebar-item	Lines 383-389	Lines 1469-1474
.sidebar-contest a, .sidebar-item a	Lines 395-405	Lines 1477-1478

Consider consolidating each selector into a single rule block to avoid confusion and ensure consistent styling.

cms/server/admin/static/tagify/tagify.css (1)

1-1: Consider CSS fallbacks for older browsers if necessary for your target environment.
The :has() selector and lh unit are well-supported in modern browsers (Chrome 105+, Edge 105+, Firefox 121+, Safari 15.4+), so they're safe for most admin panel use cases. If your deployment requires support for older browsers, consider adding fallbacks like min-height: 1.5em before min-height: 1.5lh, or use @supports selector(:has(*)) for conditional rules.

cms/server/admin/templates/participation.html (1)

11-18: Add eager loading for training_day and training_program relationships.

The template accesses contest.training_day and contest.training_day.training_program, which will trigger lazy loads if these relationships aren't eagerly fetched in the handler. Consider using:

self.contest = self.sql_session.query(Contest)\
    .options(
        joinedload(Contest.training_day)
        .joinedload(TrainingDay.training_program)
    )\
    .get(contest_id)

Or update safe_get_item() to accept options for the query, similar to the pattern already used in ContestHandler (contest.py:151).

cms/db/base.py (1)

276-285: Avoid catching TypeError from inside default callables.

The current try/except treats any TypeError as an arity mismatch, which can mask real errors thrown by the default callable itself. Consider inspecting the callable signature to decide whether to call with an argument, so genuine failures still surface.

♻️ Suggested refactor (signature-based arity detection)

+import inspect
 ...
-                if col.default is not None:
-                    if col.default.is_callable:
-                        try:
-                            # Try passing None (simulating a missing ExecutionContext)
-                            setattr(self, prp.key, col.default.arg(None))
-                        except TypeError:
-                            # Fallback for zero-argument callables (like simple lambdas)
-                            setattr(self, prp.key, col.default.arg())
-                    else:
-                        setattr(self, prp.key, col.default.arg)
+                if col.default is not None:
+                    if col.default.is_callable:
+                        default_callable = col.default.arg
+                        try:
+                            params = inspect.signature(default_callable).parameters
+                            expects_ctx = len(params) > 0
+                        except (TypeError, ValueError):
+                            # Fallback for non-introspectable callables
+                            expects_ctx = True
+                        value = default_callable(None) if expects_ctx else default_callable()
+                        setattr(self, prp.key, value)
+                    else:
+                        setattr(self, prp.key, col.default.arg)

cms/server/admin/handlers/user.py (1)

196-239: Consider eager loading to avoid N+1 contest lookups.

p.contest.training_program / p.contest.training_day inside the loop can trigger per-row lazy loads. A joined/select-in load on Participation.contest and its relationships keeps this linear.

♻️ Suggested eager loading

+from sqlalchemy.orm import joinedload
 ...
-        all_participations = self.sql_session.query(Participation)\
-            .filter(Participation.user == user)\
-            .all()
+        all_participations = (
+            self.sql_session.query(Participation)
+            .options(
+                joinedload(Participation.contest).joinedload(Contest.training_program),
+                joinedload(Participation.contest).joinedload(Contest.training_day),
+            )
+            .filter(Participation.user == user)
+            .all()
+        )

cms/server/admin/templates/fragments/user_detail_scripts.html (1)

101-127: Convert for...in loops to index-based loops for array iteration.

contest_list and contest["tasks"] are arrays (not objects), confirmed by their initialization with new Array() and array methods used throughout the codebase. The codebase already uses index-based loops elsewhere for these same arrays (DataStore.js:226-231, 621). For consistency and to avoid potential issues with inherited properties, convert both loops to index-based iteration.

♻️ Suggested refactor

-      for (var i in contests) {
-        var contest = contests[i];
+      for (var i = 0; i < contests.length; i++) {
+        var contest = contests[i];
@@
-        for (var j in tasks) {
-          var task = tasks[j];
+        for (var j = 0; j < tasks.length; j++) {
+          var task = tasks[j];

cms/server/admin/templates/macro/delay_request.html (1)

33-38: Consider extracting inline styles to a CSS class.

The warning block uses inline styles which work but could be consolidated into a reusable CSS class for consistency across the admin interface.

♻️ Optional: Extract to CSS class

/* In your CSS file */
.delay-request-warning {
  background-color: `#fff3cd`;
  border: 1px solid `#ffc107`;
  padding: 8px;
  margin: 8px 0;
  border-radius: 4px;
}
.delay-request-warning strong {
  color: `#856404`;
}

Then in the template:

<div class="delay-request-warning">
  <strong>Warning:</strong> ...
</div>

cms/server/admin/templates/fragments/user_detail_layout.html (1)

56-61: Consider making canvas dimensions responsive.

The canvas elements have hardcoded dimensions (width="920" height="200"). If this layout is used on screens with varying widths, consider using CSS to make the canvas responsive or setting dimensions via JavaScript based on container size.

cms/server/admin/handlers/contestquestion.py (1)

74-91: Consider combining the two loops over training_days for efficiency.

The code iterates over training_program.training_days twice:

1. Lines 75-80: To compute notifications (if not already cached)
2. Lines 81-91: To build the list of days with unanswered questions

These could be combined into a single loop:

♻️ Suggested optimization

-            if not td_notifications:
-                for td in training_program.training_days:
-                    if td.contest is None:
-                        continue
-                    td_notifications[td.id] = get_training_day_notifications(
-                        self.sql_session, td
-                    )
-            for td in training_program.training_days:
-                if td.contest is None:
-                    continue
-                td_notif = td_notifications.get(td.id, {})
+            for td in training_program.training_days:
+                if td.contest is None:
+                    continue
+                if td.id not in td_notifications:
+                    td_notifications[td.id] = get_training_day_notifications(
+                        self.sql_session, td
+                    )
+                td_notif = td_notifications[td.id]
                 unanswered_count = td_notif.get("unanswered_questions", 0)

cms/server/admin/templates/fragments/training_day_groups.html (1)

78-91: Inconsistent form field naming for checkbox.

The other input fields use array notation (group_id[], start_time[], duration_hours[], etc.), but the checkbox uses alphabetical_{{ group.id }}. This may require different handling on the server side.

Consider using a consistent naming pattern. If the server expects array notation:

♻️ Option 1: Use hidden + checkbox pattern for array

       <td style="text-align: center;">
-        <input type="checkbox" name="alphabetical_{{ group.id }}" {{ "checked" if group.alphabetical_task_order else "" }}/>
+        <input type="hidden" name="alphabetical_order[]" value="false"/>
+        <input type="checkbox" name="alphabetical_order[]" value="true" {{ "checked" if group.alphabetical_task_order else "" }}/>
       </td>

Alternatively, the current approach is fine if the server handler explicitly iterates over group IDs and checks for alphabetical_{id} parameters.

cms/server/admin/handlers/contest.py (2)

245-263: Consider extracting validation into a helper method.

The inline ValueError raises with f-strings work correctly but could be cleaner with a dedicated validation helper. This is a minor suggestion aligned with the Ruff hints (TRY003, TRY301).

♻️ Optional: Extract validation helper

def _validate_training_day_times(training_day, new_start, new_stop):
    """Validate contest times don't conflict with training day group times."""
    if not training_day or not training_day.groups:
        return
    for group in training_day.groups:
        if group.start_time is not None and new_start is not None:
            if new_start > group.start_time:
                raise ValueError(
                    f"Training day start cannot be after main group "
                    f"'{group.tag_name}' start time"
                )
        if group.end_time is not None and new_stop is not None:
            if new_stop < group.end_time:
                raise ValueError(
                    f"Training day end cannot be before main group "
                    f"'{group.tag_name}' end time"
                )

---

391-396: Broad exception catch is flagged by static analysis.

Catching bare Exception (BLE001) can mask unexpected errors. Consider catching more specific exceptions if the expected failure modes are known (e.g., ValueError, AssertionError, KeyError).

♻️ Optional: Narrow exception types

-        except Exception as error:
+        except (ValueError, AssertionError) as error:

cms/server/admin/templates/student.html (1)

170-170: Consider using not for list emptiness checks.

Using != [] works but {% if participation.questions %} (truthy check) is more idiomatic in Jinja2.

♻️ More idiomatic empty list check

-  {% if participation.questions != [] %}
+  {% if participation.questions %}

Same applies to line 207:

-    {% if participation.messages != [] %}
+    {% if participation.messages %}

cms/server/admin/templates/training_program_combined_ranking.html (2)

218-221: Complex URL construction with filter parameters.

The export URL building inline with multiple conditionals is hard to read and maintain. Consider using Jinja2's url_for with query params or building the params list programmatically (similar to lines 281-286).

♻️ Suggested: Use consistent URL param building

{% set export_params = [] %}
{% if start_date %}{% set _ = export_params.append('start_date=' ~ start_date.strftime('%Y-%m-%d')) %}{% endif %}
{% if end_date %}{% set _ = export_params.append('end_date=' ~ end_date.strftime('%Y-%m-%d')) %}{% endif %}
{% if training_day_types %}{% set _ = export_params.append('training_day_types=' ~ (training_day_types | join(',') | urlencode)) %}{% endif %}
{% if student_tags %}{% set _ = export_params.append('student_tags=' ~ (student_tags | join(',') | urlencode)) %}{% endif %}
{% if student_tags_mode %}{% set _ = export_params.append('student_tags_mode=' ~ student_tags_mode) %}{% endif %}

<a href="{{ url('training_program', training_program.id, 'combined_ranking', 'export') }}{% if export_params %}?{{ export_params | join('&') }}{% endif %}" ...>

---

17-47: Potential performance concern with nested template loops generating JS.

For training programs with many students and training days, the nested Jinja2 loops generating JavaScript objects (lines 18-23, 30-47, 70-82) could produce large output. Consider whether this data should be served via a separate JSON endpoint for very large datasets.

cms/db/training_day.py (1)

174-178: Consider adding passive_deletes=True for consistency.

The groups relationship has cascade="all, delete-orphan" but lacks passive_deletes=True. Since TrainingDayGroup has ondelete="CASCADE" on its FK (per the relevant code snippets), adding passive_deletes=True would allow the database to handle deletions directly without SQLAlchemy issuing separate DELETE statements, improving performance on bulk deletes.

This is consistent with how the submissions relationship (line 183) is configured.

🔧 Suggested change

     groups: list["TrainingDayGroup"] = relationship(
         "TrainingDayGroup",
         back_populates="training_day",
         cascade="all, delete-orphan",
+        passive_deletes=True,
     )

cms/server/admin/handlers/base.py (2)

278-295: Avoid user-facing notifications on try_commit failure.

The current failure notification conflicts with the established pattern for admin handlers. Consider relying on centralized error handling/logging instead.

🔧 Suggested adjustment

-        except IntegrityError as error:
-            self.service.add_notification(
-                make_datetime(), "Operation failed.", "%s" % error
-            )
-            return False
+        except IntegrityError:
+            return False

Based on learnings: 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.

---

350-413: Avoid swallowing unexpected exceptions silently.

except Exception: pass in request preparation hides redirect failures and makes debugging harder. Logging the exception keeps behavior resilient while preserving observability.

🔧 Suggested adjustment

-            except Exception:
-                pass
+            except Exception:
+                logger.exception("Failed to apply training program redirect")

cms/server/admin/handlers/trainingday.py (1)

694-702: Align try_commit failure handling with admin handler pattern.
Both handlers currently return explicit “Failed to save” JSON responses when try_commit() fails. The admin handlers typically rely on centralized error handling/logging instead of explicit failure notifications. Consider removing the custom error payload and raising/propagating a 500 instead.

🔧 Example adjustment (apply similarly in both handlers)

-            if self.try_commit():
-                self.write({
-                    "success": True,
-                    "types": training_day.training_day_types
-                })
-            else:
-                self.set_status(500)
-                self.write({"error": "Failed to save"})
+            if not self.try_commit():
+                raise tornado.web.HTTPError(500)
+            self.write({
+                "success": True,
+                "types": training_day.training_day_types
+            })

Based on learnings: 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.

Also applies to: 799-807

[coderabbitai]

Actionable comments posted: 10

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/handlers/contestdelayrequest.py (1)

362-399: ⚠️ Potential issue | 🟡 Minor

Exclude ineligible training-day participations from the CSV export.
The UI filters these out, but the export currently includes them, which can skew attendance reporting.

🔧 Suggested fix

         for participation in participations:
+            if self.contest.training_day is not None:
+                is_eligible, _, _ = check_training_day_eligibility(
+                    self.sql_session, participation, self.contest.training_day
+                )
+                if not is_eligible:
+                    continue
             starting_time = participation.starting_time.strftime('%Y-%m-%d %H:%M:%S') if participation.starting_time else '-'

🤖 Fix all issues with AI agents

In `@cms/grading/scorecache.py`:
- Around line 762-797: The current code in the training-day branch (uses
get_managing_participation, training_day, managing_participation and queries
Submission) raises a ValueError when managing_participation is None; change this
to a safe fallback: log the missing managing participation (use the module
logger, e.g., logger.warning or similar) with context including
participation.user_id and training_day.id, then return an empty list instead of
raising so cache rebuilds/rankings don't crash; keep the rest of the query logic
for the non-training case unchanged.

In `@cms/server/admin/handlers/base.py`:
- Around line 350-413: The bare "except Exception: pass" in prepare() swallows
DB/routing errors; replace it with an except Exception as e block that logs the
error (including stack trace) before proceeding—use self.logger.exception(...)
if available (fallback to logging.exception(...)) and either re-raise or at
least retain the original behavior after logging; update the except around the
self.sql_session.query(Contest)/contest.training_program/url_mappings/tp_url
redirect logic so errors are recorded instead of silently ignored.
- Around line 609-656: render_params_for_students_page currently sets
self.r_params["all_tasks"] using managing_contest.get_tasks(), which will
include training-day tasks; update this to exclude training-day tasks by
querying tasks where Task.contest == managing_contest and
Task.training_day_id.is_(None) (mirroring the pattern used in
ContestTasksHandler) so bulk-assign only shows non-training-day tasks; locate
the assignment to self.r_params["all_tasks"] in render_params_for_students_page
and replace it with the filtered query using the Task model and
self.sql_session.

In `@cms/server/admin/handlers/contestannouncement.py`:
- Around line 76-121: The current post method in this handler requires both
subject and text and thus silently no-ops when text is empty; change validation
to require subject only (mirror contest handlers) and on edits only overwrite
Announcement.text when a non-empty text argument is provided while still
updating subject and visible_to_tags; for new Announcement allow text to be
empty. Update the post method (the function named post and code paths that touch
Announcement creation/editing, announcement.text assignment, and the
announcement_id branch) so the outer check is if subject: and inside the edit
branch set announcement.text = text only when text is non-empty (leave existing
text otherwise).

In `@cms/server/admin/handlers/contestranking.py`:
- Around line 503-509: The except block in the main_group_user_ids parsing code
should chain the original ValueError when raising the HTTPError to preserve
exception context; update the except ValueError handler in the
main_group_user_ids_param processing (the block creating main_group_user_ids
set) to capture the ValueError as a variable (e.g., "err") and re-raise using
"raise tornado.web.HTTPError(400, 'Invalid main_group_user_ids parameter') from
err".

In `@cms/server/admin/handlers/student.py`:
- Around line 150-160: When adding td_participation inside the loop over
training_program.training_days, avoid creating duplicates by first checking
whether a Participation already exists for that training_day.contest and the
same user; if a matching Participation is found, skip creating/adding a new
td_participation. Implement the same presence check (querying Participation for
contest=training_day.contest and user/user_id) before calling
self.sql_session.add(td_participation), and apply this guard to the other
bulk-add loop that also creates td_participation.

In `@cms/server/admin/handlers/task.py`:
- Around line 723-734: The except block that catches any Exception should
preserve the original exception when raising the HTTPError: change the handler
to capture the exception (e.g., except Exception as e:) and raise
tornado.web.HTTPError(500, f"Couldn't create default submission format for task
{task.id}") from e so the original traceback is chained; update the block around
logger.error and the raise of tornado.web.HTTPError to use the captured
exception variable in the from clause.

In `@cms/server/admin/handlers/trainingprogramtask.py`:
- Around line 277-279: The code sets Task.num with task.num =
len(managing_contest.tasks), which can collide when tasks have gaps; instead
compute the next index by finding the current maximum num among
managing_contest.tasks and using max_num + 1 (or 0 if none) before assigning to
task.num and then set task.contest = managing_contest; update the logic around
Task.num assignment in trainingprogramtask (look for task.num and
managing_contest.tasks) to use this max-based approach.

In `@cms/server/admin/static/js/admin_table_utils.js`:
- Around line 72-86: The dragover handler currently inserts the placeholder
relative to any TR; update the guard in the tbody.addEventListener('dragover')
block to skip rows that are not draggable by checking the same attribute used in
dragstart (e.g., verify targetRow has the data draggable flag or attribute).
Specifically, after computing targetRow (from e.target.closest(rowSelector)),
also return early if the targetRow does not have the draggable marker (for
example targetRow.getAttribute('data-draggable') !== 'true' or
!targetRow.hasAttribute('draggable')), so placeholder insertion only happens for
actual draggable rows (use the existing variables targetRow, draggedRow,
placeholder, rowSelector to locate code).

In `@cms/server/admin/templates/training_program_student_remove.html`:
- Line 21: The anchor used for the destructive "Yes, remove" action isn't
reliably keyboard-focusable; replace the <a> with a semantic <button
type="button"> (keep the existing class "button-link" and the onclick call to
CMS.AWSUtils.ajax_delete('{{ url("training_program", training_program.id,
"student", user.id, "remove") }}')) so the control is keyboard-accessible and
activatable via Enter/Space, or alternatively add a valid href and call
event.preventDefault() inside the same onclick handler—ensure the element
remains focusable and preserves the existing onclick invocation.

🧹 Nitpick comments (11)

cms/server/admin/templates/macro/delay_request.html (1)

33-38: Consider extracting inline styles to a CSS class for consistency.

The warning block uses inline styles which works but could benefit from a CSS class for easier maintenance and consistency with other styled components in the admin UI. This is a minor suggestion that can be deferred.

cms/server/admin/static/aws_tp_styles.css (1)

2036-2041: Consider using only standard flexbox for multiline text truncation.

The display: box property is non-standard and deprecated. While -webkit-line-clamp is widely supported now, it's best to use only the prefixed properties that are part of the working spec.

♻️ Suggested fix

     overflow: hidden;
     text-overflow: ellipsis;
     display: -webkit-box;
-    display: box;
     -webkit-line-clamp: 2;
     line-clamp: 2;
     -webkit-box-orient: vertical;

cms/server/admin/static/aws_form_utils.js (2)

272-303: Consider potential race condition in user-initiated removal detection.

The 200ms threshold for userRemovalTriggeredAt timing detection is a reasonable heuristic, but could occasionally misclassify events on slow machines or under heavy CPU load. The current implementation errs on the side of not requiring confirmation for edge cases, which is acceptable UX.

---

381-387: Timing-based arming approach is reasonable but fragile.

The 100ms setTimeout to arm confirmations avoids dialogs during initial page load. However, this could fail on very slow page loads where Tagify takes longer than 100ms to initialize pre-existing tags.

Consider using Tagify's internal state or event to determine when initial load is complete, though the current approach is pragmatic for typical scenarios.

cms/server/admin/handlers/base.py (3)

121-157: Preserve original exception context in parsers.
For the parsers that wrap exceptions (parse_int, parse_timedelta_*, parse_datetime), chaining with from err keeps diagnostics intact.

🧩 Exception chaining

-    except (ValueError, TypeError):
-        raise ValueError("Can't cast %s to int." % value)
+    except (ValueError, TypeError) as err:
+        raise ValueError("Can't cast %s to int." % value) from err

-    except (ValueError, TypeError):
-        raise ValueError("Can't cast %s to timedelta." % value)
+    except (ValueError, TypeError) as err:
+        raise ValueError("Can't cast %s to timedelta." % value) from err

-    except (ValueError, TypeError):
-        raise ValueError("Can't cast %s to timedelta." % value)
+    except (ValueError, TypeError) as err:
+        raise ValueError("Can't cast %s to timedelta." % value) from err

-    except (ValueError, TypeError):
-        raise ValueError("Can't cast %s to datetime." % value)
+    except (ValueError, TypeError) as err:
+        raise ValueError("Can't cast %s to datetime." % value) from err

---

278-295: Avoid emitting notifications inside try_commit.
Centralized error handling keeps a consistent UX and avoids duplicate messages; consider letting callers decide when to notify.

🔧 Possible adjustment

-        except IntegrityError as error:
-            self.service.add_notification(
-                make_datetime(), "Operation failed.", "%s" % error
-            )
-            return False
-        else:
-            self.service.add_notification(make_datetime(), "Operation successful.", "")
-            return True
+        except IntegrityError:
+            return False
+        else:
+            return True

Based on learnings: 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.

---

757-792: Exception chaining would improve error diagnostics.
These parsing helpers drop the original exception context; chaining keeps root causes visible in logs.

🧩 Exception chaining

-            except ValueError as e:
-                raise ValueError("Can't cast %s to float: %s" % (value, e))
+            except ValueError as err:
+                raise ValueError("Can't cast %s to float: %s" % (value, err)) from err

-            except ValueError as e:
-                raise ValueError("Can't cast %s to int: %s" % (value, e))
+            except ValueError as err:
+                raise ValueError("Can't cast %s to int: %s" % (value, err)) from err

cms/server/admin/handlers/contestquestion.py (1)

70-93: Consider potential redundant computation of training day notifications.

Lines 74-80 recompute notifications for each training day if td_notifications is empty. However, render_params_for_training_program (called by setup_contest_or_training_program) likely already populates training_day_notifications in r_params. The condition if not td_notifications would only trigger if the helper didn't populate it, which could indicate a code path inconsistency.

Consider verifying whether this fallback computation is actually needed, or if it can be removed to simplify the code.

#!/bin/bash
# Check if render_params_for_training_program populates training_day_notifications
rg -n "training_day_notifications" --type py cms/server/admin/handlers/base.py -A 3 -B 3

cms/server/admin/templates/fragments/training_day_groups.html (1)

90-92: Checkbox naming pattern uses embedded ID instead of array notation.

The checkbox uses name="alphabetical_{{ group.id }}" which embeds the group ID in the input name. This differs from the array notation (group_alphabetical[]) used in add_training_day.html. The server handler must parse these differently.

This is a design choice rather than a bug, but ensure consistency is intentional and the handler correctly processes this format.

cms/server/admin/templates/ranking.html (1)

243-249: Minor inconsistency in toggle selector for single-table path.

The single-table JS toggle on lines 245-247 uses $("#ranking-table") (ID selector), while the multi-group path uses $(".td-ranking-table") (class selector). This works because the single table has both id="ranking-table" and class="td-ranking-table", but consider using the class selector consistently for maintainability:

🔧 Suggested improvement

     $(".show-indicators-toggle").on("change", function() {
       if (this.checked) {
-        $("#ranking-table").removeClass("hide-indicators");
+        $(".td-ranking-table").removeClass("hide-indicators");
       } else {
-        $("#ranking-table").addClass("hide-indicators");
+        $(".td-ranking-table").addClass("hide-indicators");
       }
     });

cms/server/admin/handlers/contestranking.py (1)

307-314: Consider precomputing task access to avoid N×M lookups.

In training-day contests, can_access_task typically hits the DB per call; the nested loop can explode to participations×tasks queries. Consider prefetching students/tags once (you already batch-fetch later) and computing access in-memory to keep this O(1) queries.

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE