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.

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

Add rake tasks to un/archive a course #6342

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
6c5a6b4
archive: add rake tasks to un/archive a course
mishaschwartz Nov 16, 2022
d289a94
archive: code cleanup and documentation
mishaschwartz Nov 17, 2022
65a991a
archive: move code to testable location in lib
mishaschwartz Nov 30, 2022
4ad872f
archive: make file storage locations dynamic so they can be overwritten
mishaschwartz Dec 5, 2022
cee37a9
archive: fix order of attribute assignment and file creation
mishaschwartz Dec 6, 2022
c8e1683
archive: further updates to unarchiver
mishaschwartz Dec 19, 2022
cc6cbd3
Revert "peer_review: remove unnecessary peer_review_id foreign key th…
mishaschwartz Dec 20, 2022
dab15be
archiver: remove validation since some validations are acting as call…
mishaschwartz Dec 20, 2022
44e020d
archive: add ability to remove files associated with model subclasses
mishaschwartz Dec 20, 2022
b57f2e5
Revert "Revert "peer_review: remove unnecessary peer_review_id foreig…
mishaschwartz Dec 20, 2022
44ca73c
spec: update tests
mishaschwartz Dec 20, 2022
4e09f47
spec: update tests
mishaschwartz Jan 3, 2023
7dcd859
changelog: update
mishaschwartz Jan 3, 2023
a918f68
model: add inverse relationship between courses and autotest_settings
mishaschwartz Jan 4, 2023
5b2baed
spec: update tests
mishaschwartz Jan 4, 2023
7f14e5c
archive: update unarchive script to handle git repositories
mishaschwartz Jan 6, 2023
6442ee9
exam templates: use foreign keys to discover file path, not associations
mishaschwartz Jan 13, 2023
cf7d0a5
spec: add tests that check if error messages are written to stderr
mishaschwartz Jan 13, 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
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Changelog

## [unreleased]
- Allow admins to archive, unarchive, and delete a course (#6342)
- Fix bug where error was raised when viewing jupyter notebooks if python is not available (#6418)

## [v2.2.0]
Expand Down
44 changes: 24 additions & 20 deletions app/helpers/automated_tests_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@ def fill_in_schema_data!(schema_data, files, assignment)

def autotest_settings_for(assignment)
test_specs = assignment.autotest_settings&.deep_dup || {}
test_specs['testers']&.each do |tester_specs|
test_group_ids = tester_specs['test_data'] || []
tester_specs['test_data'] = assignment.test_groups.where(id: test_group_ids).order(:position).map(&:to_json)
test_specs['testers']&.each_with_index do |tester_specs, i|
tester_specs['test_data'] = assignment.test_groups
.where(tester_index: i)
.order(:position)
.map(&:to_json)
end
test_specs
end
Expand All @@ -48,35 +50,37 @@ def update_test_groups_from_specs(assignment, test_specs)
test_group_ids = []

test_group_position = 1
test_specs['testers']&.each do |tester_specs|
current_test_group_ids = []
test_specs['testers']&.each_with_index do |tester_specs, i|
tester_specs['test_data']&.each do |test_group_specs|
test_group_specs['extra_info'] ||= {}
extra_data_specs = test_group_specs['extra_info']
test_group_id = extra_data_specs['test_group_id']
display_output = extra_data_specs['display_output'] || TestGroup.display_outputs.keys.first
test_group_name = extra_data_specs['name'] || TestGroup.model_name.human
criterion_name = extra_data_specs['criterion']
criterion = assignment.ta_criteria.find_by(name: criterion_name)
if criterion_name.present? && criterion.nil?
flash_message(:warning, I18n.t('automated_tests.no_criteria', name: criterion_name))
end
fields = { name: test_group_name, display_output: display_output,
criterion_id: criterion&.id, autotest_settings: test_group_specs,
position: test_group_position }
attrs = {
name: test_group_name,
display_output: display_output,
criterion_id: criterion&.id,
tester_index: i,
position: test_group_position,
autotest_settings: test_group_specs.except('extra_info')
}
test_group_position += 1
if test_group_id.nil?
test_group = assignment.test_groups.create!(fields)
test_group_id = test_group.id
extra_data_specs['test_group_id'] = test_group_id # update specs to contain new id
else
test_group = assignment.test_groups.find(test_group_id)
test_group.update!(fields)
end
test_group_ids << test_group_id
current_test_group_ids << test_group_id
old_test_group = assignment.test_groups.find_by(attrs.except(:autotest_settings))
identical = old_test_group&.autotest_settings == attrs[:autotest_settings]
test_group_ids << if identical
old_test_group.id
else
new_test_group = assignment.test_groups.create(attrs)
new_test_group.id
end
end
tester_specs['test_data'] = current_test_group_ids
assignment.test_groups.where.not(id: test_group_ids).update!(tester_index: nil)
tester_specs.delete('test_data')
end

# Save test specs
Expand Down
4 changes: 2 additions & 2 deletions app/lib/git_repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -417,8 +417,8 @@ def self.update_permissions_file(permissions)
end

# Create auth csv file
FileUtils.mkdir_p(File.dirname(Repository::PERMISSION_FILE))
CSV.open(Repository::PERMISSION_FILE, 'wb') do |csv|
FileUtils.mkdir_p(File.dirname(Repository.permission_file))
CSV.open(Repository.permission_file, 'wb') do |csv|
csv.flock(File::LOCK_EX)
begin
permissions.each do |repo_name, users|
Expand Down
9 changes: 7 additions & 2 deletions app/lib/repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,13 @@ class Permission
end
end

ROOT_DIR = (Settings.file_storage.repos || File.join(Settings.file_storage.default_root_path, 'repos')).freeze
PERMISSION_FILE = File.join(ROOT_DIR, '.access').freeze
def self.root_dir
Settings.file_storage.repos || File.join(Settings.file_storage.default_root_path, 'repos')
end

def self.permission_file
File.join(self.root_dir, '.access')
end

# Exceptions for repositories
class ConnectionError < StandardError; end
Expand Down
22 changes: 3 additions & 19 deletions app/models/assignment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,6 @@ class Assignment < Assessment
:display_median_to_students, :group_name_autogenerated,
:is_hidden, :vcs_submit, :has_peer_review].freeze

STARTER_FILES_DIR = (
Settings.file_storage.starter_files || File.join(Settings.file_storage.default_root_path, 'starter_files')
).freeze

# Set the default order of assignments: in ascending order of date (due_date)
default_scope { order(:due_date, :id) }

Expand Down Expand Up @@ -907,10 +903,6 @@ def create_peer_review_assignment_if_not_exist

### REPO ###

def starter_file_path
File.join(STARTER_FILES_DIR, self.id.to_s)
end

def default_starter_file_group
default = starter_file_groups.find_by(id: self.default_starter_file_group_id)
default.nil? ? starter_file_groups.order(:id).first : default
Expand Down Expand Up @@ -962,9 +954,11 @@ def each_group_repo(&block)
### /REPO ###

def autotest_path
File.join(TestRun::SETTINGS_FILES_DIR, self.id.to_s)
File.join(TestRun.settings_files_dir, self.id.to_s)
end

alias _file_location autotest_path

def autotest_files_dir
File.join(autotest_path, TestRun::FILES_DIR)
end
Expand All @@ -980,11 +974,6 @@ def autotest_files
end.compact
end

def scanned_exams_path
dir = Settings.file_storage.scanned_exams || File.join(Settings.file_storage.default_root_path, 'scanned_exams')
Rails.root.join(File.join(dir, self.id.to_s))
end

# Retrieve current grader data.
def current_grader_data
ta_counts = self.criterion_ta_associations.group(:ta_id).count
Expand Down Expand Up @@ -1261,11 +1250,6 @@ def zip_automated_test_files(user)
def automated_test_config_to_zip(zip_file, zip_dir, specs_file_path)
self.add_test_files_to_zip(zip_file, zip_dir)
test_specs = autotest_settings_for(self)
test_specs['testers']&.each do |tester_info|
tester_info['test_data']&.each do |test_info|
test_info['extra_info']&.delete('test_group_id')
end
end
zip_file.get_output_stream(specs_file_path) do |f|
f.write(test_specs.to_json)
end
Expand Down
6 changes: 6 additions & 0 deletions app/models/autotest_setting.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
class AutotestSetting < ApplicationRecord
include AutomatedTestsHelper::AutotestApi

has_many :courses

validates :url, presence: true
before_create :register_autotester

# Column name used to identify this record if the primary identifier (id) cannot be relied on.
# For example, when unarchiving courses.
IDENTIFIER = 'url'.freeze

private

def register_autotester
Expand Down
2 changes: 1 addition & 1 deletion app/models/course.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class Course < ApplicationRecord
has_many :tags, through: :roles
has_many :exam_templates, through: :assignments
has_many :lti_deployments
belongs_to :autotest_setting, optional: true
belongs_to :autotest_setting, optional: true, inverse_of: :courses

validates :name, presence: true
validates :name, uniqueness: true
Expand Down
5 changes: 4 additions & 1 deletion app/models/exam_template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,12 @@ def fix_error(filename, exam_num, page_num, upside_down)
end

def base_path
File.join self.assignment.scanned_exams_path, self.id.to_s
dir = Settings.file_storage.scanned_exams || File.join(Settings.file_storage.default_root_path, 'scanned_exams')
Rails.root.join(File.join(dir, self.assessment_id.to_s, self.id.to_s))
end

alias _file_location base_path

def file_path
File.join(base_path, 'exam_template.pdf')
end
Expand Down
11 changes: 10 additions & 1 deletion app/models/group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,16 @@ def build_repository
end

def repo_path
File.join(Repository::ROOT_DIR, self.repository_relative_path)
File.join(Repository.root_dir, self.repository_relative_path)
end

# Git repos are stored on disk but not other types of repo (ie. memory)
if Settings.repository.type == 'git'
def _file_location
return if self.course.nil?

Repository.get_class.respond_to?(:bare_path) ? Repository.get_class.bare_path(repo_path) : repo_path
end
end

# Yields a repository object, if possible, and closes it after it is finished
Expand Down
3 changes: 3 additions & 0 deletions app/models/lti_line_item.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
class LtiLineItem < ApplicationRecord
belongs_to :assessment
belongs_to :lti_deployment
has_one :course, through: :assessment

validates :assessment, uniqueness: { scope: :lti_deployment_id }
validate :courses_should_match
end
1 change: 1 addition & 0 deletions app/models/lti_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ class LtiService < ApplicationRecord
belongs_to :lti_deployment
validates :service_type, inclusion: { in: LTI_SERVICES.values }
validates :service_type, uniqueness: { scope: :lti_deployment_id }
has_one :course, through: :lti_deployment
end
8 changes: 7 additions & 1 deletion app/models/starter_file_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ class StarterFileGroup < ApplicationRecord
validates :entry_rename, presence: { if: -> { self.use_rename } }

def path
Pathname.new(assignment.starter_file_path) + id.to_s
Pathname.new(self.class.starter_files_dir) + self.assessment_id.to_s + id.to_s
end

alias _file_location path

def self.starter_files_dir
Settings.file_storage.starter_files || File.join(Settings.file_storage.default_root_path, 'starter_files')
end

def files_and_dirs
Expand Down
1 change: 0 additions & 1 deletion app/models/test_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ def to_json(*_args)
result['extra_info'] = {
'name' => name,
'display_output' => display_output,
'test_group_id' => id,
'criterion' => criterion&.name
}
result
Expand Down
6 changes: 4 additions & 2 deletions app/models/test_run.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ class TestRun < ApplicationRecord
validate :autotest_test_id_uniqueness
before_save :unset_autotest_test_id

SETTINGS_FILES_DIR = (Settings.file_storage.autotest || File.join(Settings.file_storage.default_root_path,
'autotest')).freeze
SPECS_FILE = 'specs.json'.freeze
FILES_DIR = 'files'.freeze

Expand Down Expand Up @@ -63,6 +61,10 @@ def self.all_test_categories
[Instructor.name.downcase, Student.name.downcase]
end

def self.settings_files_dir
Settings.file_storage.autotest || File.join(Settings.file_storage.default_root_path, 'autotest')
end

private

def extra_info_string(result)
Expand Down
4 changes: 4 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ class User < ApplicationRecord
AUTHENTICATE_BAD_PLATFORM = 'bad_platform'.freeze
AUTHENTICATE_BAD_CHAR = 'bad_char'.freeze

# Column name used to identify this record if the primary identifier (id) cannot be relied on.
# For example, when unarchiving courses.
IDENTIFIER = 'user_name'.freeze

# Authenticates login against its password
# through a script specified by Settings.validate_file
def self.authenticate(login, password, ip: nil)
Expand Down
29 changes: 29 additions & 0 deletions db/migrate/20221114170146_add_tester_index_to_test_groups.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
class AddTesterIndexToTestGroups < ActiveRecord::Migration[7.0]
def up
add_column :test_groups, :tester_index, :integer, default: nil
puts '-- converting test group ids in json string to tester index'
Assignment.joins(:assignment_properties)
.where.not('assignment_properties.autotest_settings': nil)
.find_each do |assignment|
test_specs = assignment.autotest_settings&.deep_dup || {}
test_specs['testers']&.each_with_index do |tester_specs, i|
test_group_ids = tester_specs['test_data'] || []
assignment.test_groups.where(id: test_group_ids).update_all(tester_index: i)
end
assignment.update!(autotest_settings: test_specs)
end
end
def down
puts '-- converting tester index in json string to test group ids'
Assignment.joins(:assignment_properties)
.where.not('assignment_properties.autotest_settings': nil)
.find_each do |assignment|
test_specs = assignment.autotest_settings&.deep_dup || {}
test_specs['testers']&.each_with_index do |tester_specs, i|
tester_specs['test_data'] = assignment.test_groups.where(tester_index: i).ids
end
assignment.update!(autotest_settings: test_specs)
end
remove_column :test_groups, :tester_index
end
end
6 changes: 3 additions & 3 deletions db/seeds.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
exit unless Rails.env.development?

# clean up existing files first
FileUtils.rm_rf(Repository::ROOT_DIR)
FileUtils.rm_rf(Assignment::STARTER_FILES_DIR)
FileUtils.rm_rf(TestRun::SETTINGS_FILES_DIR)
FileUtils.rm_rf(Repository.root_dir)
FileUtils.rm_rf(Assignment.starter_files_dir)
FileUtils.rm_rf(TestRun.settings_files_dir)

FileUtils.mkdir_p('tmp')

Expand Down
4 changes: 3 additions & 1 deletion db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -2139,7 +2139,8 @@ CREATE TABLE public.test_groups (
updated_at timestamp without time zone NOT NULL,
assessment_id bigint NOT NULL,
autotest_settings json DEFAULT '{}'::json NOT NULL,
"position" integer NOT NULL
"position" integer NOT NULL,
tester_index integer
);


Expand Down Expand Up @@ -4689,5 +4690,6 @@ INSERT INTO "schema_migrations" (version) VALUES
('20220922131809'),
('20221019191315'),
('20221111182002'),
('20221114170146'),
('20221219204837'),
('20230109190029');
Loading