re-organized course import tarball convention#2310
Open
lmaosoggypancakes wants to merge 2 commits intomasterfrom
Open
re-organized course import tarball convention#2310lmaosoggypancakes wants to merge 2 commits intomasterfrom
lmaosoggypancakes wants to merge 2 commits intomasterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughChanges update course YAML file naming convention from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/controllers/submissions_controller.rb (1)
72-72:⚠️ Potential issue | 🔴 Critical
computed_scorecall with block syntax will raiseArgumentErrorafter the signature change.The
computed_scoremethod inApplicationHelperwas changed from a yield-based interface to requiringvalueas the first positional parameter (seeapplication_helper.rbLine 111). These call sites still use the old block-based invocation with no positional arguments:# Line 72 total = computed_score { submission.final_score(cud) } # Line 75 submission_info[index]["late_penalty"] = computed_score { submission.late_penalty(cud) }Both will fail at runtime with
ArgumentError: wrong number of arguments (given 0, expected 1..3).Proposed fix
- total = computed_score { submission.final_score(cud) } + total = computed_score(submission.final_score(cud))- submission_info[index]["late_penalty"] = computed_score { submission.late_penalty(cud) } + submission_info[index]["late_penalty"] = computed_score(submission.late_penalty(cud))Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/submissions_controller.rb` at line 72, The call sites using block-style invocation of computed_score (e.g., total = computed_score { submission.final_score(cud) } and submission_info[index]["late_penalty"] = computed_score { submission.late_penalty(cud) }) must be changed to the new positional API: pass the computed value as the first argument instead of a block — e.g., total = computed_score(submission.final_score(cud)) and submission_info[index]["late_penalty"] = computed_score(submission.late_penalty(cud)); if you need to supply the optional second/third parameters supported by computed_score, pass them as additional positional arguments.app/controllers/courses_controller.rb (1)
1305-1334:⚠️ Potential issue | 🟠 MajorRemoving the depth check allows any
.ymlfile in the archive to be treated as the course config.Line 1308 comments out the guard
entry.file? && entry.full_name.count('/') == 1, and Line 1310 only checks the file extension. This means a.ymlfile nested deep in an assessment directory (e.g.,course/assessments/lab1/config.yml) could be picked up as the course configuration, leading to incorrect or failed imports.At minimum, restore the
entry.file?check and add a filename check:Proposed fix
tar_extract.each do |entry| - # next unless entry.file? && entry.full_name.count('/') == 1 - # there should only be one file in the main directory with .yml extension - next unless File.extname(entry.full_name) == '.yml' + # Only match the course config YAML at the expected path + next unless entry.file? && File.basename(entry.full_name) == 'course_config.yml'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/courses_controller.rb` around lines 1305 - 1334, Restore the original guard in get_course_from_config so only top-level files are considered: require entry.file? and that entry.full_name.count('/') == 1, and also add a filename check (e.g., File.basename(entry.full_name) == 'config.yml' or the expected course filename) before accepting the .yml; update the conditional that currently reads next unless File.extname(entry.full_name) == '.yml' to include these checks so nested YAMLs (like in assessment subfolders) are ignored.app/helpers/application_helper.rb (1)
111-117:⚠️ Potential issue | 🔴 CriticalSignature change breaks 11 call sites across the codebase — update all callers to the new parameter-based API.
The method signature was changed from yield-based to parameter-based (
valueas first positional arg), but existing callers were not migrated. This breaks runtime execution in multiple locations:Block-only calls (missing required
valueparameter →ArgumentError):
- app/views/submissions/index.html.erb:205
- app/views/gradebooks/student.html.erb:133, 158
- app/views/assessments/_submission_history_row.html.erb:81
- app/controllers/submissions_controller.rb:72, 75
- app/helpers/application_helper.rb:132
Mixed calls (positional URL arg + ignored block →
NoMethodErrorwhen calling.round(1)on string):
- app/views/assessments/viewGradesheet.html.erb:61, 63, 69
- app/helpers/application_helper.rb:127
Additionally, the
rescue ScoreComputationExceptionclause is now dead code — noyieldexists to raise it, and.round(1)on numeric won't throw this exception.All 11 call sites must be updated to pass computed values (not blocks) as the first argument, or the method signature must be reverted to support blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/application_helper.rb` around lines 111 - 117, The computed_score helper signature was changed from block-based to require a first positional value (computed_score(value, link = nil, nil_to_dash = true)) which breaks 11 call sites that still call it with a block or pass a URL as the first arg; update those call sites to supply the precomputed numeric value as the first argument and pass the link as the second argument (e.g. replace computed_score { ... } with computed_score(computed_value, link) and replace computed_score(link) where link was positional with computed_score(computed_value, link)); alternatively, revert computed_score to accept a block again (restore yield-based behavior) if you prefer the old API, and if keeping the new API remove or adjust the rescue ScoreComputationException in computed_score (or move exception handling to where the value is computed) since .round(1) will not raise that exception under the new contract.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/courses_controller.rb`:
- Line 1411: Fix the malformed and unsafe flash message: add the missing space
between "got" and "expected" and stop inserting unescaped user-derived values
into HTML by removing or not relying on flash[:html_safe]; instead HTML-escape
course_name and course_yml_path when building flash[:error] (e.g. use
ERB::Util.html_escape or CGI.escapeHTML on course_name and course_yml_path) so
the message uses the escaped values and reads "got <escaped_course_name>
expected <escaped_course_yml_path>" while avoiding setting flash[:html_safe].
- Around line 89-122: Remove the debug artifacts left in the manage action:
delete the commented-out Rails.logger.silence do and its matching `#end` around
the GradeMatrix/statistics block in the manage action (the code that builds
`@course_stats` using GradeMatrix and Statistics), and also remove the render-time
<h3> debug element from manage.html.erb; ensure the logic that constructs cols
and `@course_stats` (using GradeMatrix.new, matrix.has_assessment?,
matrix.cells_for_assessment, matrix.averages_for_category,
matrix.course_averages, and Statistics#stats) remains intact and unchanged aside
from removing the commented logger wrapper and stray HTML debug header.
- Around line 241-250: The commented-out begin/rescue removed error handling
around tar_extract.rewind, get_course_from_config(tar_extract) and
save_assessments_from_tar(tar_extract); restore protection by adding a
begin/rescue (or equivalent) around those calls to catch StandardError (or more
specific exceptions), set flash[:error] with the exception message,
render(action: "new") && return on error, and ensure tar_extract.close runs in
an ensure block so the tar is closed regardless of success or failure; reference
the existing methods get_course_from_config, save_assessments_from_tar,
tar_extract.rewind and the flash/render behavior when implementing the fix.
- Around line 1396-1397: Replace the incorrect double-nested path and filename
check: set course_yml_path to "#{course_name}/#{course_name}.yml" (instead of
File.join(course_name, course_name, "course_config.yml")) and update the boolean
assignment using that variable (course_yml_exists = course_yml_exists ||
pathname == course_yml_path); also update the error message around the later
check (referenced near the original line 1411) to mention "#{course_name}.yml"
(the actual filename produced by generate_tar in Course#generate_tar) and ensure
this logic is consistent with save_assessments_from_tar which expects a single
course_name/… nesting.
- Around line 1396-1397: The assignment of course_yml_path inside the per-entry
loop is redundant; move the computation of course_yml_path =
File.join(course_name, course_name, "course_config.yml") out of the loop so it
is calculated once after course_name has been determined (or after the loop
finishes), and keep the loop only setting course_yml_exists = course_yml_exists
|| pathname == course_yml_path (or compare against the precomputed path); update
references to course_yml_path accordingly in the surrounding code in
courses_controller.rb.
- Around line 1305-1334: Restore the original guard in get_course_from_config so
only top-level files are considered: require entry.file? and that
entry.full_name.count('/') == 1, and also add a filename check (e.g.,
File.basename(entry.full_name) == 'config.yml' or the expected course filename)
before accepting the .yml; update the conditional that currently reads next
unless File.extname(entry.full_name) == '.yml' to include these checks so nested
YAMLs (like in assessment subfolders) are ignored.
In `@app/controllers/submissions_controller.rb`:
- Around line 43-44: The current code forces every submission to score 0 by
using `@submissions.index_with` { 0 }, which can hide missing/nil scores; change
this to map to nil (e.g., `@submissions.index_with` { nil }) or only set scores
for submissions you actually have grades for so missing scores remain nil, then
update any view logic that reads `@submission_scores` to handle nil vs 0
explicitly (treat nil as "not graded" and 0 as a graded zero). Also either
remove the commented-out `@submission_styles` line or restore it with a short TODO
comment explaining its purpose (e.g., why ignored_submission_style(s) would be
needed) so intent is clear; update references to `@submission_scores` and
ignored_submission_style in the view/controller accordingly.
- Line 72: The call sites using block-style invocation of computed_score (e.g.,
total = computed_score { submission.final_score(cud) } and
submission_info[index]["late_penalty"] = computed_score {
submission.late_penalty(cud) }) must be changed to the new positional API: pass
the computed value as the first argument instead of a block — e.g., total =
computed_score(submission.final_score(cud)) and
submission_info[index]["late_penalty"] =
computed_score(submission.late_penalty(cud)); if you need to supply the optional
second/third parameters supported by computed_score, pass them as additional
positional arguments.
In `@app/helpers/application_helper.rb`:
- Around line 111-117: The computed_score helper signature was changed from
block-based to require a first positional value (computed_score(value, link =
nil, nil_to_dash = true)) which breaks 11 call sites that still call it with a
block or pass a URL as the first arg; update those call sites to supply the
precomputed numeric value as the first argument and pass the link as the second
argument (e.g. replace computed_score { ... } with
computed_score(computed_value, link) and replace computed_score(link) where link
was positional with computed_score(computed_value, link)); alternatively, revert
computed_score to accept a block again (restore yield-based behavior) if you
prefer the old API, and if keeping the new API remove or adjust the rescue
ScoreComputationException in computed_score (or move exception handling to where
the value is computed) since .round(1) will not raise that exception under the
new contract.
In `@app/models/github_integration.rb`:
- Around line 166-167: Replace the use of the low-precedence `or` operator in
the boolean guard with the idiomatic `||` operator to avoid precedence
surprises: update the conditional in the GitHub integration guard (the return
false if Rails.configuration.x.github.client_id.blank? or
Rails.configuration.x.github.client_secret.blank? line inside the
GithubIntegration class/method) to use `||` between the two blank? checks so
both boolean expressions are evaluated with normal precedence.
In `@app/views/courses/manage.html.erb`:
- Line 2: Remove the debug render-time measurement from the template: delete the
Time.current assignment (start_time) and the corresponding display block that
computes and shows render duration (the <h3> element that subtracts start_time
from Time.current), so neither the timing variable nor the HTML that exposes
render_time remains in manage.html.erb; search for the start_time assignment and
the render-time <h3> (or the expression Time.current - start_time) and remove
both occurrences.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/courses_controller.rb`: - Around line 1396-1397: The assignment of course_yml_path inside the per-entry loop is redundant; move the computation of course_yml_path = File.join(course_name, course_name, "course_config.yml") out of the loop so it is calculated once after course_name has been determined (or after the loop finishes), and keep the loop only setting course_yml_exists = course_yml_exists || pathname == course_yml_path (or compare against the precomputed path); update references to course_yml_path accordingly in the surrounding code in courses_controller.rb. In `@app/controllers/submissions_controller.rb`: - Around line 43-44: The current code forces every submission to score 0 by using `@submissions.index_with` { 0 }, which can hide missing/nil scores; change this to map to nil (e.g., `@submissions.index_with` { nil }) or only set scores for submissions you actually have grades for so missing scores remain nil, then update any view logic that reads `@submission_scores` to handle nil vs 0 explicitly (treat nil as "not graded" and 0 as a graded zero). Also either remove the commented-out `@submission_styles` line or restore it with a short TODO comment explaining its purpose (e.g., why ignored_submission_style(s) would be needed) so intent is clear; update references to `@submission_scores` and ignored_submission_style in the view/controller accordingly. In `@app/models/github_integration.rb`: - Around line 166-167: Replace the use of the low-precedence `or` operator in the boolean guard with the idiomatic `||` operator to avoid precedence surprises: update the conditional in the GitHub integration guard (the return false if Rails.configuration.x.github.client_id.blank? or Rails.configuration.x.github.client_secret.blank? line inside the GithubIntegration class/method) to use `||` between the two blank? checks so both boolean expressions are evaluated with normal precedence.app/controllers/courses_controller.rb (1)
1396-1397:course_yml_pathis reassigned on every non-root entry iteration.
course_yml_pathis set inside theelsebranch that runs for every file/directory entry (not just the root). Since it only depends oncourse_name, this redundant reassignment is harmless but wasteful and obscures intent. Move it outside the loop, aftercourse_nameis determined — or compute it once after the loop ends.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/courses_controller.rb` around lines 1396 - 1397, The assignment of course_yml_path inside the per-entry loop is redundant; move the computation of course_yml_path = File.join(course_name, course_name, "course_config.yml") out of the loop so it is calculated once after course_name has been determined (or after the loop finishes), and keep the loop only setting course_yml_exists = course_yml_exists || pathname == course_yml_path (or compare against the precomputed path); update references to course_yml_path accordingly in the surrounding code in courses_controller.rb.app/controllers/submissions_controller.rb (1)
43-44: Initializing all scores to 0 may mask missing/nil scores.
@submissions.index_with { 0 }maps every submission to0. Verify the index view actually uses@submission_scoresand that a default of0(rather thannil) is the correct semantic — a student with no graded scores is different from a student who scored0.Also, the commented-out line on Line 44 should either be removed or restored with a TODO explaining intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/submissions_controller.rb` around lines 43 - 44, The current code forces every submission to score 0 by using `@submissions.index_with` { 0 }, which can hide missing/nil scores; change this to map to nil (e.g., `@submissions.index_with` { nil }) or only set scores for submissions you actually have grades for so missing scores remain nil, then update any view logic that reads `@submission_scores` to handle nil vs 0 explicitly (treat nil as "not graded" and 0 as a graded zero). Also either remove the commented-out `@submission_styles` line or restore it with a short TODO comment explaining its purpose (e.g., why ignored_submission_style(s) would be needed) so intent is clear; update references to `@submission_scores` and ignored_submission_style in the view/controller accordingly.app/models/github_integration.rb (1)
166-167: Prefer||overorfor boolean conditions.While functionally correct here,
orhas different (lower) precedence than||in Ruby and is generally reserved for control-flow idioms. Using||is the idiomatic choice for boolean expressions and avoids subtle precedence bugs in other contexts.Proposed fix
- return false if Rails.configuration.x.github.client_id.blank? or - Rails.configuration.x.github.client_secret.blank? + return false if Rails.configuration.x.github.client_id.blank? || + Rails.configuration.x.github.client_secret.blank?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/github_integration.rb` around lines 166 - 167, Replace the use of the low-precedence `or` operator in the boolean guard with the idiomatic `||` operator to avoid precedence surprises: update the conditional in the GitHub integration guard (the return false if Rails.configuration.x.github.client_id.blank? or Rails.configuration.x.github.client_secret.blank? line inside the GithubIntegration class/method) to use `||` between the two blank? checks so both boolean expressions are evaluated with normal precedence.
Comment on lines
+89
to
+122
| # Rails.logger.silence do | ||
| matrix = GradeMatrix.new @course, @cud | ||
| cols = {} | ||
| # extract assessment final scores | ||
| @course.assessments.each do |asmt| | ||
| next unless matrix.has_assessment? asmt.id | ||
|
|
||
| cells = matrix.cells_for_assessment asmt.id | ||
| final_scores = cells.map { |c| c["final_score"] } | ||
| cols[asmt.name] = ["asmt", asmt, final_scores] | ||
| end | ||
|
|
||
| # category averages | ||
| @course.assessment_categories.each do |cat| | ||
| next unless matrix.has_category? cat | ||
| # category averages | ||
| @course.assessment_categories.each do |cat| | ||
| next unless matrix.has_category? cat | ||
|
|
||
| cols["#{cat} Average"] = ["avg", nil, matrix.averages_for_category(cat)] | ||
| end | ||
| cols["#{cat} Average"] = ["avg", nil, matrix.averages_for_category(cat)] | ||
| end | ||
|
|
||
| # course averages | ||
| cols["Course Average"] = ["avg", nil, matrix.course_averages] | ||
|
|
||
| # calculate statistics | ||
| # send course_stats back in the form of | ||
| # name of average / assesment -> [type, asmt, statistics] | ||
| # where type = "asmt" or "avg" (assessment or average) | ||
| # asmt = assessment object or nil if an average of category / class | ||
| # statistics (statistics pertaining to asmt/avg (mean, median, std dev, etc)) | ||
| @course_stats = {} | ||
| stat = Statistics.new | ||
| cols.each do |key, values| | ||
| @course_stats[key] = [values[0], values[1], stat.stats(values[2])] | ||
| end | ||
| # course averages | ||
| cols["Course Average"] = ["avg", nil, matrix.course_averages] | ||
|
|
||
| # calculate statistics | ||
| # send course_stats back in the form of | ||
| # name of average / assesment -> [type, asmt, statistics] | ||
| # where type = "asmt" or "avg" (assessment or average) | ||
| # asmt = assessment object or nil if an average of category / class | ||
| # statistics (statistics pertaining to asmt/avg (mean, median, std dev, etc)) | ||
| @course_stats = {} | ||
| stat = Statistics.new | ||
| cols.each do |key, values| | ||
| @course_stats[key] = [values[0], values[1], stat.stats(values[2])] | ||
| end | ||
| #end |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug artifacts from manage action.
The commented-out Rails.logger.silence do (Line 89) and #end (Line 122) are debug leftovers. Along with the render-time <h3> in manage.html.erb, these suggest temporary profiling code that shouldn't be merged.
Proposed fix
- # Rails.logger.silence do
- matrix = GradeMatrix.new `@course`, `@cud`
+ matrix = GradeMatrix.new `@course`, `@cud`
...
- `#end`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Rails.logger.silence do | |
| matrix = GradeMatrix.new @course, @cud | |
| cols = {} | |
| # extract assessment final scores | |
| @course.assessments.each do |asmt| | |
| next unless matrix.has_assessment? asmt.id | |
| cells = matrix.cells_for_assessment asmt.id | |
| final_scores = cells.map { |c| c["final_score"] } | |
| cols[asmt.name] = ["asmt", asmt, final_scores] | |
| end | |
| # category averages | |
| @course.assessment_categories.each do |cat| | |
| next unless matrix.has_category? cat | |
| # category averages | |
| @course.assessment_categories.each do |cat| | |
| next unless matrix.has_category? cat | |
| cols["#{cat} Average"] = ["avg", nil, matrix.averages_for_category(cat)] | |
| end | |
| cols["#{cat} Average"] = ["avg", nil, matrix.averages_for_category(cat)] | |
| end | |
| # course averages | |
| cols["Course Average"] = ["avg", nil, matrix.course_averages] | |
| # calculate statistics | |
| # send course_stats back in the form of | |
| # name of average / assesment -> [type, asmt, statistics] | |
| # where type = "asmt" or "avg" (assessment or average) | |
| # asmt = assessment object or nil if an average of category / class | |
| # statistics (statistics pertaining to asmt/avg (mean, median, std dev, etc)) | |
| @course_stats = {} | |
| stat = Statistics.new | |
| cols.each do |key, values| | |
| @course_stats[key] = [values[0], values[1], stat.stats(values[2])] | |
| end | |
| # course averages | |
| cols["Course Average"] = ["avg", nil, matrix.course_averages] | |
| # calculate statistics | |
| # send course_stats back in the form of | |
| # name of average / assesment -> [type, asmt, statistics] | |
| # where type = "asmt" or "avg" (assessment or average) | |
| # asmt = assessment object or nil if an average of category / class | |
| # statistics (statistics pertaining to asmt/avg (mean, median, std dev, etc)) | |
| @course_stats = {} | |
| stat = Statistics.new | |
| cols.each do |key, values| | |
| @course_stats[key] = [values[0], values[1], stat.stats(values[2])] | |
| end | |
| #end | |
| matrix = GradeMatrix.new `@course`, `@cud` | |
| cols = {} | |
| # extract assessment final scores | |
| `@course.assessments.each` do |asmt| | |
| next unless matrix.has_assessment? asmt.id | |
| cells = matrix.cells_for_assessment asmt.id | |
| final_scores = cells.map { |c| c["final_score"] } | |
| cols[asmt.name] = ["asmt", asmt, final_scores] | |
| end | |
| # category averages | |
| `@course.assessment_categories.each` do |cat| | |
| next unless matrix.has_category? cat | |
| cols["#{cat} Average"] = ["avg", nil, matrix.averages_for_category(cat)] | |
| end | |
| # course averages | |
| cols["Course Average"] = ["avg", nil, matrix.course_averages] | |
| # calculate statistics | |
| # send course_stats back in the form of | |
| # name of average / assesment -> [type, asmt, statistics] | |
| # where type = "asmt" or "avg" (assessment or average) | |
| # asmt = assessment object or nil if an average of category / class | |
| # statistics (statistics pertaining to asmt/avg (mean, median, std dev, etc)) | |
| `@course_stats` = {} | |
| stat = Statistics.new | |
| cols.each do |key, values| | |
| `@course_stats`[key] = [values[0], values[1], stat.stats(values[2])] | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/courses_controller.rb` around lines 89 - 122, Remove the
debug artifacts left in the manage action: delete the commented-out
Rails.logger.silence do and its matching `#end` around the GradeMatrix/statistics
block in the manage action (the code that builds `@course_stats` using GradeMatrix
and Statistics), and also remove the render-time <h3> debug element from
manage.html.erb; ensure the logic that constructs cols and `@course_stats` (using
GradeMatrix.new, matrix.has_assessment?, matrix.cells_for_assessment,
matrix.averages_for_category, matrix.course_averages, and Statistics#stats)
remains intact and unchanged aside from removing the commented logger wrapper
and stray HTML debug header.
Comment on lines
+241
to
+250
| # begin | ||
| tar_extract.rewind | ||
| @newCourse = get_course_from_config(tar_extract) | ||
| # save assessment directories | ||
| save_assessments_from_tar(tar_extract) | ||
| tar_extract.close | ||
| rescue StandardError => e | ||
| flash[:error] = "Error while extracting course to server -- #{e.message}." | ||
| render(action: "new") && return | ||
| end | ||
| #rescue StandardError => e | ||
| #flash[:error] = "Error while extracting course to server -- #{e.message}." | ||
| # render(action: "new") && return | ||
| #end |
Contributor
There was a problem hiding this comment.
Commented-out rescue block removes error handling for course extraction.
The begin/rescue that previously caught errors from get_course_from_config and save_assessments_from_tar is commented out (and the begin on Line 241 is also commented). If either method raises, the user will see a raw 500 error instead of a helpful flash message. This is a regression from the previous behavior.
Either restore the error handling or replace it with equivalent protection.
Proposed fix
-# begin
+ begin
tar_extract.rewind
`@newCourse` = get_course_from_config(tar_extract)
# save assessment directories
save_assessments_from_tar(tar_extract)
tar_extract.close
- `#rescue` StandardError => e
- `#flash`[:error] = "Error while extracting course to server -- #{e.message}."
- # render(action: "new") && return
- `#end`
+ rescue StandardError => e
+ flash[:error] = "Error while extracting course to server -- #{e.message}."
+ render(action: "new") && return
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # begin | |
| tar_extract.rewind | |
| @newCourse = get_course_from_config(tar_extract) | |
| # save assessment directories | |
| save_assessments_from_tar(tar_extract) | |
| tar_extract.close | |
| rescue StandardError => e | |
| flash[:error] = "Error while extracting course to server -- #{e.message}." | |
| render(action: "new") && return | |
| end | |
| #rescue StandardError => e | |
| #flash[:error] = "Error while extracting course to server -- #{e.message}." | |
| # render(action: "new") && return | |
| #end | |
| begin | |
| tar_extract.rewind | |
| `@newCourse` = get_course_from_config(tar_extract) | |
| # save assessment directories | |
| save_assessments_from_tar(tar_extract) | |
| tar_extract.close | |
| rescue StandardError => e | |
| flash[:error] = "Error while extracting course to server -- #{e.message}." | |
| render(action: "new") && return | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/courses_controller.rb` around lines 241 - 250, The
commented-out begin/rescue removed error handling around tar_extract.rewind,
get_course_from_config(tar_extract) and save_assessments_from_tar(tar_extract);
restore protection by adding a begin/rescue (or equivalent) around those calls
to catch StandardError (or more specific exceptions), set flash[:error] with the
exception message, render(action: "new") && return on error, and ensure
tar_extract.close runs in an ensure block so the tar is closed regardless of
success or failure; reference the existing methods get_course_from_config,
save_assessments_from_tar, tar_extract.rewind and the flash/render behavior when
implementing the fix.
Comment on lines
+1396
to
+1397
| course_yml_path = File.join(course_name, course_name, "course_config.yml") | ||
| course_yml_exists = course_yml_exists || pathname == course_yml_path |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the problematic code location first
echo "=== Code at lines 1396-1397 in courses_controller.rb ==="
sed -n '1390,1405p' app/controllers/courses_controller.rb
echo ""
echo "=== Code around line 1339 (save_assessments_from_tar) ==="
sed -n '1330,1360p' app/controllers/courses_controller.rb
echo ""
echo "=== search for generate_tar method ==="
rg -n 'def generate_tar' --type=ruby -A 30
echo ""
echo "=== Look for course_config.yml references ==="
rg -n 'course_config\.yml' --type=ruby -B 2 -A 2
echo ""
echo "=== Look for course_yml or course.yml references ==="
rg -n 'course\.yml|course_yml' --type=ruby -B 2 -A 2
echo ""
echo "=== Search for export methods ==="
rg -n 'def export' --type=ruby -A 15Repository: autolab/Autolab
Length of output: 9859
Fix both the duplicated course_name and incorrect filename in the config path check.
The code at line 1396 has two issues:
- Duplicates
course_namein the path (double nesting) - Looks for
course_config.yml, but the actual config filename generated bygenerate_tar(app/models/course.rb:360) is#{course_name}.yml
The generate_tar method creates the structure: course_name/course_name.yml, not course_name/course_name/course_config.yml. This is consistent with save_assessments_from_tar (line 1339), which expects single nesting at course_name/assessments/....
The error message at line 1411 is also incorrect and should reference the actual expected path.
Proposed fix
- course_yml_path = File.join(course_name, course_name, "course_config.yml")
+ course_yml_path = File.join(course_name, "#{course_name}.yml")Also update line 1411 error message to reference the correct path: course_name.yml instead of course_config.yml.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| course_yml_path = File.join(course_name, course_name, "course_config.yml") | |
| course_yml_exists = course_yml_exists || pathname == course_yml_path | |
| course_yml_path = File.join(course_name, "#{course_name}.yml") | |
| course_yml_exists = course_yml_exists || pathname == course_yml_path |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/courses_controller.rb` around lines 1396 - 1397, Replace the
incorrect double-nested path and filename check: set course_yml_path to
"#{course_name}/#{course_name}.yml" (instead of File.join(course_name,
course_name, "course_config.yml")) and update the boolean assignment using that
variable (course_yml_exists = course_yml_exists || pathname == course_yml_path);
also update the error message around the later check (referenced near the
original line 1411) to mention "#{course_name}.yml" (the actual filename
produced by generate_tar in Course#generate_tar) and ensure this logic is
consistent with save_assessments_from_tar which expects a single course_name/…
nesting.
| flash[:error] = "Errors found in tarball:" | ||
| if !course_yml_exists && !course_name.nil? | ||
| flash[:error] += "<br>Course yml file #{course_name}/#{course_name}.yml was not found" | ||
| flash[:error] += "<br>Course yml file #{course_name}/course_config.yml was not found: got " + course_name + "expected " + course_yml_path |
Contributor
There was a problem hiding this comment.
Missing space in error message and XSS risk from user-derived content.
The string concatenation is missing a space before "expected":
"got " + course_name + "expected " + course_yml_pathThis renders as "got mycoursexpected mycoursemycourse/course_config.yml".
Additionally, course_name is derived from the tarball's directory name (user-supplied input) and is interpolated into flash[:error] with flash[:html_safe] = true set on Line 1413. This is an XSS vector — a malicious tarball could set a directory name containing <script> tags.
Proposed fix
- flash[:error] += "<br>Course yml file #{course_name}/course_config.yml was not found: got " + course_name + "expected " + course_yml_path
+ flash[:error] += "<br>Course yml file not found. Expected #{CGI.escapeHTML(course_yml_path.to_s)}, got #{CGI.escapeHTML(course_name.to_s)}"Based on learnings: "The user confirmed the concern about potential security risks when setting flash[:error] and flash[:html_safe] with user-supplied input, specifically mentioning course_name derived from a tar file's contents."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/courses_controller.rb` at line 1411, Fix the malformed and
unsafe flash message: add the missing space between "got" and "expected" and
stop inserting unescaped user-derived values into HTML by removing or not
relying on flash[:html_safe]; instead HTML-escape course_name and
course_yml_path when building flash[:error] (e.g. use ERB::Util.html_escape or
CGI.escapeHTML on course_name and course_yml_path) so the message uses the
escaped values and reads "got <escaped_course_name> expected
<escaped_course_yml_path>" while avoiding setting flash[:html_safe].
| @@ -1,5 +1,5 @@ | |||
| <% @title = "Manage Course" %> | |||
|
|
|||
| <% start_time = Time.current %> | |||
Contributor
There was a problem hiding this comment.
Remove debug render-time measurement from production template.
Lines 2 and 163–164 add an <h3> element displaying the ERB render time directly in the page HTML. This exposes internal performance metrics to all users (including students) and looks like leftover debug instrumentation. It should be removed before merging.
Proposed fix
<% `@title` = "Manage Course" %>
- <% start_time = Time.current %>
<% content_for :stylesheets do %>-<h3>rendered in <%= ((Time.current - start_time) * 1000).round(2) %> ms</h3>
-
</div>Also applies to: 163-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/courses/manage.html.erb` at line 2, Remove the debug render-time
measurement from the template: delete the Time.current assignment (start_time)
and the corresponding display block that computes and shows render duration (the
<h3> element that subtracts start_time from Time.current), so neither the timing
variable nor the HTML that exposes render_time remains in manage.html.erb;
search for the start_time assignment and the render-time <h3> (or the expression
Time.current - start_time) and remove both occurrences.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The current format for tarballs to import is messy and confusing, especially when importing multiple courses.
Description
As it stood, the location and name for config files depend on the course name, making parsing messy and export/import courses harder than it should be. Config files should be the same name for all courses, just located in different folders, rather than having different names.
How Has This Been Tested?
tested on importing exported courses
Types of changes
Other issues / help required
might need testing on courses with the same name to make sure there's a collision
If unsure, feel free to submit first and we'll help you along.