Skip to content

Store school email domains#787

Open
PetarSimonovic wants to merge 10 commits intomainfrom
store-school-email-domains
Open

Store school email domains#787
PetarSimonovic wants to merge 10 commits intomainfrom
store-school-email-domains

Conversation

@PetarSimonovic
Copy link
Copy Markdown

@PetarSimonovic PetarSimonovic commented Apr 21, 2026

Status

Closes #1325

What's changed?

  • Add a school_email_domains table
  • Add a SchoolEmailDomain model
  • Perform minimal formatting on a domain to strip leading @ and lowercase the string
  • Update association on School
  • Add and update specs

Copilot AI review requested due to automatic review settings April 21, 2026 11:05
@cla-bot cla-bot Bot added the cla-signed label Apr 21, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Introduces first-class storage for school email domains by adding a dedicated table/model and wiring it into the School domain model, with accompanying specs.

Changes:

  • Added school_email_domains table with a composite unique index per school.
  • Added SchoolEmailDomain model with before_validation normalization (strip leading @, downcase).
  • Updated School to has_many :school_email_domains and added/updated model specs.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
spec/models/school_spec.rb Adds association + dependent-destroy coverage for school_email_domains.
spec/models/school_email_domain_spec.rb Adds model spec coverage for domain normalization behavior.
db/schema.rb Reflects new school_email_domains table and foreign key.
db/migrate/20260420104937_create_school_email_domains.rb Creates the new school_email_domains table and composite unique index.
app/models/school_email_domain.rb Introduces the new model and domain normalization callback.
app/models/school.rb Adds has_many :school_email_domains, dependent: :destroy.

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

Comment thread app/models/school_email_domain.rb
Comment thread spec/models/school_email_domain_spec.rb
Comment thread db/migrate/20260420104937_create_school_email_domains.rb
Add a migration to create the school_email_domains table, storing a domain string against a school_id
Create the model and add initial specs
Removing leading @ symbols and lower case domains
@PetarSimonovic PetarSimonovic force-pushed the store-school-email-domains branch from 4740867 to aff003a Compare April 21, 2026 12:38
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 21, 2026

Test coverage

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.


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

Comment thread app/models/school_email_domain.rb
Comment thread spec/models/school_email_domain_spec.rb
Comment thread app/models/school_email_domain.rb Outdated
Copy link
Copy Markdown
Contributor

@fspeirs fspeirs left a comment

Choose a reason for hiding this comment

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

Overall this looks like the right approach. I have one consideration about validating domains - I feel like it would be helpful to be stricter in checking that the user hasn't typo'ed their domain or misunderstood what we are asking for.

Comment thread app/models/school_email_domain.rb Outdated
Comment thread spec/models/school_email_domain_spec.rb
@PetarSimonovic PetarSimonovic requested a review from fspeirs April 22, 2026 10:31
Copy link
Copy Markdown
Contributor

@fspeirs fspeirs left a comment

Choose a reason for hiding this comment

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

Directionally, quite correct. Just some sharpening of the normalisation approach in school_email_domain and a request for a testing method in school.rb. The lack of a testing method is non-blocking if it's coming in a separate PR.

Comment on lines +14 to +18
def normalise_domain
return if domain.nil?

self.domain = build_normalised_domain_string(domain)
end
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.

Issue: I think this code could be much simpler and doesn't have to expressly handle specific things like uri_host_if_http_url. Instead, I suggest that we just try to parse a URI and, if successful take the host portion:

def normalise_domain
  return if domain.blank?
  value = domain.strip.downcase
  # Add a scheme unless it already has one, so URI can parse it
  value = "http://#{value}" unless value =~ %r{\A[a-z][a-z0-9+\-.]*://}i
  
  uri = URI.parse(value)

  host = uri.host
  return if host.blank?

  if PublicSuffix.valid?(host)
    self.domain = host
  else
    errors.add(:domain, "is not a valid domain")
  end
rescue URI::InvalidURIError
  errors.add(:domain, "is not a valid domain")
end

This is kind of the approach you're taking in uri_host_if_http_url and pulling it up to be the main way we normalise domains: (a) can we parse it with URI::parse and secondly is it a valid suffix.

Comment thread app/models/school.rb
has_many :projects, dependent: :nullify
has_many :roles, dependent: :nullify
has_many :school_projects, dependent: :nullify
has_many :school_email_domains, dependent: :destroy
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.

Issue: Callers (including my work) would like a validator method on this called something like: valid_domain?(candidate_domain) which returns true if candidate_domain is registered for this school.

This would allow testing domains presented by registering users without exposing the internal implementation of school_email_domains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants