From 9134ee455f71b00c815e8e749ce4bbdb6bdcf759 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Thu, 12 Nov 2026 15:29:07 +0000 Subject: [PATCH 1/2] Don't try to create roles errors in student import Previously we were incorrectly assuming that all records in the profile response where successful student creations. However, some records may be errors. These errors don't have a student id and have a success: false as well as an error row (see [1]) This caused the creation of students with a `null` user_id, which later caused errors. Make sure we only log successful creations. I'm logging the errors so we can see what caused them. There's a try/catch in profile with a wide scope so it's not clear what's gone wrong. I'm assuming it might be a failed creation due to a concurrency issue. [1] - https://github.com/RaspberryPiFoundation/profile/blame/main/app/lib/createGoogleSSOStudents.js Co-Authored-By: Claude Opus 5 (1M context) --- .../school_student/create_batch_sso.rb | 11 +++++- .../school_student/create_batch_sso_spec.rb | 39 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/lib/concepts/school_student/create_batch_sso.rb b/lib/concepts/school_student/create_batch_sso.rb index 435dd8f53..d9f8b4729 100644 --- a/lib/concepts/school_student/create_batch_sso.rb +++ b/lib/concepts/school_student/create_batch_sso.rb @@ -37,7 +37,10 @@ def create_batch_sso(school, students, current_user) SafeguardingFlagService.create_for_school_roles(user: current_user, school:) responses = ProfileApiClient.create_school_students_sso(token:, students:, school_id: school.id) - create_student_roles(school, responses) + successful_responses, failed_responses = responses.partition { |response| response[:success] } + + report_failures(failed_responses) + create_student_roles(school, successful_responses) format_student_responses(responses) rescue ProfileApiClient::Student422Error => e handle_validations(e.errors) @@ -50,6 +53,12 @@ def normalize_student_params(students) end end + def report_failures(responses) + responses.each do |response| + Sentry.capture_message('Student creation failed in Profile', extra: response[:error]) + end + end + def create_student_roles(school, responses) user_ids = responses.pluck(:id) existing_user_ids = Role.student.where(school_id: school.id, user_id: user_ids).pluck(:user_id) diff --git a/spec/concepts/school_student/create_batch_sso_spec.rb b/spec/concepts/school_student/create_batch_sso_spec.rb index b7bea3982..980887a67 100644 --- a/spec/concepts/school_student/create_batch_sso_spec.rb +++ b/spec/concepts/school_student/create_batch_sso_spec.rb @@ -193,6 +193,45 @@ end end + context 'when an error occurs creating an individual student in profile' do + before do + responses = [ + { id: SecureRandom.uuid, name: 'Success 1', success: true }, + { + id: nil, + error: { + path: 'student', + errorCode: 'ERROR_CREATE_STUDENT_FAILED', + message: 'here is an error message', + location: 'body' + }, + success: false + }, + { id: SecureRandom.uuid, name: 'Success 2', success: true } + ] + allow(ProfileApiClient).to receive(:create_school_students_sso).and_return(responses) + allow(Sentry).to receive(:capture_message) + end + + it 'only creates the successful students' do + initial_student_role_count = Role.student.count + + described_class.call(school:, school_students_params:, current_user:) + + expect(Role.student.count).to eq(initial_student_role_count + 2) + end + + it 'reports unexpected errors to sentry' do + described_class.call(school:, school_students_params:, current_user:) + expect(Sentry).to have_received(:capture_message).with('Student creation failed in Profile', extra: { + path: 'student', + errorCode: 'ERROR_CREATE_STUDENT_FAILED', + message: 'here is an error message', + location: 'body' + }) + end + end + context 'when a standard error occurs' do before do allow(ProfileApiClient).to receive(:create_school_students_sso) From b661415f06aa830be11969b1e53ab97b3b0ffe1f Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Thu, 12 Nov 2026 15:32:46 +0000 Subject: [PATCH 2/2] Avoid creating roles without a user id We were incorrectly handling errors from creating students and trying to create students without an id (see previous commit for details. Since this was a bulk create, we bypassed the model validation This is a extra safeguard to prevent this happening again. I've checked and we have none in our database at the moment - `Role.where(user_id: null).count` --- db/migrate/20261112152938_add_not_null_to_role_user_id.rb | 5 +++++ db/schema.rb | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20261112152938_add_not_null_to_role_user_id.rb diff --git a/db/migrate/20261112152938_add_not_null_to_role_user_id.rb b/db/migrate/20261112152938_add_not_null_to_role_user_id.rb new file mode 100644 index 000000000..68d6ff128 --- /dev/null +++ b/db/migrate/20261112152938_add_not_null_to_role_user_id.rb @@ -0,0 +1,5 @@ +class AddNotNullToRoleUserId < ActiveRecord::Migration[8.1] + def change + change_column_null(:roles, :user_id, false) + end +end diff --git a/db/schema.rb b/db/schema.rb index 100c263ba..131ce4e59 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_11_06_162624) do +ActiveRecord::Schema[8.1].define(version: 2026_11_12_152938) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -303,7 +303,7 @@ t.integer "role" t.uuid "school_id" t.datetime "updated_at", null: false - t.uuid "user_id" + t.uuid "user_id", null: false t.index ["school_id"], name: "index_roles_on_school_id" t.index ["user_id", "school_id", "role"], name: "index_roles_on_user_id_and_school_id_and_role", unique: true t.index ["user_id"], name: "index_roles_on_user_id"