-
Notifications
You must be signed in to change notification settings - Fork 5
868 add school email domain api #878
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
Open
PetarSimonovic
wants to merge
7
commits into
main
Choose a base branch
from
868-add-school-email-domain-api
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f5a8796
Allow teachers and owners to list SchoolEmailDomains
PetarSimonovic 2cf9498
Allow school owners and teachers to create school email domains
PetarSimonovic 337551c
Return error_codes in SchoolEmailDomain response
PetarSimonovic b51fcfb
Seed school with student_sso feature flag
PetarSimonovic 27cc95f
Initialise response[:school_email_domain] before assignment
PetarSimonovic dddfb10
Lock school while creating domains and syncing with Profile
PetarSimonovic 528a359
Acquire advisory lock for SchoolEmailDomain creation
PetarSimonovic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| # 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 | ||
| response[:error] = record.errors.full_messages.join(', ') | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
PetarSimonovic marked this conversation as resolved.
Dismissed
|
||
| response[:error_code] = domain_error_code(record) | ||
|
PetarSimonovic marked this conversation as resolved.
Dismissed
|
||
| response | ||
| rescue ActiveRecord::RecordNotUnique | ||
| record = response[:school_email_domain] | ||
|
PetarSimonovic marked this conversation as resolved.
Dismissed
|
||
| record.errors.add(:domain, :taken) | ||
| response[:error] = record.errors.full_messages.join(', ') | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
PetarSimonovic marked this conversation as resolved.
Dismissed
|
||
| response[:error_code] = 'taken' | ||
|
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 | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
github-code-quality[bot] marked this conversation as resolved.
Fixed
|
||
| response[:error_code] = 'profile_sync_failed' | ||
|
github-code-quality[bot] marked this conversation as resolved.
Fixed
|
||
| response | ||
| end | ||
|
|
||
| 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. | ||
|
Comment on lines
+40
to
+45
Contributor
Author
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. Happy to remove/shorten this comment if the point about identical key values is overkill |
||
| 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:) | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
| end | ||
|
PetarSimonovic marked this conversation as resolved.
|
||
|
|
||
| def domain_error_code(record) | ||
| record.errors.details[:domain].first.fetch(:error).to_s | ||
| end | ||
| end | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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}") | ||
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
I've made this changed and initialised with
nilsinceresponse[:school_email_domain]should be a single record