Split unsolved tasks into attempted/not attempted and add sortable submissions column #103
Conversation
🤖 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 bulk student ingestion and student-task submissions handlers, factors ranking logic into a reusable RankingCommonMixin with unified CSV export, updates task-archive scoring to use fresh cache lookups, and performs large admin UI/CSS and user-detail template refactors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Handler as Server: BulkAddTrainingProgramStudentsHandler
participant DB as Database (SQLAlchemy)
participant Cache as ScoreCache
Client->>Handler: POST /training_program/:id/students/bulk_add (file)
Handler->>Handler: parse_usernames_from_file(file)
Handler->>DB: lookup/create Users & Participations (bulk)
DB-->>Handler: created/updated records
Handler->>DB: commit()
Handler->>Cache: invalidate/reinit related cache entries
Cache-->>Handler: ack
Handler-->>Client: render bulk-add results (per-user statuses)
sequenceDiagram
participant User as Admin (Browser)
participant Handler as Server: RankingHandler / TrainingProgramRankingHandler
participant DB as Database
participant Cache as ScoreCache
User->>Handler: GET /ranking or ?format=csv
Handler->>DB: _load_contest_data(contest_id) (tasks, participations, users, views)
DB-->>Handler: contest data
Handler->>Cache: get_cached_score_entry per task/participation as needed
Cache-->>Handler: fresh scores
Handler->>Handler: _calculate_scores(...) -> prepare rows, show_teams
alt CSV requested
Handler->>Handler: _write_csv(...)
Handler-->>User: CSV download (Content-Disposition)
else HTML
Handler-->>User: render ranking template (includes progress columns)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
force-pushed
the
devin/1769191458-task-archive-improvements
branch
2 times, most recently
from
badb9c4 to
e965cc2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/trainingprogram.py`:
- Around line 699-705: In the loop iterating over self.contest.participations
where calculate_task_archive_progress(student, p, self.contest,
self.sql_session) is called and its result stored into
task_archive_progress_by_participation[p.id], add a commit on the SQL session
(self.sql_session.commit()) immediately after each call to
calculate_task_archive_progress to ensure any advisory locks taken inside
get_student_archive_scores are released; keep the commit tightly scoped to each
iteration and do not delay until after the loop.
🧹 Nitpick comments (6)
cms/server/admin/templates/student_tasks.html (1)
103-186: Consider moving the inline CSS/JS to a shared static asset.
Keeps templates lean and improves cacheability/CSP flexibility.cms/server/admin/handlers/student.py (2)
40-40: Unused import:get_cached_score_entry
get_cached_score_entryis imported but not directly used in this file. It's used internally byget_student_archive_scores. Consider removing this import.🧹 Suggested fix
-from cms.grading.scorecache import get_cached_score_entry
621-621: Consider handling invalid page parameter.If a non-numeric string is passed as the
pagequery argument,int()will raise aValueError, resulting in an unhandled 500 error. Consider wrapping this in a try/except or using a safer conversion.🛡️ Suggested defensive fix
- page = int(self.get_query_argument("page", "0")) + try: + page = int(self.get_query_argument("page", "0")) + except ValueError: + page = 0cms/server/util.py (1)
262-267: Consider reusing loaded relationship instead of re-querying.The function queries
StudentTaskdirectly, but ifstudent.student_tasksis already loaded (which it often is from eager loading), this creates an unnecessary additional query.♻️ Alternative using preloaded relationship
- student_task_ids = { - st.task_id - for st in sql_session.query(StudentTask) - .filter(StudentTask.student_id == student.id) - .all() - } + student_task_ids = {st.task_id for st in student.student_tasks}This would leverage SQLAlchemy's lazy loading if needed, or use already-loaded data if available, potentially saving a query.
cms/server/admin/handlers/contestranking.py (1)
43-44: Unused import:calculate_task_archive_progress
calculate_task_archive_progressis imported but not used in this file. It appears to be used intrainingprogram.pyinstead.🧹 Suggested fix
from cms.server.util import can_access_task, get_all_student_tags, \ - calculate_task_archive_progress + get_all_student_tagscms/server/admin/handlers/trainingprogram.py (1)
56-57: Missing blank line between import and class definition.Per PEP 8 style guidelines, there should be two blank lines between import statements and the first class definition.
🧹 Suggested fix
from .contestranking import RankingCommonMixin + + class TrainingProgramListHandler(SimpleHandler("training_programs.html")):
…bmissions column - Task archive (CWS): Split 'unsolved' category into 'attempted' (has submissions) and 'not attempted' (no submissions) with separate counters and filter buttons - Task archive (CWS): Add appropriate colors for each status (blue for attempted, gray for not attempted) - Admin student tasks (AWS): Add submissions count column that links to the student's submission page for that task - Admin student tasks (AWS): Make the table sortable by task name, assigned date, training score, home score, and submissions count - Add submission_counts parameter to calculate_task_archive_progress utility Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
…Scores Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
…bar link - Modified TrainingProgramRankingHandler to check StudentTask records for task access - Tasks not in a student's archive now show the 🚫 symbol on the ranking page - Renamed sidebar link from 'Ranking' to 'Home Task Scores' - Reverted incorrect changes to Combined Ranking page (kept original dash symbol) Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Add StudentTaskSubmissionsHandler for viewing submissions filtered by task
- Add route /training_program/{tp_id}/student/{user_id}/task/{task_id}/submissions
- Create student_task_submissions.html template
- Fix submissions column link in student_tasks.html to use new route
- Add get_student_archive_tasks() utility function for fetching tasks in student's archive
- Fix calculate_task_archive_progress() to use get_cached_score_entry for accurate scores
- Update callers to pass sql_session parameter for proper score cache usage
Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Fix StudentTasksHandler to use get_cached_score_entry for home_scores instead of iterating over participation.task_scores (which may be stale) - Fix _build_past_training_info to use get_cached_score_entry for home scores - Add Task Archive Progress column to ranking.html for training programs (replaces Global column, only shown for training program managing contests) - Make training_program_students.html table sortable with numeric sorting on Task Archive Progress column - Add proper session commits to release advisory locks from cache rebuilds Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
ronryv
force-pushed
the
devin/1769191458-task-archive-improvements
branch
4 times, most recently
from
b484e01 to
b72c418
Compare
Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Create shared utility parse_usernames_from_file in util.py - Add BulkAddTrainingProgramStudentsHandler for bulk adding students - Update BulkAddContestUsersHandler to use shared utility - Add bulk add form and results display to training_program_students.html - Register new handler and route in handlers/__init__.py Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
ronryv
force-pushed
the
devin/1769191458-task-archive-improvements
branch
from
b72c418 to
a6bb437
Compare
- Move shared CSS from student_tasks.html to aws_style.css for reuse - Add action buttons row with Import List, Bulk Assign Task, Add Student - Add Task Archive Progress card with visual progress bars - Modernize students table with sortable columns and consistent styling - Update handler to support new remove button format Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
The render_params_for_training_program method reinitializes r_params, so it must be called before render_params_for_remove_confirmation which adds submission_count to the existing r_params. Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Move Task Archive Progress into table as column with small inline bar - Combine username/first name/last name into single Student column - Change Add Student button from purple to teal (training program theme) - Rename 'Import List' to 'Add Students from List' for clarity - Remove separate progress card section Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
…y, fix progress colors - Remove duplicate Student Tags column (tags now shown only in Student column as badges) - Remove tagify readonly initialization (no longer needed) - Add score display (score/max) to Task Archive Progress column - Fix progress bar colors: green >= 80%, orange >= 40%, red < 40% - Update table to 3 columns: Student, Task Archive Progress, Actions Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Add profile picture circle (or initials placeholder) in Student column - Move tags inline after username instead of on separate row - Update sorting so 'No tasks' students always appear at end when sorting by progress - Narrow Student column by making layout more compact Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
Use slice notation [:1] instead of [0] to safely handle empty names Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Left-align both Student and Task Archive Progress columns (header and content) - Narrow table width (auto width with min-width instead of 100%) - Replace Remove button with trash icon SVG - Ensure 0% progress bar shows gray track clearly (min 5% width for visibility) Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Replace bulk_assign_task.html page with a modal dialog in training_program_students.html - Update BulkAssignTaskHandler to redirect to students page after POST - Remove GET method from BulkAssignTaskHandler (no longer needed) - Delete bulk_assign_task.html template - Create aws_tp_styles.css with CSS custom properties (color variables) for theming - Move inline styles from training_program_students.html to centralized CSS - Add aws_tp_styles.css to base.html for global availability This improves maintainability and enables future theme customization. Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
ronryv
force-pushed
the
devin/1769191458-task-archive-improvements
branch
from
1b7cc21 to
9a9e7fe
Compare
…ve inline styles to CSS - Fix student-info to use flex-wrap with proper truncation for names - Restore center alignment for attendance and ranking table headers - Add max-width to student-name-link for proper ellipsis truncation - Move histogram modal styles from training_program_combined_ranking.html to aws_tp_styles.css - Replace inline styles with CSS classes in combined ranking template - Add combined-ranking-* CSS classes for student wrapper, info, name row, and tags Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
- Create fragments/user_detail_styles.html for shared CSS styles - Create fragments/user_detail_layout.html for shared HTML structure - Create fragments/user_detail_scripts.html for shared JavaScript utilities - Update participation_detail.html to use shared fragments - Update training_program_combined_ranking_detail.html to use shared fragments - Add initUserDetail() function with options for contest rows and AJAX submissions This reduces code duplication from ~450 lines per file to ~80-100 lines each. Co-Authored-By: Ron Ryvchin <ron.ryv@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cms/server/admin/handlers/student.py (1)
74-79: Add commits after archive-progress calculation to release advisory locks.Both
calculate_task_archive_progresscalls invokeget_student_archive_scores, which may rebuild score caches and acquire PostgreSQL advisory locks. Without committing, these locks remain held for the entire request/session and can block concurrent operations. The codebase documents this requirement explicitly and commits are already applied in similar patterns (e.g.,trainingprogram.py:692).Add
self.sql_session.commit()after each progress loop:Locations needing commits
- Lines 74–79:
TrainingProgramStudentsHandler.get()— after progress loop, beforerender()- Lines 269–273:
BulkAddTrainingProgramStudentsHandler.post()— after progress loop, beforerender()
🤖 Fix all issues with AI agents
In `@cms/server/admin/static/aws_tp_styles.css`:
- Around line 830-847: The CSS contains a duplicate .btn-icon-only rule that
conflicts with the earlier definition (first .btn-icon-only at the top of the
file) and omits background: none, causing confusing cascading behavior; remove
the duplicate .btn-icon-only block (the later one) so only the original
.btn-icon-only, :hover and :disabled declarations remain, ensuring the intended
background: none from the first definition is preserved and no conflicting
overrides occur.
In `@cms/server/admin/templates/student_tasks.html`:
- Line 83: Fix the typo in the table header within the student_tasks.html
template by replacing the incorrect "Sumbissions" text with "Submissions" in the
<th> element (the sortable header with class "sortable-header text-center" and
data-sort="number"); ensure the visible label is corrected and keep the existing
attributes (data-sort and style) unchanged.
In `@cms/server/admin/templates/training_program_students.html`:
- Around line 18-24: The label for students-file-input currently renders two
style attributes when admin.permission_all is false; update the template so the
label has a single style (or class) attribute whose value is conditionally built
based on admin.permission_all (refer to form id "bulk-add-form", input id
"students-file-input", and the label "for=students-file-input"); e.g. assemble
one style string (or use conditional class names) that includes cursor and
opacity rules instead of emitting a second style attribute.
In `@cms/server/contest/handlers/trainingprogram.py`:
- Around line 69-94: calculate_task_archive_progress (which calls
get_student_archive_scores -> get_cached_score_entry) can acquire PostgreSQL
advisory locks that are only released on commit; to avoid holding locks for the
remainder of the request, explicitly commit the SQL transaction after the cache
lookups complete. After the call to calculate_task_archive_progress (inside the
student is not None branch), add a transaction commit using the session object
(self.sql_session.commit()) so any advisory locks taken by
get_cached_score_entry are released; keep the commit narrow (immediately after
progress is computed) and do not change the surrounding logic.
🧹 Nitpick comments (6)
cms/server/admin/static/aws_tp_styles.css (5)
172-181: Consider using modernclip-pathinstead of deprecatedclip.The
clipproperty is deprecated. The modern visually-hidden pattern usesclip-path: inset(50%)for better forward compatibility.♻️ Suggested improvement
.file-input-hidden { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; - clip: rect(0, 0, 0, 0); + clip-path: inset(50%); border: 0; }
229-241: Heavy!importantusage may indicate specificity conflicts.The multiple
!importantdeclarations here suggest overriding conflicting styles from elsewhere. While functional, this pattern can make future maintenance difficult. Consider whether the base table styles could be refactored to use modifier classes instead of requiring!importantoverrides.
1124-1128: Duplicate.page-subtitledefinition.This rule duplicates the definition at lines 72-76 with identical properties. Remove this duplicate to avoid maintenance confusion.
♻️ Suggested fix
-.page-subtitle { - margin-top: 4px; - font-size: 0.9rem; - color: var(--tp-text-muted); -}
135-166: Accessibility note: Ensure HTML includes ARIA attributes for sort state.The CSS sort indicators (via
::aftercontent) are visual-only. Ensure the corresponding HTML elements includearia-sort="ascending",aria-sort="descending", oraria-sort="none"attributes for screen reader users. This is an HTML concern, but worth noting here.
1323-1365: Hardcoded colors break theming consistency.These rules use hardcoded hex colors (
#f0f9ff,#bae6fd,#0369a1,#3b82f6,#2563eb) instead of CSS variables. This undermines the design token system established at the top of the file. Consider adding info/blue color tokens to:rootfor consistency.♻️ Example token additions
:root { /* ... existing tokens ... */ /* Info colors (blue) */ --tp-info: `#3b82f6`; --tp-info-hover: `#2563eb`; --tp-info-light: `#f0f9ff`; --tp-info-border: `#bae6fd`; --tp-info-dark: `#0369a1`; }Then use
var(--tp-info),var(--tp-info-light), etc. in the histogram and copy button styles.cms/server/util.py (1)
300-338: Eager-loadStudentTask.taskto avoid N+1 queries.
student_task.taskis dereferenced inside the loop; addjoinedloadto prevent per-task lazy loads.♻️ Proposed refactor
+from sqlalchemy.orm import joinedload @@ student_tasks = ( sql_session.query(StudentTask) + .options(joinedload(StudentTask.task)) .filter(StudentTask.student_id == student.id) .all() )
|
@coderabbitai full review |
✅ Actions performedFull review 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/templates/training_program_combined_ranking.html (1)
271-299: Prevent bucket collisions when max scores are small.
Line 271-299: rounding bucket bounds to integers can collapse multiple buckets whenmaxPossibleScore < 10(or not divisible by 10), producing duplicate keys and incorrect counts. Use stable bucket keys (index-based) and keep labels at decimal precision.🛠️ Proposed fix
- for (var i = 1; i <= 9; i++) { - var upperBound = Math.round(i * bucketSize); - var lowerBound = Math.round((i - 1) * bucketSize); - var key = upperBound.toString(); - buckets[key] = 0; - bucketLabels[key] = '(' + lowerBound + ',' + upperBound + ']'; - bucketOrder.push(key); - } + for (var i = 1; i <= 9; i++) { + var upperBound = i * bucketSize; + var lowerBound = (i - 1) * bucketSize; + var key = 'b' + i; + buckets[key] = 0; + bucketLabels[key] = '(' + lowerBound.toFixed(1) + ',' + upperBound.toFixed(1) + ']'; + bucketOrder.push(key); + } - var lastKey = Math.round(maxPossibleScore).toString(); + var lastKey = 'b10'; buckets[lastKey] = 0; - bucketLabels[lastKey] = '>' + Math.round(lastBucketThreshold); + bucketLabels[lastKey] = '>' + lastBucketThreshold.toFixed(1); bucketOrder.push(lastKey); @@ - var bucketKey = Math.round(bucketIndex * bucketSize).toString(); + var bucketKey = 'b' + bucketIndex; buckets[bucketKey]++;
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/student.py`:
- Around line 693-739: The handler get currently only checks the task belongs to
the training program and that a Student row exists for the participation, but it
must also verify the student is actually assigned the specific task before
showing the archive; add a membership check (e.g., query the StudentTask
association or Student.tasks relationship) for the given student and task inside
get (after student is loaded and before building submission_query) and raise
tornado.web.HTTPError(404) if no StudentTask exists; ensure you reference the
existing symbols student, task, and get so the new query enforces "student
assigned to task" membership before rendering.
In `@cms/server/admin/templates/fragments/user_detail_layout.html`:
- Line 12: The <img id="UserDetail_close"> currently has an empty src which can
trigger a request; either remove the element entirely if unused, or replace it
with a non-requesting placeholder (e.g., a small data URI) or convert it to a
CSS/inline SVG-based icon for the close button; locate the element by id
"UserDetail_close" in the template and apply one of these fixes so no empty src
attribute remains.
🧹 Nitpick comments (8)
cms/server/admin/templates/fragments/user_detail_scripts.html (1)
89-131: Consider potential XSS in HTML string construction.The
do_showfunction builds HTML strings using values fromDataStore(e.g.,contest['name'],task['name']). If these values originate from user input or external sources, they could contain malicious HTML/JavaScript.While this appears to be admin-only code where data is trusted, consider using jQuery's text-setting methods or escaping to be defensive:
// Example defensive approach function escapeHtml(str) { var div = document.createElement('div'); div.textContent = str; return div.innerHTML; }cms/server/admin/templates/fragments/user_detail_styles.html (1)
1-202: Consider using CSS custom properties for color consistency.This stylesheet uses hardcoded color values (e.g.,
#0F766E,#ccc,#999999) whileaws_tp_styles.cssdefines CSS custom properties like--tp-primary,--tp-border, etc. Using the shared variables would improve maintainability and ensure visual consistency across the admin UI.For example:
#0F766E→var(--tp-primary)#ccc,#999999→var(--tp-border)orvar(--tp-border-dark)#EEEEEE,#F8F8F8→var(--tp-bg-gray)orvar(--tp-bg-light)cms/server/admin/templates/student_task_submissions.html (1)
1-4: Minor style suggestion: reorder imports.The
{% import %}statement formacro_reevaluation_buttonsappears before{% extends %}. While Jinja2 allows this, the conventional pattern in this codebase is to place{% extends %}first, followed by imports.♻️ Suggested reorder
-{% 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/fragments/user_detail_layout.html (1)
19-19: Consider moving inline styles to the styles fragment.The inline style
style="font-size: 0.6em; color:cms-dev#666;"could be moved touser_detail_styles.htmlfor consistency and easier theming.cms/server/admin/static/aws_tp_styles.css (2)
236-248: Consider refactoring to reduce!importantusage.The
#students-table thead thrule uses multiple!importantdeclarations to override base styles. This indicates potential specificity conflicts. If feasible, consider adjusting base table styles or using more specific selectors to achieve the desired styling without!important.
965-991: Multiple!importantdeclarations for header styling.Consider consolidating these overrides or increasing specificity through selector composition rather than relying on
!important, which can make future maintenance more difficult.cms/server/admin/templates/student_tasks.html (1)
4-27: Consider moving the page-specific<style>block to the shared stylesheet.
Keeps templates lighter and consolidates styling in one place.cms/server/admin/handlers/student.py (1)
190-253: Batch lookups to avoid per‑username queries.Each username currently triggers a User lookup and a Participation lookup, which scales poorly for large uploads. Consider prefetching users and existing participations in bulk.
♻️ Suggested refactor
results = [] students_added = 0 - - for username in usernames: - user = self.sql_session.query(User).filter( - User.username == username).first() + users_by_username = { + u.username: u + for u in ( + self.sql_session.query(User) + .filter(User.username.in_(usernames)) + .all() + ) + } + user_ids = [u.id for u in users_by_username.values()] + existing_user_ids = set() + if user_ids: + existing_user_ids = { + row[0] + for row in ( + self.sql_session.query(Participation.user_id) + .filter(Participation.contest == managing_contest) + .filter(Participation.user_id.in_(user_ids)) + .all() + ) + } + + for username in usernames: + user = users_by_username.get(username) if user is None: results.append({ "username": username, "status": "not_found", "message": "Username does not exist in the system" }) else: - 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: results.append({ "username": username, "status": "already_exists", "message": "User is already a student in this program" }) else: participation = Participation( contest=managing_contest, user=user, starting_time=make_datetime() ) self.sql_session.add(participation) self.sql_session.flush() @@ results.append({ "username": username, "status": "success", "message": "Successfully added to training program" }) students_added += 1 + existing_user_ids.add(user.id)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/student.py`:
- Around line 254-284: The commit/rollback flow can leave "status":"success" in
results when try_commit() returns False; after calling try_commit() check its
boolean result and on failure iterate over results to convert any
"status":"success" entries into a failure entry (or clear them) and set
students_added = 0, skip calling self.service.proxy_service.reinitialize(), and
add an error notification into self.r_params (e.g., r_params["bulk_add_error"])
before rendering so the UI doesn't show misleading success; update code around
try_commit(), results, and students_added in the student handler to ensure the
rendered r_params reflect the actual commit outcome.
In `@cms/server/admin/templates/training_program_combined_ranking.html`:
- Around line 256-325: The code that sets maxPossibleScore via a truthy fallback
(e.g., "maxPossibleScore = maxPossibleScore || 100" used in openHistogramModal
and show*Histogram) forces 0 to become 100 so the new zero-guard never runs;
change those fallbacks to only default when the value is null or undefined (use
an explicit check like if (maxPossibleScore == null) maxPossibleScore = 100) so
true zero values are preserved and the existing zero-guard in the histogram
bucketing logic (the maxPossibleScore === 0 branch) can work correctly.
♻️ Duplicate comments (1)
cms/server/admin/templates/fragments/user_detail_layout.html (1)
24-27: Emptysrcattributes may trigger unnecessary HTTP requests.Similar to the previously-fixed close button issue:
- Line 24: The hidden fallback image still has
src=""- Line 27: The flag image has
src=""without any conditionalSome browsers interpret
src=""as a request to the current page URL.🔧 Suggested fix
- {% if picture_url %}<img id="UserDetail_face" src="{{ picture_url }}" alt="Profile picture of {{ first_name }} {{ last_name }}"/>{% else %}<img id="UserDetail_face" src="" alt="" style="display: none;"/>{% endif %} + {% if picture_url %}<img id="UserDetail_face" src="{{ picture_url }}" alt="Profile picture of {{ first_name }} {{ last_name }}"/>{% endif %} <div id="UserDetail_spacer"></div> <div id="UserDetail_team">{{ team_name or '' }}</div> - <img id="UserDetail_flag" src="" alt="Flag"/> + <img id="UserDetail_flag" src="data:," alt="Flag" style="display: none;"/>If the flag element needs to exist for JavaScript to populate later, use a data URI placeholder. If
UserDetail_faceneeds to exist when hidden for JS targeting, apply the same pattern.
🧹 Nitpick comments (1)
cms/server/admin/templates/fragments/user_detail_layout.html (1)
35-42: Use<th>for table header cells.Header cells inside
<thead>should use<th>instead of<td>for semantic correctness and accessibility. Screen readers use this distinction to announce column headers.♻️ Suggested fix
<thead> <tr> - <td></td> - <td style="text-align: right;">Score</td> - <td style="text-align: right;">Rank</td> - <td></td> + <th></th> + <th style="text-align: right;">Score</th> + <th style="text-align: right;">Rank</th> + <th></th> </tr> </thead>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/fragments/user_detail_layout.html`:
- Around line 12-14: Wrap the SVG close icon in a semantic <button> to make it
keyboard-focusable and announceable; move the id "UserDetail_close" from the
<svg> to the new <button> (or add it to the button), remove the invalid alt
attribute from the <svg>, set the button attribute type="button", add
aria-label="Close" on the button, and apply the existing inline styling
(cursor/position/top/right) to the button so layout/appearance remain unchanged;
finally, update any JavaScript that currently queries or attaches the click
handler to the SVG to target the button with id "UserDetail_close" (no extra
keyboard handler required since a button is natively keyboard-operable).
In `@cms/server/admin/templates/training_program_combined_ranking.html`:
- Around line 256-259: Do not overwrite maxPossibleScore when guarding against
division-by-zero; preserve its display value (e.g., displayMaxPossibleScore =
maxPossibleScore) and introduce a separate safe value (e.g.,
safeMaxPossibleScore = maxPossibleScore === 0 ? 1 : maxPossibleScore) for bucket
math and any divisions. Update the places referenced (the guard around
maxPossibleScore and the later bucket calculations) to use safeMaxPossibleScore
for computations and displayMaxPossibleScore for any UI text like "Max
possible", so the UI still shows 0 while math uses 1 for safety.
In `@cms/server/util.py`:
- Around line 301-340: The code assumes cached_scores[task.id] always exists
which can KeyError if cached_scores and student_tasks drift; change the lookup
in the loop to use cached_scores.get(task.id, 0.0) (or another sensible default)
and use that value for best_score/total_score and in the task_info["score"];
maintain existing behavior for include_task_details and submission_counts but
guard against None by using the .get default so no hard failure occurs in the
StudentTask loop (functions/variables to update: cached_scores lookup,
best_score, total_score accumulation inside the loop that iterates student_tasks
and references task.id).
♻️ Duplicate comments (1)
cms/server/admin/handlers/student.py (1)
696-757: Assignment validation in per-task submissions is solid.
🧹 Nitpick comments (2)
cms/server/admin/templates/student_tasks.html (1)
88-93: Consider addingdata-sourceattribute for consistent sorting.The Source column (index 1) uses
textContentextraction in the sorting script, while other columns use dedicateddata-*attributes. For consistency and to avoid issues with whitespace or nested HTML, consider adding adata-sourceattribute.♻️ Suggested improvement
<tr data-task="{{ st.task.name }}" data-assigned="{{ st.assigned_at.timestamp() if st.assigned_at else 0 }}" data-training-score="{{ training_scores.get(st.task.id, -1) }}" data-home-score="{{ home_scores.get(st.task.id, -1) }}" - data-submissions="{{ submission_counts.get(st.task.id, 0) }}"> + data-submissions="{{ submission_counts.get(st.task.id, 0) }}" + data-source="{{ st.source_training_day.contest.description if st.source_training_day and st.source_training_day.contest else (st.source_training_day.description or st.source_training_day.name or 'Archived Training') if st.source_training_day else 'Manual Assignment' }}">Then update the sorting script:
} else if (columnIndex === 1) { // Source - aVal = a.cells[columnIndex].textContent.trim().toLowerCase(); - bVal = b.cells[columnIndex].textContent.trim().toLowerCase(); + aVal = a.getAttribute('data-source').toLowerCase(); + bVal = b.getAttribute('data-source').toLowerCase();cms/server/admin/handlers/student.py (1)
175-291: Consider batching user + participation lookups to avoid N+1 queries.
For large uploads, a singleINquery for users and existing participations will cut DB round-trips substantially. This aligns with batch patterns already used throughout the handlers.♻️ Proposed refactor (batch queries)
- results = [] - students_added = 0 - - for username in usernames: - user = self.sql_session.query(User).filter( - User.username == username).first() + results = [] + students_added = 0 + + users = ( + self.sql_session.query(User) + .filter(User.username.in_(usernames)) + .all() + ) + user_by_username = {u.username: u for u in users} + user_ids = [u.id for u in users] + existing_participation_user_ids = set() + if user_ids: + existing_participation_user_ids = { + user_id + for (user_id,) in ( + self.sql_session.query(Participation.user_id) + .filter(Participation.contest == managing_contest) + .filter(Participation.user_id.in_(user_ids)) + .all() + ) + } + + for username in usernames: + user = user_by_username.get(username) if user is None: results.append({ "username": username, "status": "not_found", "message": "Username does not exist in the system" }) else: - 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_participation_user_ids: results.append({ "username": username, "status": "already_exists", "message": "User is already a student in this program" })
| // Guard against division by zero when maxPossibleScore is 0 | ||
| if (maxPossibleScore === 0) { | ||
| maxPossibleScore = 1; // Avoid division by zero | ||
| } |
There was a problem hiding this comment.
Preserve display max when maxPossibleScore is 0.
The division-by-zero guard sets maxPossibleScore to 1, which then shows “Max possible: 1” even if the real max is 0. Consider keeping a separate display value and only using a safe max for bucket math.
🛠️ Proposed fix
- if (maxPossibleScore === 0) {
- maxPossibleScore = 1; // Avoid division by zero
- }
+ var displayMaxPossibleScore = maxPossibleScore;
+ if (maxPossibleScore === 0) {
+ maxPossibleScore = 1; // Avoid division by zero
+ }
...
- ' | <strong>Max possible:</strong> ' + Math.round(maxPossibleScore) +
+ ' | <strong>Max possible:</strong> ' + Math.round(displayMaxPossibleScore) +Also applies to: 363-366
🤖 Prompt for AI Agents
In `@cms/server/admin/templates/training_program_combined_ranking.html` around
lines 256 - 259, Do not overwrite maxPossibleScore when guarding against
division-by-zero; preserve its display value (e.g., displayMaxPossibleScore =
maxPossibleScore) and introduce a separate safe value (e.g.,
safeMaxPossibleScore = maxPossibleScore === 0 ? 1 : maxPossibleScore) for bucket
math and any divisions. Update the places referenced (the guard around
maxPossibleScore and the later bucket calculations) to use safeMaxPossibleScore
for computations and displayMaxPossibleScore for any UI text like "Max
possible", so the UI still shows 0 while math uses 1 for safety.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Split unsolved tasks into attempted/not attempted and add sortable submissions column
Summary
This PR makes several improvements to the training program task management:
Task Archive (CWS - training_program_overview.html):
Admin Student Tasks (AWS - student_tasks.html):
Home Task Scores Page (AWS - ranking.html via TrainingProgramRankingHandler):
Sidebar Navigation (AWS - base.html):
Both task archive features use efficient batch queries to fetch submission counts rather than N+1 queries.
Updates since last revision
Styling fixes and template refactoring:
training_program_combined_ranking.htmltoaws_tp_styles.cssparticipation_detail.htmlandtraining_program_combined_ranking_detail.htmlto share common code via new fragments:fragments/user_detail_styles.html- shared CSS stylesfragments/user_detail_layout.html- shared HTML structurefragments/user_detail_scripts.html- shared JavaScript utilities withinitUserDetail()functionPrevious updates (still included):
aws_style.csstoaws_tp_styles.cssStudentTaskSubmissionsHandlerget_cached_score_entry()Review & Testing Checklist for Human
Recommended test plan:
Notes
Summary by CodeRabbit
New Features
UI Updates
Style
✏️ Tip: You can customize this high-level summary in your review settings.