Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions app/api/tutorials_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
class TutorialsApi < Grape::API
helpers AuthenticationHelpers
helpers AuthorisationHelpers
helpers MimeCheckHelpers

before do
authenticated?
Expand Down Expand Up @@ -102,4 +103,38 @@ class TutorialsApi < Grape::API
tutorial.destroy!
present true, with: Grape::Presenters::Presenter
end

desc 'Upload CSV with tutorials'
params do
requires :file, type: File, desc: 'CSV upload file.'
end
post '/csv/tutorials/upload' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the endpoint be an extension to the unit entity? So that the unit_id is used from the endpoint and not required in the CSV

It would allow the exact same CSV to be imported into two different units

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand why this is necessary. Based on my understanding, two different units cannot have the same tutorials. Will there be a situation where multiple units would have the exact same tutorials given that the information provided in the csv is tutorial specific and tutorials are unit specific?

unless authorise? current_user, User, :upload_csv
error!({ error: 'Not authorised to upload CSV of students of tutorials' }, 403)
end

if params[:file].blank?
error!({ error: 'No file uploaded' }, 403)
end

if params[:file][:tempfile].size > 5.megabytes
error!({ error: 'CSV file size exceeds the 5MB limit' }, 413)
end

path = params[:file][:tempfile].path
Tutorial.import_from_csv(File.new(path))
end

desc 'Download CSV with tutorials'
get '/csv/tutorials/download' do
unless authorise? current_user, User, :download_tutorial_csv
error!({ error: 'Not authorised to download CSV of all tutorials' }, 403)
end

content_type 'application/octet-stream'
header['Content-Disposition'] = 'attachment; filename=tutorials.csv'
header['Access-Control-Expose-Headers'] = 'Content-Disposition'
env['api.format'] = :binary
Tutorial.export_to_csv
end
end
111 changes: 111 additions & 0 deletions app/models/tutorial.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
require 'csv_helper'
require 'csv'
class Tutorial < ApplicationRecord
include CsvHelper
# Model associations
belongs_to :unit, optional: false # Foreign key
belongs_to :unit_role, optional: true # Foreign key
Expand Down Expand Up @@ -73,6 +76,114 @@ def num_students
projects.where('enrolled = true').count
end

def self.missing_headers(row, headers)
headers - row.to_hash.keys
end

def self.csv_columns
%w[code abbreviation unit_id tutor_id tutorial_stream]
end

def self.import_from_csv(file)
success = []
errors = []
ignored = []
data = FileHelper.read_file_to_str(file).gsub('\\n', "\n")

ActiveRecord::Base.transaction do
CSV.parse(data,
headers: true,
header_converters: [->(i) { i.nil? ? '' : i }, :downcase, ->(hdr) { hdr.strip unless hdr.nil? }],
converters: [->(body) { body.encode!('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '') unless body.nil? }]).each do |row|
missing = missing_headers(row, csv_columns)
if missing.count > 0
errors << { row: row, message: "Missing headers: #{missing.join(', ')}" }
raise StandardError, "Critical import error: missing headers"
end

tutorial_code = row['code'].strip unless row['code'].nil?
abbreviation = row['abbreviation'].strip unless row['abbreviation'].nil?
unit_id = row['unit_id'].strip unless row['unit_id'].nil?
user_id = row['tutor_id'].strip unless row['tutor_id'].nil?
tutorial_stream_name = row['tutorial_stream'].strip unless row['tutorial_stream'].nil?

# find unit role using tutor's user_id and unit_id
unit_role = UnitRole.find_by(user_id: user_id, unit_id: unit_id)

if unit_role.nil?
errors << { row: row, message: "Tutor with user_id (#{user_id}) not found in unit roles" }
raise StandardError, 'Critical import error: missing unit role'
end

# if tutorial is found set it, otherwise leave it blank
tutorial_stream = TutorialStream.find_by(name: tutorial_stream_name) if tutorial_stream_name.present?

# handle missing tutorial stream
tutorial_stream = nil if tutorial_stream_name.blank?

# create a new tutorial
tutorial = Tutorial.new(
unit_id: unit_id,
code: tutorial_code,
abbreviation: abbreviation,
unit_role_id: unit_role.id,
tutorial_stream_id: tutorial_stream&.id
)

if tutorial.save
success << { row: row, message: "Created tutorial #{abbreviation} #{unit_id}" }
else
errors << { row: row, message: "Failed to create tutorial #{abbreviation} #{unit_id}" }
raise StandardError, 'Critical import error: failed to save tutorial'
end
rescue StandardError => e
raise ActiveRecord::Rollback
end

raise ActiveRecord::Rollback unless errors.empty?
end

{
success: success,
ignored: ignored,
errors: errors
}
end

def self.export_to_csv
exportables = %w[code abbreviation unit_id unit_role_id tutorial_stream_id]

# Generate the CSV file
CSV.generate do |csv|
# Add header row
csv << Tutorial.attribute_names.select { |attribute| exportables.include? attribute }.map do |attribute|
if attribute == 'tutorial_stream_id'
'tutorial_stream'
elsif attribute == 'unit_role_id'
'tutor_id'
else
attribute
end
end

# Add data rows
Tutorial.order('id').each do |tutorial|
csv << tutorial.attributes.select { |attribute| exportables.include? attribute }.map do |key, value|
# make the values more readable
if key == 'tutorial_stream_id'
stream_name = TutorialStream.find_by(id: value)&.name
stream_name
elsif key == 'unit_role_id'
tutor_id = UnitRole.find_by(id: value)&.user_id
tutor_id
else
value
end
end
end
end
end

private

def can_destroy?
Expand Down
1 change: 1 addition & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ def self.permissions
:upload_csv,
:download_system_csv,
:download_unit_csv,
:download_tutorial_csv,

:create_unit,
:admin_units,
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@
mount ApiRoot => '/'
mount GrapeSwaggerRails::Engine => '/api/docs'
mount Sidekiq::Web => "/sidekiq" # mount Sidekiq::Web in your Rails app
mount TutorialsApi => '/api'
end
100 changes: 100 additions & 0 deletions test/models/tutorial_model_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,104 @@ def test_unit_inconsistency_raises_error
assert tutorial.invalid?
assert_equal 'Unit should be same as the unit in the associated tutorial stream', tutorial.errors.full_messages.last
end

def test_import_from_valid_csv
# Setup
unit = FactoryBot.create(:unit)
tutor_user = FactoryBot.create(:user, role_id: 3) # role_id 3 for tutor
unit_role = FactoryBot.create(:unit_role, user: tutor_user, unit: unit) # Create the unit_role
tutorial_stream = FactoryBot.create(:tutorial_stream, unit: unit)

file = Tempfile.new('tutorials.csv')
begin
# Writing the test CSV data
file.write("code,abbreviation,unit_id,tutor_id,tutorial_stream\n")
file.write("CODE123,ABBR,#{unit.id},#{tutor_user.id},#{tutorial_stream.name}\n")
file.rewind

# Perform the CSV import
result = Tutorial.import_from_csv(file)

# Assert the import was successful
assert_equal 1, result[:success].length, result[:errors].map { |e| e[:message] }.join(", ")
assert_empty result[:errors], "There should be no errors"

# Check if the tutorial is correctly created
tutorial = Tutorial.find_by(code: 'CODE123')
assert_not_nil tutorial, "Tutorial should be created"
assert_equal 'ABBR', tutorial.abbreviation, "Tutorial abbreviation should match"
assert_equal unit.id, tutorial.unit_id, "Tutorial unit should match"

# Assert that the tutorial's unit_role_id matches the created unit_role
assert_equal unit_role.id, tutorial.unit_role_id, "Tutorial's unit_role_id should match the created unit_role"
ensure
file.close
file.unlink
end
end

def test_import_with_missing_headers
# Prepare a CSV content with a missing header (`tutor_id` is missing)
csv_content = <<~CSV
code,unit_id
TUT102,1
CSV
# Create a temporary file with the CSV content
file = create_tempfile(csv_content)

result = Tutorial.import_from_csv(file)

# Assert that no tutorials were successfully imported
assert_equal 0, result[:success].size

# Assert that one error was raised due to the missing header
assert_equal 1, result[:errors].size

# Assert that the error message contains 'Missing headers', indicating that the import failed
assert_match /Missing headers/, result[:errors].first[:message]
ensure
file.close
file.unlink
end

def test_export_to_csv
# Ensure no tutorials exist before starting the test
Tutorial.delete_all

# Create necessary objects
unit = FactoryBot.create(:unit)
unit_role = FactoryBot.create(:unit_role, unit: unit, role_id: 3) # Ensure this is a tutor role
tutorial_stream = FactoryBot.create(:tutorial_stream, unit: unit)

# Create the tutorial, associating it with the unit_role and tutorial_stream
tutorial = Tutorial.create(
meeting_day: "Monday",
meeting_time: "17:30",
meeting_location: "ATC101",
unit: unit,
unit_role: unit_role,
tutorial_stream: tutorial_stream,
abbreviation: "ABBR",
code: "CODE123"
)

# Export tutorials to CSV
csv = Tutorial.export_to_csv
data = CSV.parse(csv, headers: true)

# Check that the exported CSV contains the tutorial that was created
assert_includes data.map { |row| row['code'] }, tutorial.code, "Expected tutorial code to be included in the export"
assert_includes data.map { |row| row['abbreviation'] }, tutorial.abbreviation, "Expected tutorial abbreviation to be included in the export"
assert_includes data.map { |row| row['unit_id'] }, tutorial.unit_id.to_s, "Expected tutorial unit_id to be included in the export"
assert_includes data.map { |row| row['tutor_id'] }, tutorial.unit_role.user_id.to_s, "Expected tutor_id to be included in the export"
end

private

def create_tempfile(content)
file = Tempfile.new(['test_csv', '.csv'])
file.write(content)
file.rewind
file
end
end
3 changes: 3 additions & 0 deletions test_files/csv_test_files/Tutorials.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
code,abbreviation,unit_id,tutor_id,tutorial_stream
test,CS3,4,8,
,FX1,1,2,Practical-1
Comment on lines +1 to +3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use the name of the tutor instead of their ID to make things easier if any changes to the CSV need to be made before importing?

This would require the tutor to already be added as a unit role to the unit, and throw a warning for each row that has a tutor name that isn't part of the unit

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do that but isn't it much better to use ID since it is a unique identifier? If two tutors have the same name or if a tutor has two accounts with the same name but different IDs, how would we know which one to choose? I feel like it is bad practice to use name, which is not a primary key to identify a user, correct me if I'm wrong :)