868 add school email domain api

[PetarSimonovic]

Status

• Closes Add school email domain API #868

Description

A REST API that allows school owners and teachers to create and list SchoolEmailDomains.

• Owners and teachers can list/create; students and users from other schools cannot
• Domains are validated (FQDN format, URI, registrable public suffix) with translated error messages
• New domains are synced to Profile; the DB transaction rolls back if Profile sync fails
• Delete is out of scope for this PR

What's changed?

• GET /api/schools/:school_id/school_email_domains — list domains
• POST /api/schools/:school_id/school_email_domains — create a domain
• Added concept with Profile sync + rollback on failure
• Validation with translated error messages
• Specs at validator, model, concept, and feature levels

[github-actions]

Test coverage

92.04% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/28231511539

[Copilot]

Pull request overview

Adds a REST API for managing a school’s allowed student email domains, intended for use by authorised school owners/teachers and kept in sync with the Profile service.

Changes:

• Introduces GET /api/schools/:school_id/school_email_domains (list) and POST /api/schools/:school_id/school_email_domains (create) endpoints.
• Adds a SchoolEmailDomain::Create concept that persists the domain and syncs the school’s full domain list to Profile (rolling back on sync failure).
• Extends authorisation and i18n error messages, plus adds factories and request/unit specs.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
spec/support/profile_api_mock.rb	Adds a stub helper for Profile “update school email domains” calls in specs.
spec/features/school_email_domain/listing_school_email_domains_spec.rb	Request specs for listing domains with auth/unauth coverage.
spec/features/school_email_domain/creating_school_email_domains_spec.rb	Request specs for creating domains, validation errors, and Profile failure handling.
spec/factories/school_email_domain.rb	Factory for SchoolEmailDomain.
spec/concepts/school_email_domain/create_spec.rb	Unit coverage for the SchoolEmailDomain::Create concept.
lib/concepts/school_email_domain/operations/create.rb	Implements the create + Profile sync operation with rollback on failure.
config/routes.rb	Wires nested school email domain routes under schools.
config/locales/en.yml	Adds translated validation messages for domain validation error codes.
app/models/ability.rb	Grants school owners/teachers ability to read/create school email domains.
app/controllers/api/school_email_domains_controller.rb	Adds the API controller for index/create with CanCan authorisation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mwtrew]

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]

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

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit e24d6af. Configure here.}

[mwtrew]

Looking good overall, but I think we'll need to do something about the problem in this thread.

[mwtrew]

Looks good, thanks for those changes.

If you'd like to look into using an advisory lock that would be great, but I think what you have will work for now.

[PetarSimonovic]

Happy to remove/shorten this comment if the point about identical key values is overkill

[mwtrew]

Yep, I think the comment can go

[mwtrew]

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

[PetarSimonovic]

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.

[mwtrew]

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]

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.