Redesign tasks page and add scoreboard sharing for archived training days #107
Conversation
…training days - Modernize the tasks page design to match the students page style - Add modern table with sortable columns - Add dropdown for adding tasks - Add release/reuse button for tasks in archived training days - Add remove button with confirmation for active training day tasks - Add move buttons (top, up, down, bottom) for reordering tasks Co-Authored-By: Ron Ryvchin <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughAdds a nullable JSONB Changes
Sequence Diagram(s)sequenceDiagram
participant AdminBrowser as Admin (Browser)
participant AdminHandler as ScoreboardSharingHandler
participant DB as Database
AdminBrowser->>AdminHandler: POST /training_program/{id}/training_day/{day_id}/scoreboard_sharing (JSON)
AdminHandler->>DB: Load TrainingDay, validate archived state, validate payload, persist `scoreboard_sharing`
DB-->>AdminHandler: OK
AdminHandler-->>AdminBrowser: 200 + stored config
sequenceDiagram
participant UserBrowser as User (Browser)
participant ContestHandler as ScoreboardDataHandler
participant DB as Database
participant Compute as Server (ranking)
UserBrowser->>ContestHandler: GET /training_days/scoreboard/{tdId}/{tag}
ContestHandler->>DB: Load TrainingDay (including scoreboard_sharing) and scores
DB-->>ContestHandler: data
ContestHandler->>Compute: Filter scores by tag, compute totals/ranks, apply top_to_show/anonymize
Compute-->>ContestHandler: scoreboard JSON
ContestHandler-->>UserBrowser: 200 + scoreboard JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
ronryv
changed the title
…dd shared utility Co-Authored-By: Ron Ryvchin <[email protected]>
ronryv
force-pushed
the
devin/1769407232-redesign-tasks-page
branch
from
fb87a9c to
0c3f657
Compare
Co-Authored-By: Ron Ryvchin <[email protected]>
…fication alerts - Fix redirect after task removal to correctly return to tasks page - Remove deprecated training_program_task_remove.html (now using modal) - Remove unused GET handler from RemoveTrainingProgramTaskHandler - Add modern info alert component (tp-info-alert) with warning/info variants - Update training_program_attendance.html with modern alert styling - Update questions.html with modern alert styling - Update announcements.html with modern alert styling - Add CSS styles for info alert badges with counts Co-Authored-By: Ron Ryvchin <[email protected]>
…asks column and histograms - Add info alert to combined ranking page explaining it shows archived training days only - Redesign training_days page with modern tp-* CSS classes - Add Tasks column to both active and archived training days tables - Active training days show task badges linking to task pages - Archived training days show task badges with histogram icons - Create reusable histogram_modal.html fragment for score distribution visualization - Clicking archived task badges opens histogram modal with score distribution Co-Authored-By: Ron Ryvchin <[email protected]>
… histogram support - Reverted to original page styling (not the modern redesign) - Added Tasks column to both active and archived training days tables - Active training days show task badges linking to task pages - Archived training days show task badges with histogram icons - Removed duplicated histogram_modal.html fragment - Use existing histogram CSS classes from aws_tp_styles.css - Histogram modal opens when clicking on archived task badges Co-Authored-By: Ron Ryvchin <[email protected]>
…time display Co-Authored-By: Ron Ryvchin <[email protected]>
- Add scoreboard_sharing JSONB column to TrainingDay model (db version 49->50) - Add admin UI to configure scoreboard sharing per archived training day - Modal to add/remove tags and set top_names count per tag - Visual indicator showing number of tags shared - Add CWS handler to fetch scoreboard data with tag-based filtering - Add CWS UI with scoreboard badges and modal - Students see badges only for tags they had during training - Scoreboard shows only students with matching tags - Scoreboard shows only tasks accessible to those tags - Top N students show full names, others anonymized - Current student's row is highlighted Co-Authored-By: Ron Ryvchin <[email protected]>
Co-Authored-By: Ron Ryvchin <[email protected]>
ronryv
changed the title
Co-Authored-By: Ron Ryvchin <[email protected]>
…icators, modernize add form Co-Authored-By: Ron Ryvchin <[email protected]>
|
@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: 6
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/trainingprogram.py`:
- Around line 588-592: If task.training_day_num can be None, calling
_shift_task_nums(self.sql_session, Task.training_day, training_day,
Task.training_day_num, training_day_num, -1) with training_day_num=None may
produce incorrect behavior; add a guard before calling _shift_task_nums to only
invoke it when training_day_num is not None (e.g., check the local variable
training_day_num or the Task.training_day_num value), and if None skip the
reorder step so _shift_task_nums is only called with a valid numeric threshold.
- Around line 144-150: The NameError occurs because DelayRequest is referenced
in the td_pending_delays query but not imported; add DelayRequest to the module
imports (the same place where Participation and other ORM models are imported)
so the query using DelayRequest, Participation and self.sql_session in
trainingprogram.py resolves correctly; ensure the import statement matches the
existing import style (e.g., add DelayRequest to the existing from ... import
Participation, ...) so td_pending_delays calculation works at runtime.
In `@cms/server/admin/templates/training_program_tasks.html`:
- Around line 119-122: The onclick handler currently interpolates template
variables (t.id, t.name, t.training_day.*) directly into JavaScript strings,
allowing quote injection/XSS; remove the inline onclick and instead render these
values into safe data-* attributes on the button (e.g., add class
'remove-task-btn' and attributes data-task-id, data-task-name,
data-has-training-day, data-training-day-name, data-is-active using proper
template escaping), then attach a DOM event listener in JS that selects
'.remove-task-btn' elements and calls openRemoveModal(...) reading values from
element.dataset (converting 'true'/'false' to booleans and empty strings to
null) so no unescaped user content is injected into inline JS.
In `@cms/server/contest/handlers/trainingprogram.py`:
- Around line 559-566: accessible_tasks is iterated to build tasks_list which
yields nondeterministic ordering because archived_tasks_data (JSONB) may not
preserve key order; update the comprehension that builds tasks_list to iterate
over a sorted sequence (e.g., sorted(accessible_tasks.items(), key=lambda kv:
int(kv[0]) or kv[1].get("short_name"))) so tasks are consistently ordered by
task_id or short_name, and make the same deterministic ordering change wherever
task_score_list is built/used so both the scoreboard columns (tasks_list) and
per-row scores (task_score_list) follow the identical sorted order.
In `@cms/server/contest/templates/training_days.html`:
- Around line 451-488: Server-provided strings are being injected into
innerHTML/attributes without escaping (e.g., data.error assigned to
body.innerHTML, task.name used in title and header cell, and possibly
error.message elsewhere). Update the render logic to HTML-escape any
user-controlled content before inserting into innerHTML or attributes: escape
data.error and any error.message when building body.innerHTML, HTML-escape
task.name when building the header cell content, and use an attribute-safe
escape when setting the title attribute (or set title via
element.title/textContent instead of concatenating into HTML). Reuse the
existing escapeHtml helper (and add an attribute-escaping helper if needed) for
training_days template rendering (references: data.error, task.name,
body.innerHTML, title attribute construction).
- Around line 151-156: The onclick embeds sb.tag into a JS string unsafely;
change the attribute to use a single-quoted HTML attribute and render the tag
with the Jinja2 tojson filter so it is safely escaped for JS (i.e., call
openScoreboardModal with pt.training_day.id and sb.tag rendered via |tojson) —
update the button's onclick that calls openScoreboardModal({{ pt.training_day.id
}}, '{{ sb.tag }}') to pass the JSON-encoded tag instead to prevent
quote-breaking/XSS.
🧹 Nitpick comments (10)
cms/server/admin/templates/fragments/histogram_modal.html (1)
1-21: Add dialog semantics for accessibility.The modal lacks
role="dialog",aria-modal, and an accessible label for the close button, which makes it harder for screen readers and keyboard users.♿ Suggested markup
-<div id="histogramModal" class="histogram-modal" style="display: none;"> +<div id="histogramModal" class="histogram-modal" style="display: none;" + role="dialog" aria-modal="true" aria-labelledby="histogramTitle" tabindex="-1"> ... - <button class="histogram-close-btn" onclick="closeHistogramModal()">×</button> + <button class="histogram-close-btn" onclick="closeHistogramModal()" aria-label="Close histogram">×</button>cms/server/admin/templates/training_program_tasks.html (1)
276-299: Drag-and-drop reorder submits form immediately without user confirmation.When a user finishes dragging,
saveNewOrder()immediately submits the form, causing a page reload. This could be surprising if the user accidentally drags a row. Consider either:
- Adding a "Save order" button that becomes visible after changes
- Using AJAX to save without a full page reload
- Adding a brief debounce/confirmation
cms/server/admin/handlers/trainingday.py (2)
180-180: Moveimport jsonto the top of the file.The
jsonmodule is imported inline within the function. For consistency and to avoid repeated import overhead, move it to the imports section at the top of the file.♻️ Proposed fix
Add at the top of the file with other imports:
import jsonThen remove line 180 and line 694.
712-717: Case-insensitive duplicate check may cause unexpected behavior.The duplicate check uses
normalized_key.lower()which means "Advanced" and "advanced" would be considered duplicates. However, the original tag casing is preserved in the stored data. This could be confusing - users might expect case-sensitive tags.Consider either:
- Making the duplicate check case-sensitive (remove
.lower())- Normalizing the stored tag to lowercase as well
- Documenting this behavior in the UI
cms/server/admin/handlers/trainingprogram.py (1)
562-566: Redundant database query in loop.The task is fetched again with
self.safe_get_item(Task, task_id)even though it was already retrieved and validated on line 556. Consider storing the task object in the map instead of just the new_num:♻️ Proposed fix
def _reorder_tasks(self, contest: Contest, reorder_data: str) -> None: """Reorder tasks based on drag-and-drop data. reorder_data: JSON string with list of {task_id, new_num} objects. """ try: order_list = json.loads(reorder_data) except json.JSONDecodeError: return # First, set all task nums to None to avoid unique constraint issues - task_map = {} + task_updates = [] # List of (task, new_num) tuples for item in order_list: task = self.safe_get_item(Task, item["task_id"]) if task.contest == contest: - task_map[item["task_id"]] = item["new_num"] + task_updates.append((task, item["new_num"])) task.num = None self.sql_session.flush() # Then set the new nums - for task_id, new_num in task_map.items(): - task = self.safe_get_item(Task, task_id) + for task, new_num in task_updates: task.num = new_num self.sql_session.flush()cms/server/admin/templates/training_program_training_days.html (5)
339-346: Use single quotes for onclick attribute with tojson.Per learnings, when using Jinja2's
|tojsoninside HTML attribute values, use single quotes for the attribute delimiter to avoid conflicts with the double quotes produced by tojson. Whileforceescapehandles this, single quotes are cleaner.Suggested fix
- <span class="source-badge badge-archived" - title="{{ task_info.get('title', task_info.get('name', 'Task')) }} - Click for histogram" - onclick="showArchivedTaskHistogram({{ td.id }}, {{ task_id_str }}, {{ task_info.get('name', 'Task ' ~ task_id_str) | tojson | forceescape }})" - style="cursor: pointer;"> + <span class="source-badge badge-archived" + title="{{ task_info.get('title', task_info.get('name', 'Task')) }} - Click for histogram" + onclick='showArchivedTaskHistogram({{ td.id }}, {{ task_id_str }}, {{ task_info.get("name", "Task " ~ task_id_str) | tojson }})' + style="cursor: pointer;">
354-365: Use single quotes for onclick attribute with tojson.Same issue as above—use single quotes for the onclick attribute delimiter when tojson is used.
Suggested fix
- <button type="button" class="btn-icon-only" title="Configure scoreboard sharing" - onclick="openScoreboardSharingModal({{ td.id }}, {{ td.scoreboard_sharing | default({}, true) | tojson | forceescape }}, {{ td.name | default('') | tojson | forceescape }})" - {% if not admin.permission_all %}disabled{% endif %}> + <button type="button" class="btn-icon-only" title="Configure scoreboard sharing" + onclick='openScoreboardSharingModal({{ td.id }}, {{ td.scoreboard_sharing | default({}, true) | tojson }}, {{ td.name | default("") | tojson }})' + {% if not admin.permission_all %}disabled{% endif %}>
505-591: Hardcoded column indices may break if table structure changes.The sorting logic uses hardcoded column indices (1, 4 for active table; 0, 3 for archived table). Consider using data attributes on the sortable headers to specify which data attribute to sort by, making the code more maintainable.
Example approach
// Instead of checking columnIndex === 1, use data attributes: // <th class="sortable-header" data-sort="string" data-sort-key="name"> // Then in JS: var sortKey = header.getAttribute('data-sort-key'); aVal = a.getAttribute('data-' + sortKey); bVal = b.getAttribute('data-' + sortKey);
649-653: Remove unusedescapeHtmlfunction.This function is defined but never called anywhere in the template. It appears to be dead code.
Suggested fix
-function escapeHtml(text) { - var div = document.createElement('div'); - div.textContent = text; - return div.innerHTML; -} -
655-679: Minor: Redundant dataset assignment.Lines 657-659 set
input.dataset.sharingTagInput = 'true'but this is already set in the HTML template at line 636 viadata-sharing-tag-input="true". This check is redundant.Suggested fix
function initSharingTagInput(input, initialTag) { if (!input || typeof Tagify === 'undefined') return; - if (!input.dataset.sharingTagInput) { - input.dataset.sharingTagInput = 'true'; - } var whitelist = (currentSharingTrainingDayId && tagsPerTrainingDay[currentSharingTrainingDayId]) ?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/training_program_tasks.html`:
- Line 179: The template injects training_program.name directly into a
JavaScript string which can break JS or allow XSS; replace the direct
interpolation with a safe JSON-encoded value (e.g., assign var programName = {{
training_program.name|tojson }} in the template) and then build the message
using programName and taskName (update the code that sets message.textContent
where taskName is used) so any quotes/backslashes are escaped and the string is
safe.
In `@cms/server/contest/handlers/trainingprogram.py`:
- Around line 541-547: The code appends scoreboard entries using
ranking.student.participation.user.username which can raise an AttributeError if
ranking.student is None or if ranking.student.participation or
participation.user is missing; update the scoreboard_entries construction in the
block that builds entries for ranking to defensively resolve the display name by
checking ranking.student and its nested attributes (e.g., ranking.student,
ranking.student.participation, ranking.student.participation.user) before
accessing .username and fall back to "Unknown" (or another safe default); ensure
the is_current_student comparison (ranking.student_id == student.id) still works
when ranking.student is None.
🧹 Nitpick comments (10)
cms/server/admin/templates/fragments/histogram_modal.html (1)
1-21: Add dialog semantics + keyboard-friendly focus target.
This modal lacks ARIA dialog attributes and a focusable container, which makes it harder for keyboard/screen-reader users.♻️ Suggested markup update
-<div id="histogramModal" class="histogram-modal" style="display: none;"> - <div class="histogram-modal-content"> +<div id="histogramModal" + class="histogram-modal" + style="display: none;" + role="dialog" + aria-modal="true" + aria-labelledby="histogramTitle" + aria-describedby="histogramStats"> + <div class="histogram-modal-content" tabindex="-1"> @@ - <button class="histogram-close-btn" onclick="closeHistogramModal()">×</button> + <button type="button" class="histogram-close-btn" onclick="closeHistogramModal()">×</button> @@ - <button class="copy-btn" onclick="copyHistogramData()">Copy to Clipboard</button> + <button type="button" class="copy-btn" onclick="copyHistogramData()">Copy to Clipboard</button>cms/server/admin/templates/questions.html (1)
23-44: Reuseinfo_alert.alertfor the “Unanswered Questions” block.
This warning block duplicates the alert markup; using the shared macro keeps styling consistent and reduces drift.Consider passing a pre-shaped
itemslist (url/label/count) from the handler or building it once in-template, then render it viainfo_alert.alert(...).cms/server/admin/templates/training_program_attendance.html (1)
47-69: Consolidate the “Pending Delay Requests” warning viainfo_alert.alert.
This block duplicates the alert structure; reusing the macro avoids future styling inconsistencies.Consider passing an
itemslist (url/label/count) from the handler and rendering it with the shared macro.cms/server/admin/handlers/trainingprogram.py (1)
543-566: Validatereorder_dataserver-side to protectTask.numuniqueness.
A malformed or partial payload can leave gaps or duplicate nums and trip the unique constraint; consider validating membership and uniqueness before applying.♻️ Suggested validation
try: order_list = json.loads(reorder_data) except json.JSONDecodeError: return + + if not isinstance(order_list, list): + raise ValueError("Reorder data must be a list") + + expected_ids = {t.id for t in contest.tasks} + received_ids = {item.get("task_id") for item in order_list} + if received_ids != expected_ids: + raise ValueError("Reorder data must include each task exactly once") + + new_nums = [item.get("new_num") for item in order_list] + if len(set(new_nums)) != len(new_nums): + raise ValueError("Duplicate task numbers in reorder data")cms/server/admin/static/aws_tp_styles.css (2)
896-896: Consider whether!importantis necessary.
!importanton button text colors suggests potential CSS specificity conflicts. While this works as a defensive measure, it may indicate underlying specificity issues worth investigating.
2303-2323: Duplicate selector definition for.tp-btn-primary.
.tp-btn-primaryis defined twice in this file (lines 894-910 and 2303-2323) with slightly different properties. This could lead to unexpected behavior depending on cascade order.Consider consolidating into a single definition or ensuring the second definition only adds new properties.
cms/server/admin/handlers/trainingday.py (2)
179-210: Consider movingimport jsonto module level.The inline import on line 180 (and similarly on line 694) works but is unconventional. Moving it to the module-level imports improves readability and follows Python best practices.
♻️ Suggested change
Add to the imports at the top of the file:
import jsonThen remove the inline imports on lines 180 and 694.
724-725: Minor: Tighten type check to exclude booleans.
isinstance(top_names, int)returnsTruefor booleans sinceboolis a subclass ofint. If a client sends{"top_names": true}in JSON, it would pass validation.♻️ Suggested fix
- if not isinstance(top_names, int) or top_names < 0: + if not isinstance(top_names, int) or isinstance(top_names, bool) or top_names < 0:cms/server/admin/templates/training_program_training_days.html (2)
526-540: Consider using data attributes instead of hardcoded column indices.The column indices (1, 4 for active; 0, 3 for archived) are hardcoded, which is brittle if columns are reordered or added.
♻️ Suggested approach
Add
data-sort-keyattributes to sortable headers and rows:<th class="sortable-header" data-sort="string" data-sort-key="name">Name</th>Then in the sort function:
var sortKey = header.getAttribute('data-sort-key'); // Use row.getAttribute('data-' + sortKey) instead of column index checks
731-748: Add response status check before parsing JSON.If the server returns a non-JSON error response (e.g., a 500 HTML error page),
response.json()will throw a confusing error. Checkresponse.okfirst.♻️ Suggested fix
.then(function(response) { + if (!response.ok) { + return response.text().then(function(text) { + throw new Error('Server error: ' + response.status); + }); + } return response.json(); })
…nd tie handling - Add '__everyone__' option to share scoreboard with all students regardless of tags - Add 'top_to_show' field to limit number of results displayed (with 'all' checkbox) - Add 'top_names' field to limit how many top results show full names (with 'all' checkbox) - Implement proper tie handling: students with same score get same rank - Show all tied students at cutoff even if exceeds top_to_show limit - Always show current user's rank even if past top_to_show limit - Add validation caps based on actual student counts in admin UI - Update admin modal with new fields and checkboxes - Update CWS handler with new filtering and ranking logic Co-Authored-By: Ron Ryvchin <[email protected]>
ronryv
force-pushed
the
devin/1769407232-redesign-tasks-page
branch
from
ff3311e to
364b04d
Compare
…nd clickable training days Contest server changes: - Add clock and hourglass SVG icons to training day cards (start/duration) - Redesign progress bar with dark navy background, percentage display, and score sections - Add upload icon to Submit button - Add grid icon to scoreboard buttons and display 'everyone' instead of '__everyone__' Admin server changes: - Modernize add_training_day.html with modern form styling (tp-form-card pattern) - Make 'ACTIVE TRAINING DAYS' label clickable, linking to training_days page Co-Authored-By: Ron Ryvchin <[email protected]>
ronryv
force-pushed
the
devin/1769407232-redesign-tasks-page
branch
from
fb993b0 to
3a9bb7d
Compare
… progress colors - Admin: Reduce hours/minutes input width from 60px to 50px in Main Groups Configuration - Admin: Replace Tagify with native select box in Scoreboard Sharing modal - Admin: Update task archive progress bar thresholds (50% instead of 40%) - Contest: Update LIVE badge to red pill with pulsing dot - Contest: Fix duration icon to proper hourglass SVG - Contest: Add conditional progress bar colors (red <50%, orange 50-80%, green >=80%) Co-Authored-By: Ron Ryvchin <[email protected]>
|
@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/trainingprogram.py`:
- Around line 544-583: The _reorder_tasks method currently doesn't validate each
item's "new_num", so missing/None/non-int/out-of-range values can leave task.num
as None or create gaps; update _reorder_tasks to validate new_num for every
entry: ensure item contains "new_num", convert it to an int (raise ValueError on
conversion failure), and check it's within the expected range 1..N where N =
len(contest.tasks); also enforce that the set of new_nums equals exactly the
set(range(1, N+1)) (or otherwise raise ValueError) before mutating tasks;
reference symbols: _reorder_tasks, order_list, Task, task.num, safe_get_item,
contest.tasks, task_updates, and sql_session.flush.
- Around line 513-535: Validate that the fetched Task actually belongs to the
current training program before calling _detach_task_from_training_day: after
obtaining task via safe_get_item(Task, task_id) check its ownership against the
current training program (managing_contest or whichever attribute identifies the
program) and abort (raise HTTPError or redirect with an error) if it does not
match; only call self._detach_task_from_training_day(task) and proceed with
try_commit()/service.proxy_service.reinitialize() when the ownership check
passes. Ensure you reference the same symbols: safe_get_item, Task,
managing_contest, _detach_task_from_training_day, try_commit, and
service.proxy_service.reinitialize.
In `@cms/server/admin/templates/training_program_training_days.html`:
- Around line 823-839: Add an explicit empty placeholder option to the built
select so the first real tag isn't auto-selected: before iterating whitelist in
the code that creates tagSelect (the select element created in this block using
tagSelect, whitelist, tagsPerTrainingDay, currentSharingTrainingDayId,
allStudentTags and tag), insert a new option with an empty value (e.g.,
option.value = '' and text like '-- none --' or empty string) and set it
selected when tag is falsy/undefined; then append the placeholder option to
tagSelect before appending the whitelist options so "no tag selected" is
explicit.
- Around line 658-703: The modal open logic for currentSharing['__everyone__']
must clamp top_to_show/top_names to valid ranges so stored values >
totalStudentsForTrainingDay or top_names > top_to_show are fixed before user
input: when reading everyoneData.top_to_show and everyoneData.top_names, coerce
to numeric (unless 'all'), clamp top_to_show to [0,
totalStudentsForTrainingDay], set the "all" radio (everyoneTopToShowAll /
everyoneTopNamesAll) if value equals totalStudentsForTrainingDay, then clamp
top_names to [0, top_to_show] (or to totalStudentsForTrainingDay if top_to_show
=== 'all'), update DOM inputs (everyoneTopToShow, everyoneTopNames) and disabled
states accordingly, and ensure validateEveryoneFields remains attached.
🧹 Nitpick comments (7)
cms/server/admin/templates/add_training_day.html (2)
94-95: Guard against nullallStudentTagsto keep Tagify stable.If
all_student_tagsis everNone/null, Tagify’s whitelist won’t be an array and select mode can misbehave. A defensive default keeps the UI stable.♻️ Proposed fix
-var allStudentTags = {{ all_student_tags | tojson }}; +var allStudentTags = {{ all_student_tags | default([]) | tojson }};
160-185: Destroy Tagify instances on row removal to avoid leaks.
row.remove()drops the DOM but leaves Tagify event listeners alive. Cleanly destroying the instance prevents lingering listeners and dropdown artifacts after many add/remove cycles.♻️ Proposed fix
- var removeBtn = document.createElement('button'); + var removeBtn = document.createElement('button'); removeBtn.type = 'button'; removeBtn.className = 'tp-btn-remove'; removeBtn.textContent = 'Remove'; - removeBtn.onclick = function() { row.remove(); }; + removeBtn.onclick = function() { + if (tagifyInstance) { + tagifyInstance.destroy(); + } + row.remove(); + }; @@ - if (typeof Tagify !== 'undefined') { - var tagify = new Tagify(tagInput, { + var tagifyInstance = null; + if (typeof Tagify !== 'undefined') { + tagifyInstance = new Tagify(tagInput, { mode: 'select', whitelist: allStudentTags, enforceWhitelist: true, @@ - tagify.DOM.scope.style.width = '160px'; + tagifyInstance.DOM.scope.style.width = '160px'; }cms/server/contest/handlers/trainingprogram.py (1)
616-647: Consider defensive handling forint()conversions.The
int()calls on lines 619 and 647 could raiseValueErrorifscoreboard_sharingcontains invalid data (e.g., a non-numeric string other than"all"). While admin validation should prevent this, defensive handling would make the API more robust against corrupt data.♻️ Suggested defensive conversion
if top_to_show == "all": entries_to_show = scoreboard_entries else: - top_to_show = int(top_to_show) + try: + top_to_show = int(top_to_show) + except (ValueError, TypeError): + top_to_show = len(scoreboard_entries) # Fallback to showing all if top_to_show <= 0:if top_names == "all": top_names_int = total_students else: - top_names_int = int(top_names) + try: + top_names_int = int(top_names) + except (ValueError, TypeError): + top_names_int = total_students # Fallback to showing all namescms/server/admin/static/aws_tp_styles.css (2)
369-382: Fix hover color override on icon-only buttons.The base color uses
!important, so the hover color never applies. Remove the!important(or add it to the hover rule) so the hover state is visible.Proposed fix
.btn-icon-only { background: none; border: none; padding: 8px; cursor: pointer; - color: var(--tp-text-light) !important; + color: var(--tp-text-light); border-radius: 50%; transition: all 0.2s; }
2240-2246: Scope.tp-form-groupto avoid global overrides.There’s already a
.tp-form-groupearlier in the file; redefining it here changes spacing across all pages. Consider scoping to the form card to keep this localized.Proposed refactor
-.tp-form-group { +.tp-form-card .tp-form-group { margin-bottom: 24px; } -.tp-form-group:last-child { +.tp-form-card .tp-form-group:last-child { margin-bottom: 0; }cms/server/admin/handlers/trainingprogram.py (2)
61-80: Docstring doesn’t match insertion behavior.
Fordelta > 0, the code shifts nums>= threshold(Line 85), but the docstring says> threshold. Please align the docstring (or logic) to avoid misuse.✏️ Proposed docstring tweak
- threshold: The threshold value - tasks with num > threshold will be shifted. + threshold: The threshold value - tasks with num >= threshold (insertion) + or > threshold (removal) will be shifted.
111-160: Potential N+1 queries for notifications.
Each active training day triggers two separate counts. This can get expensive with many training days. Consider aggregating with grouped queries (e.g., counts by contest_id) to reduce query volume.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
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/trainingprogram.py (1)
688-700: Add None guard fortraining_day_numbefore calling_shift_task_nums.Unlike
_detach_task_from_training_day, this code path doesn't guard againsttraining_day_numbeingNone. If a task has atraining_daybuttraining_day_numisNone, passing it to_shift_task_numswill cause incorrect query behavior since the threshold comparison would fail.🐛 Proposed fix
# Remove from training day if assigned if task.training_day is not None: training_day = task.training_day training_day_num = task.training_day_num task.training_day = None task.training_day_num = None self.sql_session.flush() # Reorder remaining tasks in the training day - _shift_task_nums( - self.sql_session, Task.training_day, training_day, - Task.training_day_num, training_day_num, -1 - ) + if training_day_num is not None: + _shift_task_nums( + self.sql_session, Task.training_day, training_day, + Task.training_day_num, training_day_num, -1 + )
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/training_program_training_days.html`:
- Around line 451-469: The drag-and-drop setup runs for all users; gate it by
checking the permission flag (admin.permission_all) before attaching handlers
and setting rows draggable: if the user lacks permission, return early from the
IIFE and do not call tbody.querySelectorAll(...). For example, wrap the DnD
initialization (variables like tbody, draggedRow, placeholder, originalOrder and
the createPlaceholder() / tbody.addEventListener('dragstart' ...) block) in a
conditional that only runs when admin.permission_all is truthy, and ensure you
also avoid setting the draggable attribute on rows when permission is false.
- Around line 655-657: The modal clears entries by setting
entriesContainer.innerHTML = '' which removes DOM nodes but leaves Tagify
instances and event listeners alive; before clearing, iterate the existing entry
elements under the element with id 'scoreboardSharingEntries' and call each
Tagify instance's destroy method (using the same pattern as the remove-button
handler that currently destroys Tagify on individual removal) to unregister
listeners and free memory, then safely set entriesContainer.innerHTML = ''.
🧹 Nitpick comments (7)
cms/server/admin/static/aws_tp_styles.css (3)
374-391: Consider reducing!importantusage by addressing specificity issues.The
!importantdeclarations on lines 374, 380-381 suggest specificity conflicts elsewhere in the stylesheet. While these work, they can make future maintenance harder and create an escalation pattern.Positive note: The expanded disabled state selectors (including
aria-disabled="true"and.disabledclass) withpointer-events: noneimprove accessibility for disabled anchor elements.
1488-1500: Same!importantpattern for table headers.The
!importantdeclarations here (and throughout the new sections) continue the pattern of fighting specificity issues. Consider documenting why these are needed or addressing the underlying cascade conflicts in a future refactor.
2172-2176: Redundant media query breakpoint.The
@media (max-width: 1200px)rule sets.tp-stats-rowtogrid-template-columns: repeat(3, 1fr), which is identical to its default value on line 1732. This media query has no effect and can be removed.🧹 Suggested cleanup
-/* Responsive adjustments */ -@media (max-width: 1200px) { - .tp-stats-row { - grid-template-columns: repeat(3, 1fr); - } -} - `@media` (max-width: 900px) { .tp-stats-row { grid-template-columns: 1fr; }cms/server/admin/handlers/trainingprogram.py (4)
91-97: Consider batching the flush for better performance.Flushing after each task update in the loop is inefficient. Since the ordering already prevents unique constraint violations, a single flush after all updates would suffice.
♻️ Suggested improvement
for t in sql_session.query(Task)\ .filter(filter_attr == filter_value)\ .filter(condition)\ .order_by(order)\ .all(): setattr(t, num_attr.key, getattr(t, num_attr.key) + delta) - sql_session.flush() + sql_session.flush()
135-155: N+1 query pattern may degrade performance with many training days.The current implementation runs 2 queries per active training day. For programs with many active days, consider batching these queries.
♻️ Suggested batch query approach
+ # Batch query for all unanswered questions across active training days + active_td_contest_ids = [ + td.contest_id for tp in training_programs + for td in tp.training_days if td.contest is not None + ] + + if active_td_contest_ids: + unanswered_counts = dict( + self.sql_session.query( + Participation.contest_id, + func.count(Question.id) + ) + .join(Question) + .filter(Participation.contest_id.in_(active_td_contest_ids)) + .filter(Question.reply_timestamp.is_(None)) + .filter(Question.ignored.is_(False)) + .group_by(Participation.contest_id) + .all() + ) + + pending_delay_counts = dict( + self.sql_session.query( + Participation.contest_id, + func.count(DelayRequest.id) + ) + .join(DelayRequest) + .filter(Participation.contest_id.in_(active_td_contest_ids)) + .filter(DelayRequest.status == "pending") + .group_by(Participation.contest_id) + .all() + ) + else: + unanswered_counts = {} + pending_delay_counts = {} + # Calculate notifications for each active training day for td in active_tds: - td_unanswered = ( - self.sql_session.query(Question) - .join(Participation) - .filter(Participation.contest_id == td.contest_id) - .filter(Question.reply_timestamp.is_(None)) - .filter(Question.ignored.is_(False)) - .count() - ) - td_pending_delays = ( - self.sql_session.query(DelayRequest) - .join(Participation) - .filter(Participation.contest_id == td.contest_id) - .filter(DelayRequest.status == "pending") - .count() - ) training_day_notifications[td.id] = { - "unanswered_questions": td_unanswered, - "pending_delay_requests": td_pending_delays, + "unanswered_questions": unanswered_counts.get(td.contest_id, 0), + "pending_delay_requests": pending_delay_counts.get(td.contest_id, 0), }
554-560: Consider chaining exceptions for better debugging.When re-raising exceptions within an
exceptblock, chain them to preserve the original traceback.♻️ Suggested improvement
except json.JSONDecodeError as e: logging.warning( "Failed to parse reorder data: %s. Payload: %s", e.msg, reorder_data[:500], ) - raise ValueError(f"Invalid JSON in reorder data: {e.msg}") + raise ValueError(f"Invalid JSON in reorder data: {e.msg}") from eSimilarly at line 582-584:
except (TypeError, ValueError): - raise ValueError( + raise ValueError( f"Invalid 'new_num' value: {raw_num!r} is not an integer" - ) + ) from None
709-712: Consider adding a None guard fortask_numfor defensive consistency.For consistency with the suggested fix above, consider guarding against
task_numbeingNone, even though tasks in a contest should always have a validnum.♻️ Suggested improvement
# Reorder remaining tasks in the training program - _shift_task_nums( - self.sql_session, Task.contest, managing_contest, - Task.num, task_num, -1 - ) + if task_num is not None: + _shift_task_nums( + self.sql_session, Task.contest, managing_contest, + Task.num, task_num, -1 + )
ronryv
force-pushed
the
devin/1769407232-redesign-tasks-page
branch
from
0225f19 to
6ff1f75
Compare
|
@coderabbitai review full |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/trainingday.py`:
- Around line 178-205: The reorder flow in the REORDER branch should validate
and limit changes to active training days: parse reorder_data into order_list
and verify it contains each active td id exactly once (no duplicates, no missing
ids), and that new_position values form a complete 0-based permutation with no
duplicates and within expected range; only clear positions for
active_training_days (instead of all training_program.training_days) before
applying updates to td.position using td_by_id, and reject the request with an
error if the validation fails. Ensure you reference the symbols
operation/REORDER, reorder_data, order_list, active_training_days, td_by_id,
training_program.training_days, td.position and use sql_session.flush() only
after valid updates.
In `@cms/server/admin/handlers/trainingprogram.py`:
- Around line 565-595: The reorder validation currently enforces new_num in 1..N
but task creation elsewhere sets task.num using len(managing_contest.tasks)
(0-based), causing an off-by-one and potential unique constraint conflicts;
update the validation in the reorder handler to treat new_num as 0-based (range
0..num_tasks-1) and compare received_nums to set(range(0, num_tasks)) instead of
1..N, or alternatively convert incoming 1-based new_num to 0-based before range
checks and before assigning to task.num (refer to new_num, received_nums,
expected_nums in this function and task.num / managing_contest.tasks where tasks
are created).
In `@cms/server/admin/templates/fragments/histogram_js.html`:
- Around line 50-57: Guard optional globals before using them: ensure references
to tagsPerTrainingDay, trainingDayId, trainingDayTasks, and
studentAccessibleTasks are checked with typeof or defaulted so the fragment
won’t throw when reused elsewhere. Update the histogram logic around
histogramTagify to check typeof tagsPerTrainingDay !== "undefined" and typeof
trainingDayId !== "undefined" (or treat tagsPerTrainingDay as tagsPerTrainingDay
|| {} and trainingDayTasks/studentAccessibleTasks as {} defaults) before
indexing or iterating, and keep using histogramTagify.settings.whitelist = ...
and histogramTagify.removeAllTags() only when the guarded values exist; apply
the same defensive guards where trainingDayTasks and studentAccessibleTasks are
accessed to avoid ReferenceError.
In `@cms/server/admin/templates/training_program_tasks.html`:
- Around line 318-326: When appending the hidden "operation" input in the
reorder flow, avoid creating duplicates by first checking for and reusing an
existing input named "operation" on the form (or removing any existing ones)
before appending a new one; locate the code that references the form element
with id 'tasks-form' and the created variable operationInput and change it to
search form.elements['operation'] (or querySelector for input[name="operation"])
and update its value to 'reorder' instead of blindly always creating and
appending a new input.
- Around line 55-58: The sort key on the <tr> (data-training-day) uses
t.training_day.name but the row label shows the contest name for active rows,
causing mismatched ordering; update the data-training-day attribute to use the
displayed label instead — prefer t.training_day.contest.name when t.training_day
and t.training_day.contest exist, otherwise fall back to t.training_day.name or
an empty string so the sorting key matches what is displayed (refer to the
template variables t, t.training_day, and t.training_day.contest).
In `@cms/server/admin/templates/training_program_training_days.html`:
- Around line 1147-1151: The code blindly accesses
document.querySelector('input[name="_xsrf"]').value when building formData (used
in formData.append('scoreboard_sharing', ...)), which will throw if the XSRF
input is missing; update the logic that builds formData and sharingPayload to
first query the element into a variable (e.g., xsrfEl) and null-check it, using
a safe fallback (empty string) or early error handling/notification before
calling formData.append('_xsrf', ...); ensure the change touches the block that
constructs formData and sharingPayload so the XSRF value is only read when
xsrfEl is truthy.
In `@cms/server/contest/handlers/trainingprogram.py`:
- Around line 490-648: Validate and defensively parse the scoreboard_sharing
values top_to_show and top_names before calling int() (handle None, non-numeric
strings, and the string "all"): in the block that reads settings =
scoreboard_sharing[tag] and later computes top_to_show = int(top_to_show) and
top_names_int = int(top_names), convert/normalize values with explicit checks or
try/except, defaulting to safe values (e.g., "all" for top_to_show/top_names or
0 for top_to_show when malformed) and ensure current-student behavior still
applies; update the parsing around the symbols top_to_show, top_names,
top_names_int, and the branch that checks if top_to_show == "all" so malformed
inputs no longer raise ValueError.
🧹 Nitpick comments (2)
cms/server/admin/templates/training_program_training_days.html (2)
555-643: Consider extracting a shared table sorting utility.The sorting logic for active and archived training days tables is nearly identical (differing only in table ID and row selector). Extracting this into a reusable function would reduce duplication and simplify future maintenance.
♻️ Suggested refactor
+function initTableSorting(tableId, rowSelector) { + var table = document.getElementById(tableId); + if (!table) return; + + var headers = table.querySelectorAll('.sortable-header'); + var sortState = {}; + + headers.forEach(function(header, index) { + header.addEventListener('click', function() { + var sortKey = header.getAttribute('data-sort-key') || ''; + var sortType = header.getAttribute('data-sort') || 'string'; + var ascending = !sortState[index]; + sortState[index] = ascending; + + headers.forEach(function(h) { h.classList.remove('sort-asc', 'sort-desc'); }); + header.classList.add(ascending ? 'sort-asc' : 'sort-desc'); + + var tbody = table.querySelector('tbody'); + var rows = Array.from(tbody.querySelectorAll(rowSelector)); + + rows.sort(function(a, b) { + var aVal, bVal; + if (sortType === 'date') { + aVal = parseFloat(a.getAttribute('data-' + sortKey)) || 0; + bVal = parseFloat(b.getAttribute('data-' + sortKey)) || 0; + } else { + aVal = (a.getAttribute('data-' + sortKey) || '').toLowerCase(); + bVal = (b.getAttribute('data-' + sortKey) || '').toLowerCase(); + } + if (aVal < bVal) return ascending ? -1 : 1; + if (aVal > bVal) return ascending ? 1 : -1; + return 0; + }); + + rows.forEach(function(row) { tbody.appendChild(row); }); + }); + }); +} + +initTableSorting('training-days-table', 'tr[data-training-day-id]'); +initTableSorting('archived-training-days-table', 'tr'); -// Table sorting for active training days -(function() { - var table = document.getElementById('training-days-table'); - // ... 40+ lines of duplicated code -})(); - -// Table sorting for archived training days -(function() { - var table = document.getElementById('archived-training-days-table'); - // ... 40+ lines of duplicated code -})();
1104-1140: Consider validating all entries before processing.The
returnstatements insideforEach(lines 1121, 1125) only exit the current callback iteration, notsaveScoreboardSharing. While this doesn't cause data integrity issues (entries without tags aren't saved regardless), the UX is confusing: users see an alert but the save proceeds. Consider collecting all validation errors upfront and showing a single consolidated message, or converting to aforloop with proper early exit.♻️ Alternative approach using for...of with early exit
// Handle tag entries - var entries = document.querySelectorAll('#scoreboardSharingEntries .sharing-entry'); - entries.forEach(function(entry) { + var entries = Array.from(document.querySelectorAll('#scoreboardSharingEntries .sharing-entry')); + for (var i = 0; i < entries.length; i++) { + var entry = entries[i]; normalizeTagEntry(entry); // ... rest of validation ... if (!topToShowAllCheckbox.checked && topToShow < 0) { alert('Results to show cannot be negative.'); - return; + return; // Now exits saveScoreboardSharing entirely } // ... rest of code ... - }); + }
…leanup - Extract shared table sorting utility in training_program_training_days.html - Convert forEach to for loop with proper early exit in saveScoreboardSharing - Validate reorder flow in trainingday.py - verify active td ids and positions - Update task reorder validation to 0-based in trainingprogram.py - Guard optional globals in histogram_js.html (tagsPerTrainingDay, etc.) - Avoid duplicate operation input in training_program_tasks.html - Fix sort key mismatch for training day column in training_program_tasks.html - Null-check XSRF input in training_program_training_days.html - Defensive parsing of top_to_show/top_names in contest/trainingprogram.py Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/fragments/histogram_js.html`:
- Around line 12-22: The Tagify initialization uses allStudentTags without
guarding its existence, which can throw if undefined; update the histogramTagify
creation (where Tagify is instantiated) to only pass whitelist: allStudentTags
when allStudentTags is defined (or default to an empty array) and/or skip
creating Tagify until openHistogramModal ensures allStudentTags is available;
specifically adjust the histogramTagify/Tagify initialization logic to check
typeof allStudentTags !== 'undefined' (or use allStudentTags || []) so the
Tagify constructor is never invoked with an undefined whitelist.
In `@cms/server/admin/templates/training_program_training_days.html`:
- Around line 536-541: The saveNewOrder() function appends a hidden input named
"operation" each time it's called, which pollutes the DOM on repeated reorders;
update saveNewOrder() to check the form for an existing input named "operation"
(e.g., via form.querySelector('input[name="operation"]')), and either update its
value to 'reorder' or remove the old input before creating/appending the new
operationInput so only one hidden operation input exists.
In `@cms/server/contest/handlers/trainingprogram.py`:
- Around line 470-476: The code directly casts training_day_id to int before
querying TrainingDay which raises ValueError for non-numeric input and causes a
500; update the handler to validate/cast training_day_id safely (wrap
int(training_day_id) in a try/except ValueError) and if casting fails, treat it
like a missing resource (return a 404/path that the handler uses) instead of
proceeding to self.sql_session.query(TrainingDay) with the invalid id; locate
the block using TrainingDay, training_day_id, training_program and ensure the
query uses the validated integer only when casting succeeds.
🧹 Nitpick comments (5)
cms/server/contest/handlers/trainingprogram.py (1)
648-664: Consider the redundancy in tie handling logic.The cutoff score check at line 659 is technically redundant: if a student has the same
total_scoreas the student at positiontop_to_show, the ranking logic (lines 631-634) would assign them a rank ≤top_to_show, so they'd already be included by theentry["rank"] <= top_to_showcheck.However, this is a minor observation—the current code is defensive and correctly handles the edge case, so no change is required.
cms/server/admin/templates/training_program_tasks.html (1)
232-334: Skip confirmation when order hasn’t changed (minor UX polish).
Right now a drag that ends without reordering still triggers the confirm and submit. You can short‑circuit if the order is unchanged.♻️ Suggested diff
- function saveNewOrder() { - if (!confirm('Save the new task order?')) { - restoreOriginalOrder(); - return; - } - var rows = tbody.querySelectorAll('tr[data-task-id]'); - var order = []; - rows.forEach(function(row, index) { + function saveNewOrder() { + var rows = Array.from(tbody.querySelectorAll('tr[data-task-id]')); + if (originalOrder && + rows.length === originalOrder.length && + rows.every(function(row, i) { return row === originalOrder[i]; })) { + originalOrder = null; + return; + } + if (!confirm('Save the new task order?')) { + restoreOriginalOrder(); + return; + } + var order = []; + rows.forEach(function(row, index) { order.push({ task_id: row.getAttribute('data-task-id'), new_num: index }); });cms/server/admin/handlers/trainingday.py (2)
181-181: Remove redundantimport jsoninside function.
jsonis already imported at module level (line 26). This inner import is unnecessary.♻️ Proposed fix
if operation == self.REORDER: - import json try:
780-793: Consider validating thattop_names≤top_to_showwhen both are integers.The handler validates each field individually but doesn't enforce that
top_names(names shown) cannot exceedtop_to_show(results shown). This logical constraint is enforced client-side but not server-side.🛡️ Suggested server-side validation
# Validate top_to_show (optional, defaults to "all") top_to_show = settings.get("top_to_show", "all") if top_to_show != "all": if not isinstance(top_to_show, int) or top_to_show < 0: raise ValueError(f"Invalid 'top_to_show' for tag '{tag}': must be non-negative integer or 'all'") + + # Validate top_names <= top_to_show when both are integers + if top_names != "all" and top_to_show != "all": + if top_names > top_to_show: + raise ValueError( + f"Invalid settings for tag '{tag}': top_names ({top_names}) " + f"cannot exceed top_to_show ({top_to_show})" + )cms/server/admin/handlers/trainingprogram.py (1)
118-161: Consider N+1 query optimization for training day notifications.The current implementation queries
QuestionandDelayRequestcounts inside nested loops (lines 137-155), which can result in 2×N queries where N is the total number of active training days across all programs. For large deployments, consider batching these counts.This is acceptable for now given the admin context and typical training day counts, but worth noting for future scalability.
- Guard allStudentTags in Tagify initialization to prevent ReferenceError - Avoid duplicate operation input in saveNewOrder for training days - Validate training_day_id safely with try/except in ScoreboardDataHandler - Remove redundant import json inside function (already imported at module level) - Skip confirmation when task order hasn't changed (UX improvement) - Validate top_names <= top_to_show when both are integers Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
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/trainingprogram.py (1)
696-712: Potential None threshold in _shift_task_nums calls.Unlike
_detach_task_from_training_daywhich guards againstNonevalues, this handler passestraining_day_num(line 699) andtask_num(line 711) directly to_shift_task_numswithout null checks.If either is
None, the query conditionnum_attr > Nonecould produce unexpected results.🛡️ Suggested defensive guard
# Reorder remaining tasks in the training day - _shift_task_nums( - self.sql_session, Task.training_day, training_day, - Task.training_day_num, training_day_num, -1 - ) + if training_day_num is not None: + _shift_task_nums( + self.sql_session, Task.training_day, training_day, + Task.training_day_num, training_day_num, -1 + ) # Remove from training program task.contest = None task.num = None self.sql_session.flush() # Reorder remaining tasks in the training program - _shift_task_nums( - self.sql_session, Task.contest, managing_contest, - Task.num, task_num, -1 - ) + if task_num is not None: + _shift_task_nums( + self.sql_session, Task.contest, managing_contest, + Task.num, task_num, -1 + )
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/training_program_tasks.html`:
- Around line 121-124: The template's data-training-day-name can be empty when a
training_day has a contest with an empty description; update the value
expression used for data-training-day-name so it falls back to contest.name
before falling back to training_day.description/name or 'Archived' (i.e. prefer
t.training_day.contest.description or t.training_day.contest.name or
t.training_day.description or t.training_day.name or 'Archived'), keeping
data-task-name and data-is-active logic unchanged so the modal warning always
receives a non-empty label for active training days.
- Around line 55-58: Update the data-training-day attribute expression so its
fallback matches the displayed label for archived rows: when t.training_day
exists but has no name and is_archived is true return "(archived)"; otherwise
fall back to the same empty/Not-assigned value as before. Modify the
data-training-day attribute (the expression using t.training_day,
t.training_day.contest, and is_archived) to include the extra conditional that
returns "(archived)" when appropriate so sort keys align with the displayed
label.
In `@cms/server/contest/templates/training_program_overview.html`:
- Around line 105-126: The displayed percentage uses the raw variable percentage
while the progress bar uses clamped_percentage, causing mismatch and possible
>100% or negative display; update the stats-percentage-value to use
clamped_percentage (formatted to one decimal) instead of percentage so the
visible number matches the clamped progress bar (refer to the clamped_percentage
variable and the stats-percentage-value element).
🧹 Nitpick comments (3)
cms/server/admin/handlers/trainingprogram.py (1)
135-155: Consider batching notification queries for better performance.The current implementation executes two queries per active training day (one for unanswered questions, one for pending delays). For training programs with many active training days, this could result in a large number of queries.
A single aggregated query could reduce database round-trips, though this may be acceptable for typical admin workloads.
♻️ Optional batched query approach
# Instead of per-td queries, batch them: active_td_ids = [td.id for tp in training_programs for td in tp.training_days if td.contest is not None] active_contest_ids = [td.contest_id for tp in training_programs for td in tp.training_days if td.contest is not None] # Single query for all unanswered questions grouped by contest unanswered_counts = dict( self.sql_session.query(Participation.contest_id, func.count(Question.id)) .join(Question) .filter(Participation.contest_id.in_(active_contest_ids)) .filter(Question.reply_timestamp.is_(None)) .filter(Question.ignored.is_(False)) .group_by(Participation.contest_id) .all() ) # Similar for pending delays...cms/server/admin/templates/fragments/histogram_js.html (1)
251-252: Potential NaN in hue calculation with single bucket.If
bucketOrder.lengthis 1, the calculationindex / (bucketOrder.length - 1)becomes0 / 0 = NaN, resulting inhsl(NaN, 75%, 60%). While browsers handle this gracefully, consider adding a guard.♻️ Optional fix
- var hue = (index / (bucketOrder.length - 1)) * 120; + var hue = bucketOrder.length > 1 ? (index / (bucketOrder.length - 1)) * 120 : 60;cms/server/contest/static/training_day_cards.css (1)
24-24: Consider specifying transition properties explicitly.Using
transition: all 0.3s easecan lead to unintended animations on properties you didn't mean to animate (e.g., color, background changes). While this works fine currently, being explicit improves maintainability.♻️ Optional: Specify transition properties
- transition: all 0.3s ease; + transition: transform 0.3s ease, box-shadow 0.3s ease;
- Guard allStudentTags in Tagify initialization to prevent ReferenceError - Avoid duplicate operation input in saveNewOrder for training days - Validate training_day_id safely with try/except in ScoreboardDataHandler - Remove redundant import json inside function (already imported at module level) - Skip confirmation when task order hasn't changed (UX improvement) - Validate top_names <= top_to_show when both are integers Co-Authored-By: Ron Ryvchin <[email protected]>
|
@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/templates/training_program_tasks.html`:
- Around line 248-252: The dragstart handler on tbody currently only starts
dragging when e.target is a TR, which fails if the user starts the drag on an
inner element (e.g., grip SVG); update the listener
(tbody.addEventListener('dragstart', ...)) to find the row via
e.target.closest('tr') instead of checking e.target.tagName, assign that to
draggedRow, ensure you bail out if closest returns null, then set originalOrder
= Array.from(tbody.querySelectorAll('tr')) and add the 'dragging' class to
draggedRow as before.
In `@cms/server/contest/templates/training_program_overview.html`:
- Line 1: The file starts with a UTF-8 BOM character before the template
declaration ({% extends "contest.html" %}), which can break template parsing;
remove the BOM so the file begins exactly with the `{% extends "contest.html"
%}` token (e.g., open the file in an editor that can show/strip BOM or run a
tool to remove the leading U+FEFF) and save as UTF-8 without BOM to ensure
template inheritance and parsing work correctly.
🧹 Nitpick comments (1)
cms/server/contest/templates/training_program_overview.html (1)
131-148: Consider extracting repeated inline styles to CSS classes.The four stat boxes share identical layout patterns (flex, border-radius, padding, text-align, min-width) with only color variations. Extracting these to a
.stat-boxbase class with color modifiers would improve maintainability and reduce template size.♻️ Example refactor
Add to
<style>block:.stat-box { flex: 1; border-radius: 8px; padding: 15px; text-align: center; min-width: 100px; border-width: 2px; border-style: solid; } .stat-box-value { font-size: 36px; font-weight: 800; line-height: 1; } .stat-box-label { font-size: 11px; text-transform: uppercase; letter-spacing: 1px; margin-top: 5px; opacity: 0.8; } .stat-box-solved { background: `#e8f5e9`; border-color: `#28a745`; } .stat-box-solved .stat-box-value { color: `#28a745`; } .stat-box-solved .stat-box-label { color: `#2e7d32`; } /* ... similar for partial, attempted, not-attempted */Then simplify the HTML:
<div class="stat-box stat-box-solved"> <div class="stat-box-value" id="solved-count">0</div> <div class="stat-box-label">{% trans %}Solved{% endtrans %}</div> </div>
ronryv
force-pushed
the
devin/1769407232-redesign-tasks-page
branch
from
96f043a to
1424d5d
Compare
…days (#107) * Redesign tasks page and add release/reuse functionality for archived training days - Modernize the tasks page design to match the students page style - Add modern table with sortable columns - Add dropdown for adding tasks - Add release/reuse button for tasks in archived training days - Add remove button with confirmation for active training day tasks - Add move buttons (top, up, down, bottom) for reordering tasks Co-Authored-By: Ron Ryvchin <[email protected]> * Rename to detach + style imp * Add drag-and-drop reordering, consolidate CSS, fix remove behavior, add shared utility Co-Authored-By: Ron Ryvchin <[email protected]> * Use card-style add task UI and convert remove page to modal Co-Authored-By: Ron Ryvchin <[email protected]> * Fix task removal redirect, remove deprecated page, and modernize notification alerts - Fix redirect after task removal to correctly return to tasks page - Remove deprecated training_program_task_remove.html (now using modal) - Remove unused GET handler from RemoveTrainingProgramTaskHandler - Add modern info alert component (tp-info-alert) with warning/info variants - Update training_program_attendance.html with modern alert styling - Update questions.html with modern alert styling - Update announcements.html with modern alert styling - Add CSS styles for info alert badges with counts Co-Authored-By: Ron Ryvchin <[email protected]> * Add info note to combined ranking, redesign training days page with tasks column and histograms - Add info alert to combined ranking page explaining it shows archived training days only - Redesign training_days page with modern tp-* CSS classes - Add Tasks column to both active and archived training days tables - Active training days show task badges linking to task pages - Archived training days show task badges with histogram icons - Create reusable histogram_modal.html fragment for score distribution visualization - Clicking archived task badges opens histogram modal with score distribution Co-Authored-By: Ron Ryvchin <[email protected]> * Fix training_days page - keep original styling, add tasks column with histogram support - Reverted to original page styling (not the modern redesign) - Added Tasks column to both active and archived training days tables - Active training days show task badges linking to task pages - Archived training days show task badges with histogram icons - Removed duplicated histogram_modal.html fragment - Use existing histogram CSS classes from aws_tp_styles.css - Histogram modal opens when clicking on archived task badges Co-Authored-By: Ron Ryvchin <[email protected]> * Modernize training_days page with drag-and-drop, icons, and improved time display Co-Authored-By: Ron Ryvchin <[email protected]> * Add scoreboard sharing feature for archived training days - Add scoreboard_sharing JSONB column to TrainingDay model (db version 49->50) - Add admin UI to configure scoreboard sharing per archived training day - Modal to add/remove tags and set top_names count per tag - Visual indicator showing number of tags shared - Add CWS handler to fetch scoreboard data with tag-based filtering - Add CWS UI with scoreboard badges and modal - Students see badges only for tags they had during training - Scoreboard shows only students with matching tags - Scoreboard shows only tasks accessible to those tags - Top N students show full names, others anonymized - Current student's row is highlighted Co-Authored-By: Ron Ryvchin <[email protected]> * Fix: keep db version at 49, add schema migration for scoreboard_sharing Co-Authored-By: Ron Ryvchin <[email protected]> * share and fix common histogram logic * UI improvements * Modernize training_programs page with card-based layout Co-Authored-By: Ron Ryvchin <[email protected]> * Refine training_programs page: remove header button/subtitle, add indicators, modernize add form Co-Authored-By: Ron Ryvchin <[email protected]> * Address review comments * Enhance scoreboard sharing with everyone option, top_to_show limit, and tie handling - Add '__everyone__' option to share scoreboard with all students regardless of tags - Add 'top_to_show' field to limit number of results displayed (with 'all' checkbox) - Add 'top_names' field to limit how many top results show full names (with 'all' checkbox) - Implement proper tie handling: students with same score get same rank - Show all tied students at cutoff even if exceeds top_to_show limit - Always show current user's rank even if past top_to_show limit - Add validation caps based on actual student counts in admin UI - Update admin modal with new fields and checkboxes - Update CWS handler with new filtering and ranking logic Co-Authored-By: Ron Ryvchin <[email protected]> * UI improvements: modernize training cards, progress bar, add icons, and clickable training days Contest server changes: - Add clock and hourglass SVG icons to training day cards (start/duration) - Redesign progress bar with dark navy background, percentage display, and score sections - Add upload icon to Submit button - Add grid icon to scoreboard buttons and display 'everyone' instead of '__everyone__' Admin server changes: - Modernize add_training_day.html with modern form styling (tp-form-card pattern) - Make 'ACTIVE TRAINING DAYS' label clickable, linking to training_days page Co-Authored-By: Ron Ryvchin <[email protected]> * Fix UI feedback: input widths, select box, LIVE badge, duration icon, progress colors - Admin: Reduce hours/minutes input width from 60px to 50px in Main Groups Configuration - Admin: Replace Tagify with native select box in Scoreboard Sharing modal - Admin: Update task archive progress bar thresholds (50% instead of 40%) - Contest: Update LIVE badge to red pill with pulsing dot - Contest: Fix duration icon to proper hourglass SVG - Contest: Add conditional progress bar colors (red <50%, orange 50-80%, green >=80%) Co-Authored-By: Ron Ryvchin <[email protected]> * address review comments * nits * Address PR review comments: validation, defensive parsing, and code cleanup - Extract shared table sorting utility in training_program_training_days.html - Convert forEach to for loop with proper early exit in saveScoreboardSharing - Validate reorder flow in trainingday.py - verify active td ids and positions - Update task reorder validation to 0-based in trainingprogram.py - Guard optional globals in histogram_js.html (tagsPerTrainingDay, etc.) - Avoid duplicate operation input in training_program_tasks.html - Fix sort key mismatch for training day column in training_program_tasks.html - Null-check XSRF input in training_program_training_days.html - Defensive parsing of top_to_show/top_names in contest/trainingprogram.py Co-Authored-By: Ron Ryvchin <[email protected]> * Address additional CodeRabbit review comments - Guard allStudentTags in Tagify initialization to prevent ReferenceError - Avoid duplicate operation input in saveNewOrder for training days - Validate training_day_id safely with try/except in ScoreboardDataHandler - Remove redundant import json inside function (already imported at module level) - Skip confirmation when task order hasn't changed (UX improvement) - Validate top_names <= top_to_show when both are integers Co-Authored-By: Ron Ryvchin <[email protected]> * Address additional CodeRabbit review comments - Guard allStudentTags in Tagify initialization to prevent ReferenceError - Avoid duplicate operation input in saveNewOrder for training days - Validate training_day_id safely with try/except in ScoreboardDataHandler - Remove redundant import json inside function (already imported at module level) - Skip confirmation when task order hasn't changed (UX improvement) - Validate top_names <= top_to_show when both are integers Co-Authored-By: Ron Ryvchin <[email protected]> * nits --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Redesign tasks page and add scoreboard sharing for archived training days
Summary
This PR modernizes the training program tasks page with drag-and-drop reordering and detach functionality, and adds a new scoreboard sharing feature for archived training days.
UI Changes:
aws_tp_styles.cssinstead of inline stylesNew Functionality:
training_day_idandtraining_day_num, making the task available for new training daysCode Quality:
_shift_task_nums()to eliminate repetitive task reordering code across handlersUpdates since last revision
Code Review Fixes (Latest Commit):
initTableSorting(tableId, rowSelector)utility to eliminate duplicate sorting logic for active/archived training days tablesforEachtoforloop insaveScoreboardSharingfor proper early exit on validation errors (previously alerts showed but save proceeded)len(managing_contest.tasks))typeofguards for optional globals in histogram JS (tagsPerTrainingDay,trainingDayTasks,studentAccessibleTasks) to prevent ReferenceError when fragment is reusedinput[name="operation"]before creating new onedata-training-dayattribute now usescontest.namefor active rows to match displayed label.valuetop_to_showandtop_namesin scoreboard handler: handles None, non-numeric strings, and "all" with try/except and safe defaultsReview & Testing Checklist for Human
__everyone__in admin, then verify a student with no matching tags can still view the scoreboardRecommended test plan:
__everyone__enabled,top_to_show=5,top_names=3Notes
{"__everyone__": {"top_to_show": 10, "top_names": 5}, "tag1": {"top_to_show": "all", "top_names": 3}}ScoreboardDataHandlerreturnstotal_studentscount for UI referencehistoricalStudentTagsdataLink to Devin run: https://app.devin.ai/sessions/d64a1e7bfd5f4effa1a9f9e108ec9436
Continuation: https://app.devin.ai/sessions/01fa6557d9cb4ee2839086da7034e517
Continuation: https://app.devin.ai/sessions/efdf6fa30c7c4e08a6421947c78bd085
Continuation: https://app.devin.ai/sessions/a5d62407ca984101a766bcc2dc87b81f
Continuation: https://app.devin.ai/sessions/8a468c555b71446a91d366df11e89f9d
Requested by: Ron Ryvchin (@ronryv)
Summary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.