-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
10 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 e180140
Remove advisory_lock comment in SchoolEmailDomain::Create
PetarSimonovic 71e4b63
Add SchoolEmailDomains::Create spec to test pg_advisory_xact_lock
PetarSimonovic d4e13e1
Remove concurrent update spec for SchoolEmailDomain advisory lock
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
Some comments aren't visible on the classic Files Changed page.
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,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 | ||
| 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) | ||
| 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}") | ||
|
mwtrew marked this conversation as resolved.
|
||
| 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