-
Notifications
You must be signed in to change notification settings - Fork 5
Store school email domains #787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8261e61
49bb35e
8391fbb
aff003a
6417af7
b528185
cb94318
ce7c580
142142c
80f2296
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class SchoolEmailDomain < ApplicationRecord | ||
| belongs_to :school | ||
|
|
||
| validates :domain, presence: true | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| validates :domain, uniqueness: { scope: :school_id } | ||
|
|
||
| before_validation :normalise_domain | ||
| validate :validate_public_suffix | ||
|
|
||
| private | ||
|
|
||
| def normalise_domain | ||
| return if domain.nil? | ||
|
|
||
| self.domain = build_normalised_domain_string(domain) | ||
| end | ||
|
Comment on lines
+14
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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")
endThis is kind of the approach you're taking in |
||
|
|
||
| # Uses the Public Suffix List via the public_suffix gem: values must be a real | ||
| # hostname with a registrable name, not a bare suffix like com or co.uk. | ||
| # https://publicsuffix.org | ||
| def validate_public_suffix | ||
| return if domain.blank? | ||
|
|
||
| errors.add(:domain, :invalid) unless PublicSuffix.valid?(domain) | ||
| end | ||
|
|
||
| def build_normalised_domain_string(raw) | ||
| str = raw.to_s.strip.sub(/\A@+/, '') | ||
| str = uri_host_if_http_url(str) || str | ||
| str.downcase | ||
| end | ||
|
|
||
| def uri_host_if_http_url(str) | ||
| return unless str.match?(%r{\Ahttps?://}i) | ||
|
|
||
| uri = URI.parse(str) | ||
| uri.host if uri.is_a?(URI::HTTP) && uri.host.present? | ||
| rescue URI::InvalidURIError | ||
| nil | ||
| end | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class CreateSchoolEmailDomains < ActiveRecord::Migration[7.2] | ||
| def change | ||
| create_table :school_email_domains, id: :uuid do |t| | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| t.references :school, null: false, foreign_key: true, type: :uuid | ||
| t.string :domain, null: false | ||
|
|
||
| t.timestamps | ||
| end | ||
|
|
||
| add_index :school_email_domains, %i[school_id domain], unique: true | ||
| end | ||
| end | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe SchoolEmailDomain do | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| subject { described_class.new(school:, domain: 'example.edu') } | ||
|
|
||
| let(:school) { create(:school, creator_id: SecureRandom.uuid) } | ||
|
|
||
| it { is_expected.to belong_to(:school) } | ||
| it { is_expected.to validate_presence_of(:domain) } | ||
|
|
||
|
PetarSimonovic marked this conversation as resolved.
|
||
| describe 'domain normalisation' do | ||
| it 'takes the host from an http URL before other normalisation' do | ||
| record = described_class.new(school:, domain: 'http://mail.school.edu/path?query=1') | ||
| record.valid? | ||
|
|
||
| expect(record.domain).to eq('mail.school.edu') | ||
| end | ||
|
|
||
| it 'takes the host from an https URL before other normalisation' do | ||
| record = described_class.new(school:, domain: 'https://EXAMPLE.EDU/') | ||
| record.valid? | ||
|
|
||
| expect(record.domain).to eq('example.edu') | ||
| end | ||
|
|
||
| it 'downcases the domain' do | ||
| record = described_class.new(school:, domain: 'EXAMPLE.EDU') | ||
| record.valid? | ||
|
|
||
| expect(record.domain).to eq('example.edu') | ||
| end | ||
|
|
||
| it 'removes a leading @' do | ||
| record = described_class.new(school:, domain: '@example.edu') | ||
| record.valid? | ||
|
|
||
| expect(record.domain).to eq('example.edu') | ||
| end | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| end | ||
|
|
||
| describe 'public suffix list validation' do | ||
| context 'when the hostname is only a suffix' do | ||
| it 'rejects com' do | ||
| record = described_class.new(school:, domain: 'com') | ||
| record.valid? | ||
|
|
||
| expect(record).not_to be_valid | ||
| expect(record.errors.of_kind?(:domain, :invalid)).to be(true) | ||
| end | ||
|
|
||
| it 'rejects edu' do | ||
| record = described_class.new(school:, domain: 'edu') | ||
| record.valid? | ||
|
|
||
| expect(record).not_to be_valid | ||
| expect(record.errors.of_kind?(:domain, :invalid)).to be(true) | ||
| end | ||
|
|
||
| it 'rejects co.uk' do | ||
| record = described_class.new(school:, domain: 'co.uk') | ||
| record.valid? | ||
|
|
||
| expect(record).not_to be_valid | ||
| expect(record.errors.of_kind?(:domain, :invalid)).to be(true) | ||
| end | ||
| end | ||
|
|
||
| context 'when there is at least one registrable label before the public suffix' do | ||
| it 'accepts a typical school-style .edu domain' do | ||
| record = described_class.new(school:, domain: 'example.edu') | ||
| expect(record).to be_valid | ||
| end | ||
|
|
||
| it 'accepts a subdomain' do | ||
| record = described_class.new(school:, domain: 'mail.example.edu') | ||
| expect(record).to be_valid | ||
| end | ||
|
|
||
| it 'accepts a hostname under a multi-part public suffix' do | ||
| record = described_class.new(school:, domain: 'school.example.co.uk') | ||
| expect(record).to be_valid | ||
| end | ||
|
|
||
| it 'accepts district-style hosts' do | ||
| record = described_class.new(school:, domain: 'school.k12.tx.us') | ||
| expect(record).to be_valid | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe 'domain uniqueness' do | ||
| it 'rejects a duplicate domain for the same school after normalisation' do | ||
| described_class.create!(school:, domain: 'example.edu') | ||
| duplicate = described_class.new(school:, domain: 'EXAMPLE.EDU') | ||
| duplicate.valid? | ||
|
|
||
| expect(duplicate.errors.of_kind?(:domain, :taken)).to be(true) | ||
| end | ||
|
|
||
| it 'allows the same domain for a different school' do | ||
| described_class.create!(school:, domain: 'example.edu') | ||
| other_school = create(:school, creator_id: SecureRandom.uuid) | ||
| other = described_class.new(school: other_school, domain: 'example.edu') | ||
|
|
||
| expect(other).to be_valid | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
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 ifcandidate_domainis registered for this school.This would allow testing domains presented by registering users without exposing the internal implementation of
school_email_domains.