Fix training program sidebar issues and add contest configuration #93
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Show training day side menu when viewing a task in an active training day
- Modified TaskHandler.get() to set self.contest to the training day's
contest when the task has an active training day (not archived)
2. Fix Overview and Resource Usage links to stay within training program context
- Changed the condition in base.html to link Overview to the training
program's general page when in training program context
- Resource usage still links to the main resource usage page
3. Add collapsible Training Days submenu in training program sidebar
- Replaced the simple 'Training Days' link with a collapsible section
- Shows all active training days with links to their contest pages
- Includes 'Add a training day' button for admins with permission_all
Co-Authored-By: Ron Ryvchin <[email protected]>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughAdds two training-program redirect handlers and routes; centralizes contest configuration via a new fragment and contest_attrs/set_attrs flow with validation; reinitializes proxy after training-program updates; moves tasks into a training program's managing contest when assigned to a training day; updates multiple admin templates and client-side checks. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI
participant Handler as TrainingProgramHandler
participant DB as Database
participant Proxy as ProxyService
AdminUI->>Handler: POST /training_program/{id} (form with contest attrs)
Handler->>DB: load training_program and managing_contest
Handler->>Handler: build contest_attrs, validate start/stop
Handler->>DB: contest.set_attrs(contest_attrs) and commit
Handler->>Proxy: proxy_service.reinitialize()
Proxy-->>Handler: ack
Handler->>AdminUI: redirect back to training_program page
sequenceDiagram
participant AdminUI as Admin UI
participant AddHandler as AddContestTaskHandler
participant DB as Database
participant TaskModel as Task model
AdminUI->>AddHandler: POST add task to training day (task_id)
AddHandler->>DB: load task, training_program.managing_contest
alt Task not in managing contest
AddHandler->>TaskModel: update task.contest and task.num (move to managing contest)
AddHandler->>DB: save task move
end
AddHandler->>TaskModel: set task.training_day_num, save
AddHandler->>AdminUI: redirect to contest tasks page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. ✨ Finishing touches
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Co-Authored-By: Ron Ryvchin <[email protected]>
… page Co-Authored-By: Ron Ryvchin <[email protected]>
devin-ai-integration
bot
changed the title
- Create fragments/contest_config_fields.html with all contest configuration fields - Update contest.html to use the shared fragment - Update training_program.html to use the shared fragment - Remove tasks and students sections from training program general page (they have their own sections) Co-Authored-By: Ron Ryvchin <[email protected]>
… URLs
When users navigate to /training_program/{id}/overview or
/training_program/{id}/resourceslist, these handlers redirect them to
the managing contest's overview and resourceslist pages respectively.
This fixes 404 errors that occur when users access these URLs directly
or from cached pages.
Co-Authored-By: Ron Ryvchin <[email protected]>
Add /overview and /resourceslist to the list of paths that should not be
redirected from managing contest URLs. This prevents the infinite redirect
loop between /training_program/{id}/overview and /contest/{id}/overview.
Co-Authored-By: Ron Ryvchin <[email protected]>
Update the 'Add a new student' form to use the same TomSelect-based searchable dropdown that is used elsewhere in the system (e.g., adding users to contests, adding tasks). Co-Authored-By: Ron Ryvchin <[email protected]>
- Add searchable-select to 'Select a task to add to this student's task archive' - Add searchable-select to 'Select a task to assign to students' - Update training day task selection to show all available tasks - Tasks not in the training program are marked with '(not in training program)' - Show confirmation dialog when selecting a task not in the training program - Automatically add the task to the training program when confirmed Co-Authored-By: Ron Ryvchin <[email protected]>
Collaborator
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/fragments/contest_config_fields.html`:
- Line 187: Update the documentation link in the template fragment so it uses
HTTPS instead of HTTP: in the tokens parameters header line where the anchor
href currently begins with "http://cms.readthedocs.org/en/{{ rtd_version
}}/Configuring%20a%20contest.html#feedback-to-contestants", change the scheme to
"https://" while keeping the existing rtd_version interpolation and target
attribute intact.
In `@cms/server/admin/templates/training_program.html`:
- Around line 27-32: The post handler is not persisting contest config inputs
rendered by contest_config_fields.html; update TrainingProgramHandler.post() to
read the posted values for allowed_localizations, allow_delay_requests,
allow_unofficial_submission_before_analysis_mode, block_hidden_participations,
allow_password_authentication, allow_registration, ip_restriction, ip_autologin,
per_user_time, and min_submission_interval_grace_period from the request
(respecting their types: lists/booleans/ints/strings), apply them to
training_program.managing_contest (the Contest object manipulated in the
handler) and save/update the contest (persist changes) so user edits in the
login section and other rendered fields are not lost. Ensure you use the same
field names as in the fragment and validate/coerce inputs consistently with
existing Contest handling in TrainingProgramHandler.post().
cms/server/admin/templates/fragments/contest_config_fields.html
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Add handling for the following fields that were rendered by contest_config_fields.html but not being persisted: - allowed_localizations (comma-separated list) - allow_delay_requests (boolean) - allow_unofficial_submission_before_analysis_mode (boolean) - block_hidden_participations (boolean) - allow_password_authentication (boolean) - allow_registration (boolean) - ip_restriction (boolean) - ip_autologin (boolean) - per_user_time (timedelta in seconds) - min_submission_interval_grace_period (timedelta in seconds) These fields are now read from the request and applied to the managing_contest using the same patterns as ContestHandler.post(). Co-Authored-By: Ron Ryvchin <[email protected]>
Collaborator
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/base.html`:
- Around line 496-520: The new sidebar section uses
data-section-key="training-days" but the client-side collapsed/expanded
whitelist (variable/array named validSectionKeys or similar used when hydrating
collapsed state) doesn't include "training-days", causing a flash; update the
validSectionKeys array (or the whitelist used in the hydration logic/function
that reads data-section-key) to include the string "training-days" so the
server-rendered collapsed state is honored on first paint (search for
validSectionKeys, collapsedSections, or the hydration function that references
section keys to locate the exact spot).
In `@cms/server/admin/templates/bulk_assign_task.html`:
- Around line 53-55: The form submits task_id="" which the template now
produces, but BulkAssignTaskHandler.post only rejects the string "null", causing
safe_get_item to potentially raise a 404; update BulkAssignTaskHandler.post to
treat both an empty string ("") and the literal "null" as invalid task
selections (return the same validation error/early return), or alternatively
change the select's placeholder value to "null"—locate the handler method named
BulkAssignTaskHandler.post and add a check like if task_id in ("", "null") to
short-circuit with the validation response before calling safe_get_item.
In `@cms/server/admin/templates/fragments/contest_config_fields.html`:
- Line 187: The external documentation anchor in the template fragment (the <a>
tag linking to "https://cms.readthedocs.io/en/{{ rtd_version
}}/Configuring%20a%20contest.html#feedback-to-contestants") should include
rel="noopener noreferrer" in addition to target="_blank" to prevent tabnabbing;
update that <a> element in contest_config_fields.html to add the rel attribute
while preserving the existing href, target, and surrounding conditional.
cms/server/admin/templates/fragments/contest_config_fields.html
Outdated
Show resolved
Hide resolved
- Auto-set starting_time when adding student to training program (so they can see everything immediately without a start button) - Auto-set starting_time on registration to managing contest (training programs don't have a start button) - Add 'training-days' to validSectionKeys in base.html to prevent flash of expanded menu on page load - Fix BulkAssignTaskHandler.post to reject empty string task_id (in addition to 'null') - Add rel='noopener noreferrer' to documentation link in contest_config_fields.html to prevent tabnabbing Co-Authored-By: Ron Ryvchin <[email protected]>
Collaborator
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/student_tasks.html`:
- Around line 23-25: The AddStudentTaskHandler currently only rejects the string
"null" so empty selections from the template (which submit value="") bypass
validation and cause safe_get_item() to 404; update the validation in
AddStudentTaskHandler to treat both empty string and "null" as invalid (i.e.,
check if task_id in ("", "null")) before calling safe_get_item(), mirroring the
validation used elsewhere in the file.
In `@cms/server/admin/templates/training_program_students.html`:
- Around line 21-23: The form sends an empty string for the placeholder option
but the validation in trainingprogram.py checks for the literal "null"; update
the validation to check for an empty string instead. Locate the handler/function
in trainingprogram.py that reads user_id (the code using assert user_id !=
"null") and change the assertion to assert user_id != "" (or equivalently check
if user_id), mirroring the validation used in contestuser.py to prevent empty
submissions from passing. Ensure any related error messages or control flow
still behave correctly after this change.
♻️ Duplicate comments (3)
cms/server/admin/templates/bulk_assign_task.html (1)
53-55: Server-side validation may still reject only"null", not empty values.If
BulkAssignTaskHandler.poststill checks only"null", the new empty-string placeholder can fall through and end in a 404 instead of a clean validation error.cms/server/admin/templates/training_program.html (1)
27-32: Verify all contest config fields are processed by the handler.The fragment is included with
show_login_section=true, which renders login-related fields. I've verified thatTrainingProgramHandler.post()now processes these fields (lines 153-158 in trainingprogram.py), includingblock_hidden_participations,allow_password_authentication,allow_registration,ip_restriction, andip_autologin.The previous review concern about missing field processing has been addressed.
cms/server/admin/templates/base.html (1)
9-9: Past review concern addressed.The
'training-days'key has been added tovalidSectionKeys, preventing the flash of expanded menus during hydration.
🧹 Nitpick comments (1)
cms/server/admin/templates/contest_tasks.html (1)
15-34: LGTM! Client-side confirmation for non-program tasks is well-implemented.The confirmation logic correctly:
- Parses the task ID and checks against
programTaskIds- Guards against empty selection with
selectedTaskId &&(NaN is falsy)- Prevents form submission if user cancels
Consider using
Array.includes()instead ofindexOf()for slightly cleaner code:Optional modernization
- if (selectedTaskId && programTaskIds.indexOf(selectedTaskId) === -1) { + if (selectedTaskId && !programTaskIds.includes(selectedTaskId)) {
- Fix AddStudentTaskHandler to reject empty string task_id - Fix AddTrainingProgramStudentHandler to check for empty string user_id - Modernize indexOf to includes in contest_tasks.html Co-Authored-By: Ron Ryvchin <[email protected]>
- Create Student record when user registers to training program's managing contest (mirrors AddTrainingProgramStudentHandler behavior) - Also add participations to all existing training days on registration - Hide training day contests from 'Add a new participation' dropdown - Hide managing contests (training programs) from 'Add a new participation' dropdown - Separate training program participations into their own section in user page - Hide training day participations from user page (managed via training program) - Add confirmation dialog for removing regular participations - Training program removal links to 'remove from training program' flow Co-Authored-By: Ron Ryvchin <[email protected]>
Collaborator
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/user.html`:
- Line 26: The onclick confirm call directly injects p.user.username and
p.contest.training_program.name into a JS string; change those interpolations to
use the Jinja2 |tojson filter so the values are safely escaped for JavaScript
(e.g., replace p.user.username and p.contest.training_program.name with their
|tojson forms in the onclick string), matching the safe usage already present on
lines ~100/104 in this template.
🧹 Nitpick comments (3)
cms/server/admin/handlers/user.py (1)
51-76: Participation categorization logic is clear and well-documented.The separation into training program, training day, and regular participations aligns with the PR objectives.
One consideration: accessing
p.contest.training_programandp.contest.training_dayfor each participation may trigger N+1 queries if the relationships aren't eagerly loaded. If user pages become slow with many participations, consider usingjoinedloadorselectinloadon the initial query.cms/server/admin/handlers/trainingprogram.py (2)
143-145: Guard against clearing contest languages unintentionally.If the form submits no languages, this will persist an empty list; please confirm that’s valid for a contest. If not, preserve the existing list or require at least one language.
🔧 Possible guard
- # Programming languages - contest_attrs["languages"] = self.get_arguments("languages") + # Programming languages + selected_languages = self.get_arguments("languages") + if not selected_languages: + raise ValueError("Please select at least one language") + contest_attrs["languages"] = selected_languages
489-506: Avoidassertfor request validation.
assertcan be stripped in optimized Python, which would bypass the check. Prefer an explicit guard.🔧 Suggested change
- user_id: str = self.get_argument("user_id") - assert user_id != "", "Please select a valid user" + user_id: str = self.get_argument("user_id") + if user_id == "": + raise ValueError("Please select a valid user")
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses three issues with the training program sidebar and adds a new feature:
Task sidebar context: When viewing a task that belongs to an active training day, the sidebar now shows the training day's contest menu instead of the training program menu. If the training day is archived (no contest), it falls back to showing the training program sidebar.
Overview and Resource Usage links: The "Overview" and "Resource Usage" links in the training program sidebar now link to the managing contest's overview and resource usage pages, keeping the user within the training program's contest context.
Collapsible Training Days submenu: Replaced the simple "Training Days" menu entry with a collapsible sidebar section (similar to how contests work in the main menu) that lists all active training days and includes an "Add a training day" button for admins.
Managing contest configuration on training program page: The training program general page now displays and allows editing of the managing contest's configuration fields (languages, limits, tokens, times, etc.).
Updates since last revision
student_tasks.html) and "Assign task to students" (bulk_assign_task.html) now use the same TomSelect-based searchable component./overviewand/resourceslistto the list of paths excluded from the managing contest redirect logic inbase.py./training_program/{id}/overviewand/training_program/{id}/resourceslist: These handlers redirect users to the managing contest's overview and resourceslist pages.fragments/contest_config_fields.html)TrainingProgramHandler.post()now correctly reads and saves all fields rendered bycontest_config_fields.html, including:allowed_localizations,allow_delay_requests,allow_unofficial_submission_before_analysis_mode,block_hidden_participations,allow_password_authentication,allow_registration,ip_restriction,ip_autologin,per_user_time, andmin_submission_interval_grace_period.Review & Testing Checklist for Human
allowed_localizations,per_user_time, andmin_submission_interval_grace_periodfields and verify they persist.Recommended test plan:
allowed_localizations(e.g., "en,he") andper_user_time- verify they persistNotes
selectattr("contest")Jinja2 filter is used to identify active training days (those with a contest, not archived)training_program.managing_contest.idto link to the managing contest's pagesLANGUAGES,TOKEN_MODE_DISABLED,TOKEN_MODE_FINITE,TOKEN_MODE_INFINITEto be available in the template contextprepare()method inbase.pynow excludes/overviewand/resourceslistfrom the redirect logic to prevent loopscontesttask.pyautomatically adds the task to the managing contest firstTrainingProgramHandler.post()follows the same patterns asContestHandler.post()for consistencyLink to Devin run: https://app.devin.ai/sessions/a9d8ff324e80435a8266decdbff6e58f
Requested by: Ron Ryvchin (@ronryv)
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.