Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent failing import from file system from deleting assessment files #1945

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Prevent failing import from file system from deleting assessment files #1945

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57e3201
avoid deleting assessment files on non-tarball based imports
20wildmanj Jul 18, 2023
769055f
rubocop
20wildmanj Jul 18, 2023
d59842c
Address PR comments, rename isTarImport param to be cleanup_on_failure
20wildmanj Jul 25, 2023
3841b5c
Merge branch 'master' into joeywildman-fix-import-assessment-deletion
20wildmanj Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 32 additions & 8 deletions app/controllers/assessments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,21 @@ def install_assessment
next
end

# each assessment must have an associated yaml file
unless File.exist?(File.join(dir_path, filename, "#{filename}.yml"))
# each assessment must have an associated yaml file,
# and it must have a name field that matches its filename
if File.exist?(File.join(dir_path, filename, "#{filename}.yml"))
props = YAML.safe_load(File.open(
File.join(dir_path, filename, "#{filename}.yml"), "r", &:read
))
unless props["general"] && (props["general"]["name"] == filename)
flash.now[:error] = flash.now[:error] ? "#{flash.now[:error]} <br>" : ""
flash.now[:error] += "An error occurred while trying to display an existing assessment" \
20wildmanj marked this conversation as resolved.
Show resolved Hide resolved
"on file directory #{filename}: Name in yaml (#{props['general']['name']}) " \
"doesn't match #{filename}"
flash.now[:html_safe] = true
next
end
else
flash.now[:error] = flash.now[:error] ? "#{flash.now[:error]} <br>" : ""
flash.now[:error] += "An error occurred while trying to display an existing assessment " \
"on file directory #{filename}: #{filename}.yml does not exist"
Expand Down Expand Up @@ -219,6 +232,7 @@ def importAsmtFromTar
end

params[:assessment_name] = asmt_name
params[:isTarImport] = true
20wildmanj marked this conversation as resolved.
Show resolved Hide resolved
importAssessment && return
end

Expand All @@ -228,6 +242,7 @@ def importAsmtFromTar
action_auth_level :importAssessment, :instructor

def importAssessment
isTarImport = params[:isTarImport]
@assessment = @course.assessments.new(name: params[:assessment_name])
assessment_path = Rails.root.join("courses/#{@course.name}/#{@assessment.name}")
# not sure if this check is 100% necessary anymore, but is a last resort
Expand All @@ -237,8 +252,11 @@ def importAssessment
but assessment file name is #{params[:assessment_name]}"
# destroy model
destroy_no_redirect
# need to delete explicitly b/c the paths don't match
FileUtils.rm_rf(assessment_path)
# delete files explicitly b/c the paths don't match ONLY if
# import was from tarball
if isTarImport
FileUtils.rm_rf(assessment_path)
20wildmanj marked this conversation as resolved.
Show resolved Hide resolved
end
redirect_to(install_assessment_course_assessments_path(@course)) && return
end

Expand All @@ -247,8 +265,11 @@ def importAssessment
rescue StandardError => e
flash[:error] = "Error loading yaml: #{e}"
destroy_no_redirect
# need to delete explicitly b/c the paths don't match
FileUtils.rm_rf(assessment_path)
# delete files explicitly b/c the paths don't match ONLY if
# import was from tarball
if isTarImport
FileUtils.rm_rf(assessment_path)
end
redirect_to(install_assessment_course_assessments_path(@course)) && return
end
@assessment.load_embedded_quiz # this will check and load embedded quiz
Expand All @@ -258,8 +279,11 @@ def importAssessment
rescue StandardError => e
flash[:error] = "Error loading config module: #{e}"
destroy_no_redirect
# need to delete explicitly b/c the paths don't match
FileUtils.rm_rf(assessment_path)
# delete files explicitly b/c the paths don't match ONLY if
# import was from tarball
if isTarImport
FileUtils.rm_rf(assessment_path)
end
redirect_to(install_assessment_course_assessments_path(@course)) && return
end
flash[:success] = "Successfully imported #{@assessment.name}"
Expand Down