diff --git a/.gitignore b/.gitignore index 3ef1f7a03a..977065f4e4 100644 --- a/.gitignore +++ b/.gitignore @@ -37,3 +37,7 @@ student-work/ .byebug_history coverage/ _history + +# Institution specific config +config/*_setting.rb +!config/no_institution_setting.rb diff --git a/app/api/lti_api.rb b/app/api/lti_api.rb index 0abdfa116d..7e9bbd4e23 100644 --- a/app/api/lti_api.rb +++ b/app/api/lti_api.rb @@ -71,17 +71,22 @@ class LtiApi < Grape::API error!({ error: "Missing required fields: #{missing.join(', ')}" }, 400) end - if current_user.role_id != Role.student_id - return status 204 - end + # if current_user.role_id != Role.student_id + # return status 204 + # end - role = unit.role_for(current_user) - if !role.nil? && role != Role.student - # error!({ error: 'Failed to enrol, user is already staff.' }, 400) - return status 204 + # role = unit.role_for(current_user) + # if !role.nil? && role != Role.student + # # error!({ error: 'Failed to enrol, user is already staff.' }, 400) + # return status 204 + # end + + unit_role = Doubtfire::Application.config.institution_settings.should_employ_lti_member(member) + unless unit_role.nil? + unit.employ_staff(current_user, unit_role) end - unless Doubtfire::Application.config.institution_settings.should_enrol_lti_member(token['member']) + unless Doubtfire::Application.config.institution_settings.should_enrol_lti_member(member) # error!({ error: 'User can not be enrolled into this unit.' }, 404) return status 204 end diff --git a/app/sidekiq/import_students_lti_job.rb b/app/sidekiq/import_students_lti_job.rb index 17a4451499..18a6d3f9f4 100644 --- a/app/sidekiq/import_students_lti_job.rb +++ b/app/sidekiq/import_students_lti_job.rb @@ -46,7 +46,7 @@ def perform(unit_id, members) user = User.find_by(login_id: user_id_data[:login_id]) || User.find_by(username: user_id_data[:username]) || User.find_by(email: user_id_data[:email]) || - User.create do |new_user| + User.create! do |new_user| # Update new user with details from the SAML response Doubtfire::Application.config.institution_settings.update_user_from_lti_response( new_user, @@ -56,9 +56,22 @@ def perform(unit_id, members) end if user.valid? + unit_role = Doubtfire::Application.config.institution_settings.should_employ_lti_member(member) + unless unit_role.nil? + staff = unit.employ_staff(user, unit_role) + if staff&.valid? + result[:success] << { row: member, message: "Successfully added staff (#{unit_role.name})" } + end + end + + unless Doubtfire::Application.config.institution_settings.should_enrol_lti_member(member) + result[:ignored] << { row: member, message: "Enrolment skipped by institution setting" } + next + end + project = unit.enrol_student(user, nil) if project.valid? - result[:success] << { row: member, message: "Successfully enrolled user." } + result[:success] << { row: member, message: "Successfully enrolled user" } else result[:errors] << { row: member, message: "Failed to enrol student" } end diff --git a/config/no_institution_setting.rb b/config/no_institution_setting.rb index b788323b45..e47b250583 100644 --- a/config/no_institution_setting.rb +++ b/config/no_institution_setting.rb @@ -140,7 +140,7 @@ def update_user_from_lti_response(user, user_id_data, member) user.last_name = last_name.squish.capitalize user.nickname = nickname.squish.capitalize - user.role_id = Role.student.id + user.role = should_employ_lti_member(member) || Role.student # Assigning tutors automatically: # if member['roles'].include?('Instructor') @@ -150,6 +150,15 @@ def update_user_from_lti_response(user, user_id_data, member) user end + # If this returns nil, LTI will move on to check if this member should be enrolled as a student + def should_employ_lti_member(member) + return nil if member['roles'].include?('Student') || member['roles'].include?('Learner') + return Role.convenor if member['roles'].include?("http://purl.imsglobal.org/vocab/lis/v2/person#Administrator") + return Role.tutor if member['roles'].include?("Instructor") + + nil + end + def should_enrol_lti_member(member) # Example "roles" for a Student => ["Learner"] # Example "roles" for an Instructor, who is a global Administrator => ["Instructor", "http://purl.imsglobal.org/vocab/lis/v2/person#Administrator"], diff --git a/test/api/lti_api_test.rb b/test/api/lti_api_test.rb index 86e73c20d0..607c30a3b8 100644 --- a/test/api/lti_api_test.rb +++ b/test/api/lti_api_test.rb @@ -230,18 +230,25 @@ def test_convenor_can_link_requested_unit unit.destroy end - def test_staff_arent_enrolled_into_units - users_can_be_enrolled = [ - FactoryBot.create(:user, :student) - ] - - users_cant_be_enrolled = [ + def test_correct_roles_are_enrolled + users = [ + FactoryBot.create(:user, :student), FactoryBot.create(:user, :admin), FactoryBot.create(:user, :convenor), FactoryBot.create(:user, :auditor), FactoryBot.create(:user, :tutor) ] + roles_can_be_enrolled = %w[ + Student + Learner + ] + + roles_cant_be_enrolled = %w[ + Admin + Instructor + ] + unit = FactoryBot.create(:unit, with_students: false) payload = { @@ -262,19 +269,29 @@ def test_staff_arent_enrolled_into_units secret_key = Doubtfire::Application.config.lti_api_secret token = JWT.encode(payload, secret_key, 'HS256') - users_cant_be_enrolled.each do |user| - add_auth_header_for(user: user) + roles_cant_be_enrolled.each do |role| + payload[:member][:roles] = [role] + + token = JWT.encode(payload, secret_key, 'HS256') + + add_auth_header_for(user: users.sample) post '/api/lti/enrol', { ltik: token } + assert_equal 204, last_response.status end - users_can_be_enrolled.each do |user| - add_auth_header_for(user: user) + roles_can_be_enrolled.each do |role| + payload[:member][:roles] = [role] + + token = JWT.encode(payload, secret_key, 'HS256') + + add_auth_header_for(user: users.sample) # or whichever user you want as caller post '/api/lti/enrol', { ltik: token } assert_equal 201, last_response.status id = last_response_body['id'] assert_not_nil id, "Expected project ID in response" + project = Project.find(id) assert project.valid?, "Expected project to be created" assert_equal unit.id, project.unit.id @@ -283,7 +300,7 @@ def test_staff_arent_enrolled_into_units def test_enrol_students_bulk Sidekiq::Testing.inline! do - unit = FactoryBot.create(:unit, with_students: false) + unit = FactoryBot.create(:unit, with_students: false, student_count: 0) payload = { unit_id: unit.id, @@ -350,9 +367,10 @@ def test_enrol_students_bulk add_auth_header_for(user: convenor) - expected_enrolled_projects_count = 3 + expected_enrolled_projects_count = 2 + expected_success_count = 3 # 2 Projects enrolled + 1 Tutor added as staff. The staff will also be added to the ignored row for not being enrolled as a project. expected_error_count = 1 - expected_ignore_count = 1 + expected_ignore_count = 2 assert_equal expected_enrolled_projects_count + expected_error_count + expected_ignore_count, payload[:members].count post '/api/lti/enrol/bulk', { ltik: token } @@ -364,9 +382,10 @@ def test_enrol_students_bulk results = JSON.parse(job['result']) - assert_equal expected_enrolled_projects_count, unit.projects.count - assert_equal expected_enrolled_projects_count, results['success'].count - assert_equal expected_error_count, results['errors'].count + assert_equal expected_enrolled_projects_count, unit.projects.count, results + assert_equal expected_success_count, results['success'].count, results + assert_equal expected_error_count, results['errors'].count, results + assert_equal expected_ignore_count, results['ignored'].count, results student = FactoryBot.create(:user, :student) unit.enrol_student(student, nil)