Skip to content

refactor: lti improvements #562

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

Merged
b0ink merged 10 commits into from
Feb 11, 2026
+77 −27
Merged

refactor: lti improvements #562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bd2d1c0
fix: ensure bulk enrolment validates member
b0ink Dec 17, 2025
4ec7285
chore: ignore institution settings
b0ink Dec 17, 2025
e4613bb
refactor: allow institution config to employ lti members as staff
b0ink Dec 17, 2025
5313137
Merge branch '10.0.x' into refactor/lti-improvements
b0ink Feb 11, 2026
12137cb
chore: allow for both employment and enrolment in bulk import
b0ink Feb 11, 2026
48db419
test: ensure employment and enrolment settings work
b0ink Feb 11, 2026
21db72b
chore: debug results on failed test
b0ink Feb 11, 2026
eef82f5
fix: null check staff
b0ink Feb 11, 2026
aa59f39
fix: throw error on failed user validation
b0ink Feb 11, 2026
76c31bd
chore: set tutor role if roles are present on initial user creation
b0ink Feb 11, 2026
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
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,7 @@ student-work/
.byebug_history
coverage/
_history

# Institution specific config
config/*_setting.rb
!config/no_institution_setting.rb
21 changes: 13 additions & 8 deletions app/api/lti_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 15 additions & 2 deletions app/sidekiq/import_students_lti_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
11 changes: 10 additions & 1 deletion config/no_institution_setting.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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"],
Expand Down
51 changes: 35 additions & 16 deletions test/api/lti_api_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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 }
Expand All @@ -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)
Expand Down