-
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
base: main
Are you sure you want to change the base?
Changes from all commits
f5a8796
2cf9498
337551c
b51fcfb
27cc95f
dddfb10
528a359
e180140
71e4b63
d4e13e1
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,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 |
| 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 | ||
| 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}") | ||
|
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. Could we also check that the lock was released at the end?
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. 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 2. pg_try_advisory_xact_lock We could use But within a given session I don't think we can ever get a 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.
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. 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.
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. 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 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 Complicated structure Use a gem An alternative would be to use the 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.
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. Good points - I was hoping we could test it in context but the lack of a real Profile API complicates things a little too much. Let's perhaps skip the automated test then. |
||
| 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 | ||
| 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 |
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