Hide deactivated/missing students and restore re-enrolled students during roster sync

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
 

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

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.

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