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 1/8] 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 2/8] 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 3/8] 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 4/8] 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 5/8] 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 6/8] 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 7/8] 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 8/8] 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