diff --git a/Changelog.md b/Changelog.md index e096873ab6..9aa1b9daa4 100644 --- a/Changelog.md +++ b/Changelog.md @@ -16,6 +16,7 @@ - Fixed the editing form of marking schemes to include newly added assessments (#7788) - Prevent "No rows found" message from displaying in tables when data is loading (#7790) - Fixed assignment YML export to correctly nest assignment property attributes under `assignment_properties_attributes` (#7792) +- Hide deactivated/missing students and restore re-enrolled students during roster sync (#7799) ### 🔧 Internal changes diff --git a/app/helpers/lti_helper.rb b/app/helpers/lti_helper.rb index 751fccb51e..ea2cbd27b2 100644 --- a/app/helpers/lti_helper.rb +++ b/app/helpers/lti_helper.rb @@ -24,29 +24,41 @@ def roster_sync(lti_deployment, role_types, can_create_users: false, can_create_ additional_data, follow = get_member_data(lti_deployment, follow, auth_data) member_info.concat(additional_data) end - user_data = member_info.filter_map do |user| - unless user['status'] == 'Inactive' || user['roles'].include?(LtiDeployment::LTI_ROLES['test_user']) || - role_types.none? { |role| user['roles'].include?(role) } + if member_info.nil? + raise I18n.t('lti.connection_error') + end + + # Separate users into Active and Inactive based on Canvas status + canvas_active_data = [] + canvas_inactive_ids = [] + member_info.each do |user| + next if user['roles'].include?(LtiDeployment::LTI_ROLES['test_user']) + next if role_types.none? { |role| user['roles'].include?(role) } + if user['status'] == 'Inactive' + canvas_inactive_ids << user['user_id'] + else student_number = user.dig('message', 0, LtiDeployment::LTI_CLAIMS[:custom], 'student_number') id_number_value = if student_number.blank? || student_number == '$Canvas.user.sisIntegrationId' nil else student_number end - { user_name: user['lis_person_sourcedid'].nil? ? user['name'].delete(' ') : user['lis_person_sourcedid'], + canvas_active_data << { + user_name: user['lis_person_sourcedid'].nil? ? user['name'].delete(' ') : user['lis_person_sourcedid'], first_name: user['given_name'], last_name: user['family_name'], display_name: user['name'], email: user['email'], id_number: id_number_value, lti_user_id: user['user_id'], - roles: user['roles'] } + roles: user['roles'] + } end end - if user_data.empty? - raise I18n.t('lti.no_users') - end - user_data.each do |lms_user| + + # Process Active Users + processed_lti_ids = [] + canvas_active_data.each do |lms_user| markus_user = EndUser.find_by(user_name: lms_user[:user_name]) if markus_user.nil? && can_create_users markus_user = EndUser.create(lms_user.except(:lti_user_id, :roles)) @@ -68,9 +80,26 @@ def roster_sync(lti_deployment, role_types, can_create_users: false, can_create_ course_role = Student.create!(user: markus_user, course: lti_deployment.course) end end - next if course_role.nil? - lti_user = LtiUser.find_or_initialize_by(user: markus_user, lti_client: lti_deployment.lti_client) - lti_user.update!(lti_user_id: lms_user[:lti_user_id]) + if course_role + course_role.update!(hidden: false) if course_role.is_a?(Student) + lti_user = LtiUser.find_or_initialize_by(user: markus_user, lti_client: lti_deployment.lti_client) + lti_user.update!(lti_user_id: lms_user[:lti_user_id]) + processed_lti_ids << lms_user[:lti_user_id] + end + end + + # Handle Inactive & Missing Students + all_linked_lti_ids = LtiUser.where(lti_client: lti_deployment.lti_client) + .where(user_id: course.students.select(:user_id)) + .pluck(:lti_user_id) + missing_ids = all_linked_lti_ids - processed_lti_ids + ids_to_hide = (canvas_inactive_ids + missing_ids).uniq + if ids_to_hide.any? + Student.joins(user: :lti_users) + .where(course: course, + 'lti_users.lti_client_id': lti_deployment.lti_client_id, + 'lti_users.lti_user_id': ids_to_hide) + .update_all(hidden: true) end error end diff --git a/config/locales/common/en.yml b/config/locales/common/en.yml index 322f962453..18278b657d 100644 --- a/config/locales/common/en.yml +++ b/config/locales/common/en.yml @@ -22,6 +22,7 @@ en: help: Help lti: config_error: Error configuring LTI. + connection_error: "Could not connect to the LMS. Please try again later." course_creation_denied: Course Creation Not Allowed course_exists: A course with this name already exists on MarkUs. Please select a course to link to. course_link_error: Unsuccessful. Please relaunch MarkUs from your LMS. @@ -45,7 +46,6 @@ en: no_courses: No courses available for matching. no_grades: No grades to sync no_platform: No platforms selected - no_users: No user data returned from LMS roster_sync: Sync Roster roster_sync_complete: Roster Sync Complete roster_sync_errors: Some students could not be synced. diff --git a/spec/helpers/lti_helper_spec.rb b/spec/helpers/lti_helper_spec.rb index 247657e60b..6376af4f75 100644 --- a/spec/helpers/lti_helper_spec.rb +++ b/spec/helpers/lti_helper_spec.rb @@ -165,6 +165,83 @@ expect(LtiUser.count).to eq(2) end end + + context 'when handling user deactivation' do + subject { roster_sync lti_deployment, [LtiDeployment::LTI_ROLES[:learner]], can_create_users: true, can_create_roles: true } + + let(:active_lti_user_id) { 'lti_user_id' } + let(:inactive_lti_user_id) { 'inactive_user_id' } + let(:missing_lti_user_id) { 'missing_user_id' } + + let!(:active_student) { create(:student, course: course) } + let!(:inactive_student) { create(:student, course: course) } + let!(:missing_student) { create(:student, course: course) } + + let(:memberships_with_inactives) do + [ + # User 1: active + { + status: 'Active', + user_id: active_lti_user_id, + lis_person_sourcedid: active_student.user_name, + name: 'Active Student', + roles: [LtiDeployment::LTI_ROLES[:learner]] + }, + # User 2: inactive + { + status: 'Inactive', + user_id: inactive_lti_user_id, + lis_person_sourcedid: inactive_student.user_name, + name: 'Inactive Student', + roles: [LtiDeployment::LTI_ROLES[:learner]] + } + # User 3: missing + ] + end + + before do + create(:lti_user, user: active_student.user, lti_client: lti_deployment.lti_client, + lti_user_id: active_lti_user_id) + create(:lti_user, user: inactive_student.user, lti_client: lti_deployment.lti_client, + lti_user_id: inactive_lti_user_id) + create(:lti_user, user: missing_student.user, lti_client: lti_deployment.lti_client, + lti_user_id: missing_lti_user_id) + + lti_payload = { id: '...', context: { id: '...', label: 'tst', title: 'test' }, + members: memberships_with_inactives }.to_json + allow_any_instance_of(LtiDeployment).to( + receive(:send_lti_request!).and_return(OpenStruct.new(body: lti_payload)) + ) + end + + it 'keeps the active student visible' do + subject + expect(active_student.reload.hidden).to be false + end + + it 'hides the student marked as Inactive in Canvas' do + subject + expect(inactive_student.reload.hidden).to be true + end + + it 'hides the student missing from the Canvas payload' do + subject + expect(missing_student.reload.hidden).to be true + end + + it 'unhides a previously hidden student who becomes active again' do + active_student.update!(hidden: true) + subject + expect(active_student.reload.hidden).to be false + end + + it 'does not hide users who are not students (e.g., TAs) even if missing' do + ta = create(:ta, course: course) + create(:lti_user, user: ta.user, lti_client: lti_deployment.lti_client, lti_user_id: 'ta_lti_id') + subject + expect(Role.find(ta.id).hidden).to be false + end + end end context 'when handling student ID numbers' do