From f5a879696b86f87502e6c8b4fc7ba2297f1ca14e Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Mon, 15 Jun 2026 09:50:57 +0100 Subject: [PATCH 01/10] Allow teachers and owners to list SchoolEmailDomains Add SchoolEmailDomainsController and school_email_domains route. Return a list of domain strings. Update CanCan ability to allow access only for teachers and owners. Co-authored-by: Cursor --- .../api/school_email_domains_controller.rb | 19 ++++ app/models/ability.rb | 2 + config/routes.rb | 1 + spec/factories/school_email_domain.rb | 8 ++ .../listing_school_email_domains_spec.rb | 95 +++++++++++++++++++ 5 files changed, 125 insertions(+) create mode 100644 app/controllers/api/school_email_domains_controller.rb create mode 100644 spec/factories/school_email_domain.rb create mode 100644 spec/features/school_email_domain/listing_school_email_domains_spec.rb diff --git a/app/controllers/api/school_email_domains_controller.rb b/app/controllers/api/school_email_domains_controller.rb new file mode 100644 index 000000000..63dc1e5a7 --- /dev/null +++ b/app/controllers/api/school_email_domains_controller.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Api + class SchoolEmailDomainsController < ApiController + before_action :authorize_user + load_and_authorize_resource :school + authorize_resource :school_email_domain, class: false + + def index + render json: school_email_domains, status: :ok + end + + private + + def school_email_domains + @school.school_email_domains.order(:created_at).pluck(:domain) + end + end +end diff --git a/app/models/ability.rb b/app/models/ability.rb index 3db74d435..f9cceab75 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -72,6 +72,7 @@ def define_school_owner_abilities(school:) can(%i[read update destroy], Lesson, school_id: school.id, visibility: %w[teachers students public]) can(%i[read destroy], Feedback, school_project: { school_id: school.id }) can(%i[exchange_code], :google_auth) + can(%i[read create], :school_email_domain) end def define_school_teacher_abilities(user:, school:) @@ -102,6 +103,7 @@ def define_school_teacher_abilities(user:, school:) can(%i[show_status unsubmit return complete], SchoolProject, project: { remixed_from_id: teacher_project_ids }) can(%i[read create destroy], Feedback, school_project: { project: { remixed_from_id: teacher_project_ids } }) can(%i[exchange_code], :google_auth) + can(%i[read create], :school_email_domain) end def define_school_student_abilities(user:, school:) diff --git a/config/routes.rb b/config/routes.rb index b85c9f545..8217e173c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -82,6 +82,7 @@ post :batch, on: :collection, to: 'school_students#create_batch' delete :batch, on: :collection, to: 'school_students#destroy_batch' end + resources :school_email_domains, only: %i[index], controller: 'school_email_domains' end resources :lessons, only: %i[index create show update destroy] do diff --git a/spec/factories/school_email_domain.rb b/spec/factories/school_email_domain.rb new file mode 100644 index 000000000..eb0f2802b --- /dev/null +++ b/spec/factories/school_email_domain.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :school_email_domain do + school + sequence(:domain) { |n| "domain#{n}.example.edu" } + end +end diff --git a/spec/features/school_email_domain/listing_school_email_domains_spec.rb b/spec/features/school_email_domain/listing_school_email_domains_spec.rb new file mode 100644 index 000000000..87e8d8758 --- /dev/null +++ b/spec/features/school_email_domain/listing_school_email_domains_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Listing school email domains', type: :request do + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + let(:school) { create(:school) } + let(:school_email_domains) { create_list(:school_email_domain, 5, school: school) } + + before do + school_email_domains + end + + describe '#index' do + shared_examples 'a successful school email domains index' do + it 'responds 200 OK' do + expect(response).to have_http_status(:ok) + end + + context 'when the school has domains' do + it 'returns a list of domains for the school' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data).to match_array(school.school_email_domains.pluck(:domain)) + end + end + + context 'when domains do not belong to the school' do + let(:other_school) { create(:school) } + let(:school_email_domains) { create_list(:school_email_domain, 5, school: other_school) } + + it 'returns an empty array' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data).to eq([]) + end + end + end + + context 'with an authorised owner' do + let(:owner) { create(:owner, school:, name: 'School Owner') } + + before do + authenticated_in_hydra_as(owner) + get("/api/schools/#{school.id}/school_email_domains", headers:) + end + + it_behaves_like 'a successful school email domains index' + end + + context 'with an authorised teacher' do + let(:teacher) { create(:teacher, school:, name: 'School Teacher') } + + before do + authenticated_in_hydra_as(teacher) + get("/api/schools/#{school.id}/school_email_domains", headers:) + end + + it_behaves_like 'a successful school email domains index' + end + + context 'when the user does not have access' do + it 'responds 403 Forbidden when the user is a school-owner for a different school' do + other_school = create(:school) + other_owner = create(:owner, school: other_school, name: 'School Owner') + authenticated_in_hydra_as(other_owner) + + get("/api/schools/#{school.id}/school_email_domains", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is a school-teacher for a different school' do + other_school = create(:school) + other_teacher = create(:teacher, school: other_school, name: 'School Teacher') + authenticated_in_hydra_as(other_teacher) + + get("/api/schools/#{school.id}/school_email_domains", headers:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is a student at the school' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + + get("/api/schools/#{school.id}/school_email_domains", headers:) + expect(response).to have_http_status(:forbidden) + end + end + + context 'when the user is not authenticated' do + it 'responds 401 Unauthorized when no token is given' do + get "/api/schools/#{school.id}/school_email_domains" + expect(response).to have_http_status(:unauthorized) + end + end + end +end From 2cf9498eb76285d51c0eee8f3506adad8d4c958d Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Wed, 17 Jun 2026 09:13:00 +0100 Subject: [PATCH 02/10] Allow school owners and teachers to create school email domains Add POST create to SchoolEmailDomainsController and route. Add SchoolEmailDomain::Create concept to persist domains and sync the full list with Profile. --- .../api/school_email_domains_controller.rb | 13 ++ config/locales/en.yml | 9 + config/routes.rb | 2 +- .../school_email_domain/operations/create.rb | 41 ++++ .../school_email_domain/create_spec.rb | 179 +++++++++++++++ .../creating_school_email_domains_spec.rb | 209 ++++++++++++++++++ spec/support/profile_api_mock.rb | 4 + 7 files changed, 456 insertions(+), 1 deletion(-) create mode 100644 lib/concepts/school_email_domain/operations/create.rb create mode 100644 spec/concepts/school_email_domain/create_spec.rb create mode 100644 spec/features/school_email_domain/creating_school_email_domains_spec.rb diff --git a/app/controllers/api/school_email_domains_controller.rb b/app/controllers/api/school_email_domains_controller.rb index 63dc1e5a7..39e3b5865 100644 --- a/app/controllers/api/school_email_domains_controller.rb +++ b/app/controllers/api/school_email_domains_controller.rb @@ -10,10 +10,23 @@ def index render json: school_email_domains, status: :ok end + def create + result = SchoolEmailDomain::Create.call(school: @school, domain: school_email_domain_params[:domain], token: current_user.token) + if result.success? + render json: { domain: result[:school_email_domain].domain }, status: :created + else + render json: { error: result[:error] }, status: :unprocessable_content + end + end + private def school_email_domains @school.school_email_domains.order(:created_at).pluck(:domain) end + + def school_email_domain_params + params.expect(school_email_domain: [:domain]) + end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index ce3d9c076..664fc5670 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -28,3 +28,12 @@ en: attributes: school_class: import_id: "Import id" + errors: + models: + school_email_domain: + attributes: + domain: + invalid: "is invalid" + invalid_host: "must be a fully qualified domain name" + invalid_public_suffix: "must be a registrable domain name" + invalid_uri: "must be a valid domain format" diff --git a/config/routes.rb b/config/routes.rb index 8217e173c..581c92710 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -82,7 +82,7 @@ post :batch, on: :collection, to: 'school_students#create_batch' delete :batch, on: :collection, to: 'school_students#destroy_batch' end - resources :school_email_domains, only: %i[index], controller: 'school_email_domains' + resources :school_email_domains, only: %i[index create], controller: 'school_email_domains' end resources :lessons, only: %i[index create show update destroy] do diff --git a/lib/concepts/school_email_domain/operations/create.rb b/lib/concepts/school_email_domain/operations/create.rb new file mode 100644 index 000000000..2059b8508 --- /dev/null +++ b/lib/concepts/school_email_domain/operations/create.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +class SchoolEmailDomain + class Create + class << self + def call(school:, domain:, token:) + response = OperationResponse.new + response[:school_email_domain] = build_domain(school, domain) + SchoolEmailDomain.transaction do + response[:school_email_domain].save! + update_profile(school, token) + end + response + rescue ActiveRecord::RecordInvalid => e + record = response[:school_email_domain] || e.record + response[:error] = record.errors.full_messages.join(', ') + response + rescue ActiveRecord::RecordNotUnique + record = response[:school_email_domain] + record.errors.add(:domain, :taken) + response[:error] = record.errors.full_messages.join(', ') + response + rescue StandardError => e + Sentry.capture_exception(e) # Send unexpected/Profile errors to Sentry + response[:error] = e.message + response + end + + private + + def build_domain(school, domain) + school.school_email_domains.build(domain:) + end + + def update_profile(school, token) + school_email_domains = school.school_email_domains.order(:created_at).pluck(:domain) + ProfileApiClient.update_school_email_domains(token:, school_id: school.id, school_email_domains:) + end + end + end +end diff --git a/spec/concepts/school_email_domain/create_spec.rb b/spec/concepts/school_email_domain/create_spec.rb new file mode 100644 index 000000000..49b541a37 --- /dev/null +++ b/spec/concepts/school_email_domain/create_spec.rb @@ -0,0 +1,179 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe SchoolEmailDomain::Create, type: :unit do + let(:school) { create(:school) } + let(:domain) { 'school.edu' } + let(:token) { UserProfileMock::TOKEN } + + before { stub_profile_api_update_school_email_domains } + + context 'with valid values' do + it 'returns a successful operation response' do + response = described_class.call(school:, domain:, token:) + expect(response.success?).to be(true) + end + + it 'creates a school email domain' do + expect { described_class.call(school:, domain:, token:) }.to change(SchoolEmailDomain, :count).by(1) + end + + it 'returns the domain in the operation response' do + response = described_class.call(school:, domain:, token:) + expect(response[:school_email_domain]).to be_a(SchoolEmailDomain) + end + + it 'assigns the domain' do + response = described_class.call(school:, domain:, token:) + expect(response[:school_email_domain].domain).to eq(domain) + end + + it 'assigns the school' do + response = described_class.call(school:, domain:, token:) + expect(response[:school_email_domain].school_id).to eq(school.id) + end + + it 'syncs the domains to Profile' do + described_class.call(school:, domain:, token:) + expect(ProfileApiClient).to have_received(:update_school_email_domains).with( + token:, + school_id: school.id, + school_email_domains: [domain] + ) + end + + context 'when multiple domains already exist' do + before do + create(:school_email_domain, school:, domain: 'first.edu') + create(:school_email_domain, school:, domain: 'second.edu') + create(:school_email_domain, school:, domain: 'third.edu') + end + + it 'syncs all domains to Profile' do + described_class.call(school:, domain:, token:) + expect(ProfileApiClient).to have_received(:update_school_email_domains).with( + token:, + school_id: school.id, + school_email_domains: ['first.edu', 'second.edu', 'third.edu', domain] + ) + end + end + end + + shared_examples 'an invalid record' do + before { allow(Sentry).to receive(:capture_exception) } + + it 'does not create a school email domain' do + expect { described_class.call(school:, domain:, token:) }.not_to change(SchoolEmailDomain, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(school:, domain:, token:) + expect(response.failure?).to be(true) + end + + it 'does not send the exception to Sentry' do + described_class.call(school:, domain:, token:) + expect(Sentry).not_to have_received(:capture_exception).with(kind_of(StandardError)) + end + + it 'returns the error message in the operation response' do + response = described_class.call(school:, domain:, token:) + expect(response[:error]).to include('') + end + + it 'does not attempt to update Profile' do + described_class.call(school:, domain:, token:) + expect(ProfileApiClient).not_to have_received(:update_school_email_domains) + end + end + + context 'when domain is blank' do + let(:domain) { '' } + let(:expected_error_message) { "Domain can't be blank" } + + it_behaves_like 'an invalid record' + end + + context 'when domain is not an FQDN' do + let(:domain) { 'edu' } + let(:expected_error_message) { 'Domain must be a fully qualified domain name' } + + it_behaves_like 'an invalid record' + end + + context 'when domain has an invalid URI' do + let(:domain) { 'exa mple.com' } + let(:expected_error_message) { 'Domain must be a valid domain format' } + + it_behaves_like 'an invalid record' + end + + context 'when domain has an invalid public suffix' do + let(:domain) { 'co.uk' } + let(:expected_error_message) { 'Domain must be a registrable domain name' } + + it_behaves_like 'an invalid record' + end + + context 'when domain is a duplicate' do + before { create(:school_email_domain, school:, domain:) } + + let(:expected_error_message) { 'Domain has already been taken' } + + it_behaves_like 'an invalid record' + end + + context 'when a concurrent request creates the same domain' do + let(:expected_error_message) { 'Domain has already been taken' } + let(:school_email_domain) { SchoolEmailDomain.new(school:, domain:) } + + before do + allow(Sentry).to receive(:capture_exception) + allow(school.school_email_domains).to receive(:build).with(domain:).and_return(school_email_domain) + allow(school_email_domain).to receive(:save!).and_raise(ActiveRecord::RecordNotUnique) + end + + it_behaves_like 'an invalid record' + end + + context 'when Profile sync fails' do + let(:profile_error) do + ProfileApiClient::UnexpectedResponse.new( + instance_double(Faraday::Response, status: 500, headers: {}, body: '') + ) + end + + before do + allow(Sentry).to receive(:capture_exception) + + allow(ProfileApiClient).to receive(:update_school_email_domains) + .and_raise(profile_error) + end + + it 'attempts to sync to Profile' do + described_class.call(school:, domain:, token:) + expect(ProfileApiClient).to have_received(:update_school_email_domains).once + end + + it 'does not persist the domain' do + expect { described_class.call(school:, domain:, token:) } + .not_to change(SchoolEmailDomain, :count) + end + + it 'sends the exception to Sentry' do + described_class.call(school:, domain:, token:) + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end + + it 'returns a failed operation response' do + expect(described_class.call(school:, domain:, token:)).to be_failure + end + + it 'returns the error message in the operation response' do + response = described_class.call(school:, domain:, token:) + expect(response[:error]).to eq('Unexpected response from Profile API (status code 500)') + end + end +end diff --git a/spec/features/school_email_domain/creating_school_email_domains_spec.rb b/spec/features/school_email_domain/creating_school_email_domains_spec.rb new file mode 100644 index 000000000..b0d686234 --- /dev/null +++ b/spec/features/school_email_domain/creating_school_email_domains_spec.rb @@ -0,0 +1,209 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Creating school email domains', type: :request do + let(:headers) { { Authorization: UserProfileMock::TOKEN } } + let(:school) { create(:school) } + let(:domain) { 'school.edu' } + let(:params) do + { + school_email_domain: { + domain: + } + } + end + let(:owner) { create(:owner, school:, name: 'School Owner') } + + before { stub_profile_api_update_school_email_domains } + + describe '#create' do + shared_examples 'a successful school email domain creation response' do + it 'responds 201 created' do + expect(response).to have_http_status(:created) + end + + it 'creates the domain' do + expect(SchoolEmailDomain.exists?(school:, domain:)).to be(true) + end + + it 'returns the domain' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:domain]).to eq(domain) + end + end + + shared_examples 'an unprocessable school email domain creation response' do + before do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + end + + it 'responds 422 Unprocessable Entity' do + expect(response).to have_http_status(:unprocessable_content) + end + + it 'returns the error in the response body' do + data = JSON.parse(response.body, symbolize_names: true) + expect(data[:error]).to match(expected_error) + end + end + + context 'with an authorised owner' do + before do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + end + + it_behaves_like 'a successful school email domain creation response' + end + + context 'with an authorised teacher' do + let(:teacher) { create(:teacher, school:, name: 'School Teacher') } + + before do + authenticated_in_hydra_as(teacher) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + end + + it_behaves_like 'a successful school email domain creation response' + end + + context 'with missing params' do + it 'responds 400 Bad Request when params are missing' do + authenticated_in_hydra_as(owner) + + post("/api/schools/#{school.id}/school_email_domains", headers:) + + expect(response).to have_http_status(:bad_request) + end + end + + context 'when the user does not have access' do + it 'responds 403 Forbidden when the user is a school-owner for a different school' do + other_school = create(:school) + other_owner = create(:owner, school: other_school, name: 'School Owner') + authenticated_in_hydra_as(other_owner) + + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is a school-teacher for a different school' do + other_school = create(:school) + other_teacher = create(:teacher, school: other_school, name: 'School Teacher') + authenticated_in_hydra_as(other_teacher) + + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(response).to have_http_status(:forbidden) + end + + it 'responds 403 Forbidden when the user is a student at the school' do + student = create(:student, school:) + authenticated_in_hydra_as(student) + + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(response).to have_http_status(:forbidden) + end + end + + context 'when the user is not authenticated' do + it 'responds 401 Unauthorized when no token is given' do + post "/api/schools/#{school.id}/school_email_domains" + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when the domain cannot be processed' do + context 'when the domain is blank' do + let(:domain) { '' } + let(:expected_error) { /Domain can't be blank/ } + + it_behaves_like 'an unprocessable school email domain creation response' + + it 'does not create a school email domain' do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(SchoolEmailDomain.where(school:)).to be_none + end + end + + context 'when the domain is not an FQDN' do + let(:domain) { 'edu' } + let(:expected_error) { /Domain must be a fully qualified domain name/ } + + it_behaves_like 'an unprocessable school email domain creation response' + + it 'does not create a school email domain' do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(SchoolEmailDomain.where(school:)).to be_none + end + end + + context 'when the uri is invalid' do + let(:domain) { 'exa mple.com' } + let(:expected_error) { /Domain must be a valid domain format/ } + + it_behaves_like 'an unprocessable school email domain creation response' + + it 'does not create a school email domain' do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(SchoolEmailDomain.where(school:)).to be_none + end + end + + context 'when the public suffix is invalid' do + let(:domain) { 'co.uk' } + let(:expected_error) { /Domain must be a registrable domain name/ } + + it_behaves_like 'an unprocessable school email domain creation response' + + it 'does not create a school email domain' do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(SchoolEmailDomain.where(school:)).to be_none + end + end + + context 'when the domain is a duplicate' do + let(:expected_error) { /Domain has already been taken/ } + + before do + create(:school_email_domain, school:, domain:) + end + + it_behaves_like 'an unprocessable school email domain creation response' + + it 'does not create a duplicate school email domain' do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + expect(SchoolEmailDomain.where(school:, domain:).count).to eq(1) + end + end + end + + context 'when Profile sync fails' do + let(:profile_error) do + ProfileApiClient::UnexpectedResponse.new( + instance_double(Faraday::Response, status: 500, headers: {}, body: '') + ) + end + let(:expected_error) { /Unexpected response from Profile API \(status code 500\)/ } + + before do + allow(ProfileApiClient).to receive(:update_school_email_domains).and_raise(profile_error) + end + + it_behaves_like 'an unprocessable school email domain creation response' + + it 'does not persist the domain' do + authenticated_in_hydra_as(owner) + post("/api/schools/#{school.id}/school_email_domains", headers:, params:) + + expect(SchoolEmailDomain.exists?(school:, domain:)).to be(false) + end + end + end +end diff --git a/spec/support/profile_api_mock.rb b/spec/support/profile_api_mock.rb index b3fae6cc5..8ed2cb3a8 100644 --- a/spec/support/profile_api_mock.rb +++ b/spec/support/profile_api_mock.rb @@ -129,4 +129,8 @@ def stub_profile_api_validate_students_with_validation_error ) ) end + + def stub_profile_api_update_school_email_domains + allow(ProfileApiClient).to receive(:update_school_email_domains).and_return(true) + end end From 337551cd278427701cdcd7335233f0c70e6d3d9d Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Mon, 22 Jun 2026 08:49:05 +0100 Subject: [PATCH 03/10] Return error_codes in SchoolEmailDomain response --- .../api/school_email_domains_controller.rb | 2 +- .../school_email_domain/operations/create.rb | 7 +++++++ .../school_email_domain/create_spec.rb | 20 +++++++++---------- .../creating_school_email_domains_spec.rb | 16 +++++++-------- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/school_email_domains_controller.rb b/app/controllers/api/school_email_domains_controller.rb index 39e3b5865..18a9c2662 100644 --- a/app/controllers/api/school_email_domains_controller.rb +++ b/app/controllers/api/school_email_domains_controller.rb @@ -15,7 +15,7 @@ def create if result.success? render json: { domain: result[:school_email_domain].domain }, status: :created else - render json: { error: result[:error] }, status: :unprocessable_content + render json: { error: result[:error], error_code: result[:error_code] }, status: :unprocessable_content end end diff --git a/lib/concepts/school_email_domain/operations/create.rb b/lib/concepts/school_email_domain/operations/create.rb index 2059b8508..0f9c33646 100644 --- a/lib/concepts/school_email_domain/operations/create.rb +++ b/lib/concepts/school_email_domain/operations/create.rb @@ -14,15 +14,18 @@ def call(school:, domain:, token:) rescue ActiveRecord::RecordInvalid => e record = response[:school_email_domain] || e.record response[:error] = record.errors.full_messages.join(', ') + response[:error_code] = domain_error_code(record) response rescue ActiveRecord::RecordNotUnique record = response[:school_email_domain] record.errors.add(:domain, :taken) response[:error] = record.errors.full_messages.join(', ') + response[:error_code] = 'taken' response rescue StandardError => e Sentry.capture_exception(e) # Send unexpected/Profile errors to Sentry response[:error] = e.message + response[:error_code] = 'profile_sync_failed' response end @@ -36,6 +39,10 @@ def update_profile(school, token) school_email_domains = school.school_email_domains.order(:created_at).pluck(:domain) ProfileApiClient.update_school_email_domains(token:, school_id: school.id, school_email_domains:) end + + def domain_error_code(record) + record.errors.details[:domain].first.fetch(:error).to_s + end end end end diff --git a/spec/concepts/school_email_domain/create_spec.rb b/spec/concepts/school_email_domain/create_spec.rb index 49b541a37..3cc1ba140 100644 --- a/spec/concepts/school_email_domain/create_spec.rb +++ b/spec/concepts/school_email_domain/create_spec.rb @@ -78,9 +78,9 @@ expect(Sentry).not_to have_received(:capture_exception).with(kind_of(StandardError)) end - it 'returns the error message in the operation response' do + it 'returns the error code in the operation response' do response = described_class.call(school:, domain:, token:) - expect(response[:error]).to include('') + expect(response[:error_code]).to eq(expected_error_code) end it 'does not attempt to update Profile' do @@ -91,28 +91,28 @@ context 'when domain is blank' do let(:domain) { '' } - let(:expected_error_message) { "Domain can't be blank" } + let(:expected_error_code) { 'blank' } it_behaves_like 'an invalid record' end context 'when domain is not an FQDN' do let(:domain) { 'edu' } - let(:expected_error_message) { 'Domain must be a fully qualified domain name' } + let(:expected_error_code) { 'invalid_host' } it_behaves_like 'an invalid record' end context 'when domain has an invalid URI' do let(:domain) { 'exa mple.com' } - let(:expected_error_message) { 'Domain must be a valid domain format' } + let(:expected_error_code) { 'invalid_uri' } it_behaves_like 'an invalid record' end context 'when domain has an invalid public suffix' do let(:domain) { 'co.uk' } - let(:expected_error_message) { 'Domain must be a registrable domain name' } + let(:expected_error_code) { 'invalid_public_suffix' } it_behaves_like 'an invalid record' end @@ -120,13 +120,13 @@ context 'when domain is a duplicate' do before { create(:school_email_domain, school:, domain:) } - let(:expected_error_message) { 'Domain has already been taken' } + let(:expected_error_code) { 'taken' } it_behaves_like 'an invalid record' end context 'when a concurrent request creates the same domain' do - let(:expected_error_message) { 'Domain has already been taken' } + let(:expected_error_code) { 'taken' } let(:school_email_domain) { SchoolEmailDomain.new(school:, domain:) } before do @@ -171,9 +171,9 @@ expect(described_class.call(school:, domain:, token:)).to be_failure end - it 'returns the error message in the operation response' do + it 'returns the error code in the operation response' do response = described_class.call(school:, domain:, token:) - expect(response[:error]).to eq('Unexpected response from Profile API (status code 500)') + expect(response[:error_code]).to eq('profile_sync_failed') end end end diff --git a/spec/features/school_email_domain/creating_school_email_domains_spec.rb b/spec/features/school_email_domain/creating_school_email_domains_spec.rb index b0d686234..206fc017e 100644 --- a/spec/features/school_email_domain/creating_school_email_domains_spec.rb +++ b/spec/features/school_email_domain/creating_school_email_domains_spec.rb @@ -43,9 +43,9 @@ expect(response).to have_http_status(:unprocessable_content) end - it 'returns the error in the response body' do + it 'returns the error code in the response body' do data = JSON.parse(response.body, symbolize_names: true) - expect(data[:error]).to match(expected_error) + expect(data[:error_code]).to eq(expected_error_code) end end @@ -117,7 +117,7 @@ context 'when the domain cannot be processed' do context 'when the domain is blank' do let(:domain) { '' } - let(:expected_error) { /Domain can't be blank/ } + let(:expected_error_code) { 'blank' } it_behaves_like 'an unprocessable school email domain creation response' @@ -130,7 +130,7 @@ context 'when the domain is not an FQDN' do let(:domain) { 'edu' } - let(:expected_error) { /Domain must be a fully qualified domain name/ } + let(:expected_error_code) { 'invalid_host' } it_behaves_like 'an unprocessable school email domain creation response' @@ -143,7 +143,7 @@ context 'when the uri is invalid' do let(:domain) { 'exa mple.com' } - let(:expected_error) { /Domain must be a valid domain format/ } + let(:expected_error_code) { 'invalid_uri' } it_behaves_like 'an unprocessable school email domain creation response' @@ -156,7 +156,7 @@ context 'when the public suffix is invalid' do let(:domain) { 'co.uk' } - let(:expected_error) { /Domain must be a registrable domain name/ } + let(:expected_error_code) { 'invalid_public_suffix' } it_behaves_like 'an unprocessable school email domain creation response' @@ -168,7 +168,7 @@ end context 'when the domain is a duplicate' do - let(:expected_error) { /Domain has already been taken/ } + let(:expected_error_code) { 'taken' } before do create(:school_email_domain, school:, domain:) @@ -190,7 +190,7 @@ instance_double(Faraday::Response, status: 500, headers: {}, body: '') ) end - let(:expected_error) { /Unexpected response from Profile API \(status code 500\)/ } + let(:expected_error_code) { 'profile_sync_failed' } before do allow(ProfileApiClient).to receive(:update_school_email_domains).and_raise(profile_error) From b51fcfb602fe37d68ba02c37d6a1b70aaac14524 Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Tue, 23 Jun 2026 14:14:10 +0100 Subject: [PATCH 04/10] Seed school with student_sso feature flag --- lib/tasks/test_seeds.rake | 2 ++ spec/lib/test_seeds_spec.rb | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/lib/tasks/test_seeds.rake b/lib/tasks/test_seeds.rake index dc0aec7aa..264963e96 100644 --- a/lib/tasks/test_seeds.rake +++ b/lib/tasks/test_seeds.rake @@ -53,6 +53,8 @@ namespace :test_seeds do school = create_school(creator_id, TEST_SCHOOL) Flipper.enable_actor :cat_mode, school + Flipper.enable_actor :student_sso, school + verify_school(school) assign_a_teacher(teacher_id, school) diff --git a/spec/lib/test_seeds_spec.rb b/spec/lib/test_seeds_spec.rb index 40e828347..ae5457478 100644 --- a/spec/lib/test_seeds_spec.rb +++ b/spec/lib/test_seeds_spec.rb @@ -59,6 +59,11 @@ expect(Flipper.enabled?(:cat_mode, school)).to be(true) end + it 'enables student sso for the school' do + school = School.find_by(creator_id:) + expect(Flipper.enabled?(:student_sso, school)).to be(true) + end + it 'creates lessons with projects' do school = School.find_by(creator_id:) expect(SchoolClass.where(school_id: school.id)).to exist From 27cc95f427e183b5071aea4a9b4ae6216b058d09 Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Tue, 23 Jun 2026 14:17:49 +0100 Subject: [PATCH 05/10] Initialise response[:school_email_domain] before assignment --- lib/concepts/school_email_domain/operations/create.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/concepts/school_email_domain/operations/create.rb b/lib/concepts/school_email_domain/operations/create.rb index 0f9c33646..65fdc3358 100644 --- a/lib/concepts/school_email_domain/operations/create.rb +++ b/lib/concepts/school_email_domain/operations/create.rb @@ -5,6 +5,7 @@ class Create class << self def call(school:, domain:, token:) response = OperationResponse.new + response[:school_email_domain] = nil response[:school_email_domain] = build_domain(school, domain) SchoolEmailDomain.transaction do response[:school_email_domain].save! From dddfb107992412687611b5aa09a4d0e8d74eb779 Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Tue, 23 Jun 2026 16:35:05 +0100 Subject: [PATCH 06/10] Lock school while creating domains and syncing with Profile --- lib/concepts/school_email_domain/operations/create.rb | 2 +- spec/concepts/school_email_domain/create_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/concepts/school_email_domain/operations/create.rb b/lib/concepts/school_email_domain/operations/create.rb index 65fdc3358..2d96ab401 100644 --- a/lib/concepts/school_email_domain/operations/create.rb +++ b/lib/concepts/school_email_domain/operations/create.rb @@ -7,7 +7,7 @@ def call(school:, domain:, token:) response = OperationResponse.new response[:school_email_domain] = nil response[:school_email_domain] = build_domain(school, domain) - SchoolEmailDomain.transaction do + school.with_lock do response[:school_email_domain].save! update_profile(school, token) end diff --git a/spec/concepts/school_email_domain/create_spec.rb b/spec/concepts/school_email_domain/create_spec.rb index 3cc1ba140..64f904506 100644 --- a/spec/concepts/school_email_domain/create_spec.rb +++ b/spec/concepts/school_email_domain/create_spec.rb @@ -43,6 +43,14 @@ ) end + it 'locks the school while creating and syncing to Profile' do + allow(school).to receive(:with_lock).and_call_original + + described_class.call(school:, domain:, token:) + + expect(school).to have_received(:with_lock) + end + context 'when multiple domains already exist' do before do create(:school_email_domain, school:, domain: 'first.edu') From 528a359eb79f8acc879a2c172ba87be561776504 Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Thu, 25 Jun 2026 09:31:22 +0100 Subject: [PATCH 07/10] Acquire advisory lock for SchoolEmailDomain creation Serialise concurrent creates per school so Profile always receives a complete domain list. --- .../school_email_domain/operations/create.rb | 17 ++++++++++++++++- .../concepts/school_email_domain/create_spec.rb | 9 ++++----- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/concepts/school_email_domain/operations/create.rb b/lib/concepts/school_email_domain/operations/create.rb index 2d96ab401..3e952d95b 100644 --- a/lib/concepts/school_email_domain/operations/create.rb +++ b/lib/concepts/school_email_domain/operations/create.rb @@ -2,13 +2,17 @@ class SchoolEmailDomain class Create + LOCK_NAMESPACE = Zlib.crc32(name) # convert the class name into a 32-bit int to derive the namespace for our advisory lock + class << self def call(school:, domain:, token:) response = OperationResponse.new response[:school_email_domain] = nil response[:school_email_domain] = build_domain(school, domain) - school.with_lock do + + SchoolEmailDomain.transaction do response[:school_email_domain].save! + acquire_advisory_lock_for_school(school) update_profile(school, token) end response @@ -32,6 +36,17 @@ def call(school:, domain:, token:) private + def acquire_advisory_lock_for_school(school) + # Advisory lock: queue school email domain creation for the same school so Profile + # always gets a complete domain list. Released automatically on commit or rollback. + # + # lock_key is a CRC32 hash, so two different schools could theoretically share the same + # key (~1 in 4 billion per pair). That wouldn't corrupt data — it would only queue + # unrelated schools' updates briefly. + lock_key = Zlib.crc32("#{LOCK_NAMESPACE}:#{school.id}") + SchoolEmailDomain.connection.execute("SELECT pg_advisory_xact_lock(#{lock_key})") + end + def build_domain(school, domain) school.school_email_domains.build(domain:) end diff --git a/spec/concepts/school_email_domain/create_spec.rb b/spec/concepts/school_email_domain/create_spec.rb index 64f904506..a79083137 100644 --- a/spec/concepts/school_email_domain/create_spec.rb +++ b/spec/concepts/school_email_domain/create_spec.rb @@ -43,12 +43,11 @@ ) end - it 'locks the school while creating and syncing to Profile' do - allow(school).to receive(:with_lock).and_call_original - + it 'acquires an advisory lock while creating and syncing to Profile' do + allow(SchoolEmailDomain.connection).to receive(:execute).and_call_original described_class.call(school:, domain:, token:) - - expect(school).to have_received(:with_lock) + lock_key = Zlib.crc32("#{SchoolEmailDomain::Create::LOCK_NAMESPACE}:#{school.id}") + expect(SchoolEmailDomain.connection).to have_received(:execute).with("SELECT pg_advisory_xact_lock(#{lock_key})") end context 'when multiple domains already exist' do From e18014039084bf9890b1d4b8c8142edd982d720a Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Thu, 25 Jun 2026 10:42:51 +0100 Subject: [PATCH 08/10] Remove advisory_lock comment in SchoolEmailDomain::Create --- lib/concepts/school_email_domain/operations/create.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/concepts/school_email_domain/operations/create.rb b/lib/concepts/school_email_domain/operations/create.rb index 3e952d95b..0be25c667 100644 --- a/lib/concepts/school_email_domain/operations/create.rb +++ b/lib/concepts/school_email_domain/operations/create.rb @@ -37,12 +37,6 @@ def call(school:, domain:, token:) private def acquire_advisory_lock_for_school(school) - # Advisory lock: queue school email domain creation for the same school so Profile - # always gets a complete domain list. Released automatically on commit or rollback. - # - # lock_key is a CRC32 hash, so two different schools could theoretically share the same - # key (~1 in 4 billion per pair). That wouldn't corrupt data — it would only queue - # unrelated schools' updates briefly. lock_key = Zlib.crc32("#{LOCK_NAMESPACE}:#{school.id}") SchoolEmailDomain.connection.execute("SELECT pg_advisory_xact_lock(#{lock_key})") end From 71e4b63153e3d1ec1e42073ace58988fec2961ac Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Fri, 26 Jun 2026 11:09:07 +0100 Subject: [PATCH 09/10] Add SchoolEmailDomains::Create spec to test pg_advisory_xact_lock --- .../school_email_domain/create_spec.rb | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/spec/concepts/school_email_domain/create_spec.rb b/spec/concepts/school_email_domain/create_spec.rb index a79083137..d143326b3 100644 --- a/spec/concepts/school_email_domain/create_spec.rb +++ b/spec/concepts/school_email_domain/create_spec.rb @@ -50,6 +50,51 @@ expect(SchoolEmailDomain.connection).to have_received(:execute).with("SELECT pg_advisory_xact_lock(#{lock_key})") end + context 'when two requests race for the same school', :no_transaction do + # RSpec's use_transactional_tests config wraps each test in a transaction + # This example tests the behaviour of a pg_advisory_xact_lock, which is released when a transaction commits or rolls back + # We need to set use_transactional_tests to false, otherwise the advisory lock is released + # only when the spec itself finishes, not when the transaction under test completes + self.use_transactional_tests = false + + # Since this test example is no longer scoped to a transaction, we need to ensure + # that we've cleaned the DB before and after it runs so data doesn't leak into the other specs + around do |example| + DatabaseCleaner.strategy = :truncation + DatabaseCleaner.clean_with(:truncation) + DatabaseCleaner.cleaning { example.run } + end + + it 'blocks a second session while the lock is held and frees it on commit' do + lock_key = Zlib.crc32("#{SchoolEmailDomain::Create::LOCK_NAMESPACE}:#{school.id}") + + acquired_while_held = nil + + ActiveRecord::Base.transaction do # open a transaction on the main thread + ActiveRecord::Base.connection.execute("SELECT pg_advisory_xact_lock(#{lock_key})") # acquire and hold the lock + # Attempt to acquire the lock from a separate thread + # acquired_while_held should be false since the main thread has yet to complete its transaction + acquired_while_held = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do |conn| + conn.select_value("SELECT pg_try_advisory_xact_lock(#{lock_key})") + end + end.value + end + + # Now that the transaction is complete we should be able to acquire the lock, + # acquired_after_commit should be true demonstrating that the main thread has released the lock + # and another thread can acquire it + acquired_after_commit = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do |conn| + conn.select_value("SELECT pg_try_advisory_xact_lock(#{lock_key})") + end + end.value + + expect(acquired_while_held).to be(false) # a concurrent session is blocked + expect(acquired_after_commit).to be(true) # a subsequent session can acquire the lock + end + end + context 'when multiple domains already exist' do before do create(:school_email_domain, school:, domain: 'first.edu') From d4e13e1c5e3e341ebef3faf5387836f4863bcbea Mon Sep 17 00:00:00 2001 From: Pete Simonovic <69108995+PetarSimonovic@users.noreply.github.com> Date: Fri, 26 Jun 2026 16:29:16 +0100 Subject: [PATCH 10/10] Remove concurrent update spec for SchoolEmailDomain advisory lock The spec tests PG directly, rather than our code, and its complicated structure requires local overrides to RSpec configuration. Full discussion, with considerations of other ways of testing the lock, is recorded on the PR (#878): https://github.com/RaspberryPiFoundation/editor-api/pull/878#discussion_r3473389365 --- .../school_email_domain/create_spec.rb | 45 ------------------- 1 file changed, 45 deletions(-) diff --git a/spec/concepts/school_email_domain/create_spec.rb b/spec/concepts/school_email_domain/create_spec.rb index d143326b3..a79083137 100644 --- a/spec/concepts/school_email_domain/create_spec.rb +++ b/spec/concepts/school_email_domain/create_spec.rb @@ -50,51 +50,6 @@ expect(SchoolEmailDomain.connection).to have_received(:execute).with("SELECT pg_advisory_xact_lock(#{lock_key})") end - context 'when two requests race for the same school', :no_transaction do - # RSpec's use_transactional_tests config wraps each test in a transaction - # This example tests the behaviour of a pg_advisory_xact_lock, which is released when a transaction commits or rolls back - # We need to set use_transactional_tests to false, otherwise the advisory lock is released - # only when the spec itself finishes, not when the transaction under test completes - self.use_transactional_tests = false - - # Since this test example is no longer scoped to a transaction, we need to ensure - # that we've cleaned the DB before and after it runs so data doesn't leak into the other specs - around do |example| - DatabaseCleaner.strategy = :truncation - DatabaseCleaner.clean_with(:truncation) - DatabaseCleaner.cleaning { example.run } - end - - it 'blocks a second session while the lock is held and frees it on commit' do - lock_key = Zlib.crc32("#{SchoolEmailDomain::Create::LOCK_NAMESPACE}:#{school.id}") - - acquired_while_held = nil - - ActiveRecord::Base.transaction do # open a transaction on the main thread - ActiveRecord::Base.connection.execute("SELECT pg_advisory_xact_lock(#{lock_key})") # acquire and hold the lock - # Attempt to acquire the lock from a separate thread - # acquired_while_held should be false since the main thread has yet to complete its transaction - acquired_while_held = Thread.new do - ActiveRecord::Base.connection_pool.with_connection do |conn| - conn.select_value("SELECT pg_try_advisory_xact_lock(#{lock_key})") - end - end.value - end - - # Now that the transaction is complete we should be able to acquire the lock, - # acquired_after_commit should be true demonstrating that the main thread has released the lock - # and another thread can acquire it - acquired_after_commit = Thread.new do - ActiveRecord::Base.connection_pool.with_connection do |conn| - conn.select_value("SELECT pg_try_advisory_xact_lock(#{lock_key})") - end - end.value - - expect(acquired_while_held).to be(false) # a concurrent session is blocked - expect(acquired_after_commit).to be(true) # a subsequent session can acquire the lock - end - end - context 'when multiple domains already exist' do before do create(:school_email_domain, school:, domain: 'first.edu')