diff --git a/app/controllers/api/school_students_controller.rb b/app/controllers/api/school_students_controller.rb index 5eba8a5c2..6f9d3295e 100644 --- a/app/controllers/api/school_students_controller.rb +++ b/app/controllers/api/school_students_controller.rb @@ -2,6 +2,10 @@ module Api class SchoolStudentsController < ApiController + # This constant is the maximum batch size we can post to + # profile in the create step. We can validate larger batches. + MAX_BATCH_CREATION_SIZE = 50 + before_action :authorize_user load_and_authorize_resource :school authorize_resource :school_student, class: false @@ -32,16 +36,65 @@ def create end def create_batch - result = SchoolStudent::CreateBatch.call( - school: @school, school_students_params:, token: current_user.token, user_id: current_user.id + if school_students_params.blank? + render json: { + error: StandardError, + error_type: :unprocessable_entity + }, + status: :unprocessable_entity + return + end + + students = StudentHelpers.normalise_nil_values_to_empty_strings(school_students_params) + + # We validate the entire batch here in one go and then, if the validation succeds, + # feed the batch to Profile in chunks of 50. + validation_result = SchoolStudent::ValidateBatch.call( + school: @school, students: students, token: current_user.token ) - if result.success? - @job_id = result[:job_id] - render :create_batch, formats: [:json], status: :accepted - else - render json: { error: result[:error], error_type: result[:error_type] }, status: :unprocessable_entity + if validation_result.failure? + render json: { + error: validation_result[:error], + error_type: validation_result[:error_type] + }, status: :unprocessable_entity + return end + + # If we get this far, validation of the entire batch succeeded, so we enqueue it in chunks + begin + enqueue_batches(students) + rescue StandardError => e + Rails.logger.error "Failed to enqueue GoodJob Batch: #{e}" + render json: { error: e, error_type: :batch_error }, status: :unprocessable_entity + return + end + + # We enqueued everything! Yay! + render :create_batch, formats: [:json], status: :accepted + end + + # This method takes a large list of students to insert and enqueues a GoodJob + # Batch to insert them, 50 at a time. We use a GoodJob::Batch to enqueue the + # set of jobs atomically. + # + # This method will throw an error if any batch fails to enqueue, so callers + # should assume the entire student import has failed. + def enqueue_batches(students) + # Raise if a batch is already in progress for this school. + raise ConcurrencyExceededForSchool if @school.import_in_progress? + + @batch = GoodJob::Batch.new(description: @school.id) + @batch.enqueue do + students.each_slice(MAX_BATCH_CREATION_SIZE) do |student_batch| + SchoolStudent::CreateBatch.call( + school: @school, school_students_params: student_batch, token: current_user.token + ) + end + end + + UserJob.create!(user_id: current_user.id, good_job_batch_id: @batch.id) + Rails.logger.info("Batch #{@batch.id} enqueued successfully with school identifier #{@school.id}!") end def update @@ -75,7 +128,7 @@ def school_student_params def school_students_params school_students = params.require(:school_students) - school_students.map do |student| + school_students.filter_map do |student| next if student.blank? student.permit(:username, :password, :name).to_h.with_indifferent_access diff --git a/app/controllers/api/user_jobs_controller.rb b/app/controllers/api/user_jobs_controller.rb index a51dbb016..b064d79e6 100644 --- a/app/controllers/api/user_jobs_controller.rb +++ b/app/controllers/api/user_jobs_controller.rb @@ -5,20 +5,28 @@ class UserJobsController < ApiController before_action :authorize_user def index - user_jobs = UserJob.where(user_id: current_user.id).includes(:good_job) - jobs = user_jobs&.map { |user_job| job_attributes(user_job&.good_job) } - if jobs.any? - render json: { jobs: }, status: :ok + user_jobs = UserJob.where(user_id: current_user.id) + + batches = user_jobs.filter_map do |user_job| + batch_attributes_for(user_job) + end + + if batches.any? + render json: { jobs: batches }, status: :ok else render json: { error: 'No jobs found' }, status: :not_found end end def show - user_job = UserJob.find_by(good_job_id: params[:id], user_id: current_user.id) - job = job_attributes(user_job&.good_job) unless user_job.nil? - if job.present? - render json: { job: }, status: :ok + user_job = UserJob.find_by( + good_job_batch_id: params[:id], + user_id: current_user.id + ) + + batch = batch_attributes_for(user_job) if user_job + if batch.present? + render json: { job: batch }, status: :ok else render json: { error: 'Job not found' }, status: :not_found end @@ -26,16 +34,47 @@ def show private - def job_attributes(job) + def batch_attributes_for(user_job) + return nil unless user_job + + begin + batch = GoodJob::Batch.find(user_job.good_job_batch_id) + batch_attributes(batch) + rescue GoodJob::Batch::NotFoundError + nil + end + end + + def batch_attributes(batch) { - id: job.id, - concurrency_key: job.concurrency_key, - status: job.status, - scheduled_at: job.scheduled_at, - performed_at: job.performed_at, - finished_at: job.finished_at, - error: job.error + id: batch.id, + concurrency_key: batch.description, + status: batch_status(batch), + finished_at: batch.finished_at } end + + # Try to emulate a Job's .status field for a batch. + def batch_status(batch) + # If the batch is finished or discarded, report that. + return 'discarded' if batch.discarded? + return 'finished' if batch.finished? + + # If the batch is in progress, try and summarise the state of the jobs + job_summary_status(GoodJob::Job.where(batch: batch.id)) + end + + # Summarise the status of a list of jobs. + def job_summary_status(jobs) + if jobs.any? { |j| j.status == :running } + 'running' + elsif jobs.any? { |j| j.status == :retried } + 'retrying' + elsif jobs.any? { |j| j.status == :scheduled } + 'scheduled' + else + 'queued' + end + end end end diff --git a/app/jobs/create_students_job.rb b/app/jobs/create_students_job.rb index 446724ae0..18f14c386 100644 --- a/app/jobs/create_students_job.rb +++ b/app/jobs/create_students_job.rb @@ -24,37 +24,14 @@ class CreateStudentsJob < ApplicationJob include GoodJob::ActiveJobExtensions::Concurrency - queue_as :default + queue_as :create_students_job - # Restrict to one job per school to avoid duplicates - good_job_control_concurrency_with( - key: -> { "create_students_job_#{arguments.first[:school_id]}" }, - total_limit: 1 - ) - - def self.attempt_perform_later(school_id:, students:, token:, user_id:) - concurrency_key = "create_students_job_#{school_id}" - existing_jobs = GoodJob::Job.where(concurrency_key:, finished_at: nil) - - raise ConcurrencyExceededForSchool, 'Only one job per school can be enqueued at a time.' if existing_jobs.exists? - - ActiveRecord::Base.transaction do - job = perform_later(school_id:, students:, token:) - UserJob.create!(user_id:, good_job_id: job.job_id) unless job.nil? - - job - end + def self.attempt_perform_later(school_id:, students:, token:) + perform_later(school_id:, students:, token:) end def perform(school_id:, students:, token:) - decrypted_students = students.map do |student| - { - name: student[:name], - username: student[:username], - password: DecryptionHelpers.decrypt_password(student[:password]) - } - end - + decrypted_students = StudentHelpers.decrypt_students(students) responses = ProfileApiClient.create_school_students(token:, students: decrypted_students, school_id:) return if responses[:created].blank? diff --git a/app/models/school.rb b/app/models/school.rb index e2dd475e7..776419c23 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -78,6 +78,15 @@ def postal_code=(str) super(str.to_s.upcase) end + # This method returns true if there is an existing, unfinished, batch whose description + # matches the current school ID. This prevents two users enqueueing a batch for + # the same school, since GoodJob::Batch doesn't support a concurrency key. + def import_in_progress? + GoodJob::BatchRecord.where(finished_at: nil) + .where(discarded_at: nil) + .exists?(description: id) + end + private # Ensure the reference is nil, not an empty string diff --git a/app/models/user_job.rb b/app/models/user_job.rb index 5a0058669..512d60228 100644 --- a/app/models/user_job.rb +++ b/app/models/user_job.rb @@ -1,8 +1,7 @@ # frozen_string_literal: true class UserJob < ApplicationRecord - belongs_to :good_job, class_name: 'GoodJob::Job' - + validates :good_job_batch_id, presence: true validates :user_id, presence: true attr_accessor :user diff --git a/app/views/api/school_students/create_batch.json.jbuilder b/app/views/api/school_students/create_batch.json.jbuilder index 7c4447b23..318fa2808 100644 --- a/app/views/api/school_students/create_batch.json.jbuilder +++ b/app/views/api/school_students/create_batch.json.jbuilder @@ -1,3 +1,3 @@ # frozen_string_literal: true -json.job_id @job_id +json.job_id @batch.id diff --git a/app/views/api/schools/_school.json.jbuilder b/app/views/api/schools/_school.json.jbuilder index 7dea9024d..a453b8bb5 100644 --- a/app/views/api/schools/_school.json.jbuilder +++ b/app/views/api/schools/_school.json.jbuilder @@ -25,3 +25,5 @@ json.code(school.code) if include_code include_user_origin = local_assigns.fetch(:user_origin, false) json.user_origin(school.user_origin) if include_user_origin + +json.import_in_progress school.import_in_progress? diff --git a/config/initializers/good_job.rb b/config/initializers/good_job.rb index 54a245bea..a1f84cbe4 100644 --- a/config/initializers/good_job.rb +++ b/config/initializers/good_job.rb @@ -13,3 +13,10 @@ def authenticate_admin end end end + +Rails.application.configure do + # The create_students_job queue is a serial queue that allows only one job at a time. + # DO NOT change the the value of create_students_job:1 without understanding the implications + # of processing more than one user creation job at once.: + config.good_job.queues = 'create_students_job:1;default:5' +end diff --git a/db/migrate/20250925135238_change_user_jobs_to_batch_id.rb b/db/migrate/20250925135238_change_user_jobs_to_batch_id.rb new file mode 100644 index 000000000..691fc5157 --- /dev/null +++ b/db/migrate/20250925135238_change_user_jobs_to_batch_id.rb @@ -0,0 +1,6 @@ +class ChangeUserJobsToBatchId < ActiveRecord::Migration[7.2] + def change + add_column :user_jobs, :good_job_batch_id, :uuid + remove_column :user_jobs, :good_job_id, :uuid + end +end diff --git a/db/schema.rb b/db/schema.rb index be56697f3..e3df5c1a0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2025_08_25_102042) do +ActiveRecord::Schema[7.2].define(version: 2025_09_25_135238) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -289,10 +289,9 @@ create_table "user_jobs", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "user_id", null: false - t.uuid "good_job_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["user_id", "good_job_id"], name: "index_user_jobs_on_user_id_and_good_job_id", unique: true + t.uuid "good_job_batch_id" end create_table "versions", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| @@ -324,5 +323,4 @@ add_foreign_key "school_projects", "projects" add_foreign_key "school_projects", "schools" add_foreign_key "teacher_invitations", "schools" - add_foreign_key "user_jobs", "good_jobs" end diff --git a/lib/concepts/school_student/create_batch.rb b/lib/concepts/school_student/create_batch.rb index a1048e75f..a7c0d6ef3 100644 --- a/lib/concepts/school_student/create_batch.rb +++ b/lib/concepts/school_student/create_batch.rb @@ -3,26 +3,13 @@ module SchoolStudent class Error < StandardError; end - class ValidationError < StandardError - attr_reader :errors - - def initialize(errors) - @errors = errors - super() - end - end - class ConcurrencyExceededForSchool < StandardError; end class CreateBatch class << self - def call(school:, school_students_params:, token:, user_id:) + def call(school:, school_students_params:, token:) response = OperationResponse.new - response[:job_id] = create_batch(school, school_students_params, token, user_id) - response - rescue ValidationError => e - response[:error] = e.errors - response[:error_type] = :validation_error + response[:job_id] = create_batch(school, school_students_params, token) response rescue ConcurrencyExceededForSchool => e response[:error] = e @@ -37,44 +24,10 @@ def call(school:, school_students_params:, token:, user_id:) private - def create_batch(school, students, token, user_id) - # Ensure that nil values are empty strings, else Profile will swallow validations - students = students.map do |student| - student.transform_values { |value| value.nil? ? '' : value } - end - - validate(school:, students:, token:) - - job = CreateStudentsJob.attempt_perform_later(school_id: school.id, students:, token:, user_id:) + def create_batch(school, students, token) + job = CreateStudentsJob.attempt_perform_later(school_id: school.id, students:, token:) job&.job_id end - - 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]) if student[:password].present? - end - end - - def handle_student422_error(errors) - formatted_errors = errors.each_with_object({}) do |error, hash| - username = error['username'] || error['path'] - - hash[username] ||= [] - hash[username] << (error['errorCode'] || 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 end diff --git a/lib/concepts/school_student/validate_batch.rb b/lib/concepts/school_student/validate_batch.rb new file mode 100644 index 000000000..bb768c3cf --- /dev/null +++ b/lib/concepts/school_student/validate_batch.rb @@ -0,0 +1,62 @@ +# 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 ValidateBatch + class << self + def call(school:, students:, token:) + response = OperationResponse.new + validate_batch(school:, students:, token:) + response + rescue ValidationError => e + response[:error] = e.errors + response[:error_type] = :validation_error + response + rescue StandardError => e + Sentry.capture_exception(e) + response[:error] = "Error creating school students: #{e}" + response[:error_type] = :standard_error + response + end + + private + + def validate_batch(school:, students:, token:) + decrypted_students = StudentHelpers.decrypt_students(students) + ProfileApiClient.validate_school_students(token:, students: decrypted_students, school_id: school.id) + rescue ProfileApiClient::Student422Error => e + handle_student422_error(e.errors) + end + + # This method converts the error structure returned by Profile (an array of error objects) to + # the structure expected by the React front-end, which is a hash with the structure: + # + # username => [array of error codes] + # + # The front end will translate the error codes into user-readable error messages. + def handle_student422_error(errors) + formatted_errors = errors.each_with_object({}) do |error, hash| + username = error['username'] || error['path'] + + hash[username] ||= [] + hash[username] << error['errorCode'] + + # 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 +end diff --git a/lib/helpers/student_helpers.rb b/lib/helpers/student_helpers.rb new file mode 100644 index 000000000..62004ef3f --- /dev/null +++ b/lib/helpers/student_helpers.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class StudentHelpers + # Ensure that nil values are empty strings, else Profile will swallow validations + def self.normalise_nil_values_to_empty_strings(students) + students.map do |student| + student.transform_values { |value| value.nil? ? '' : value } + end + end + + def self.decrypt_students(students) + students.deep_dup.each do |student| + student[:password] = DecryptionHelpers.decrypt_password(student[:password]) if student[:password].present? + end + end +end diff --git a/lib/profile_api_client.rb b/lib/profile_api_client.rb index 9983e42d3..bd70fe796 100644 --- a/lib/profile_api_client.rb +++ b/lib/profile_api_client.rb @@ -93,6 +93,21 @@ def create_school_student(token:, username:, password:, name:, school_id:) raise Student422Error, JSON.parse(e.response_body)['errors'].first end + def validate_school_students(token:, students:, school_id:) + return nil if token.blank? + + students = Array(students) + endpoint = "/api/v1/schools/#{school_id}/students/preflight-student-upload" + 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 == 200 + rescue Faraday::UnprocessableEntityError => e + raise Student422Error, JSON.parse(e.response_body)['errors'] + end + def create_school_students(token:, students:, school_id:, preflight: false) return nil if token.blank? diff --git a/spec/concepts/school_student/create_batch_spec.rb b/spec/concepts/school_student/create_batch_spec.rb index 116a0125f..776e0a9f5 100644 --- a/spec/concepts/school_student/create_batch_spec.rb +++ b/spec/concepts/school_student/create_batch_spec.rb @@ -29,7 +29,7 @@ it 'queues CreateStudentsJob' do expect do - described_class.call(school:, school_students_params:, token:, user_id:) + described_class.call(school:, school_students_params:, token:) end.to have_enqueued_job(CreateStudentsJob).with(school_id: school.id, students: school_students_params, token:) end end @@ -43,70 +43,13 @@ end it 'returns a successful operation response' do - response = described_class.call(school:, school_students_params:, token:, user_id:) + response = described_class.call(school:, school_students_params:, token:) expect(response.success?).to be(true) end it 'returns the job id' do - response = described_class.call(school:, school_students_params:, token:, user_id:) + response = described_class.call(school:, school_students_params:, token:) expect(response[:job_id]).to be_truthy end end - - context 'when a normal error occurs' do - let(:school_students_params) do - [ - { - username: 'a-student', - password: 'Password', - name: 'School Student' - }, - { - username: 'second-student-to-create', - 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 - - it 'does not queue a new job' do - expect do - described_class.call(school:, school_students_params:, token:, user_id:) - end.not_to have_enqueued_job(CreateStudentsJob) - end - - it 'returns a failed operation response' do - response = described_class.call(school:, school_students_params:, token:, user_id:) - expect(response.failure?).to be(true) - end - - 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] - expect(error_message).to match(/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 error codes' do - response = described_class.call(school:, school_students_params:, token:, user_id:) - expect(response[:error]).to eq( - { 'student-to-create' => %w[isUniqueInBatch isComplex notEmpty], 'another-student-to-create-2' => %w[minLength notEmpty] } - ) - end - end end diff --git a/spec/configuration/rails_config_spec.rb b/spec/configuration/rails_config_spec.rb new file mode 100644 index 000000000..f77bb2c27 --- /dev/null +++ b/spec/configuration/rails_config_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Rails do + it 'ensures the create_students_job queue only has one worker' do + queues_config = described_class.application.config.good_job.queues + # queues_config is a string like "create_students_job:1;default:5" + + # Parse the queues into a hash + parsed_queues = queues_config.split(';').to_h do |q| + name, concurrency = q.split(':') + [name, concurrency.to_i] + end + + expect(parsed_queues['create_students_job']).to eq(1) + end +end diff --git a/spec/factories/user_job.rb b/spec/factories/user_job.rb index 23bf0266d..551a69d40 100644 --- a/spec/factories/user_job.rb +++ b/spec/factories/user_job.rb @@ -2,7 +2,7 @@ FactoryBot.define do factory :user_job do - good_job + good_job_batch_id { nil } user_id { SecureRandom.uuid } end end diff --git a/spec/features/my_school/showing_my_school_spec.rb b/spec/features/my_school/showing_my_school_spec.rb index eb3a2d4c2..db7c1ceef 100644 --- a/spec/features/my_school/showing_my_school_spec.rb +++ b/spec/features/my_school/showing_my_school_spec.rb @@ -18,7 +18,7 @@ it "includes the school details and user's roles in the JSON" do school_json = school.to_json(only: %i[id name website reference address_line_1 address_line_2 municipality administrative_area postal_code country_code code verified_at created_at updated_at]) - expected_data = JSON.parse(school_json, symbolize_names: true).merge(roles: ['owner']) + expected_data = JSON.parse(school_json, symbolize_names: true).merge(roles: ['owner'], import_in_progress: school.import_in_progress?) get('/api/school', headers:) data = JSON.parse(response.body, symbolize_names: true) diff --git a/spec/features/school_student/creating_a_batch_of_school_students_spec.rb b/spec/features/school_student/creating_a_batch_of_school_students_spec.rb index 536187f6c..3310870fb 100644 --- a/spec/features/school_student/creating_a_batch_of_school_students_spec.rb +++ b/spec/features/school_student/creating_a_batch_of_school_students_spec.rb @@ -6,6 +6,7 @@ before do authenticated_in_hydra_as(owner) stub_profile_api_create_school_students + stub_profile_api_validate_school_students stub_profile_api_create_safeguarding_flag # UserJob will fail validation as it won't find our test job, so we need to double it @@ -68,6 +69,27 @@ expect(response).to have_http_status(:accepted) end + it 'responds 422 when a batch already exists for this school' do + # Create a fake batch for the school. + GoodJob::BatchRecord.create!( + description: school.id, + finished_at: nil, + discarded_at: nil + ) + + expect do + post("/api/schools/#{school.id}/students/batch", headers:, params:) + end.not_to change(GoodJob::BatchRecord, :count) + expect(response).to have_http_status(:unprocessable_entity) + + active_batches = GoodJob::BatchRecord.where( + description: school.id, + finished_at: nil, + discarded_at: nil + ) + expect(active_batches.count).to eq(1) + end + it 'responds 202 Accepted when the user is a school-teacher' do teacher = create(:teacher, school:) authenticated_in_hydra_as(teacher) @@ -76,6 +98,16 @@ expect(response).to have_http_status(:accepted) end + it 'splits students into jobs of 50 each' do + total_students = 169 + students = Array.new(total_students) do |i| + { username: "student-#{i}", password: 'SaoXlDBAyiAFoMH3VsddhdA7JWnM8P8by1wOjBUWH2g=', name: "Student #{i}" } + end + + post("/api/schools/#{school.id}/students/batch", headers:, params: { school_students: students }) + expect(CreateStudentsJob).to have_received(:attempt_perform_later).exactly((total_students.to_f / 50).ceil).times + end + it 'does not create the school owner safeguarding flag when the user is a school-teacher' do teacher = create(:teacher, school:) authenticated_in_hydra_as(teacher) @@ -105,9 +137,18 @@ it 'responds 422 Unprocessable Entity with a JSON array of validation errors' do stub_profile_api_create_school_students_validation_error + stub_profile_api_validate_students_with_validation_error post("/api/schools/#{school.id}/students/batch", headers:, params:) expect(response).to have_http_status(:unprocessable_entity) - expect(response.body).to eq('{"error":{"student-to-create":["isUniqueInBatch","isComplex","notEmpty"],"another-student-to-create-2":["minLength","notEmpty"]},"error_type":"validation_error"}') + expect(response.body).to eq( + { + error: { + 'student-to-create' => %w[isUniqueInBatch isComplex notEmpty], + 'another-student-to-create-2' => %w[minLength notEmpty] + }, + error_type: 'validation_error' + }.to_json + ) end it 'responds 401 Unauthorized when no token is given' do diff --git a/spec/features/user_job/showing_user_jobs_spec.rb b/spec/features/user_job/showing_user_jobs_spec.rb index 54eac2112..fd25bfccd 100644 --- a/spec/features/user_job/showing_user_jobs_spec.rb +++ b/spec/features/user_job/showing_user_jobs_spec.rb @@ -7,8 +7,8 @@ let(:school) { create(:school) } let(:owner) { create(:owner, school:) } - let(:good_jobs) { create_list(:good_job, 2) } - let!(:user_jobs) { good_jobs.map { |job| create(:user_job, good_job: job, user_id: owner.id) } } + let!(:batch) { GoodJob::BatchRecord.create!(description: school.id) } + let!(:user_job) { create(:user_job, good_job_batch_id: batch.id, user_id: owner.id) } before do authenticated_in_hydra_as(owner) @@ -28,11 +28,11 @@ get('/api/user_jobs', headers:) data = JSON.parse(response.body, symbolize_names: true) - expect(data[:jobs].pluck(:id)).to eq(user_jobs.pluck(:good_job_id)) + expect(data[:jobs].first[:id]).to eq(user_job.good_job_batch_id) end it 'responds with the expected job' do - job_id = user_jobs.first.good_job_id + job_id = user_job.good_job_batch_id get("/api/user_jobs/#{job_id}", headers:) data = JSON.parse(response.body, symbolize_names: true) diff --git a/spec/jobs/create_students_job_spec.rb b/spec/jobs/create_students_job_spec.rb index 91b65ebf9..bc4a76193 100644 --- a/spec/jobs/create_students_job_spec.rb +++ b/spec/jobs/create_students_job_spec.rb @@ -45,13 +45,4 @@ expect(Role.student.where(school:, user_id:)).to exist end - - it 'does not enqueue a job if one is already running for that school' do - # Enqueue the job - GoodJob::Job.enqueue(described_class.new(school_id: school.id, students:, token:)) - - expect do - described_class.attempt_perform_later(school_id: school.id, students:, token:, user_id:) - end.to raise_error(ConcurrencyExceededForSchool) - end end diff --git a/spec/models/user_job_spec.rb b/spec/models/user_job_spec.rb index eb4ecd60f..3ae2de1fe 100644 --- a/spec/models/user_job_spec.rb +++ b/spec/models/user_job_spec.rb @@ -3,6 +3,6 @@ require 'rails_helper' RSpec.describe UserJob do - it { is_expected.to belong_to(:good_job) } + it { is_expected.to validate_presence_of(:good_job_batch_id) } it { is_expected.to validate_presence_of(:user_id) } end diff --git a/spec/support/profile_api_mock.rb b/spec/support/profile_api_mock.rb index d27f97bc2..61ddb5e1b 100644 --- a/spec/support/profile_api_mock.rb +++ b/spec/support/profile_api_mock.rb @@ -108,4 +108,21 @@ def stub_profile_api_create_school_students_sso_validation_error ) ) end + + def stub_profile_api_validate_school_students + allow(ProfileApiClient).to receive(:validate_school_students) + end + + def stub_profile_api_validate_students_with_validation_error + allow(ProfileApiClient).to receive(:validate_school_students).and_raise( + ProfileApiClient::Student422Error.new( + [{ 'path' => '0.username', 'errorCode' => 'isUniqueInBatch', 'message' => 'Username must be unique in the batch data', 'location' => 'body', 'username' => 'student-to-create' }, + { 'path' => '0.password', 'errorCode' => 'isComplex', 'message' => 'Password is too simple (it should not be easily guessable, need password help?)', 'location' => 'body', 'username' => 'student-to-create' }, + { 'path' => '0.name', 'errorCode' => 'notEmpty', 'message' => 'Validation notEmpty on name failed', 'location' => 'body', 'username' => 'student-to-create' }, + { 'path' => '1.username', 'errorCode' => 'isUniqueInBatch', 'message' => 'Username must be unique in the batch data', 'location' => 'body', 'username' => 'student-to-create' }, + { 'path' => '2.password', 'errorCode' => 'minLength', 'message' => 'Password must be at least 8 characters', 'location' => 'body', 'username' => 'another-student-to-create-2' }, + { 'path' => '2.name', 'errorCode' => 'notEmpty', 'message' => 'Validation notEmpty on name failed', 'location' => 'body', 'username' => 'another-student-to-create-2' }] + ) + ) + end end