Autograder improvements #2209
Autograder improvements #2209
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve enhancements to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
Outside diff range and nitpick comments (2)
app/controllers/autograders_controller.rb (2)
95-95: Consistency: Redirect to the autograder edit page on errorWhen a file is not found, it would be more consistent to redirect back to the autograder edit page instead of the general assessment edit page.
Apply this diff to fix the inconsistency:
flash[:error] = "File not found" - redirect_to(edit_course_assessment_path(@course, @assessment)) + redirect_to(edit_course_assessment_autograder_path(@course, @assessment))
109-111: Style: Insert an empty line between method definitionsAccording to the Ruby style guide, there should be an empty line between method definitions for better readability.
Apply this diff:
end + def assessment_params params[:autograder][:assessment].permit(:disable_network) endTools
rubocop
[convention] 109-109: Expected 1 empty line between method definitions; found 0.
(Layout/EmptyLineBetweenDefs)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/controllers/autograders_controller.rb (4 hunks)
- app/views/assessments/_edit_advanced.html.erb (0 hunks)
- app/views/autograders/_form.html.erb (1 hunks)
- config/routes.rb (1 hunks)
Files not reviewed due to no reviewable changes (1)
- app/views/assessments/_edit_advanced.html.erb
Additional context used
rubocop
app/controllers/autograders_controller.rb
[convention] 41-41: Use
&&instead ofand.(Style/AndOr)
[convention] 109-109: Expected 1 empty line between method definitions; found 0.
(Layout/EmptyLineBetweenDefs)
GitHub Check: CodeQL
app/controllers/autograders_controller.rb
[failure] 91-91: Uncontrolled data used in path expression
This path depends on a user-provided value.
Additional comments not posted (4)
app/views/autograders/_form.html.erb (3)
29-31: Consolidation of 'Disable Network' Setting Enhances UsabilityMoving the
disable_networkcheckbox into the Autograder settings section aligns related settings and improves the user experience by having all autograder configurations in one place.
43-45: Verify Security of File Download LinksAs with the Makefile download link, ensure that
@tar_existsused in thefile_pathparameter is properly sanitized to prevent unauthorized file access.Run the following script to verify that
@tar_existsis properly sanitized:#!/bin/bash # Description: Verify that @tar_exists is assigned only with allowed file paths. # Test: Search for the assignment of @tar_exists in the controller and check the paths. # Expected: @tar_exists is assigned safely and cannot be manipulated by users. rg --type ruby '@tar_exists\s*='
39-41: Verify Security of File Download LinksPassing
file_path: @makefile_existsdirectly to thedownload_file_course_assessment_autograder_pathmay introduce security risks if@makefile_existscan be manipulated to access unauthorized files. Ensure that thefile_pathparameter is properly validated and restricted to allowed directories.Run the following script to verify that
@makefile_existsis properly sanitized:config/routes.rb (1)
142-144: LGTMThe addition of the
download_fileaction within theautograderresource is appropriate and aligns with the goal of allowing users to download autograder files directly from the UI.
|
Would it be possible to reduce the spacing and align the new text more? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
app/views/autograders/_form.html.erb (1)
15-16: Consider inlining the help text variables.These variables are only used once each. Inlining them would reduce unnecessary complexity without sacrificing readability.
- <% help_tar_text = "Tar file exists, upload a file to override." %> - <% help_makefile_text = "Makefile exists, upload a file to override." %>app/assets/stylesheets/style.css.scss (1)
962-965: LGTM! Consider documenting the !important override.The spacing adjustments for file input fields look good and address the UI feedback about reducing spacing. However, it would be helpful to add a comment explaining why the
!importantflag is necessary here to override other styles.div.file-field.input-field { + /* Override default materialize spacing for file inputs */ padding-top: 0; margin-top: -0.5rem !important; }app/controllers/autograders_controller.rb (2)
4-4: Consider removing unnecessaryrequire 'pathname'Rails applications usually have
Pathnameavailable by default viaRails.root, so explicitly requiring'pathname'may be redundant.
Line range hint
41-47: Consider wrapping updates in a database transactionTo ensure that updates to
@autograderand@assessmentare atomic, consider wrapping them inside a database transaction. This will maintain data integrity if one update succeeds and the other fails.Here's how you might implement it:
def update ActiveRecord::Base.transaction do if @autograder.update(autograder_params) && @assessment.update(assessment_params) flash[:success] = "Autograder saved." begin upload rescue StandardError flash[:error] = "Autograder could not be uploaded." raise ActiveRecord::Rollback end else flash[:error] = "Autograder could not be saved.<br>" flash[:error] += @autograder.errors.full_messages.join("<br>") flash[:error] += @assessment.errors.full_messages.join("<br>") flash[:html_safe] = true raise ActiveRecord::Rollback end end redirect_to(edit_course_assessment_autograder_path(@course, @assessment)) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- app/assets/stylesheets/style.css.scss (1 hunks)
- app/controllers/autograders_controller.rb (4 hunks)
- app/form_builders/form_builder_with_date_time_input.rb (2 hunks)
- app/views/autograders/_form.html.erb (1 hunks)
🧰 Additional context used
📓 Learnings (1)
app/controllers/autograders_controller.rb (2)
Learnt from: KesterTan PR: autolab/Autolab#2209 File: app/controllers/autograders_controller.rb:32-36 Timestamp: 2024-09-19T06:35:55.985Z Learning: In the `AutogradersController#edit` method, passing the file paths to the view is necessary to send the files in `download_file`.Learnt from: evanyeyeye PR: autolab/Autolab#2209 File: app/controllers/autograders_controller.rb:0-0 Timestamp: 2024-09-20T20:46:52.775Z Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
🔇 Additional comments (8)
app/views/autograders/_form.html.erb (3)
17-18: LGTM! File upload fields effectively implement the requested indicators.The implementation successfully addresses the PR objective of indicating file existence through the new attributes.
23-25: LGTM! Network access setting successfully moved to autograder form.The implementation effectively addresses the PR objective of consolidating autograder settings by moving the network access control from advanced settings.
33-35: Verify authorization checks for file downloads.While the download functionality meets the PR objectives, ensure proper authorization checks are in place in the controller's download_file action.
Consider improving button spacing.
Based on the UI feedback from evanyeyeye, consider removing the empty line between download buttons and adjusting their layout for better visual alignment.
<% unless @makefile_exists.nil? %> <%= link_to "Download Makefile", download_file_course_assessment_autograder_path(file_path: @makefile_exists, file_key: 'makefile'), class: "btn" %> <% end %> - <% unless @tar_exists.nil? %> <%= link_to "Download Tar", download_file_course_assessment_autograder_path(file_path: @tar_exists, file_key: 'tar'), class: "btn" %> <% end %>Also applies to: 37-39
✅ Verification successful
Let me verify the authorization checks by searching for any before filters or authentication mechanisms in the controller.
Authorization checks for file downloads are properly implemented
The
download_fileaction is well-secured with multiple layers of protection:
- Restricted to instructor-level access via
action_auth_level :download_file, :instructor- Protected by
before_actionfilters that ensure course and assessment context- Uses a whitelist approach with
allowed_fileshash to prevent path traversal- Validates file existence before sending
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for download_file action implementation ast-grep --pattern 'def download_file $$$ end'Length of output: 1745
Script:
#!/bin/bash # Search for before_action filters in the autograders controller ast-grep --pattern 'class AutogradersController < $_ $$$ end' # Also search for any authentication related code rg "before_" app/controllers/autograders_controller.rbLength of output: 9553
app/form_builders/form_builder_with_date_time_input.rb (2)
30-31: LGTM! Clean formatting improvement.The parameter alignment enhances code readability while maintaining functionality.
86-94: Implementation aligns well with PR objectives.The conditional logic successfully implements the requirement to indicate whether an autograder file exists, improving the user experience as intended in issue #2198.
app/controllers/autograders_controller.rb (3)
32-37: LGTM! Theeditmethod enhancements are appropriateThe additions correctly set up instance variables needed for the view to display the existence of the Makefile and tar file. This allows instructors to see if the files are present and provides the necessary paths for downloading.
88-106: LGTM! Thedownload_filemethod is secure and well-implementedThe
download_fileaction safely restricts downloads to predefined files and checks for file existence before sending. Usingaction_auth_level :download_file, :instructorensures that only instructors can access this action.
119-120: LGTM! Proper handling of potentially missing parametersThe
assessment_paramsmethod safely handles cases where parameters might be missing by usingfetchwith default empty hashes. This prevents potentialNoMethodErrorexceptions due tonilvalues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
config/routes.rb (1)
143-145: Consider adding route documentation.To improve maintainability, consider adding comments above the
autograderresource block to document:
- The types of files that can be downloaded (Makefile, tar file)
- The authorization requirements
- The relationship with existing file management routes
Add documentation like this:
# Handles autograder configuration and file management # GET /courses/:name/assessments/:name/autograder/download_file # - Allows instructors to download autograder files (Makefile, tar) # - Requires course_assistant or higher permissions resource :autograder, except: [:new, :show] do get "download_file" end
* Autograder improvements * Linting * Sanitized path * Reduce code duplication * Fixed spacing and added auth check for method (cherry picked from commit 342cc0f)
Description
This resolves #2198.
1. UI now indicates whether an autograder tar and makefile exists
2. UI now provides an option to download the existing autograder tar and makefile
3. Disable network access is now shifted from advanced settings to autograde settings.
Note that this does not appear when there is no file being selected, and the buttons to download should not appear as well.
Motivation and Context
This consolidates all autograder settings into a single place. This also minimizes confusion that a person has when uploading autograder tar and makefiles. This provides a fast and easy way for an instructor to check if they have uploaded a makefile or tar file and to also check the contents of the tar and makefile. Instructors will also no longer have to navigate out of autograder settings to disable or enable network access which is an autograder setting.
How Has This Been Tested?
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting