Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions app/controllers/api/school_email_domains_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# 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

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], error_code: result[:error_code] }, 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
2 changes: 2 additions & 0 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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:)
Expand Down Expand Up @@ -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:)
Expand Down
9 changes: 9 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 create], controller: 'school_email_domains'
end

resources :lessons, only: %i[index create show update destroy] do
Expand Down
58 changes: 58 additions & 0 deletions lib/concepts/school_email_domain/operations/create.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# frozen_string_literal: true

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)

SchoolEmailDomain.transaction do
response[:school_email_domain].save!
acquire_advisory_lock_for_school(school)
update_profile(school, token)
end
response
rescue ActiveRecord::RecordInvalid => e
record = response[:school_email_domain] || e.record

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth fixing this after all - I don't expect it to be a problem but it's a trivial change. It's similar to what's been done here.

@PetarSimonovic PetarSimonovic Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this changed and initialised with nil since response[:school_email_domain] should be a single record

response[:error] = record.errors.full_messages.join(', ')
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
Comment thread
PetarSimonovic marked this conversation as resolved.
Dismissed
response[:error_code] = domain_error_code(record)
Comment thread
PetarSimonovic marked this conversation as resolved.
Dismissed
response
rescue ActiveRecord::RecordNotUnique
record = response[:school_email_domain]
Comment thread
PetarSimonovic marked this conversation as resolved.
Dismissed
record.errors.add(:domain, :taken)
response[:error] = record.errors.full_messages.join(', ')
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
Comment thread
PetarSimonovic marked this conversation as resolved.
Dismissed
response[:error_code] = 'taken'
Comment thread
PetarSimonovic marked this conversation as resolved.
Dismissed
response
rescue StandardError => e
Sentry.capture_exception(e) # Send unexpected/Profile errors to Sentry
response[:error] = e.message
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
response[:error_code] = 'profile_sync_failed'
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
response
end

private

def acquire_advisory_lock_for_school(school)
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

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:)
Comment thread
PetarSimonovic marked this conversation as resolved.
end
Comment thread
PetarSimonovic marked this conversation as resolved.

def domain_error_code(record)
record.errors.details[:domain].first.fetch(:error).to_s
end
end
end
end
2 changes: 2 additions & 0 deletions lib/tasks/test_seeds.rake
Original file line number Diff line number Diff line change
Expand Up @@ -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

Comment thread
mwtrew marked this conversation as resolved.
verify_school(school)
assign_a_teacher(teacher_id, school)

Expand Down
186 changes: 186 additions & 0 deletions spec/concepts/school_email_domain/create_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
# 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

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:)
lock_key = Zlib.crc32("#{SchoolEmailDomain::Create::LOCK_NAMESPACE}:#{school.id}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also check that the lock was released at the end?

@PetarSimonovic PetarSimonovic Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered a couple of approaches to check that the lock is released and I'm not sure it's possible to do this in a simple way for a couple of reasons.

1. Ask postgres

I thought the simplest would be to query the DB to see if any matching locks remain after the transaction.

But Rspec is configured to use_transactional_fixtures so I think the spec example itself is wrapped in a transaction. The advisory lock is only released once the spec has concluded so it's still present in the test.

2. pg_try_advisory_xact_lock

We could use pg_try_advisory_xact_lock to see if we can obtain a lock after our transaction has run. A false result would mean it was "still locked"

But within a given session I don't think we can ever get a false result: pg_try_advisory_xact_lock either acquires the lock, or returns true because the session already holds it.

Cursor suggestion

Cursor made the following suggestion about how we could practically check whether the lock has released. I'm happy to pursue it if it looks like a valid approach.

Use a hook in rails_helper.rb to disable transactional fixtures per-example:

Then create a new Thread to check out its own connection from the pool, which is a distinct PG session, so it can actually contend for the lock.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, didn't know that about Rspec tests. Yep, I was thinking of the first option as well.

I don't know if you've been able to test the contention situation manually - if you have and it works I think that's good enough, but if not it would be ideal to try it via a test like Cursor has suggested.

@PetarSimonovic PetarSimonovic Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been genuinely interesting and I think I've learned a fair bit about locks - and the pain of testing them! I've added a spec but I want to raise a couple of points

Tests Postgres, not the class
The test checks the behaviour of pg_advisory_xact_lock, not our code — it never calls described_class.call (our interface) and instead probes Postgres directly. Postgres documents that transaction-level advisory locks are released when the transaction ends (Advisory Locks), so this largely re-verifies the library's own guarantee.

To test our class directly we'd have to stub ProfileApiClient to hold the transaction window open long enough to race a concurrent thread. That introduces a hang risk: if the spec is changed so the lock is never released, the example would block indefinitely and stall the suite. The alternative would be to set a timeout so the wait always ends, but that itself feels flaky for a unit test.

I'm happy to try a different approach if there are other ways to test this.

The existing acquires an advisory lock... test checks that we apply the right kind of lock (scoped to a transaction) so I think that's definitely worth keeping.

Complicated structure
The test is complicated, and seems to go against the grain of our other specs: it overrides RSpec configuration locally and adds its own cleanup hook. I've annotated it (partly to help with reviewing) but I can remove or cut back the comments if needed. Again, happy to try a different approach if there are ways to test this without changing config.

Use a gem

An alternative would be to use the with_advisory_lock gem.

https://github.com/ClosureTree/with_advisory_lock

This doesn't directly solve the testing problem but does mean we can apply a widely used, production-tested library, removing the need for hand-written SQL and key creation (which is prob the most error-prone part of the existing code).

This does mean that we'd be adding an entire library for a single method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points - I was hoping we could test it in context but the lack of a real Profile API complicates things a little too much. Let's perhaps skip the automated test then.

expect(SchoolEmailDomain.connection).to have_received(:execute).with("SELECT pg_advisory_xact_lock(#{lock_key})")
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 code in the operation response' do
response = described_class.call(school:, domain:, token:)
expect(response[:error_code]).to eq(expected_error_code)
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_code) { 'blank' }

it_behaves_like 'an invalid record'
end

context 'when domain is not an FQDN' do
let(:domain) { 'edu' }
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_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_code) { 'invalid_public_suffix' }

it_behaves_like 'an invalid record'
end

context 'when domain is a duplicate' do
before { create(:school_email_domain, school:, domain:) }

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_code) { '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 code in the operation response' do
response = described_class.call(school:, domain:, token:)
expect(response[:error_code]).to eq('profile_sync_failed')
end
end
end
8 changes: 8 additions & 0 deletions spec/factories/school_email_domain.rb
Original file line number Diff line number Diff line change
@@ -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
Loading
Loading