319-bulk-student-validation (#458)

## Status

- Closes
RaspberryPiFoundation/digital-editor-issues#319

## What's changed?

- Handles validations from profile when creating bulk students

## Steps to perform after deploying to production

N/A

---------

Co-authored-by: Conor <[email protected]>

Author: danhalson

Diff for: .devcontainer/devcontainer.json

@@ -66,7 +66,7 @@
         "codezombiech.gitignore",
         "shopify.ruby-lsp",
         "koichisasada.vscode-rdbg",
-        "rangav.vscode-thunder-client",
+        "postman.postman-for-vscode",
         "ninoseki.vscode-mogami"
       ],
       "settings": {

Diff for: app/controllers/api/school_students_controller.rb

@@ -37,7 +37,7 @@ def create_batch
       )
 
       if result.success?
-        head :no_content
+        head :accepted
       else
         render json: { error: result[:error] }, status: :unprocessable_entity
       end

Diff for: app/controllers/api/user_jobs_controller.rb

@@ -15,7 +15,7 @@ def index
     end
 
     def show
-      user_job = UserJob.find_by(job_id: params[:id], teacher_id: current_user.id)
+      user_job = UserJob.find_by(good_job_id: params[:id], user_id: current_user.id)
       job = job_attributes(user_job.good_job)
       if job
         render json: { job: }, status: :ok

Diff for: app/jobs/create_students_job.rb

@@ -28,11 +28,7 @@ def self.attempt_perform_later(school_id:, students:, token:, user_id:)
   end
 
   def perform(school_id:, students:, token:)
-    students = Array(students).map do |student|
-      student[:password] = DecryptionHelpers.decrypt_password(student[:password])
-      student
-    end
-
+    students.each { |student| student[:password] = DecryptionHelpers.decrypt_password(student[:password]) }
     responses = ProfileApiClient.create_school_students(token:, students:, school_id:)
     return if responses[:created].blank?
 

Diff for: app/models/user_job.rb

@@ -3,5 +3,7 @@
 class UserJob < ApplicationRecord
   belongs_to :good_job, class_name: 'GoodJob::Job'
 
+  validates :user_id, presence: true
+
   attr_accessor :user
 end

Diff for: config/locales/en.yml

@@ -1,17 +1,19 @@
 en:
   errors:
     admin:
-      unauthorized: 'Not authorized.'
+      unauthorized: "Not authorized."
     project:
       editing:
-        delete_default_component: 'Cannot delete default file'
-        change_default_name: 'Cannot amend default file name'
-        change_default_extension: 'Cannot amend default file extension'
+        delete_default_component: "Cannot delete default file"
+        change_default_name: "Cannot amend default file name"
+        change_default_extension: "Cannot amend default file extension"
       remixing:
-        invalid_params: 'Invalid parameters'
-        cannot_save: 'Cannot create project remix'
+        invalid_params: "Invalid parameters"
+        cannot_save: "Cannot create project remix"
   validations:
     school:
-      website: 'must be a valid URL'
+      website: "must be a valid URL"
     invitation:
       email_address: "'%<value>s' is invalid"
+    school_student:
+      not_empty: "You must supply a %{field}"

Diff for: lib/concepts/school_student/create_batch.rb

@@ -1,38 +1,71 @@
 # frozen_string_literal: true
 
 module SchoolStudent
+  class Error < StandardError; end
+
+  class ValidationError < StandardError
+    attr_reader :errors
+
+    def initialize(errors)
+      @errors = errors
+      super()
+    end
+  end
+
   class CreateBatch
     class << self
       def call(school:, school_students_params:, token:, user_id:)
         response = OperationResponse.new
         response[:job_id] = create_batch(school, school_students_params, token, user_id)
         response
+      rescue ValidationError => e
+        response[:error] = e.errors
+        response
       rescue StandardError => e
         Sentry.capture_exception(e)
-        response[:error] = e
+        response[:error] = "Error creating school students: #{e}"
         response
       end
 
       private
 
       def create_batch(school, students, token, user_id)
-        validate(students:)
-        # TODO: Do the preflight checks here
+        validate(school:, students:, token:)
 
         job = CreateStudentsJob.attempt_perform_later(school_id: school.id, students:, token:, user_id:)
         job&.job_id
       end
 
-      def validate(students:)
-        errors = []
-        students.each_with_index do |student, n|
-          student_errors = []
-          student_errors << "username '#{student[:username]}' is invalid" if student[:username].blank?
-          student_errors << "password '#{student[:password]}' is invalid" if student[:password].blank?
-          student_errors << "name '#{student[:name]}' is invalid" if student[:name].blank?
-          errors << "Error creating student #{n + 1}: #{student_errors.join(', ')}" unless student_errors.empty?
+      def validate(school:, students:, token:)
+        decrypted_students = decrypt_students(students)
+        ProfileApiClient.create_school_students(token:, students: decrypted_students, school_id: school.id, preflight: true)
+      rescue ProfileApiClient::Student422Error => e
+        handle_student422_error(e.errors)
+      end
+
+      def decrypt_students(students)
+        students.deep_dup.each do |student|
+          student[:password] = DecryptionHelpers.decrypt_password(student[:password])
         end
-        raise ArgumentError, errors.join(', ') unless errors.empty?
+      end
+
+      def handle_student422_error(errors)
+        formatted_errors = errors.each_with_object({}) do |error, hash|
+          username = error['username'] || error['path']
+          field = error['path'].split('.').last
+
+          hash[username] ||= []
+          hash[username] << I18n.t(
+            "validations.school_student.#{error['errorCode'].underscore}",
+            field:,
+            default: error['message']
+          )
+
+          # Ensure uniqueness to avoid repeat errors with duplicate usernames
+          hash[username] = hash[username].uniq
+        end
+
+        raise ValidationError, formatted_errors unless formatted_errors.nil? || formatted_errors.blank?
       end
     end
   end

Diff for: lib/profile_api_client.rb

@@ -12,10 +12,16 @@ class ProfileApiClient
 
   class Error < StandardError; end
 
-  class Student422Error < Error
-    def initialize(error)
-      @message = error['message']
-      super @message
+  class Student422Error < StandardError
+    attr_reader :errors
+
+    def initialize(errors)
+      @errors = errors
+      if errors.is_a?(Hash)
+        super(errors['message'])
+      else
+        super()
+      end
     end
   end
 
@@ -101,19 +107,23 @@ def create_school_student(token:, username:, password:, name:, school_id:)
       raise Student422Error, JSON.parse(e.response_body)['errors'].first
     end
 
-    def create_school_students(token:, students:, school_id:)
+    def create_school_students(token:, students:, school_id:, preflight: false)
       return nil if token.blank?
 
       students = Array(students)
-      response = connection(token).post("/api/v1/schools/#{school_id}/students") do |request|
-        request.body = students
+      endpoint = "/api/v1/schools/#{school_id}/students"
+      endpoint += '/preflight' if preflight
+      response = connection(token).post(endpoint) do |request|
+        request.body = students.to_json
+        request.headers['Content-Type'] = 'application/json'
       end
 
-      raise UnexpectedResponse, response unless response.status == 201
+      raise UnexpectedResponse, response unless [200, 201].include?(response.status)
 
       response.body.deep_symbolize_keys
     rescue Faraday::BadRequestError => e
-      raise Student422Error, JSON.parse(e.response_body)['errors'].first
+      errors = JSON.parse(e.response_body)['errors']
+      raise Student422Error, errors
     end
 
     def update_school_student(token:, school_id:, student_id:, name: nil, username: nil, password: nil) # rubocop:disable Metrics/ParameterLists

Diff for: spec/concepts/school_student/create_batch_spec.rb

@@ -11,31 +11,36 @@
     [
       {
         username: 'student-to-create',
-        password: 'at-least-8-characters',
+        password: 'SaoXlDBAyiAFoMH3VsddhdA7JWnM8P8by1wOjBUWH2g=',
         name: 'School Student'
       },
       {
         username: 'second-student-to-create',
-        password: 'at-least-8-characters',
+        password: 'SaoXlDBAyiAFoMH3VsddhdA7JWnM8P8by1wOjBUWH2g=',
         name: 'School Student 2'
       }
     ]
   end
 
   before do
-    stub_profile_api_create_school_students(user_ids: [SecureRandom.uuid, SecureRandom.uuid])
-
     ActiveJob::Base.queue_adapter = :test
   end
 
-  it 'queues CreateStudentsJob' do
-    expect do
-      described_class.call(school:, school_students_params:, token:, user_id:)
-    end.to have_enqueued_job(CreateStudentsJob).with(school_id: school.id, students: school_students_params, token:)
+  context 'when queuing a job' do
+    before do
+      stub_profile_api_create_school_students(user_ids: [SecureRandom.uuid, SecureRandom.uuid])
+    end
+
+    it 'queues CreateStudentsJob' do
+      expect do
+        described_class.call(school:, school_students_params:, token:, user_id:)
+      end.to have_enqueued_job(CreateStudentsJob).with(school_id: school.id, students: school_students_params, token:)
+    end
   end
 
   context 'when a job has been queued' do
     before do
+      stub_profile_api_create_school_students(user_ids: [SecureRandom.uuid, SecureRandom.uuid])
       allow(CreateStudentsJob).to receive(:attempt_perform_later).and_return(
         instance_double(CreateStudentsJob, job_id: SecureRandom.uuid)
       )
@@ -52,23 +57,24 @@
     end
   end
 
-  context 'when validation fails' do
+  context 'when a normal error occurs' do
     let(:school_students_params) do
       [
         {
-          username: '',
-          password: 'at-least-8-characters',
+          username: 'a-student',
+          password: 'Password',
           name: 'School Student'
         },
         {
           username: 'second-student-to-create',
-          password: 'at-least-8-characters',
+          password: 'Password',
           name: 'School Student 2'
         }
       ]
     end
 
     before do
+      stub_profile_api_create_school_students(user_ids: [SecureRandom.uuid, SecureRandom.uuid])
       allow(Sentry).to receive(:capture_exception)
     end
 
@@ -85,13 +91,26 @@
 
     it 'returns the error message in the operation response' do
       response = described_class.call(school:, school_students_params:, token:, user_id:)
-      error_message = response[:error].message
-      expect(error_message).to match(/Error creating student 1: username '' is invalid/)
+      error_message = response[:error]
+      expect(error_message).to match(/Error creating school students: Decryption failed: iv must be 16 bytes/)
     end
 
     it 'sent the exception to Sentry' do
       described_class.call(school:, school_students_params:, token:, user_id:)
       expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError))
     end
   end
+
+  context 'when a validation error occurs' do
+    before do
+      stub_profile_api_create_school_students_validation_error
+    end
+
+    it 'returns the expected formatted errors' do
+      response = described_class.call(school:, school_students_params:, token:, user_id:)
+      expect(response[:error]).to eq(
+        { 'student-to-create' => ['Username must be unique in the batch data', 'Password is too simple (it should not be easily guessable, <a href="https://my.raspberrypi.org/password-help">need password help?</a>)', 'You must supply a name'], 'another-student-to-create-2' => ['Password must be at least 8 characters', 'You must supply a name'] }
+      )
+    end
+  end
 end

Diff for: spec/factories/good_job.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :good_job, class: 'GoodJob::Job' do
+    # Add necessary attributes here
+    queue_name { 'default' }
+    priority { 0 }
+    serialized_params { {} }
+    scheduled_at { Time.current }
+    performed_at { nil }
+    finished_at { nil }
+    error { nil }
+  end
+end

Diff for: spec/factories/user_job.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :user_job do
+    association :good_job, factory: :good_job
+    user_id { SecureRandom.uuid }
+  end
+end