From cd077d52b8914db650778e6485ce870008ce4af3 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Fri, 21 Nov 2025 08:12:55 +0000 Subject: [PATCH 01/21] Add CanCan ability and routes for import schools endpoints. We allow `editor-admin` and `experience-cs-admin` users to import schools and monitor the state of the resulting jobs. --- app/models/ability.rb | 10 ++++++++++ config/routes.rb | 2 ++ 2 files changed, 12 insertions(+) diff --git a/app/models/ability.rb b/app/models/ability.rb index 65bc1987a..f09f226b0 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -15,6 +15,7 @@ def initialize(user) define_school_owner_abilities(school:) if user.school_owner?(school) end + define_editor_admin_abilities(user) define_experience_cs_admin_abilities(user) end @@ -120,10 +121,19 @@ def define_school_student_abilities(user:, school:) can(%i[show_finished set_finished show_status unsubmit submit], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id) end + def define_editor_admin_abilities(user) + return unless user&.admin? + + can :import, School + can :read, :school_import_job + end + def define_experience_cs_admin_abilities(user) return unless user&.experience_cs_admin? can %i[read create update destroy], Project, user_id: nil + can :import, School + can :read, :school_import_job end def school_teacher_can_manage_lesson?(user:, school:, lesson:) diff --git a/config/routes.rb b/config/routes.rb index 813ad8737..9dd82fab2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -57,6 +57,7 @@ resource :school, only: [:show], controller: 'my_school' resources :schools, only: %i[index show create update destroy] do + post :import, on: :collection resources :members, only: %i[index], controller: 'school_members' resources :classes, only: %i[index show create update destroy], controller: 'school_classes' do post :import, on: :collection @@ -81,6 +82,7 @@ end resources :user_jobs, only: %i[index show] + resources :school_import_jobs, only: %i[show] post '/google/auth/exchange-code', to: 'google_auth#exchange_code', defaults: { format: :json } end From 8be3b25fbaaf6e3414f6e5180125fb26b75038b8 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 20 Nov 2025 10:12:08 +0000 Subject: [PATCH 02/21] Update UserInfoApiClient to allow user lookup by email. This change adds a search_by_email method to allow lookup of users by email address to acquire the ID of proposed school owners. --- lib/user_info_api_client.rb | 43 ++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/lib/user_info_api_client.rb b/lib/user_info_api_client.rb index 25a84c826..d624e1076 100644 --- a/lib/user_info_api_client.rb +++ b/lib/user_info_api_client.rb @@ -18,14 +18,34 @@ def fetch_by_ids(user_ids) transform_result(response.body.fetch('users', [])) end + def find_user_by_email(email) + return nil if email.blank? + return stubbed_user_by_email(email) if bypass_oauth? + + response = conn.get do |r| + r.url "/users/#{CGI.escape(email)}" + end + return nil if response.body.blank? + + # Single user response has 'user' key, not 'users' + user = response.body.fetch('user', nil) + user.present? ? transform_user(user) : nil + rescue Faraday::ResourceNotFound + nil + end + private def bypass_oauth? ENV.fetch('BYPASS_OAUTH', nil) == 'true' end + def transform_user(user) + user.transform_keys { |k| k.to_s.underscore.to_sym } + end + def transform_result(result) - { result: }.transform_keys { |k| k.to_s.underscore.to_sym }.fetch(:result) + result.map { |user| transform_user(user) } end def conn @@ -62,5 +82,26 @@ def stubbed_users(user_ids) } end end + + def stubbed_user_by_email(email) + { + id: Digest::UUID.uuid_v5(Digest::UUID::DNS_NAMESPACE, email), + email: email, + username: nil, + parentalEmail: nil, + name: 'School Owner', + nickname: 'Owner', + country: 'United Kingdom', + country_code: 'GB', + postcode: nil, + dateOfBirth: nil, + verifiedAt: '2024-01-01T12:00:00.000Z', + createdAt: '2024-01-01T12:00:00.000Z', + updatedAt: '2024-01-01T12:00:00.000Z', + discardedAt: nil, + lastLoggedInAt: '2024-01-01T12:00:00.000Z', + roles: '' + } + end end end From de3afa395e8e1af2b30d54806089a6e315ec83c1 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Mon, 1 Dec 2025 10:03:39 +0000 Subject: [PATCH 03/21] Add SchoolImportJob to track imports. This commit introduces a SchoolImportJob type of GoodJob job to allow for enqueueing imports and tracking their asynchronous completion. The controller will return to the caller a Job ID which can be used to track the progress of these jobs and retrieve their results when done. The SchoolImportResult class tracks the job and provides access to the results. There are structured error codes for possible issues in SchoolImportError. --- app/jobs/school_import_job.rb | 141 ++++++++++++++++++ app/models/school_import_error.rb | 36 +++++ app/models/school_import_result.rb | 7 + config/initializers/good_job.rb | 2 +- ...1120092548_create_school_import_results.rb | 15 ++ db/schema.rb | 59 ++------ spec/jobs/school_import_job_spec.rb | 119 +++++++++++++++ 7 files changed, 328 insertions(+), 51 deletions(-) create mode 100644 app/jobs/school_import_job.rb create mode 100644 app/models/school_import_error.rb create mode 100644 app/models/school_import_result.rb create mode 100644 db/migrate/20251120092548_create_school_import_results.rb create mode 100644 spec/jobs/school_import_job_spec.rb diff --git a/app/jobs/school_import_job.rb b/app/jobs/school_import_job.rb new file mode 100644 index 000000000..95a9544f2 --- /dev/null +++ b/app/jobs/school_import_job.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +class SchoolImportJob < ApplicationJob + retry_on StandardError, wait: :polynomially_longer, attempts: 3 do |_job, e| + Sentry.capture_exception(e) + raise e + end + + queue_as :import_schools_job + + def perform(schools_data:, user_id:) + @results = { + successful: [], + failed: [] + } + + schools_data.map(&:with_indifferent_access).each do |school_data| + import_school(school_data) + end + + # Store results in dedicated table + store_results(@results, user_id) + + @results + end + + private + + def store_results(results, user_id) + SchoolImportResult.create!( + job_id: job_id, + user_id: user_id, + results: results + ) + rescue StandardError => e + Sentry.capture_exception(e) + # Don't fail the job if we can't store results + end + + def import_school(school_data) + owner = find_owner(school_data[:owner_email]) + + unless owner + @results[:failed] << { + name: school_data[:name], + error_code: SchoolImportError::CODES[:owner_not_found], + error: "Owner not found: #{school_data[:owner_email]}", + owner_email: school_data[:owner_email] + } + return + end + + # Check if this owner already has a school as creator + existing_school = School.find_by(creator_id: owner[:id]) + if existing_school + @results[:failed] << { + name: school_data[:name], + error_code: SchoolImportError::CODES[:owner_already_creator], + error: "Owner #{school_data[:owner_email]} is already the creator of school '#{existing_school.name}'", + owner_email: school_data[:owner_email], + existing_school_id: existing_school.id + } + return + end + + school_params = build_school_params(school_data) + + # Use transaction for atomicity + School.transaction do + result = School::Create.call( + school_params: school_params, + creator_id: owner[:id] + ) + + if result.success? + school = result[:school] + + # Auto-verify the imported school + school.verify! + + # Create owner role + Role.owner.create!(school_id: school.id, user_id: owner[:id]) + + @results[:successful] << { + name: school.name, + id: school.id, + code: school.code, + owner_email: school_data[:owner_email] + } + else + @results[:failed] << { + name: school_data[:name], + error_code: SchoolImportError::CODES[:school_validation_failed], + error: format_errors(result[:error]), + owner_email: school_data[:owner_email] + } + end + end + rescue StandardError => e + @results[:failed] << { + name: school_data[:name], + error_code: SchoolImportError::CODES[:unknown_error], + error: e.message, + owner_email: school_data[:owner_email] + } + Sentry.capture_exception(e) + end + + def find_owner(email) + return nil if email.blank? + + UserInfoApiClient.find_user_by_email(email) + rescue StandardError => e + Sentry.capture_exception(e) + nil + end + + def build_school_params(school_data) + { + name: school_data[:name], + website: school_data[:website], + address_line_1: school_data[:address_line_1], + address_line_2: school_data[:address_line_2], + municipality: school_data[:municipality], + administrative_area: school_data[:administrative_area], + postal_code: school_data[:postal_code], + country_code: school_data[:country_code]&.upcase, + reference: school_data[:reference], + creator_agree_authority: true, + creator_agree_terms_and_conditions: true, + creator_agree_responsible_safeguarding: true, + user_origin: 'experience_cs' + }.compact + end + + def format_errors(errors) + return errors.to_s unless errors.respond_to?(:full_messages) + + errors.full_messages.join(', ') + end +end diff --git a/app/models/school_import_error.rb b/app/models/school_import_error.rb new file mode 100644 index 000000000..72fc968cd --- /dev/null +++ b/app/models/school_import_error.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module SchoolImportError + CODES = { + csv_invalid_format: 'CSV_INVALID_FORMAT', + csv_malformed: 'CSV_MALFORMED', + csv_validation_failed: 'CSV_VALIDATION_FAILED', + owner_not_found: 'OWNER_NOT_FOUND', + owner_already_creator: 'OWNER_ALREADY_CREATOR', + duplicate_owner_email: 'DUPLICATE_OWNER_EMAIL', + school_validation_failed: 'SCHOOL_VALIDATION_FAILED', + job_not_found: 'JOB_NOT_FOUND', + csv_file_required: 'CSV_FILE_REQUIRED', + unknown_error: 'UNKNOWN_ERROR' + }.freeze + + class << self + def format_error(code, message, details = {}) + { + error_code: CODES[code] || CODES[:unknown_error], + message: message, + details: details + }.compact + end + + def format_row_errors(errors_array) + { + error_code: CODES[:csv_validation_failed], + message: 'CSV validation failed', + details: { + row_errors: errors_array + } + } + end + end +end diff --git a/app/models/school_import_result.rb b/app/models/school_import_result.rb new file mode 100644 index 000000000..eca70e3b5 --- /dev/null +++ b/app/models/school_import_result.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class SchoolImportResult < ApplicationRecord + validates :job_id, presence: true, uniqueness: true + validates :user_id, presence: true + validates :results, presence: true +end diff --git a/config/initializers/good_job.rb b/config/initializers/good_job.rb index 65d1b301e..600f199eb 100644 --- a/config/initializers/good_job.rb +++ b/config/initializers/good_job.rb @@ -18,5 +18,5 @@ def authenticate_admin # The create_students_job queue is a serial queue that allows only one job at a time. # DO NOT change 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' + config.good_job.queues = 'create_students_job:1;import_schools_job:1;default:5' end diff --git a/db/migrate/20251120092548_create_school_import_results.rb b/db/migrate/20251120092548_create_school_import_results.rb new file mode 100644 index 000000000..84f826a7f --- /dev/null +++ b/db/migrate/20251120092548_create_school_import_results.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CreateSchoolImportResults < ActiveRecord::Migration[7.1] + def change + create_table :school_import_results, id: :uuid do |t| + t.uuid :job_id, null: false + t.uuid :user_id, null: false + t.jsonb :results, null: false, default: {} + t.timestamps + end + + add_index :school_import_results, :job_id, unique: true + add_index :school_import_results, :user_id + end +end diff --git a/db/schema.rb b/db/schema.rb index a7141a0bc..da76c7fac 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -43,14 +43,14 @@ t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end - create_table "class_students", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + create_table "class_members", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "school_class_id", null: false t.uuid "student_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["school_class_id", "student_id"], name: "index_class_students_on_school_class_id_and_student_id", unique: true - t.index ["school_class_id"], name: "index_class_students_on_school_class_id" - t.index ["student_id"], name: "index_class_students_on_student_id" + t.index ["school_class_id", "student_id"], name: "index_class_members_on_school_class_id_and_student_id", unique: true + t.index ["school_class_id"], name: "index_class_members_on_school_class_id" + t.index ["student_id"], name: "index_class_members_on_student_id" end create_table "class_teachers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| @@ -74,16 +74,6 @@ t.index ["project_id"], name: "index_components_on_project_id" end - create_table "feedback", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| - t.uuid "school_project_id" - t.text "content" - t.uuid "user_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.datetime "read_at" - t.index ["school_project_id"], name: "index_feedback_on_school_project_id" - end - create_table "good_job_batches", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false @@ -218,7 +208,7 @@ t.string "remix_origin" t.uuid "school_id" t.uuid "lesson_id" - t.text "instructions" + t.boolean "finished" t.index ["identifier", "locale"], name: "index_projects_on_identifier_and_locale", unique: true t.index ["identifier"], name: "index_projects_on_identifier" t.index ["lesson_id"], name: "index_projects_on_lesson_id" @@ -239,40 +229,15 @@ create_table "school_classes", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "school_id", null: false + t.uuid "teacher_id", null: false t.string "name", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "description" - t.string "code" - t.integer "import_origin" - t.string "import_id" - t.index ["code", "school_id"], name: "index_school_classes_on_code_and_school_id", unique: true + t.index ["school_id", "teacher_id"], name: "index_school_classes_on_school_id_and_teacher_id" t.index ["school_id"], name: "index_school_classes_on_school_id" end - create_table "school_project_transitions", force: :cascade do |t| - t.string "from_state", null: false - t.string "to_state", null: false - t.text "metadata", default: "{}" - t.integer "sort_key", null: false - t.uuid "school_project_id", null: false - t.boolean "most_recent", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["school_project_id", "most_recent"], name: "index_school_project_transitions_parent_most_recent", unique: true, where: "most_recent" - t.index ["school_project_id", "sort_key"], name: "index_school_project_transitions_parent_sort", unique: true - end - - create_table "school_projects", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| - t.uuid "school_id" - t.uuid "project_id", null: false - t.boolean "finished", default: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["project_id"], name: "index_school_projects_on_project_id" - t.index ["school_id"], name: "index_school_projects_on_school_id" - end - create_table "schools", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "name", null: false t.string "reference" @@ -315,10 +280,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" + t.uuid "good_job_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.uuid "good_job_batch_id" t.index ["user_id", "good_job_id"], name: "index_user_jobs_on_user_id_and_good_job_id", unique: true end @@ -332,16 +296,14 @@ t.uuid "meta_project_id" t.uuid "meta_school_id" t.uuid "meta_remixed_from_id" - t.string "meta_school_project_id" t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id" end add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" - add_foreign_key "class_students", "school_classes" + add_foreign_key "class_members", "school_classes" add_foreign_key "class_teachers", "school_classes" add_foreign_key "components", "projects" - add_foreign_key "feedback", "school_projects" add_foreign_key "lessons", "lessons", column: "copied_from_id" add_foreign_key "lessons", "school_classes" add_foreign_key "lessons", "schools" @@ -350,9 +312,6 @@ add_foreign_key "projects", "schools" add_foreign_key "roles", "schools" add_foreign_key "school_classes", "schools" - add_foreign_key "school_project_transitions", "school_projects" - 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/spec/jobs/school_import_job_spec.rb b/spec/jobs/school_import_job_spec.rb new file mode 100644 index 000000000..b1342751a --- /dev/null +++ b/spec/jobs/school_import_job_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe SchoolImportJob do + describe '#perform' do + let(:token) { 'test-token' } + let(:user_id) { SecureRandom.uuid } + let(:owner1_id) { SecureRandom.uuid } + let(:owner2_id) { SecureRandom.uuid } + + let(:schools_data) do + [ + { + name: 'Test School 1', + website: 'https://test1.example.com', + address_line_1: '123 Main St', + municipality: 'Springfield', + country_code: 'US', + owner_email: 'owner1@example.com' + }, + { + name: 'Test School 2', + website: 'https://test2.example.com', + address_line_1: '456 Oak Ave', + municipality: 'Boston', + country_code: 'US', + owner_email: 'owner2@example.com' + } + ] + end + + before do + allow(UserInfoApiClient).to receive(:find_user_by_email) + .with('owner1@example.com') + .and_return({ id: owner1_id, email: 'owner1@example.com' }) + + allow(UserInfoApiClient).to receive(:find_user_by_email) + .with('owner2@example.com') + .and_return({ id: owner2_id, email: 'owner2@example.com' }) + end + + context 'when all schools can be created successfully' do + it 'creates schools and returns successful results' do + results = described_class.new.perform( + schools_data: schools_data, + user_id: user_id + ) + + expect(results[:successful].count).to eq(2) + expect(results[:failed].count).to eq(0) + + school_1 = School.find_by(name: 'Test School 1') + expect(school_1).to be_present + expect(school_1.verified?).to be true + expect(school_1.code).to be_present + expect(school_1.user_origin).to eq('experience_cs') + + # Check owner role was created + expect(Role.owner.exists?(school_id: school_1.id, user_id: owner1_id)).to be true + end + end + + context 'when owner email is not found' do + before do + allow(UserInfoApiClient).to receive(:find_user_by_email) + .with('owner1@example.com') + .and_return(nil) + end + + it 'adds failed result for that school' do + results = described_class.new.perform( + schools_data: [schools_data.first], + user_id: user_id + ) + + expect(results[:successful].count).to eq(0) + expect(results[:failed].count).to eq(1) + expect(results[:failed].first[:error_code]).to eq('OWNER_NOT_FOUND') + expect(results[:failed].first[:error]).to include('Owner not found') + end + end + + context 'when schools_data has string keys (simulating ActiveJob serialization)' do + let(:schools_data_with_string_keys) do + [ + { + 'name' => 'Test School 1', + 'website' => 'https://test1.example.com', + 'address_line_1' => '123 Main St', + 'municipality' => 'Springfield', + 'country_code' => 'us', + 'owner_email' => 'owner1@example.com' + } + ] + end + + before do + allow(UserInfoApiClient).to receive(:find_user_by_email) + .with('owner1@example.com') + .and_return({ id: owner1_id, email: 'owner1@example.com' }) + end + + it 'handles string keys correctly' do + results = described_class.new.perform( + schools_data: schools_data_with_string_keys, + user_id: user_id + ) + + expect(results[:successful].count).to eq(1) + expect(results[:failed].count).to eq(0) + + school = School.find_by(name: 'Test School 1') + expect(school).to be_present + expect(school.country_code).to eq('US') + end + end + end +end From 1baf9fbe4f9c8b1f59bd9ccdad005bd18ef4b941 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Tue, 2 Dec 2025 11:09:44 +0000 Subject: [PATCH 04/21] Add School::ImportBatch operation to import a CSV of schools. This operation parses and validates the CSV and reports on errors or enqueues the job. --- db/schema.rb | 69 +++++++- .../school/operations/import_batch.rb | 163 ++++++++++++++++++ spec/concepts/school/import_batch_spec.rb | 92 ++++++++++ 3 files changed, 315 insertions(+), 9 deletions(-) create mode 100644 lib/concepts/school/operations/import_batch.rb create mode 100644 spec/concepts/school/import_batch_spec.rb diff --git a/db/schema.rb b/db/schema.rb index da76c7fac..188db48cc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -43,14 +43,14 @@ t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end - create_table "class_members", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + create_table "class_students", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "school_class_id", null: false t.uuid "student_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["school_class_id", "student_id"], name: "index_class_members_on_school_class_id_and_student_id", unique: true - t.index ["school_class_id"], name: "index_class_members_on_school_class_id" - t.index ["student_id"], name: "index_class_members_on_student_id" + t.index ["school_class_id", "student_id"], name: "index_class_students_on_school_class_id_and_student_id", unique: true + t.index ["school_class_id"], name: "index_class_students_on_school_class_id" + t.index ["student_id"], name: "index_class_students_on_student_id" end create_table "class_teachers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| @@ -74,6 +74,16 @@ t.index ["project_id"], name: "index_components_on_project_id" end + create_table "feedback", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "school_project_id" + t.text "content" + t.uuid "user_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.datetime "read_at" + t.index ["school_project_id"], name: "index_feedback_on_school_project_id" + end + create_table "good_job_batches", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false @@ -208,7 +218,7 @@ t.string "remix_origin" t.uuid "school_id" t.uuid "lesson_id" - t.boolean "finished" + t.text "instructions" t.index ["identifier", "locale"], name: "index_projects_on_identifier_and_locale", unique: true t.index ["identifier"], name: "index_projects_on_identifier" t.index ["lesson_id"], name: "index_projects_on_lesson_id" @@ -229,15 +239,50 @@ create_table "school_classes", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "school_id", null: false - t.uuid "teacher_id", null: false t.string "name", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "description" - t.index ["school_id", "teacher_id"], name: "index_school_classes_on_school_id_and_teacher_id" + t.string "code" + t.integer "import_origin" + t.string "import_id" + t.index ["code", "school_id"], name: "index_school_classes_on_code_and_school_id", unique: true t.index ["school_id"], name: "index_school_classes_on_school_id" end + create_table "school_import_results", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "job_id", null: false + t.uuid "user_id", null: false + t.jsonb "results", default: {}, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["job_id"], name: "index_school_import_results_on_job_id", unique: true + t.index ["user_id"], name: "index_school_import_results_on_user_id" + end + + create_table "school_project_transitions", force: :cascade do |t| + t.string "from_state", null: false + t.string "to_state", null: false + t.text "metadata", default: "{}" + t.integer "sort_key", null: false + t.uuid "school_project_id", null: false + t.boolean "most_recent", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["school_project_id", "most_recent"], name: "index_school_project_transitions_parent_most_recent", unique: true, where: "most_recent" + t.index ["school_project_id", "sort_key"], name: "index_school_project_transitions_parent_sort", unique: true + end + + create_table "school_projects", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "school_id" + t.uuid "project_id", null: false + t.boolean "finished", default: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["project_id"], name: "index_school_projects_on_project_id" + t.index ["school_id"], name: "index_school_projects_on_school_id" + end + create_table "schools", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "name", null: false t.string "reference" @@ -280,9 +325,10 @@ 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.uuid "good_job_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.uuid "good_job_batch_id" t.index ["user_id", "good_job_id"], name: "index_user_jobs_on_user_id_and_good_job_id", unique: true end @@ -296,14 +342,16 @@ t.uuid "meta_project_id" t.uuid "meta_school_id" t.uuid "meta_remixed_from_id" + t.string "meta_school_project_id" t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id" end add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" - add_foreign_key "class_members", "school_classes" + add_foreign_key "class_students", "school_classes" add_foreign_key "class_teachers", "school_classes" add_foreign_key "components", "projects" + add_foreign_key "feedback", "school_projects" add_foreign_key "lessons", "lessons", column: "copied_from_id" add_foreign_key "lessons", "school_classes" add_foreign_key "lessons", "schools" @@ -312,6 +360,9 @@ add_foreign_key "projects", "schools" add_foreign_key "roles", "schools" add_foreign_key "school_classes", "schools" + add_foreign_key "school_project_transitions", "school_projects" + 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/operations/import_batch.rb b/lib/concepts/school/operations/import_batch.rb new file mode 100644 index 000000000..3cd880b45 --- /dev/null +++ b/lib/concepts/school/operations/import_batch.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true + +class School + class ImportBatch + class << self + def call(csv_file:, current_user:) + response = OperationResponse.new + + parsed_schools = parse_csv(csv_file) + + if parsed_schools[:error] + response[:error] = parsed_schools[:error] + return response + end + + # Check for duplicate owner emails in the CSV + duplicate_check = check_duplicate_owners(parsed_schools[:schools]) + if duplicate_check[:error] + response[:error] = duplicate_check[:error] + return response + end + + job = enqueue_import_job( + schools_data: parsed_schools[:schools], + user_id: current_user.id + ) + + response[:job_id] = job.job_id + response[:total_schools] = parsed_schools[:schools].length + response + rescue StandardError => e + Sentry.capture_exception(e) + response ||= OperationResponse.new + response[:error] = SchoolImportError.format_error(:unknown_error, e.message) + response + end + + private + + def check_duplicate_owners(schools) + owner_emails = schools.filter_map { |s| s[:owner_email]&.strip&.downcase } + duplicates = owner_emails.select { |e| owner_emails.count(e) > 1 }.uniq + + if duplicates.any? + error = SchoolImportError.format_error( + :duplicate_owner_email, + 'Duplicate owner emails found in CSV', + { duplicate_emails: duplicates } + ) + return { error: error } + end + + {} + end + + def parse_csv(csv_file) + require 'csv' + + csv_content = csv_file.read + return { error: SchoolImportError.format_error(:csv_invalid_format, 'CSV file is empty') } if csv_content.blank? + + csv_data = CSV.parse(csv_content, headers: true, header_converters: :symbol) + + if csv_data.headers.nil? || !valid_headers?(csv_data.headers) + return { + error: SchoolImportError.format_error( + :csv_invalid_format, + 'Invalid CSV format. Required headers: name, website, address_line_1, municipality, country_code, owner_email' + ) + } + end + + process_csv_rows(csv_data) + rescue CSV::MalformedCSVError => e + { error: SchoolImportError.format_error(:csv_malformed, "Invalid CSV file format: #{e.message}") } + rescue StandardError => e + Sentry.capture_exception(e) + { error: SchoolImportError.format_error(:unknown_error, "Failed to parse CSV: #{e.message}") } + end + + def process_csv_rows(csv_data) + schools = [] + errors = [] + + csv_data.each_with_index do |row, index| + row_number = index + 2 # +2 because index starts at 0 and we skip header row + school_data = row.to_h + + validation_errors = validate_school_data(school_data, row_number) + if validation_errors + errors << validation_errors + next + end + + schools << school_data + end + + if errors.any? + { error: SchoolImportError.format_row_errors(errors) } + else + { schools: schools } + end + end + + def valid_headers?(headers) + required = %i[name website address_line_1 municipality country_code owner_email] + required.all? { |h| headers.include?(h) } + end + + def validate_school_data(data, row_number) + errors = [] + + # Strip whitespace from all string fields + data.each do |key, value| + data[key] = value.strip if value.is_a?(String) + end + + # Validate required fields + required_fields = %i[name website address_line_1 municipality country_code owner_email] + required_fields.each do |field| + errors << { field: field.to_s, message: 'is required' } if data[field].blank? + end + + # Validate field formats + validate_country_code(data, errors) + validate_website_format(data, errors) + validate_email_format(data, errors) + + return nil if errors.empty? + + { row: row_number, errors: errors } + end + + def validate_country_code(data, errors) + return if data[:country_code].blank? + return if ISO3166::Country.codes.include?(data[:country_code].upcase) + + errors << { field: 'country_code', message: "invalid code: #{data[:country_code]}" } + end + + def validate_website_format(data, errors) + return if data[:website].blank? + return if data[:website].match?(School::VALID_URL_REGEX) + + errors << { field: 'website', message: 'invalid format' } + end + + def validate_email_format(data, errors) + return if data[:owner_email].blank? + return if data[:owner_email].match?(URI::MailTo::EMAIL_REGEXP) + + errors << { field: 'owner_email', message: 'invalid email format' } + end + + def enqueue_import_job(schools_data:, user_id:) + SchoolImportJob.perform_later( + schools_data: schools_data, + user_id: user_id + ) + end + end + end +end diff --git a/spec/concepts/school/import_batch_spec.rb b/spec/concepts/school/import_batch_spec.rb new file mode 100644 index 000000000..8da53a947 --- /dev/null +++ b/spec/concepts/school/import_batch_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe School::ImportBatch do + describe '.call' do + let(:current_user) { instance_double(User, id: SecureRandom.uuid, token: 'test-token') } + let(:csv_content) do + <<~CSV + name,website,address_line_1,municipality,country_code,owner_email + Test School 1,https://test1.example.com,123 Main St,Springfield,US,owner1@example.com + Test School 2,https://test2.example.com,456 Oak Ave,Boston,US,owner2@example.com + CSV + end + let(:csv_file) { StringIO.new(csv_content) } + + before do + allow(SchoolImportJob).to receive(:perform_later).and_return( + instance_double(SchoolImportJob, job_id: 'test-job-id') + ) + end + + context 'with valid CSV' do + it 'returns success response with job_id' do + result = described_class.call(csv_file: csv_file, current_user: current_user) + + expect(result.success?).to be true + expect(result[:job_id]).to eq('test-job-id') + expect(result[:total_schools]).to eq(2) + end + + it 'enqueues SchoolImportJob' do + described_class.call(csv_file: csv_file, current_user: current_user) + + expect(SchoolImportJob).to have_received(:perform_later).with( + hash_including( + schools_data: array_including( + hash_including(name: 'Test School 1'), + hash_including(name: 'Test School 2') + ), + user_id: current_user.id + ) + ) + end + end + + context 'with missing required headers' do + let(:csv_content) do + <<~CSV + name,website + Test School,https://test.example.com + CSV + end + + it 'returns error response' do + result = described_class.call(csv_file: csv_file, current_user: current_user) + + expect(result.failure?).to be true + expect(result[:error][:error_code]).to eq('CSV_INVALID_FORMAT') + expect(result[:error][:message]).to include('Invalid CSV format') + end + end + + context 'with invalid country code' do + let(:csv_content) do + <<~CSV + name,website,address_line_1,municipality,country_code,owner_email + Test School,https://test.example.com,123 Main St,Springfield,INVALID,owner@example.com + CSV + end + + it 'returns validation error' do + result = described_class.call(csv_file: csv_file, current_user: current_user) + + expect(result.failure?).to be true + expect(result[:error][:error_code]).to eq('CSV_VALIDATION_FAILED') + expect(result[:error][:details][:row_errors]).to be_present + end + end + + context 'with malformed CSV' do + let(:csv_file) { StringIO.new('this is not csv,,"') } + + it 'returns error response' do + result = described_class.call(csv_file: csv_file, current_user: current_user) + + expect(result.failure?).to be true + expect(result[:error][:error_code]).to eq('CSV_MALFORMED') + end + end + end +end From b2d5ece719a757d47ae6505c853458c0ba74dc42 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 20 Nov 2025 10:13:51 +0000 Subject: [PATCH 05/21] Update/Add controllers for school import and job tracking. This commit modifies SchoolsController to add the endpoint for importing schools. It introduces SchoolImportJobsController as the controller for the endpoint though which clients can track job status. --- .../api/school_import_jobs_controller.rb | 70 +++++++++++++++++++ app/controllers/api/schools_controller.rb | 26 +++++++ 2 files changed, 96 insertions(+) create mode 100644 app/controllers/api/school_import_jobs_controller.rb diff --git a/app/controllers/api/school_import_jobs_controller.rb b/app/controllers/api/school_import_jobs_controller.rb new file mode 100644 index 000000000..1af6d1215 --- /dev/null +++ b/app/controllers/api/school_import_jobs_controller.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Api + class SchoolImportJobsController < ApiController + before_action :authorize_user + + def show + authorize! :read, :school_import_job + job = find_job + + if job.nil? + render json: SchoolImportError.format_error(:job_not_found, 'Job not found'), status: :not_found + return + end + + render json: build_job_response(job), status: :ok + end + + private + + def find_job + # GoodJob stores jobs in the good_jobs table as Executions + # The job_id returned to users is the ActiveJob ID + # We need to query by active_job_id, not by execution id + job = GoodJob::Execution.find_by(active_job_id: params[:id]) + + # Verify this is an import job (security check) + return nil unless job && job.job_class == SchoolImportJob.name + + job + end + + def build_job_response(job) + response = { + id: job.active_job_id, + status: job_status(job), + created_at: job.created_at, + finished_at: job.finished_at, + job_class: job.job_class + } + + # If job is finished successfully, get results from dedicated table + if job.finished_at.present? && job.error.blank? + result = SchoolImportResult.find_by(job_id: job.active_job_id) + if result + response[:results] = result.results + else + response[:message] = 'Job completed successfully' + end + end + + # Include error if job failed + if job.error.present? + response[:error] = job.error + response[:status] = 'failed' + end + + response + end + + def job_status(job) + return 'failed' if job.error.present? + return 'completed' if job.finished_at.present? + return 'scheduled' if job.scheduled_at.present? && job.scheduled_at > Time.current + return 'running' if job.performed_at.present? && job.finished_at.nil? + + 'queued' + end + end +end diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index 6f50aed0d..9b6bb2cf1 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -4,6 +4,7 @@ module Api class SchoolsController < ApiController before_action :authorize_user load_and_authorize_resource + skip_load_and_authorize_resource only: :import def index @schools = School.accessible_by(current_ability) @@ -47,6 +48,31 @@ def destroy end end + def import + authorize! :import, School + + if params[:csv_file].blank? + render json: SchoolImportError.format_error(:csv_file_required, 'CSV file is required'), + status: :unprocessable_entity + return + end + + result = School::ImportBatch.call( + csv_file: params[:csv_file], + current_user: current_user + ) + + if result.success? + render json: { + job_id: result[:job_id], + total_schools: result[:total_schools], + message: 'Import job started successfully' + }, status: :accepted + else + render json: result[:error], status: :unprocessable_entity + end + end + private def school_params From d8115ad496b40acee51c623141648bceb01187bb Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 20 Nov 2025 10:15:31 +0000 Subject: [PATCH 06/21] Test coverage for CSV school import. These tests cover the core functionality and some additional complex edge cases. --- .../features/school/importing_schools_spec.rb | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 spec/features/school/importing_schools_spec.rb diff --git a/spec/features/school/importing_schools_spec.rb b/spec/features/school/importing_schools_spec.rb new file mode 100644 index 000000000..8f1a0c7dc --- /dev/null +++ b/spec/features/school/importing_schools_spec.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Importing schools', type: :request do + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + + let(:csv_content) do + <<~CSV + name,website,address_line_1,municipality,country_code,owner_email + Test School 1,https://test1.example.com,123 Main St,Springfield,US,owner1@example.com + Test School 2,https://test2.example.com,456 Oak Ave,Boston,US,owner2@example.com + CSV + end + + describe 'POST /api/schools/import' do + let(:csv_file) do + tempfile = Tempfile.new(['schools', '.csv']) + tempfile.write(csv_content) + tempfile.rewind + Rack::Test::UploadedFile.new(tempfile.path, 'text/csv') + end + + context 'when user is an experience_cs_admin' do + let(:admin_user) { create(:user, roles: 'experience-cs-admin') } + + before do + authenticated_in_hydra_as(admin_user) + allow(UserInfoApiClient).to receive(:find_user_by_email).and_return({ id: SecureRandom.uuid, email: 'owner@example.com' }) + allow(SchoolImportJob).to receive(:perform_later).and_return(instance_double(SchoolImportJob, job_id: 'test-job-id')) + end + + it 'accepts CSV file and returns 202 Accepted' do + post('/api/schools/import', headers:, params: { csv_file: csv_file }) + + expect(response).to have_http_status(:accepted) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:job_id]).to eq('test-job-id') + expect(data[:total_schools]).to eq(2) + end + end + + context 'when CSV file is missing' do + let(:admin_user) { create(:user, roles: 'experience-cs-admin') } + + before do + authenticated_in_hydra_as(admin_user) + end + + it 'responds 422 Unprocessable Entity' do + post('/api/schools/import', headers:) + + expect(response).to have_http_status(:unprocessable_entity) + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:error_code]).to eq('CSV_FILE_REQUIRED') + end + end + + context 'when user is not an admin' do + let(:regular_user) { create(:user, roles: '') } + + before do + authenticated_in_hydra_as(regular_user) + end + + it 'responds 403 Forbidden' do + post('/api/schools/import', headers:, params: { csv_file: csv_file }) + + expect(response).to have_http_status(:forbidden) + end + end + end + + describe 'GET /api/school_import_jobs/:id' do + let(:admin_user) { create(:user, roles: 'experience-cs-admin') } + let(:job_id) { SecureRandom.uuid } + + before do + authenticated_in_hydra_as(admin_user) + end + + context 'when job exists and is completed' do + before do + GoodJob::Execution.create!( + id: SecureRandom.uuid, + active_job_id: job_id, + queue_name: 'import_schools_job', + job_class: 'SchoolImportJob', + serialized_params: {}, + created_at: 1.hour.ago, + finished_at: Time.current + ) + + SchoolImportResult.create!( + job_id: job_id, + user_id: admin_user.id, + results: { + successful: [{ name: 'Test School', id: SecureRandom.uuid, code: '12-34-56' }], + failed: [] + } + ) + end + + it 'returns job status with results' do + get("/api/school_import_jobs/#{job_id}", headers:) + + expect(response).to have_http_status(:ok) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:status]).to eq('completed') + expect(data[:results][:successful].count).to eq(1) + end + end + + context 'when job does not exist' do + it 'returns 404' do + get("/api/school_import_jobs/#{SecureRandom.uuid}", headers:) + + expect(response).to have_http_status(:not_found) + end + end + + context 'when user is not an admin' do + let(:regular_user) { create(:user) } + + before do + authenticated_in_hydra_as(regular_user) + end + + it 'responds 403 Forbidden' do + get("/api/school_import_jobs/#{job_id}", headers:) + + expect(response).to have_http_status(:forbidden) + end + end + end +end From 2e7d32b8b1d9478831d6480f417fc03f7e1182c4 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 20 Nov 2025 10:28:01 +0000 Subject: [PATCH 07/21] Add documentation - CSM guide and example CSV template --- docs/import/school_import_csm_guide.md | 128 +++++++++++++++++++++++++ docs/import/school_import_template.csv | 4 + 2 files changed, 132 insertions(+) create mode 100644 docs/import/school_import_csm_guide.md create mode 100644 docs/import/school_import_template.csv diff --git a/docs/import/school_import_csm_guide.md b/docs/import/school_import_csm_guide.md new file mode 100644 index 000000000..87520eb11 --- /dev/null +++ b/docs/import/school_import_csm_guide.md @@ -0,0 +1,128 @@ +# School Import Quick Reference for Customer Success Managers + +## Prerequisites + +1. School owners must have existing Code Editor for Education accounts. +3. School owners must be unique within the CSV, and in the existing database. +4. CSV file must be properly formatted (see template) + +## Step-by-Step Process + +### 1. Prepare the CSV File + +Download the template: `docs/school_import_template.csv` + +Required columns: +- `name` - School name +- `website` - School website URL (e.g., https://example.edu) +- `address_line_1` - Street address +- `municipality` - City/town +- `country_code` - 2-letter code (US, GB, CA, etc.) +- `owner_email` - Email of school owner (must exist in Profile) + +Optional columns: +- `address_line_2` - Additional address info +- `administrative_area` - State/province +- `postal_code` - ZIP/postal code +- `reference` - School reference number (must be unique if provided) + +### 2. Validate Owner Emails + +Before importing, verify that all owner emails in your CSV correspond to existing accounts in Code Editor for Education. Owners without accounts will cause those schools to fail during import, but schools with correct owners will succeed. + +### 3. Upload the CSV + +Visit the `/admin` page and select **School Import Results**. On this page, click the **New Import** button. + +On the following page, you can click **Choose file** to select a CSV, then choose **Upload and Start Import** to import the CSV. + +### 4. Track Progress + +You can refresh the **School Import Results** page to see the status of your upload. It should not take very long to complete. + +### 5. Review Results + +On the **School Import Results** page, any administrator can see the history of successful uploads and inspect any particular upload. A brief summary of the number of schools that succeeded and failed is shown in the table. + +Click on a row in the table to inspect an upload futher. In the following page, you can also download a CSV of the results which will include the system-generated school code. + +### 6. Handle Failures + +Common failure reasons and solutions: + +| Error Code | Error Message | Solution | +|------------|---------------|----------| +| `OWNER_NOT_FOUND` | Owner not found: email@example.com | Verify owner has CEfE account, create if needed | +| `OWNER_ALREADY_CREATOR` | Owner is already the creator of school 'X' | Each person can only create one school; use different owner | +| `SCHOOL_VALIDATION_FAILED` | Validation errors | Check country code, website format, required fields | +| `CSV_VALIDATION_FAILED` | CSV validation failed | Check error details for specific row and field errors | +| `DUPLICATE_OWNER_EMAIL` | Duplicate owner emails found in CSV | Same owner email appears multiple times - only one school per owner allowed | + +For failed schools, you can either: +1. Fix the issues and re-import (only failed schools) +2. Create them manually through the standard process + +## Important Notes + +- **Automatic Verification**: Imported schools are automatically verified and receive school codes +- **Owner Roles**: Owner roles are automatically created +- **One Owner Per School**: Each owner can only create one school (enforced at database level) +- **No Duplicate Owners in CSV**: The CSV cannot contain the same owner email multiple times +- **No Teacher Creation**: Teachers are NOT created during import - they must be invited separately +- **Country Codes**: Use ISO 3166-1 alpha-2 codes (find at https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2) +- **Structured Errors**: All errors include error codes for programmatic handling + +## Common Country Codes + +| Country | Code | +|---------|------| +| United States | US | +| United Kingdom | GB | +| Canada | CA | +| Mexico | MX | + +## Troubleshooting + +### CSV Validation Fails Immediately + +The system validates the entire CSV before starting the import. If validation fails, you'll receive an error immediately on the upload page. + +To fix: +1. Check that all required headers are present (case-sensitive) +2. Verify all required fields have values for every row +3. Check country codes are valid 2-letter codes (US not USA) +4. Verify website URLs are properly formatted (must include https://) +5. Look for the row number in the error details + +### Import Succeeds But Some Schools Failed + +This is normal - the system processes all schools it can. Review the failed list and address issues individually. + +### Duplicate Owner Emails in CSV + +If your CSV contains the same owner email more than once, the import will be rejected before processing. + +**Solution**: Each owner can only create one school. Either: +- Use different owner emails for each school +- Have one owner create their school first, then import the rest +- Remove duplicate entries and import in batches + +### All Schools Failed + +Common causes: +- Owner emails don't match any accounts +- CSV formatting issues +- Network/system errors (check with technical team) + +## Getting Help + +If you encounter issues: + +1. Check the error message and error_code for specific details +2. Verify your CSV matches the template format +3. Confirm all owner emails are valid accounts +4. Ensure no duplicate owner emails in your CSV +5. Contact the technical team with: + - The job_id + - The CSV file (or sample rows) + - Error codes and messages received diff --git a/docs/import/school_import_template.csv b/docs/import/school_import_template.csv new file mode 100644 index 000000000..c5412ead6 --- /dev/null +++ b/docs/import/school_import_template.csv @@ -0,0 +1,4 @@ +name,website,address_line_1,address_line_2,municipality,administrative_area,postal_code,country_code,reference,owner_email +Springfield Elementary School,https://springfield-elem.edu,123 Main Street,,Springfield,IL,62701,US,SPE-001,principal@springfield-elem.edu +Shelbyville High School,https://shelbyville-high.edu,456 Oak Avenue,Suite 100,Shelbyville,IL,62565,US,SHS-002,admin@shelbyville-high.edu +Capital City Academy,https://capital-city-academy.edu,789 State Road,,Capital City,IL,62702,US,CCA-003,headteacher@capital-city-academy.edu From 3e834a2efc5deabce8444ef867fff212c1ef8134 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 27 Nov 2025 10:52:48 +0000 Subject: [PATCH 08/21] Add a new Administrate page to allow and track uploads. This commit adds new UI in /admin to allow users to upload CSVs of schools and track the results of the upload jobs in the UI. They can also download CSV files of the resulting school structures. Add index, show and new views for Administrate Add SchoolImportResultDashboard Add routes for admin interface for school_import_results Add SchoolImportResultsController to show results in the UI. This commit also adds code to new.html.erb to show validation errors if the uploaded CSV can't be enqueued as a batch. --- .../admin/school_import_results_controller.rb | 151 ++++++++++++++ .../school_import_result_dashboard.rb | 38 ++++ app/fields/results_summary_field.rb | 21 ++ app/fields/status_field.rb | 33 ++++ app/fields/user_info_field.rb | 67 +++++++ .../school_import_results/index.html.erb | 43 ++++ .../admin/school_import_results/new.html.erb | 165 ++++++++++++++++ .../admin/school_import_results/show.html.erb | 166 ++++++++++++++++ .../results_summary_field/_index.html.erb | 6 + .../results_summary_field/_show.html.erb | 6 + app/views/fields/status_field/_index.html.erb | 6 + app/views/fields/status_field/_show.html.erb | 6 + .../fields/user_info_field/_index.html.erb | 4 + .../fields/user_info_field/_show.html.erb | 4 + config/routes.rb | 1 + .../admin/school_import_results_spec.rb | 186 ++++++++++++++++++ 16 files changed, 903 insertions(+) create mode 100644 app/controllers/admin/school_import_results_controller.rb create mode 100644 app/dashboards/school_import_result_dashboard.rb create mode 100644 app/fields/results_summary_field.rb create mode 100644 app/fields/status_field.rb create mode 100644 app/fields/user_info_field.rb create mode 100644 app/views/admin/school_import_results/index.html.erb create mode 100644 app/views/admin/school_import_results/new.html.erb create mode 100644 app/views/admin/school_import_results/show.html.erb create mode 100644 app/views/fields/results_summary_field/_index.html.erb create mode 100644 app/views/fields/results_summary_field/_show.html.erb create mode 100644 app/views/fields/status_field/_index.html.erb create mode 100644 app/views/fields/status_field/_show.html.erb create mode 100644 app/views/fields/user_info_field/_index.html.erb create mode 100644 app/views/fields/user_info_field/_show.html.erb create mode 100644 spec/requests/admin/school_import_results_spec.rb diff --git a/app/controllers/admin/school_import_results_controller.rb b/app/controllers/admin/school_import_results_controller.rb new file mode 100644 index 000000000..77d25bdef --- /dev/null +++ b/app/controllers/admin/school_import_results_controller.rb @@ -0,0 +1,151 @@ +# frozen_string_literal: true + +require 'csv' + +module Admin + class SchoolImportResultsController < Admin::ApplicationController + def index + search_term = params[:search].to_s.strip + resources = Administrate::Search.new( + SchoolImportResult.all, + dashboard_class, + search_term + ).run + resources = apply_collection_includes(resources) + resources = order.apply(resources) + resources = resources.page(params[:_page]).per(records_per_page) + + # Batch load user info to avoid N+1 queries + user_ids = resources.filter_map(&:user_id).uniq + RequestStore.store[:user_info_cache] = fetch_users_batch(user_ids) + + page = Administrate::Page::Collection.new(dashboard, order: order) + + render locals: { + resources: resources, + search_term: search_term, + page: page, + show_search_bar: show_search_bar? + } + end + + def show + respond_to do |format| + format.html do + render locals: { + page: Administrate::Page::Show.new(dashboard, requested_resource) + } + end + format.csv do + send_data generate_csv(requested_resource), + filename: "school_import_#{requested_resource.job_id}_#{Date.current.strftime('%Y-%m-%d')}.csv", + type: 'text/csv' + end + end + end + + def new + @error_details = flash[:error_details] + render locals: { + page: Administrate::Page::Form.new(dashboard, SchoolImportResult.new) + } + end + + def create + if params[:csv_file].blank? + flash[:error] = 'CSV file is required' + redirect_to new_admin_school_import_result_path + return + end + + # Call the same service that the API endpoint uses, ensuring all validation is applied + result = School::ImportBatch.call( + csv_file: params[:csv_file], + current_user: current_user + ) + + if result.success? + flash[:notice] = "Import job started successfully. Job ID: #{result[:job_id]}" + redirect_to admin_school_import_results_path + else + # Display error inline on the page + flash.now[:error] = format_error_message(result[:error]) + @error_details = extract_error_details(result[:error]) + render :new, locals: { + page: Administrate::Page::Form.new(dashboard, SchoolImportResult.new) + } + end + end + + private + + def default_sorting_attribute + :created_at + end + + def default_sorting_direction + :desc + end + + def format_error_message(error) + return error.to_s unless error.is_a?(Hash) + + error[:message] || error['message'] || 'Import failed' + end + + def extract_error_details(error) + return nil unless error.is_a?(Hash) + + error[:details] || error['details'] + end + + def generate_csv(import_result) + CSV.generate(headers: true) do |csv| + # Header row + csv << ['Status', 'School Name', 'School Code', 'School ID', 'Owner Email', 'Error Code', 'Error Message'] + + results = import_result.results + successful = results['successful'] || [] + failed = results['failed'] || [] + + # Successful schools + successful.each do |school| + csv << [ + 'Success', + school['name'], + school['code'], + school['id'], + school['owner_email'], + '', + '' + ] + end + + # Failed schools + failed.each do |school| + csv << [ + 'Failed', + school['name'], + '', + '', + school['owner_email'], + school['error_code'], + school['error'] + ] + end + end + end + + def fetch_users_batch(user_ids) + return {} if user_ids.empty? + + users = UserInfoApiClient.fetch_by_ids(user_ids) + users.index_by do |user| + user[:id] + end + rescue StandardError => e + Rails.logger.error("Failed to batch fetch user info: #{e.message}") + {} + end + end +end diff --git a/app/dashboards/school_import_result_dashboard.rb b/app/dashboards/school_import_result_dashboard.rb new file mode 100644 index 000000000..0f0a42c86 --- /dev/null +++ b/app/dashboards/school_import_result_dashboard.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'administrate/base_dashboard' + +class SchoolImportResultDashboard < Administrate::BaseDashboard + ATTRIBUTE_TYPES = { + id: Field::String, + job_id: StatusField, + user_id: UserInfoField, + results: ResultsSummaryField, + created_at: Field::DateTime, + updated_at: Field::DateTime + }.freeze + + COLLECTION_ATTRIBUTES = %i[ + job_id + user_id + results + created_at + ].freeze + + SHOW_PAGE_ATTRIBUTES = %i[ + id + job_id + user_id + results + created_at + updated_at + ].freeze + + FORM_ATTRIBUTES = [].freeze + + COLLECTION_FILTERS = {}.freeze + + def display_resource(school_import_result) + "Import Job #{school_import_result.job_id}" + end +end diff --git a/app/fields/results_summary_field.rb b/app/fields/results_summary_field.rb new file mode 100644 index 000000000..d054078c9 --- /dev/null +++ b/app/fields/results_summary_field.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'administrate/field/base' + +class ResultsSummaryField < Administrate::Field::Base + def to_s + "#{successful_count} successful, #{failed_count} failed" + end + + def successful_count + data['successful']&.count || 0 + end + + def failed_count + data['failed']&.count || 0 + end + + def total_count + successful_count + failed_count + end +end diff --git a/app/fields/status_field.rb b/app/fields/status_field.rb new file mode 100644 index 000000000..2ec352f51 --- /dev/null +++ b/app/fields/status_field.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'administrate/field/base' + +class StatusField < Administrate::Field::Base + def to_s + job_status + end + + def job_status + return 'unknown' if data.blank? + + job = GoodJob::Execution.find_by(active_job_id: data) + return 'not_found' unless job + + return 'failed' if job.error.present? + return 'completed' if job.finished_at.present? + return 'scheduled' if job.scheduled_at.present? && job.scheduled_at > Time.current + return 'running' if job.performed_at.present? && job.finished_at.nil? + + 'queued' + end + + def status_class + case job_status + when 'completed' then 'status-completed' + when 'failed' then 'status-failed' + when 'running' then 'status-running' + when 'queued', 'scheduled' then 'status-queued' + else 'status-unknown' + end + end +end diff --git a/app/fields/user_info_field.rb b/app/fields/user_info_field.rb new file mode 100644 index 000000000..c329561c9 --- /dev/null +++ b/app/fields/user_info_field.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'administrate/field/base' + +class UserInfoField < Administrate::Field::Base + def to_s + user_display + end + + def user_display + return 'Unknown User' if data.blank? + + user_info = fetch_user_info + return data if user_info.nil? + + if user_info[:name].present? && user_info[:email].present? + "#{user_info[:name]} <#{user_info[:email]}>" + elsif user_info[:name].present? + user_info[:name] + elsif user_info[:email].present? + user_info[:email] + else + data + end + end + + def user_name + return nil if data.blank? + + user_info = fetch_user_info + user_info&.dig(:name) + end + + def user_email + return nil if data.blank? + + user_info = fetch_user_info + user_info&.dig(:email) + end + + def user_id + data + end + + private + + def fetch_user_info + return @fetch_user_info if defined?(@fetch_user_info) + + @fetch_user_info = begin + # Try to get from request-level cache first (set by controller) + cache = RequestStore.store[:user_info_cache] || {} + cached = cache[data] + + if cached + cached + else + # Fallback to individual API call if not in cache + result = UserInfoApiClient.fetch_by_ids([data]) + result&.first + end + rescue StandardError => e + Rails.logger.error("Failed to fetch user info for #{data}: #{e.message}") + nil + end + end +end diff --git a/app/views/admin/school_import_results/index.html.erb b/app/views/admin/school_import_results/index.html.erb new file mode 100644 index 000000000..0a601f2f4 --- /dev/null +++ b/app/views/admin/school_import_results/index.html.erb @@ -0,0 +1,43 @@ +<%# +# Index + +This view is the template for the index page. +It renders the search bar, header and pagination. +It renders the `_table` partial to display details about the resources. + +## Local variables: + +- `resources`: + An ActiveModel::Relation collection of resources to be displayed in the table. + By default, the number of resources is limited by pagination + or by a hard limit to prevent excessive page load times +%> + +<% content_for(:title) { "School Import History" } %> + +
+

+ <%= content_for(:title) %> +

+ +
+ <%= link_to( + "New Import", + new_admin_school_import_result_path, + class: "button button--primary", + ) %> +
+
+ +
+ <%= render( + "collection", + collection_presenter: page, + collection_field_name: :resources, + page: page, + resources: resources, + table_title: "page_title" + ) %> + + <%= paginate resources, param_name: :_page %> +
diff --git a/app/views/admin/school_import_results/new.html.erb b/app/views/admin/school_import_results/new.html.erb new file mode 100644 index 000000000..b2b1810a6 --- /dev/null +++ b/app/views/admin/school_import_results/new.html.erb @@ -0,0 +1,165 @@ +<% content_for(:title) { "Upload School Import CSV" } %> + +
+

+ <%= content_for(:title) %> +

+
+ <%= link_to( + "View Import History", + admin_school_import_results_path, + class: "button" + ) %> +
+
+ +
+ <% if flash[:error] %> +
+ <%= flash[:error] %> + + <% if @error_details %> + <% if @error_details[:row_errors] %> +
+ Row Errors: + + + + + + + + + <% @error_details[:row_errors].each do |error| %> + + + + + <% end %> + +
RowErrors
<%= error[:row] || error['row'] %> + <% errors = error[:errors] || error['errors'] || [] %> + <% errors.each do |err| %> +
<%= err[:field] || err['field'] %>: <%= err[:message] || err['message'] %>
+ <% end %> +
+
+ <% elsif @error_details[:duplicate_emails] %> +
+ Duplicate Owner Emails Found: +
    + <% @error_details[:duplicate_emails].each do |email| %> +
  • <%= email %>
  • + <% end %> +
+
+ <% end %> + <% end %> +
+ <% end %> + +
+

+ Use this form to import multiple schools from a CSV file. + The import will run in the background and you can track its progress on the Import History page. +

+
+ +
+ + <%= form_with url: admin_school_import_results_path, method: :post, multipart: true, class: "form" do |f| %> +
+ <%= f.label :csv_file, "CSV File", class: "field-unit__label" %> + <%= f.file_field :csv_file, accept: ".csv", required: true, class: "field-unit__field" %> +

+ Select a CSV file containing school data to import. + Download the <%= link_to "CSV template", "/docs/import/school_import_template.csv" %> to see the required format. +

+
+ +
+ <%= f.submit "Upload and Start Import", class: "button button--primary" %> +
+ <% end %> + +
+ +
+

CSV Format Requirements

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
ColumnRequiredDescription
nameYesSchool name
websiteYesSchool website URL (must include https://)
address_line_1YesStreet address
municipalityYesCity/town
country_codeYes2-letter ISO country code (e.g., US, GB, CA)
owner_emailYesEmail of school owner (must have existing RPF account)
address_line_2NoAdditional address information
administrative_areaNoState/province
postal_codeNoZIP/postal code
referenceNoSchool reference number (must be unique if provided)
+
+ +
+

Important Notes

+
+
+
    +
  • All owner emails must correspond to existing Code Editor for Education accounts.
  • +
  • Each owner can only create one school.
  • +
  • The same owner email cannot appear multiple times in the CSV.
  • +
  • Imported schools are automatically verified and receive school codes.
  • +
  • Owner roles are automatically created for each school.
  • +
  • The import runs in the background and may take several minutes for large files.
  • +
+
+
diff --git a/app/views/admin/school_import_results/show.html.erb b/app/views/admin/school_import_results/show.html.erb new file mode 100644 index 000000000..cfe872a17 --- /dev/null +++ b/app/views/admin/school_import_results/show.html.erb @@ -0,0 +1,166 @@ +<% content_for(:title) { "School Import Job #{page.resource.job_id}" } %> + +
+

+ <%= content_for(:title) %> +

+
+ <%= link_to( + "Download CSV", + admin_school_import_result_path(page.resource, format: :csv), + class: "button button--primary", + style: "margin-right: 10px;" + ) %> + <%= link_to( + "Back to Import History", + admin_school_import_results_path, + class: "button" + ) %> +
+
+ +
+
+
Job ID
+
<%= page.resource.job_id %>
+ +
User
+
+ <% user_field = UserInfoField.new(:user_id, page.resource.user_id, "show") %> + <%= user_field.user_display %> +
+ +
Status
+
+ <% status_field = StatusField.new(:job_id, page.resource.job_id, "show") %> + + <%= status_field.job_status.upcase %> + +
+ +
Created At
+
<%= page.resource.created_at.strftime("%B %d, %Y at %I:%M %p") %>
+ + <% if page.resource.updated_at != page.resource.created_at %> +
Updated At
+
<%= page.resource.updated_at.strftime("%B %d, %Y at %I:%M %p") %>
+ <% end %> +
+ + <% results = page.resource.results %> + <% if results.present? %> +
+
Results
+
+ <% results_field = ResultsSummaryField.new(:results, page.resource.results, "show") %> + <%= render "fields/results_summary_field/show", field: results_field %> +
+
+ + <% successful = results['successful'] || [] %> + <% failed = results['failed'] || [] %> + + <% if successful.any? %> +

Successful Imports (<%= successful.count %>)

+ + + + + + + + + + + + <% successful.each do |school| %> + + + + + + + + <% end %> + +
School NameSchool CodeSchool IDOwner EmailActions
<%= school['name'] %><%= school['code'] %><%= school['id'] %><%= school['owner_email'] %> + <%= link_to "View School", admin_school_path(school['id']), class: "button button--small" if school['id'] %> +
+ <% end %> + + <% if failed.any? %> +

Failed Imports (<%= failed.count %>)

+ + + + + + + + + + + <% failed.each do |school| %> + + + + + + + <% end %> + +
School NameOwner EmailError CodeError Message
<%= school['name'] %><%= school['owner_email'] %><%= school['error_code'] %><%= school['error'] %>
+ +
+

Common Error Codes

+
+
OWNER_NOT_FOUND
+
The owner email doesn't correspond to an existing account. The owner needs to create a Code Editor for Education account first.
+ +
OWNER_ALREADY_CREATOR
+
This owner has already created a school. Each person can only create one school.
+ +
SCHOOL_VALIDATION_FAILED
+
The school data failed validation. Check the error message for details about which fields are invalid.
+
+
+ <% end %> + <% else %> +
+

No results available yet. The job may still be running or may have failed to store results.

+
+ <% end %> +
+ + diff --git a/app/views/fields/results_summary_field/_index.html.erb b/app/views/fields/results_summary_field/_index.html.erb new file mode 100644 index 000000000..563ba43e6 --- /dev/null +++ b/app/views/fields/results_summary_field/_index.html.erb @@ -0,0 +1,6 @@ +<%# +Administrate field partial for ResultsSummaryField in index view +%> +<%= field.successful_count %> ✓ +/ +<%= field.failed_count %> ✗ diff --git a/app/views/fields/results_summary_field/_show.html.erb b/app/views/fields/results_summary_field/_show.html.erb new file mode 100644 index 000000000..fa41d2d45 --- /dev/null +++ b/app/views/fields/results_summary_field/_show.html.erb @@ -0,0 +1,6 @@ +<%# +Administrate field partial for ResultsSummaryField in show view +%> +<%= field.successful_count %> ✓ +/ +<%= field.failed_count %> ✗ diff --git a/app/views/fields/status_field/_index.html.erb b/app/views/fields/status_field/_index.html.erb new file mode 100644 index 000000000..f4490227c --- /dev/null +++ b/app/views/fields/status_field/_index.html.erb @@ -0,0 +1,6 @@ +<%# +Administrate field partial for StatusField in index view +%> + + <%= field.job_status.upcase %> + diff --git a/app/views/fields/status_field/_show.html.erb b/app/views/fields/status_field/_show.html.erb new file mode 100644 index 000000000..5cb80b6dc --- /dev/null +++ b/app/views/fields/status_field/_show.html.erb @@ -0,0 +1,6 @@ +<%# +Administrate field partial for StatusField in show view +%> + + <%= field.job_status.upcase %> + diff --git a/app/views/fields/user_info_field/_index.html.erb b/app/views/fields/user_info_field/_index.html.erb new file mode 100644 index 000000000..f0a6e8a2c --- /dev/null +++ b/app/views/fields/user_info_field/_index.html.erb @@ -0,0 +1,4 @@ +<%# +Administrate field partial for UserInfoField in index view +%> +<%= field.user_display %> diff --git a/app/views/fields/user_info_field/_show.html.erb b/app/views/fields/user_info_field/_show.html.erb new file mode 100644 index 000000000..83e538df3 --- /dev/null +++ b/app/views/fields/user_info_field/_show.html.erb @@ -0,0 +1,4 @@ +<%# +Administrate field partial for UserInfoField in show view +%> +<%= field.user_display %> diff --git a/config/routes.rb b/config/routes.rb index 9dd82fab2..daa54ef0b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -19,6 +19,7 @@ resources :school_classes, only: %i[show] resources :lessons, only: %i[show] + resources :school_import_results, only: %i[index show new create] root to: 'projects#index' end diff --git a/spec/requests/admin/school_import_results_spec.rb b/spec/requests/admin/school_import_results_spec.rb new file mode 100644 index 000000000..156ffa87b --- /dev/null +++ b/spec/requests/admin/school_import_results_spec.rb @@ -0,0 +1,186 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Admin::SchoolImportResults' do + let(:admin_user) { create(:user, roles: 'editor-admin') } + + before do + allow_any_instance_of(Admin::ApplicationController).to receive(:current_user).and_return(admin_user) + + # Stub UserInfoApiClient to avoid external API calls + allow(UserInfoApiClient).to receive(:fetch_by_ids).and_return([ + { + id: admin_user.id, + name: 'Test Admin', + email: 'admin@test.com' + } + ]) + end + + describe 'GET /admin/school_import_results' do + it 'returns successfully' do + get admin_school_import_results_path + expect(response).to have_http_status(:success) + end + + context 'with existing import results' do + let!(:import_result) do + SchoolImportResult.create!( + job_id: SecureRandom.uuid, + user_id: admin_user.id, + results: { 'successful' => [], 'failed' => [] } + ) + end + + it 'displays the import results' do + get admin_school_import_results_path + expect(response.body).to include('School Import History') + expect(response.body).to include('Test Admin <admin@test.com>') + end + end + end + + describe 'GET /admin/school_import_results/new' do + it 'returns successfully' do + get new_admin_school_import_result_path + expect(response).to have_http_status(:success) + end + + it 'displays the upload form' do + get new_admin_school_import_result_path + expect(response.body).to include('Upload School Import CSV') + expect(response.body).to include('csv_file') + end + end + + describe 'GET /admin/school_import_results/:id' do + let(:import_result) do + SchoolImportResult.create!( + job_id: SecureRandom.uuid, + user_id: admin_user.id, + results: { + 'successful' => [ + { 'name' => 'Test School', 'code' => '12-34-56', 'id' => SecureRandom.uuid, 'owner_email' => 'owner@test.com' } + ], + 'failed' => [] + } + ) + end + + it 'returns successfully' do + get admin_school_import_result_path(import_result) + expect(response).to have_http_status(:success) + end + + it 'displays the job details' do + get admin_school_import_result_path(import_result) + expect(response.body).to include(import_result.job_id) + expect(response.body).to include('Test School') + end + + it 'displays user name and email' do + get admin_school_import_result_path(import_result) + expect(response.body).to include('Test Admin <admin@test.com>') + end + + it 'allows downloading results as CSV' do + get admin_school_import_result_path(import_result, format: :csv) + expect(response).to have_http_status(:success) + expect(response.content_type).to include('text/csv') + expect(response.body).to include('Status,School Name,School Code') + expect(response.body).to include('Success,Test School,12-34-56') + end + + context 'with failed schools' do + let(:import_result_with_failures) do + SchoolImportResult.create!( + job_id: SecureRandom.uuid, + user_id: admin_user.id, + results: { + 'successful' => [ + { 'name' => 'Good School', 'code' => '11-22-33', 'id' => SecureRandom.uuid, 'owner_email' => 'good@test.com' } + ], + 'failed' => [ + { 'name' => 'Bad School', 'owner_email' => 'bad@test.com', 'error_code' => 'OWNER_NOT_FOUND', 'error' => 'Owner not found' } + ] + } + ) + end + + it 'includes both successful and failed schools in CSV' do + get admin_school_import_result_path(import_result_with_failures, format: :csv) + expect(response.body).to include('Success,Good School,11-22-33') + expect(response.body).to include('Failed,Bad School') + expect(response.body).to include('OWNER_NOT_FOUND,Owner not found') + end + end + end + + describe 'POST /admin/school_import_results' do + let(:csv_content) { "name,website,address_line_1,municipality,country_code,owner_email\nTest,https://test.edu,123 Main,City,US,test@test.com" } + let(:csv_file) { Rack::Test::UploadedFile.new(StringIO.new(csv_content), 'text/csv', original_filename: 'test.csv') } + let(:job_id) { SecureRandom.uuid } + + before do + allow(School::ImportBatch).to receive(:call).and_return( + OperationResponse.new.tap do |response| + response[:job_id] = job_id + response[:total_schools] = 3 + end + ) + end + + it 'starts an import job' do + post admin_school_import_results_path, params: { csv_file: csv_file } + expect(School::ImportBatch).to have_received(:call) + expect(response).to redirect_to(admin_school_import_results_path) + expect(flash[:notice]).to include(job_id) + end + + context 'without a CSV file' do + it 'shows an error' do + post admin_school_import_results_path, params: {} + expect(response).to redirect_to(new_admin_school_import_result_path) + expect(flash[:error]).to include('CSV file is required') + end + end + + context 'when import fails' do + before do + allow(School::ImportBatch).to receive(:call).and_return( + OperationResponse.new.tap do |response| + response[:error] = { + message: 'CSV validation failed', + details: { + row_errors: [ + { row: 2, errors: [{ field: 'country_code', message: 'invalid code' }] } + ] + } + } + end + ) + end + + it 'shows an error message with details' do + post admin_school_import_results_path, params: { csv_file: csv_file } + expect(response).to have_http_status(:success) + expect(response.body).to include('CSV validation failed') + expect(response.body).to include('Row Errors') + end + end + end + + describe 'authorization' do + let(:non_admin_user) { create(:user, roles: nil) } + + before do + allow_any_instance_of(Admin::ApplicationController).to receive(:current_user).and_return(non_admin_user) + end + + it 'redirects non-admin users' do + get admin_school_import_results_path + expect(response).to redirect_to('/') + end + end +end From bfe099420232758c9e420dbb9860a320f7bf87f0 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Tue, 2 Dec 2025 14:06:39 +0000 Subject: [PATCH 09/21] Change from GoodJob::Execution to GoodJob::Job for upload tracking. Only one Job is created per job, but an upload which is failed or retried could result in more than one execution. --- .../api/school_import_jobs_controller.rb | 28 +++++++++++-------- app/fields/status_field.rb | 11 +++++--- .../features/school/importing_schools_spec.rb | 4 +-- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/app/controllers/api/school_import_jobs_controller.rb b/app/controllers/api/school_import_jobs_controller.rb index 1af6d1215..73061b9e6 100644 --- a/app/controllers/api/school_import_jobs_controller.rb +++ b/app/controllers/api/school_import_jobs_controller.rb @@ -19,10 +19,7 @@ def show private def find_job - # GoodJob stores jobs in the good_jobs table as Executions - # The job_id returned to users is the ActiveJob ID - # We need to query by active_job_id, not by execution id - job = GoodJob::Execution.find_by(active_job_id: params[:id]) + job = GoodJob::Job.find_by(active_job_id: params[:id]) # Verify this is an import job (security check) return nil unless job && job.job_class == SchoolImportJob.name @@ -40,7 +37,7 @@ def build_job_response(job) } # If job is finished successfully, get results from dedicated table - if job.finished_at.present? && job.error.blank? + if job.succeeded? result = SchoolImportResult.find_by(job_id: job.active_job_id) if result response[:results] = result.results @@ -49,22 +46,31 @@ def build_job_response(job) end end - # Include error if job failed + # Include error if job failed or was discarded if job.error.present? response[:error] = job.error - response[:status] = 'failed' + response[:status] = job.discarded? ? 'discarded' : 'failed' end response end def job_status(job) - return 'failed' if job.error.present? - return 'completed' if job.finished_at.present? - return 'scheduled' if job.scheduled_at.present? && job.scheduled_at > Time.current - return 'running' if job.performed_at.present? && job.finished_at.nil? + return 'discarded' if job.discarded? + return 'succeeded' if job.succeeded? + return 'failed' if job_failed?(job) + return 'running' if job.running? + return 'scheduled' if job_scheduled?(job) 'queued' end + + def job_failed?(job) + job.finished? && job.error.present? + end + + def job_scheduled?(job) + job.scheduled_at.present? && job.scheduled_at > Time.current + end end end diff --git a/app/fields/status_field.rb b/app/fields/status_field.rb index 2ec352f51..4914c9f27 100644 --- a/app/fields/status_field.rb +++ b/app/fields/status_field.rb @@ -10,21 +10,24 @@ def to_s def job_status return 'unknown' if data.blank? - job = GoodJob::Execution.find_by(active_job_id: data) + job = GoodJob::Job.find_by(active_job_id: data) return 'not_found' unless job - return 'failed' if job.error.present? - return 'completed' if job.finished_at.present? + return 'discarded' if job.discarded? + return 'succeeded' if job.succeeded? + return 'failed' if job.finished? && job.error.present? + return 'running' if job.running? return 'scheduled' if job.scheduled_at.present? && job.scheduled_at > Time.current - return 'running' if job.performed_at.present? && job.finished_at.nil? 'queued' end def status_class case job_status + when 'succeeded' then 'status-completed' when 'completed' then 'status-completed' when 'failed' then 'status-failed' + when 'discarded' then 'status-failed' when 'running' then 'status-running' when 'queued', 'scheduled' then 'status-queued' else 'status-unknown' diff --git a/spec/features/school/importing_schools_spec.rb b/spec/features/school/importing_schools_spec.rb index 8f1a0c7dc..855b6c11c 100644 --- a/spec/features/school/importing_schools_spec.rb +++ b/spec/features/school/importing_schools_spec.rb @@ -81,7 +81,7 @@ context 'when job exists and is completed' do before do - GoodJob::Execution.create!( + GoodJob::Job.create!( id: SecureRandom.uuid, active_job_id: job_id, queue_name: 'import_schools_job', @@ -107,7 +107,7 @@ expect(response).to have_http_status(:ok) data = JSON.parse(response.body, symbolize_names: true) - expect(data[:status]).to eq('completed') + expect(data[:status]).to eq('succeeded') expect(data[:results][:successful].count).to eq(1) end end From ba75b7cb9352cb18d6ebe8230ee1fac45ec2f5ee Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Tue, 2 Dec 2025 14:31:17 +0000 Subject: [PATCH 10/21] Number of small Rubocop fixings and refactorings. --- .../admin/school_import_results_controller.rb | 8 ++--- app/fields/status_field.rb | 30 ++++++++++++------- config/locales/en.yml | 1 + .../admin/school_import_results_spec.rb | 15 +++++++--- 4 files changed, 35 insertions(+), 19 deletions(-) diff --git a/app/controllers/admin/school_import_results_controller.rb b/app/controllers/admin/school_import_results_controller.rb index 77d25bdef..4970b55e2 100644 --- a/app/controllers/admin/school_import_results_controller.rb +++ b/app/controllers/admin/school_import_results_controller.rb @@ -53,7 +53,7 @@ def new def create if params[:csv_file].blank? - flash[:error] = 'CSV file is required' + flash[:error] = I18n.t('errors.admin.csv_file_required') redirect_to new_admin_school_import_result_path return end @@ -140,9 +140,9 @@ def fetch_users_batch(user_ids) return {} if user_ids.empty? users = UserInfoApiClient.fetch_by_ids(user_ids) - users.index_by do |user| - user[:id] - end + return {} if users.nil? + + users.index_by { |user| user[:id] } rescue StandardError => e Rails.logger.error("Failed to batch fetch user info: #{e.message}") {} diff --git a/app/fields/status_field.rb b/app/fields/status_field.rb index 4914c9f27..70ee68103 100644 --- a/app/fields/status_field.rb +++ b/app/fields/status_field.rb @@ -13,24 +13,32 @@ def job_status job = GoodJob::Job.find_by(active_job_id: data) return 'not_found' unless job - return 'discarded' if job.discarded? - return 'succeeded' if job.succeeded? - return 'failed' if job.finished? && job.error.present? - return 'running' if job.running? - return 'scheduled' if job.scheduled_at.present? && job.scheduled_at > Time.current - - 'queued' + determine_job_status(job) end def status_class case job_status - when 'succeeded' then 'status-completed' - when 'completed' then 'status-completed' - when 'failed' then 'status-failed' - when 'discarded' then 'status-failed' + when 'succeeded', 'completed' then 'status-completed' + when 'failed', 'discarded' then 'status-failed' when 'running' then 'status-running' when 'queued', 'scheduled' then 'status-queued' else 'status-unknown' end end + + private + + def determine_job_status(job) + return 'discarded' if job.discarded? + return 'succeeded' if job.succeeded? + return 'failed' if job.finished? && job.error.present? + return 'running' if job.running? + return 'scheduled' if scheduled?(job) + + 'queued' + end + + def scheduled?(job) + job.scheduled_at.present? && job.scheduled_at > Time.current + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 674aeb89e..df0bccc02 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2,6 +2,7 @@ en: errors: admin: unauthorized: "Not authorized." + csv_file_required: "A CSV file is required." project: editing: delete_default_component: "Cannot delete default file" diff --git a/spec/requests/admin/school_import_results_spec.rb b/spec/requests/admin/school_import_results_spec.rb index 156ffa87b..fc7bb6386 100644 --- a/spec/requests/admin/school_import_results_spec.rb +++ b/spec/requests/admin/school_import_results_spec.rb @@ -3,10 +3,10 @@ require 'rails_helper' RSpec.describe 'Admin::SchoolImportResults' do - let(:admin_user) { create(:user, roles: 'editor-admin') } + let(:admin_user) { create(:admin_user) } before do - allow_any_instance_of(Admin::ApplicationController).to receive(:current_user).and_return(admin_user) + sign_in_as(admin_user) # Stub UserInfoApiClient to avoid external API calls allow(UserInfoApiClient).to receive(:fetch_by_ids).and_return([ @@ -25,7 +25,7 @@ end context 'with existing import results' do - let!(:import_result) do + before do SchoolImportResult.create!( job_id: SecureRandom.uuid, user_id: admin_user.id, @@ -175,7 +175,7 @@ let(:non_admin_user) { create(:user, roles: nil) } before do - allow_any_instance_of(Admin::ApplicationController).to receive(:current_user).and_return(non_admin_user) + sign_in_as(non_admin_user) end it 'redirects non-admin users' do @@ -183,4 +183,11 @@ expect(response).to redirect_to('/') end end + + private + + def sign_in_as(user) + allow(User).to receive(:from_omniauth).and_return(user) + get '/auth/callback' + end end From 4ace5d77583e859244fbafef766cc43ac1dbfdc1 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 08:24:17 +0000 Subject: [PATCH 11/21] Modify SchoolImportJobsController to use jbuilder for JSON response. --- .../api/school_import_jobs_controller.rb | 36 +++---------------- .../api/school_import_jobs/show.json.jbuilder | 15 ++++++++ 2 files changed, 20 insertions(+), 31 deletions(-) create mode 100644 app/views/api/school_import_jobs/show.json.jbuilder diff --git a/app/controllers/api/school_import_jobs_controller.rb b/app/controllers/api/school_import_jobs_controller.rb index 73061b9e6..a8cac9d2c 100644 --- a/app/controllers/api/school_import_jobs_controller.rb +++ b/app/controllers/api/school_import_jobs_controller.rb @@ -6,14 +6,16 @@ class SchoolImportJobsController < ApiController def show authorize! :read, :school_import_job - job = find_job + @job = find_job - if job.nil? + if @job.nil? render json: SchoolImportError.format_error(:job_not_found, 'Job not found'), status: :not_found return end - render json: build_job_response(job), status: :ok + @status = job_status(@job) + @result = SchoolImportResult.find_by(job_id: @job.active_job_id) if @job.succeeded? + render :show, formats: [:json], status: :ok end private @@ -27,34 +29,6 @@ def find_job job end - def build_job_response(job) - response = { - id: job.active_job_id, - status: job_status(job), - created_at: job.created_at, - finished_at: job.finished_at, - job_class: job.job_class - } - - # If job is finished successfully, get results from dedicated table - if job.succeeded? - result = SchoolImportResult.find_by(job_id: job.active_job_id) - if result - response[:results] = result.results - else - response[:message] = 'Job completed successfully' - end - end - - # Include error if job failed or was discarded - if job.error.present? - response[:error] = job.error - response[:status] = job.discarded? ? 'discarded' : 'failed' - end - - response - end - def job_status(job) return 'discarded' if job.discarded? return 'succeeded' if job.succeeded? diff --git a/app/views/api/school_import_jobs/show.json.jbuilder b/app/views/api/school_import_jobs/show.json.jbuilder new file mode 100644 index 000000000..b675ed52b --- /dev/null +++ b/app/views/api/school_import_jobs/show.json.jbuilder @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +json.id @job.active_job_id +json.status @status +json.created_at @job.created_at +json.finished_at @job.finished_at +json.job_class @job.job_class + +if @result.present? + json.results @result.results +elsif @status == 'succeeded' + json.message 'Job completed successfully' +end + +json.error @job.error if @job.error.present? From 103e430c8dc6a19ab8704b7e6ac05a3a4a801c66 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 08:30:32 +0000 Subject: [PATCH 12/21] Modify SchoolsController to use jbuilder --- app/controllers/api/schools_controller.rb | 12 +++++------- app/views/api/schools/import.json.jbuilder | 5 +++++ spec/features/school/importing_schools_spec.rb | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) create mode 100644 app/views/api/schools/import.json.jbuilder diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index 9b6bb2cf1..422a47b70 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -52,7 +52,7 @@ def import authorize! :import, School if params[:csv_file].blank? - render json: SchoolImportError.format_error(:csv_file_required, 'CSV file is required'), + render json: { error: SchoolImportError.format_error(:csv_file_required, 'CSV file is required') }, status: :unprocessable_entity return end @@ -63,13 +63,11 @@ def import ) if result.success? - render json: { - job_id: result[:job_id], - total_schools: result[:total_schools], - message: 'Import job started successfully' - }, status: :accepted + @job_id = result[:job_id] + @total_schools = result[:total_schools] + render :import, formats: [:json], status: :accepted else - render json: result[:error], status: :unprocessable_entity + render json: { error: result[:error] }, status: :unprocessable_entity end end diff --git a/app/views/api/schools/import.json.jbuilder b/app/views/api/schools/import.json.jbuilder new file mode 100644 index 000000000..b56dc59d5 --- /dev/null +++ b/app/views/api/schools/import.json.jbuilder @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +json.job_id @job_id +json.total_schools @total_schools +json.message 'Import job started successfully' diff --git a/spec/features/school/importing_schools_spec.rb b/spec/features/school/importing_schools_spec.rb index 855b6c11c..71d64f2cb 100644 --- a/spec/features/school/importing_schools_spec.rb +++ b/spec/features/school/importing_schools_spec.rb @@ -52,7 +52,7 @@ expect(response).to have_http_status(:unprocessable_entity) data = JSON.parse(response.body, symbolize_names: true) - expect(data[:error_code]).to eq('CSV_FILE_REQUIRED') + expect(data[:error][:error_code]).to eq('CSV_FILE_REQUIRED') end end From f24addac2a4b3afcff6182dabbe580a9e9ab69ad Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 08:36:52 +0000 Subject: [PATCH 13/21] Remove redundant reinitialisation of OperationResponse. --- lib/concepts/school/operations/import_batch.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/concepts/school/operations/import_batch.rb b/lib/concepts/school/operations/import_batch.rb index 3cd880b45..f509192dd 100644 --- a/lib/concepts/school/operations/import_batch.rb +++ b/lib/concepts/school/operations/import_batch.rb @@ -30,7 +30,6 @@ def call(csv_file:, current_user:) response rescue StandardError => e Sentry.capture_exception(e) - response ||= OperationResponse.new response[:error] = SchoolImportError.format_error(:unknown_error, e.message) response end From 1ab74dd7745b9632abe349df7f75ad54028495f4 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 08:40:23 +0000 Subject: [PATCH 14/21] Make the array of required school CSV fields a constant. --- lib/concepts/school/operations/import_batch.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/concepts/school/operations/import_batch.rb b/lib/concepts/school/operations/import_batch.rb index f509192dd..094d9945b 100644 --- a/lib/concepts/school/operations/import_batch.rb +++ b/lib/concepts/school/operations/import_batch.rb @@ -2,6 +2,8 @@ class School class ImportBatch + REQUIRED_FIELDS = %i[name website address_line_1 municipality country_code owner_email].freeze + class << self def call(csv_file:, current_user:) response = OperationResponse.new @@ -102,8 +104,7 @@ def process_csv_rows(csv_data) end def valid_headers?(headers) - required = %i[name website address_line_1 municipality country_code owner_email] - required.all? { |h| headers.include?(h) } + REQUIRED_FIELDS.all? { |h| headers.include?(h) } end def validate_school_data(data, row_number) @@ -115,8 +116,7 @@ def validate_school_data(data, row_number) end # Validate required fields - required_fields = %i[name website address_line_1 municipality country_code owner_email] - required_fields.each do |field| + REQUIRED_FIELDS.each do |field| errors << { field: field.to_s, message: 'is required' } if data[field].blank? end From dbcaa1f761d550b7062695682d5d52eb75551e53 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 08:46:36 +0000 Subject: [PATCH 15/21] Modify check on owner to fail if owner has any role in any other school. --- app/jobs/school_import_job.rb | 11 ++++++----- app/models/school_import_error.rb | 1 + spec/jobs/school_import_job_spec.rb | 21 +++++++++++++++++++++ 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/app/jobs/school_import_job.rb b/app/jobs/school_import_job.rb index 95a9544f2..2ada88b66 100644 --- a/app/jobs/school_import_job.rb +++ b/app/jobs/school_import_job.rb @@ -50,13 +50,14 @@ def import_school(school_data) return end - # Check if this owner already has a school as creator - existing_school = School.find_by(creator_id: owner[:id]) - if existing_school + # Check if this owner already has any role in any school + existing_role = Role.find_by(user_id: owner[:id]) + if existing_role + existing_school = existing_role.school @results[:failed] << { name: school_data[:name], - error_code: SchoolImportError::CODES[:owner_already_creator], - error: "Owner #{school_data[:owner_email]} is already the creator of school '#{existing_school.name}'", + error_code: SchoolImportError::CODES[:owner_has_existing_role], + error: "Owner #{school_data[:owner_email]} already has a role in school '#{existing_school.name}'", owner_email: school_data[:owner_email], existing_school_id: existing_school.id } diff --git a/app/models/school_import_error.rb b/app/models/school_import_error.rb index 72fc968cd..eab0680c0 100644 --- a/app/models/school_import_error.rb +++ b/app/models/school_import_error.rb @@ -7,6 +7,7 @@ module SchoolImportError csv_validation_failed: 'CSV_VALIDATION_FAILED', owner_not_found: 'OWNER_NOT_FOUND', owner_already_creator: 'OWNER_ALREADY_CREATOR', + owner_has_existing_role: 'OWNER_HAS_EXISTING_ROLE', duplicate_owner_email: 'DUPLICATE_OWNER_EMAIL', school_validation_failed: 'SCHOOL_VALIDATION_FAILED', job_not_found: 'JOB_NOT_FOUND', diff --git a/spec/jobs/school_import_job_spec.rb b/spec/jobs/school_import_job_spec.rb index b1342751a..b79b2e806 100644 --- a/spec/jobs/school_import_job_spec.rb +++ b/spec/jobs/school_import_job_spec.rb @@ -81,6 +81,27 @@ end end + context 'when owner already has a role in another school' do + let(:existing_school) { create(:school, name: 'Existing School') } + + before do + Role.owner.create!(school_id: existing_school.id, user_id: owner1_id) + end + + it 'adds failed result for that school' do + results = described_class.new.perform( + schools_data: [schools_data.first], + user_id: user_id + ) + + expect(results[:successful].count).to eq(0) + expect(results[:failed].count).to eq(1) + expect(results[:failed].first[:error_code]).to eq('OWNER_HAS_EXISTING_ROLE') + expect(results[:failed].first[:error]).to include('already has a role in school') + expect(results[:failed].first[:existing_school_id]).to eq(existing_school.id) + end + end + context 'when schools_data has string keys (simulating ActiveJob serialization)' do let(:schools_data_with_string_keys) do [ From f1822934a3cb86a08fb8f3f0d520d9b1fdcd340f Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 08:57:44 +0000 Subject: [PATCH 16/21] Consolidate school import abilities into one method --- app/models/ability.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index f09f226b0..02c844c1e 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -121,19 +121,24 @@ def define_school_student_abilities(user:, school:) can(%i[show_finished set_finished show_status unsubmit submit], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id) end - def define_editor_admin_abilities(user) - return unless user&.admin? + def define_school_import_abilities(user) + return unless user&.admin? || user&.experience_cs_admin? can :import, School can :read, :school_import_job end + def define_editor_admin_abilities(user) + return unless user&.admin? + + define_school_import_abilities(user) + end + def define_experience_cs_admin_abilities(user) return unless user&.experience_cs_admin? can %i[read create update destroy], Project, user_id: nil - can :import, School - can :read, :school_import_job + define_school_import_abilities(user) end def school_teacher_can_manage_lesson?(user:, school:, lesson:) From 62710abe826be8be19c4cd8da3702ccb1b9b53d8 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 09:19:09 +0000 Subject: [PATCH 17/21] Refactor SchoolImportJob to use SchoolVerificationService directly instead of duplicating. --- app/jobs/school_import_job.rb | 10 ++++------ lib/concepts/school/operations/import_batch.rb | 7 ++++--- spec/concepts/school/import_batch_spec.rb | 3 ++- spec/jobs/school_import_job_spec.rb | 17 ++++++++++++----- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/app/jobs/school_import_job.rb b/app/jobs/school_import_job.rb index 2ada88b66..a51c07da6 100644 --- a/app/jobs/school_import_job.rb +++ b/app/jobs/school_import_job.rb @@ -8,7 +8,8 @@ class SchoolImportJob < ApplicationJob queue_as :import_schools_job - def perform(schools_data:, user_id:) + def perform(schools_data:, user_id:, token:) + @token = token @results = { successful: [], failed: [] @@ -76,11 +77,8 @@ def import_school(school_data) if result.success? school = result[:school] - # Auto-verify the imported school - school.verify! - - # Create owner role - Role.owner.create!(school_id: school.id, user_id: owner[:id]) + # Auto-verify the imported school using the verification service + SchoolVerificationService.new(school).verify(token: @token) @results[:successful] << { name: school.name, diff --git a/lib/concepts/school/operations/import_batch.rb b/lib/concepts/school/operations/import_batch.rb index 094d9945b..282f3c50b 100644 --- a/lib/concepts/school/operations/import_batch.rb +++ b/lib/concepts/school/operations/import_batch.rb @@ -24,7 +24,7 @@ def call(csv_file:, current_user:) job = enqueue_import_job( schools_data: parsed_schools[:schools], - user_id: current_user.id + current_user: current_user ) response[:job_id] = job.job_id @@ -151,10 +151,11 @@ def validate_email_format(data, errors) errors << { field: 'owner_email', message: 'invalid email format' } end - def enqueue_import_job(schools_data:, user_id:) + def enqueue_import_job(schools_data:, current_user:) SchoolImportJob.perform_later( schools_data: schools_data, - user_id: user_id + user_id: current_user.id, + token: current_user.token ) end end diff --git a/spec/concepts/school/import_batch_spec.rb b/spec/concepts/school/import_batch_spec.rb index 8da53a947..759c4d18f 100644 --- a/spec/concepts/school/import_batch_spec.rb +++ b/spec/concepts/school/import_batch_spec.rb @@ -38,7 +38,8 @@ hash_including(name: 'Test School 1'), hash_including(name: 'Test School 2') ), - user_id: current_user.id + user_id: current_user.id, + token: current_user.token ) ) end diff --git a/spec/jobs/school_import_job_spec.rb b/spec/jobs/school_import_job_spec.rb index b79b2e806..4f9e86fe1 100644 --- a/spec/jobs/school_import_job_spec.rb +++ b/spec/jobs/school_import_job_spec.rb @@ -38,13 +38,16 @@ allow(UserInfoApiClient).to receive(:find_user_by_email) .with('owner2@example.com') .and_return({ id: owner2_id, email: 'owner2@example.com' }) + + allow(ProfileApiClient).to receive(:create_school).and_return(true) end context 'when all schools can be created successfully' do it 'creates schools and returns successful results' do results = described_class.new.perform( schools_data: schools_data, - user_id: user_id + user_id: user_id, + token: token ) expect(results[:successful].count).to eq(2) @@ -56,8 +59,9 @@ expect(school_1.code).to be_present expect(school_1.user_origin).to eq('experience_cs') - # Check owner role was created + # Check owner and teacher roles were created expect(Role.owner.exists?(school_id: school_1.id, user_id: owner1_id)).to be true + expect(Role.teacher.exists?(school_id: school_1.id, user_id: owner1_id)).to be true end end @@ -71,7 +75,8 @@ it 'adds failed result for that school' do results = described_class.new.perform( schools_data: [schools_data.first], - user_id: user_id + user_id: user_id, + token: token ) expect(results[:successful].count).to eq(0) @@ -91,7 +96,8 @@ it 'adds failed result for that school' do results = described_class.new.perform( schools_data: [schools_data.first], - user_id: user_id + user_id: user_id, + token: token ) expect(results[:successful].count).to eq(0) @@ -125,7 +131,8 @@ it 'handles string keys correctly' do results = described_class.new.perform( schools_data: schools_data_with_string_keys, - user_id: user_id + user_id: user_id, + token: token ) expect(results[:successful].count).to eq(1) From a3b577d01907d25aa8ac3af48788940b2af8e687 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 09:48:21 +0000 Subject: [PATCH 18/21] Rename find_owner method to find_user_account_for_proposed_owner --- app/jobs/school_import_job.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/jobs/school_import_job.rb b/app/jobs/school_import_job.rb index a51c07da6..85a54823e 100644 --- a/app/jobs/school_import_job.rb +++ b/app/jobs/school_import_job.rb @@ -39,9 +39,9 @@ def store_results(results, user_id) end def import_school(school_data) - owner = find_owner(school_data[:owner_email]) + proposed_owner = find_user_account_for_proposed_owner(school_data[:owner_email]) - unless owner + unless proposed_owner @results[:failed] << { name: school_data[:name], error_code: SchoolImportError::CODES[:owner_not_found], @@ -52,7 +52,7 @@ def import_school(school_data) end # Check if this owner already has any role in any school - existing_role = Role.find_by(user_id: owner[:id]) + existing_role = Role.find_by(user_id: proposed_owner[:id]) if existing_role existing_school = existing_role.school @results[:failed] << { @@ -71,7 +71,7 @@ def import_school(school_data) School.transaction do result = School::Create.call( school_params: school_params, - creator_id: owner[:id] + creator_id: proposed_owner[:id] ) if result.success? @@ -105,7 +105,7 @@ def import_school(school_data) Sentry.capture_exception(e) end - def find_owner(email) + def find_user_account_for_proposed_owner(email) return nil if email.blank? UserInfoApiClient.find_user_by_email(email) From 2d4bb6c390cc1ec4f0a5921e82a67d0a3cb1f985 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 09:59:56 +0000 Subject: [PATCH 19/21] Refactor the stubbed_users methods out of UserInfoApiClient; refactor tests. --- lib/user_info_api_client.rb | 50 ----------------- .../features/school/importing_schools_spec.rb | 5 +- spec/jobs/school_import_job_spec.rb | 36 ++++++++----- spec/models/user_spec.rb | 36 ------------- spec/rails_helper.rb | 1 + .../admin/school_import_results_spec.rb | 15 +++--- spec/support/user_info_api_mock.rb | 53 +++++++++++++++++++ 7 files changed, 88 insertions(+), 108 deletions(-) create mode 100644 spec/support/user_info_api_mock.rb diff --git a/lib/user_info_api_client.rb b/lib/user_info_api_client.rb index d624e1076..7caf3f39c 100644 --- a/lib/user_info_api_client.rb +++ b/lib/user_info_api_client.rb @@ -7,7 +7,6 @@ class UserInfoApiClient class << self def fetch_by_ids(user_ids) return [] if user_ids.blank? - return stubbed_users(user_ids) if bypass_oauth? response = conn.get do |r| r.url '/users' @@ -20,7 +19,6 @@ def fetch_by_ids(user_ids) def find_user_by_email(email) return nil if email.blank? - return stubbed_user_by_email(email) if bypass_oauth? response = conn.get do |r| r.url "/users/#{CGI.escape(email)}" @@ -36,10 +34,6 @@ def find_user_by_email(email) private - def bypass_oauth? - ENV.fetch('BYPASS_OAUTH', nil) == 'true' - end - def transform_user(user) user.transform_keys { |k| k.to_s.underscore.to_sym } end @@ -59,49 +53,5 @@ def conn f.response :json # decode response bodies as JSON end end - - def stubbed_users(user_ids) - user_ids.map do |user_id| - { - id: user_id, - email: "user-#{user_id}@example.com", - username: nil, - parentalEmail: nil, - name: 'School Owner', - nickname: 'Owner', - country: 'United Kingdom', - country_code: 'GB', - postcode: nil, - dateOfBirth: nil, - verifiedAt: '2024-01-01T12:00:00.000Z', - createdAt: '2024-01-01T12:00:00.000Z', - updatedAt: '2024-01-01T12:00:00.000Z', - discardedAt: nil, - lastLoggedInAt: '2024-01-01T12:00:00.000Z', - roles: '' - } - end - end - - def stubbed_user_by_email(email) - { - id: Digest::UUID.uuid_v5(Digest::UUID::DNS_NAMESPACE, email), - email: email, - username: nil, - parentalEmail: nil, - name: 'School Owner', - nickname: 'Owner', - country: 'United Kingdom', - country_code: 'GB', - postcode: nil, - dateOfBirth: nil, - verifiedAt: '2024-01-01T12:00:00.000Z', - createdAt: '2024-01-01T12:00:00.000Z', - updatedAt: '2024-01-01T12:00:00.000Z', - discardedAt: nil, - lastLoggedInAt: '2024-01-01T12:00:00.000Z', - roles: '' - } - end end end diff --git a/spec/features/school/importing_schools_spec.rb b/spec/features/school/importing_schools_spec.rb index 71d64f2cb..9a3949d2f 100644 --- a/spec/features/school/importing_schools_spec.rb +++ b/spec/features/school/importing_schools_spec.rb @@ -26,7 +26,10 @@ before do authenticated_in_hydra_as(admin_user) - allow(UserInfoApiClient).to receive(:find_user_by_email).and_return({ id: SecureRandom.uuid, email: 'owner@example.com' }) + stub_user_info_api_find_by_email( + email: 'owner@example.com', + user: { id: SecureRandom.uuid, email: 'owner@example.com' } + ) allow(SchoolImportJob).to receive(:perform_later).and_return(instance_double(SchoolImportJob, job_id: 'test-job-id')) end diff --git a/spec/jobs/school_import_job_spec.rb b/spec/jobs/school_import_job_spec.rb index 4f9e86fe1..a66e073af 100644 --- a/spec/jobs/school_import_job_spec.rb +++ b/spec/jobs/school_import_job_spec.rb @@ -31,18 +31,22 @@ end before do - allow(UserInfoApiClient).to receive(:find_user_by_email) - .with('owner1@example.com') - .and_return({ id: owner1_id, email: 'owner1@example.com' }) - - allow(UserInfoApiClient).to receive(:find_user_by_email) - .with('owner2@example.com') - .and_return({ id: owner2_id, email: 'owner2@example.com' }) - allow(ProfileApiClient).to receive(:create_school).and_return(true) end context 'when all schools can be created successfully' do + before do + stub_user_info_api_find_by_email( + email: 'owner1@example.com', + user: { id: owner1_id, email: 'owner1@example.com' } + ) + + stub_user_info_api_find_by_email( + email: 'owner2@example.com', + user: { id: owner2_id, email: 'owner2@example.com' } + ) + end + it 'creates schools and returns successful results' do results = described_class.new.perform( schools_data: schools_data, @@ -67,9 +71,7 @@ context 'when owner email is not found' do before do - allow(UserInfoApiClient).to receive(:find_user_by_email) - .with('owner1@example.com') - .and_return(nil) + stub_user_info_api_find_by_email(email: 'owner1@example.com', user: nil) end it 'adds failed result for that school' do @@ -91,6 +93,11 @@ before do Role.owner.create!(school_id: existing_school.id, user_id: owner1_id) + + stub_user_info_api_find_by_email( + email: 'owner1@example.com', + user: { id: owner1_id, email: 'owner1@example.com' } + ) end it 'adds failed result for that school' do @@ -123,9 +130,10 @@ end before do - allow(UserInfoApiClient).to receive(:find_user_by_email) - .with('owner1@example.com') - .and_return({ id: owner1_id, email: 'owner1@example.com' }) + stub_user_info_api_find_by_email( + email: 'owner1@example.com', + user: { id: owner1_id, email: 'owner1@example.com' } + ) end it 'handles string keys correctly' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index ce42c44b3..afb07b069 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -73,23 +73,6 @@ it 'returns a user without a username' do expect(user.username).to be_nil end - - context 'when BYPASS_OAUTH is true' do - around do |example| - ClimateControl.modify(BYPASS_OAUTH: 'true') do - example.run - end - end - - it 'does not call the API' do - user - expect(WebMock).not_to have_requested(:get, /.*/) - end - - it 'returns a stubbed user' do - expect(user.name).to eq('School Owner') - end - end end context 'when logged into a student account' do @@ -370,25 +353,6 @@ it 'returns a user with the correct email' do expect(user.email).to eq 'school-owner@example.com' end - - context 'when BYPASS_OAUTH is true' do - around do |example| - ClimateControl.modify(BYPASS_OAUTH: 'true') do - example.run - end - end - - let(:owner) { create(:owner, school:, id: '00000000-0000-0000-0000-000000000000') } - - it 'does not call the API' do - user - expect(WebMock).not_to have_requested(:get, /.*/) - end - - it 'returns a stubbed user' do - expect(user.name).to eq('School Owner') - end - end end describe '#schools' do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index e1f9e7283..c3ab74904 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -85,6 +85,7 @@ config.include PhraseIdentifierMock config.include ProfileApiMock config.include UserProfileMock + config.include UserInfoApiMock config.include SignInStubs, type: :request config.include SignInStubs, type: :system diff --git a/spec/requests/admin/school_import_results_spec.rb b/spec/requests/admin/school_import_results_spec.rb index fc7bb6386..eb19274cb 100644 --- a/spec/requests/admin/school_import_results_spec.rb +++ b/spec/requests/admin/school_import_results_spec.rb @@ -9,13 +9,14 @@ sign_in_as(admin_user) # Stub UserInfoApiClient to avoid external API calls - allow(UserInfoApiClient).to receive(:fetch_by_ids).and_return([ - { - id: admin_user.id, - name: 'Test Admin', - email: 'admin@test.com' - } - ]) + stub_user_info_api_fetch_by_ids( + user_ids: [admin_user.id], + users: [{ + id: admin_user.id, + name: 'Test Admin', + email: 'admin@test.com' + }] + ) end describe 'GET /admin/school_import_results' do diff --git a/spec/support/user_info_api_mock.rb b/spec/support/user_info_api_mock.rb new file mode 100644 index 000000000..912a4016f --- /dev/null +++ b/spec/support/user_info_api_mock.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module UserInfoApiMock + def stub_user_info_api_fetch_by_ids(user_ids:, users: []) + users = default_stubbed_users(user_ids) if users.empty? + + allow(UserInfoApiClient).to receive(:fetch_by_ids) + .with(user_ids) + .and_return(users) + end + + def stub_user_info_api_find_by_email(email:, user: :not_provided) + user = default_stubbed_user_by_email(email) if user == :not_provided + + allow(UserInfoApiClient).to receive(:find_user_by_email) + .with(email) + .and_return(user) + end + + private + + def default_stubbed_users(user_ids) + user_ids.map { |user_id| default_stubbed_user(id: user_id, email: "user-#{user_id}@example.com") } + end + + def default_stubbed_user_by_email(email) + default_stubbed_user( + id: Digest::UUID.uuid_v5(Digest::UUID::DNS_NAMESPACE, email), + email: email + ) + end + + def default_stubbed_user(id:, email:) + { + id: id, + email: email, + username: nil, + parental_email: nil, + name: 'School Owner', + nickname: 'Owner', + country: 'United Kingdom', + country_code: 'GB', + postcode: nil, + date_of_birth: nil, + verified_at: '2024-01-01T12:00:00.000Z', + created_at: '2024-01-01T12:00:00.000Z', + updated_at: '2024-01-01T12:00:00.000Z', + discarded_at: nil, + last_logged_in_at: '2024-01-01T12:00:00.000Z', + roles: '' + } + end +end From dfa6b0351442454052656a65715b2613cb93eb5e Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 4 Dec 2025 10:05:48 +0000 Subject: [PATCH 20/21] Update path to CSV template file. --- docs/import/school_import_csm_guide.md | 2 +- public/docs/import/school_import_template.csv | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 public/docs/import/school_import_template.csv diff --git a/docs/import/school_import_csm_guide.md b/docs/import/school_import_csm_guide.md index 87520eb11..7e516f1a4 100644 --- a/docs/import/school_import_csm_guide.md +++ b/docs/import/school_import_csm_guide.md @@ -10,7 +10,7 @@ ### 1. Prepare the CSV File -Download the template: `docs/school_import_template.csv` +Download the template from the admin interface. Required columns: - `name` - School name diff --git a/public/docs/import/school_import_template.csv b/public/docs/import/school_import_template.csv new file mode 100644 index 000000000..c5412ead6 --- /dev/null +++ b/public/docs/import/school_import_template.csv @@ -0,0 +1,4 @@ +name,website,address_line_1,address_line_2,municipality,administrative_area,postal_code,country_code,reference,owner_email +Springfield Elementary School,https://springfield-elem.edu,123 Main Street,,Springfield,IL,62701,US,SPE-001,principal@springfield-elem.edu +Shelbyville High School,https://shelbyville-high.edu,456 Oak Avenue,Suite 100,Shelbyville,IL,62565,US,SHS-002,admin@shelbyville-high.edu +Capital City Academy,https://capital-city-academy.edu,789 State Road,,Capital City,IL,62702,US,CCA-003,headteacher@capital-city-academy.edu From 5f26d02c0e1aa1b140d0d8b4fb2580be22637609 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Mon, 8 Dec 2025 11:35:03 +0000 Subject: [PATCH 21/21] Retain BYPASS_OAUTH funcationlity but defer to stubs. --- lib/user_info_api_client.rb | 18 +++++++++++++++ spec/models/user_spec.rb | 36 ++++++++++++++++++++++++++++++ spec/support/user_info_api_mock.rb | 3 +++ 3 files changed, 57 insertions(+) diff --git a/lib/user_info_api_client.rb b/lib/user_info_api_client.rb index 7caf3f39c..74f96370b 100644 --- a/lib/user_info_api_client.rb +++ b/lib/user_info_api_client.rb @@ -7,6 +7,7 @@ class UserInfoApiClient class << self def fetch_by_ids(user_ids) return [] if user_ids.blank? + return stubbed_users(user_ids) if bypass_oauth? response = conn.get do |r| r.url '/users' @@ -19,6 +20,7 @@ def fetch_by_ids(user_ids) def find_user_by_email(email) return nil if email.blank? + return stubbed_user_by_email(email) if bypass_oauth? response = conn.get do |r| r.url "/users/#{CGI.escape(email)}" @@ -34,6 +36,10 @@ def find_user_by_email(email) private + def bypass_oauth? + ENV.fetch('BYPASS_OAUTH', nil) == 'true' + end + def transform_user(user) user.transform_keys { |k| k.to_s.underscore.to_sym } end @@ -53,5 +59,17 @@ def conn f.response :json # decode response bodies as JSON end end + + # Development/test stubbing methods - only active when BYPASS_OAUTH=true + # Delegates to UserInfoApiMock to avoid code duplication + def stubbed_users(user_ids) + require_relative '../spec/support/user_info_api_mock' unless defined?(UserInfoApiMock) + UserInfoApiMock.default_stubbed_users(user_ids) + end + + def stubbed_user_by_email(email) + require_relative '../spec/support/user_info_api_mock' unless defined?(UserInfoApiMock) + UserInfoApiMock.default_stubbed_user_by_email(email) + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index afb07b069..ce42c44b3 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -73,6 +73,23 @@ it 'returns a user without a username' do expect(user.username).to be_nil end + + context 'when BYPASS_OAUTH is true' do + around do |example| + ClimateControl.modify(BYPASS_OAUTH: 'true') do + example.run + end + end + + it 'does not call the API' do + user + expect(WebMock).not_to have_requested(:get, /.*/) + end + + it 'returns a stubbed user' do + expect(user.name).to eq('School Owner') + end + end end context 'when logged into a student account' do @@ -353,6 +370,25 @@ it 'returns a user with the correct email' do expect(user.email).to eq 'school-owner@example.com' end + + context 'when BYPASS_OAUTH is true' do + around do |example| + ClimateControl.modify(BYPASS_OAUTH: 'true') do + example.run + end + end + + let(:owner) { create(:owner, school:, id: '00000000-0000-0000-0000-000000000000') } + + it 'does not call the API' do + user + expect(WebMock).not_to have_requested(:get, /.*/) + end + + it 'returns a stubbed user' do + expect(user.name).to eq('School Owner') + end + end end describe '#schools' do diff --git a/spec/support/user_info_api_mock.rb b/spec/support/user_info_api_mock.rb index 912a4016f..6fff96cf2 100644 --- a/spec/support/user_info_api_mock.rb +++ b/spec/support/user_info_api_mock.rb @@ -50,4 +50,7 @@ def default_stubbed_user(id:, email:) roles: '' } end + + # Class methods for use by UserInfoApiClient when BYPASS_OAUTH=true + module_function :default_stubbed_users, :default_stubbed_user_by_email, :default_stubbed_user end