1036: Fix importer n+1 (#625)

## Status

- Closes
RaspberryPiFoundation/digital-editor-issues#1036

## Points for consideration:

- Security
- Performance

## What's changed?

- Refactor sso batch endpoint to only create new roles where necessary,
avoiding an n+1
- Refactors to smaller methods to avoid a cyclomatic depdency issue

## Steps to perform after deploying to production

N/A

Author: danhalson

lib/concepts/school_student/create_batch_sso.rb

@@ -32,17 +32,45 @@ def call(school:, school_students_params:, current_user:)
       private
 
       def create_batch_sso(school, students, token)
+        students = normalize_student_params(students)
+        responses = ProfileApiClient.create_school_students_sso(token:, students:, school_id: school.id)
+
+        create_student_roles(school, responses)
+        format_student_responses(responses)
+      rescue ProfileApiClient::Student422Error => e
+        handle_validations(e.errors)
+      end
+
+      def normalize_student_params(students)
         # Ensure that nil values are empty strings, else Profile will swallow validations
-        students = students.map do |student|
+        students.map do |student|
           student.transform_values { |value| value.nil? ? '' : value }
         end
+      end
 
-        responses = ProfileApiClient.create_school_students_sso(token:, students:, school_id: school.id)
+      def create_student_roles(school, responses)
+        user_ids = responses.pluck(:id)
+        existing_user_ids = Role.student.where(school_id: school.id, user_id: user_ids).pluck(:user_id)
+        new_user_ids = user_ids - existing_user_ids
 
-        responses.each do |student|
-          Role.student.find_or_create_by(school_id: school.id, user_id: student[:id])
+        return if new_user_ids.empty?
+
+        # Use insert_all to avoid N+1 INSERT queries
+        new_roles = new_user_ids.map do |user_id|
+          {
+            role: Role.roles[:student],
+            school_id: school.id,
+            user_id: user_id
+          }
         end
 
+        # We know the school and uniqueness is ok at this stage, so we can skip validations
+        # rubocop:disable Rails/SkipsModelValidations
+        Role.insert_all(new_roles, unique_by: %i[user_id school_id role])
+        # rubocop:enable Rails/SkipsModelValidations
+      end
+
+      def format_student_responses(responses)
         # Convert hash responses to User objects with separate metadata
         # This separates student data from metadata (success, error, created flags)
         responses.map do |student_data|
@@ -53,8 +81,6 @@ def create_batch_sso(school, students, token)
             created: student_data[:created]
           }
         end
-      rescue ProfileApiClient::Student422Error => e
-        handle_validations(e.errors)
       end
 
       def handle_validations(errors)

spec/concepts/school_student/create_batch_sso_spec.rb

@@ -23,6 +23,7 @@
     let(:user_ids) { [SecureRandom.uuid, SecureRandom.uuid] }
 
     before do
+      user_ids
       stub_profile_api_create_school_students_sso(user_ids:)
     end
 
@@ -34,7 +35,6 @@
     it 'makes a profile API call with correct parameters' do
       described_class.call(school:, school_students_params:, current_user:)
 
-      # TODO: Replace with WebMock assertion once the profile API has been built.
       expect(ProfileApiClient).to have_received(:create_school_students_sso)
         .with(token: current_user.token, students: school_students_params, school_id: school.id)
     end
@@ -63,6 +63,35 @@
       end
     end
 
+    # This test fails if Role validations change. Check create_batch_sso insert_all usage is still safe, otherwise
+    # add the validation to the expected list
+    it 'fails if Role validations change to ensure CreateBatchSSO is updated' do
+      actual_custom = Role._validate_callbacks
+                          .select { |cb| cb.filter.is_a?(Symbol) }
+                          .map(&:filter)
+                          .reject { |v| v.to_s.start_with?('cant_modify_encrypted_attributes', 'validate_associated_records') }
+                          .sort
+
+      actual_builtin = Role.validators
+                           .map { |v| { attributes: v.attributes.sort, kind: v.class.name.demodulize } }
+                           .sort_by { |v| [v[:kind], v[:attributes]] }
+
+      expected_custom = %i[students_cannot_have_additional_roles users_can_only_have_roles_in_one_school]
+      expected_builtin = [
+        { attributes: [:role], kind: 'PresenceValidator' },
+        { attributes: [:school], kind: 'PresenceValidator' },
+        { attributes: [:user_id], kind: 'PresenceValidator' },
+        { attributes: [:role], kind: 'UniquenessValidator' }
+      ]
+
+      expect(actual_custom).to eq(expected_custom),
+                               "Custom Role validations changed! Got: #{actual_custom.inspect}
+                                Consider updating CreateBatchSSO to ensure insert_all usage is still safe."
+      expect(actual_builtin).to eq(expected_builtin),
+                                "Built-in Role validations changed! Got: #{actual_builtin.inspect}
+                                Consider updating CreateBatchSSO to ensure insert_all usage is still safe."
+    end
+
     it 'returns the student data from Profile API' do
       response = described_class.call(school:, school_students_params:, current_user:)
       students = response[:school_students]
@@ -83,6 +112,38 @@
       expect(second_student_item[:student].name).to eq('SSO Test Student 2')
       expect(second_student_item[:success]).to be(true)
     end
+
+    context 'when roles already exist for some students' do
+      let(:user_ids) { [SecureRandom.uuid, SecureRandom.uuid] }
+
+      before do
+        Role.create!(role: :student, school_id: school.id, user_id: user_ids[0])
+      end
+
+      it 'does not duplicate existing roles' do
+        roles_before_call = Role.student.where(school_id: school.id).to_a
+
+        described_class.call(school:, school_students_params:, current_user:)
+
+        roles_after_call = Role.student.where(school_id: school.id).to_a
+        new_student_roles = roles_after_call - roles_before_call
+
+        # Should only create 1 new student role (for second student) since first already exists
+        expect(new_student_roles.length).to eq(1)
+        expect(new_student_roles.first.user_id).to eq(user_ids[1])
+      end
+
+      it 'does not raise an error' do
+        expect do
+          described_class.call(school:, school_students_params:, current_user:)
+        end.not_to raise_error
+      end
+
+      it 'still returns all students in the response' do
+        response = described_class.call(school:, school_students_params:, current_user:)
+        expect(response[:school_students].length).to eq(2)
+      end
+    end
   end
 
   context 'when validation errors occur' do