From bd2d1c095610fdf1c4b4d663ecb5a89e140f95e6 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 17 Dec 2025 11:32:56 +1100 Subject: [PATCH 1/9] fix: ensure bulk enrolment validates member --- app/sidekiq/import_students_lti_job.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/sidekiq/import_students_lti_job.rb b/app/sidekiq/import_students_lti_job.rb index 17a4451499..f37d682ba7 100644 --- a/app/sidekiq/import_students_lti_job.rb +++ b/app/sidekiq/import_students_lti_job.rb @@ -56,6 +56,10 @@ def perform(unit_id, members) end if user.valid? + 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." } From 4ec7285cdfc754b4b717b03f935df7e5ed50a1f0 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 17 Dec 2025 11:51:20 +1100 Subject: [PATCH 2/9] chore: ignore institution settings --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) 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 From e4613bb8fb919fde989bcb679503e425f243eca4 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 17 Dec 2025 12:30:42 +1100 Subject: [PATCH 3/9] refactor: allow institution config to employ lti members as staff --- app/api/lti_api.rb | 21 +++++++++++++-------- app/sidekiq/import_students_lti_job.rb | 12 +++++++++++- config/no_institution_setting.rb | 9 +++++++++ 3 files changed, 33 insertions(+), 9 deletions(-) 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 f37d682ba7..90a31003ef 100644 --- a/app/sidekiq/import_students_lti_job.rb +++ b/app/sidekiq/import_students_lti_job.rb @@ -56,13 +56,23 @@ 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.role.name})" } + next + 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..67a5006692 100644 --- a/config/no_institution_setting.rb +++ b/config/no_institution_setting.rb @@ -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"], From 12137cb2aa381fc0b19a5f9a76a70bb157f7035a Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 11 Feb 2026 11:47:23 +1100 Subject: [PATCH 4/9] chore: allow for both employment and enrolment in bulk import --- app/sidekiq/import_students_lti_job.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/sidekiq/import_students_lti_job.rb b/app/sidekiq/import_students_lti_job.rb index 90a31003ef..192440dff3 100644 --- a/app/sidekiq/import_students_lti_job.rb +++ b/app/sidekiq/import_students_lti_job.rb @@ -60,8 +60,7 @@ def perform(unit_id, members) unless unit_role.nil? staff = unit.employ_staff(user, unit_role) if staff.valid? - result[:success] << { row: member, message: "Successfully added staff (#{unit_role.role.name})" } - next + result[:success] << { row: member, message: "Successfully added staff (#{unit_role.name})" } end end From 48db41935fb8d4ffac80e9258640bb7d90c34760 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 11 Feb 2026 11:49:16 +1100 Subject: [PATCH 5/9] test: ensure employment and enrolment settings work --- test/api/lti_api_test.rb | 45 ++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/test/api/lti_api_test.rb b/test/api/lti_api_test.rb index 86e73c20d0..e663069d80 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 @@ -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 } @@ -365,8 +383,9 @@ 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_success_count, results['success'].count assert_equal expected_error_count, results['errors'].count + assert_equal expected_ignore_count, results['ignored'].count student = FactoryBot.create(:user, :student) unit.enrol_student(student, nil) From 21db72b4825594b9a38f44d00d9ea08a9bb5ffad Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 11 Feb 2026 13:42:25 +1100 Subject: [PATCH 6/9] chore: debug results on failed test --- test/api/lti_api_test.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/api/lti_api_test.rb b/test/api/lti_api_test.rb index e663069d80..607c30a3b8 100644 --- a/test/api/lti_api_test.rb +++ b/test/api/lti_api_test.rb @@ -300,7 +300,7 @@ def test_correct_roles_are_enrolled 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, @@ -382,10 +382,10 @@ def test_enrol_students_bulk results = JSON.parse(job['result']) - assert_equal expected_enrolled_projects_count, unit.projects.count - assert_equal expected_success_count, results['success'].count - assert_equal expected_error_count, results['errors'].count - assert_equal expected_ignore_count, results['ignored'].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) From eef82f5643abb38b764f80625f40846478b0dff6 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 11 Feb 2026 15:41:40 +1100 Subject: [PATCH 7/9] fix: null check staff --- app/sidekiq/import_students_lti_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/sidekiq/import_students_lti_job.rb b/app/sidekiq/import_students_lti_job.rb index 192440dff3..3b9c2957ac 100644 --- a/app/sidekiq/import_students_lti_job.rb +++ b/app/sidekiq/import_students_lti_job.rb @@ -59,7 +59,7 @@ def perform(unit_id, members) 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? + if staff&.valid? result[:success] << { row: member, message: "Successfully added staff (#{unit_role.name})" } end end From aa59f392ab0b75cbebcd9116d5938c9f17702563 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 11 Feb 2026 16:23:02 +1100 Subject: [PATCH 8/9] fix: throw error on failed user validation --- app/sidekiq/import_students_lti_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/sidekiq/import_students_lti_job.rb b/app/sidekiq/import_students_lti_job.rb index 3b9c2957ac..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, From 76c31bd7944476f9523e134a96c5a35b16c5694a Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Wed, 11 Feb 2026 16:33:33 +1100 Subject: [PATCH 9/9] chore: set tutor role if roles are present on initial user creation --- config/no_institution_setting.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/no_institution_setting.rb b/config/no_institution_setting.rb index 67a5006692..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')