diff --git a/AGENTS.md b/AGENTS.md index 9358fcb0b..77e8c594f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -40,7 +40,7 @@ docker compose up ## Salesforce / Heroku Connect - Sync writes to the `salesforce_connect` DB (not a Salesforce API). Pattern from editor-api PR #677. - Feature flag: `SALESFORCE_ENABLED=true`. -- After deploy, backfill: `rails salesforce_sync:school`, `salesforce_sync:role`, `salesforce_sync:contact`, `salesforce_sync:school_class`, `salesforce_sync:class_teacher`, `salesforce_sync:lesson`. +- After deploy, backfill: `rails salesforce_sync:school`, `salesforce_sync:role`, `salesforce_sync:school_class`, `salesforce_sync:class_teacher`, `salesforce_sync:lesson`. - **Parent-sync race guard (required for any job using `__r__` external-ID lookups).** Heroku Connect rejects an INSERT permanently with `Foreign key external ID … not found` if the parent record isn't yet in Salesforce — the mirror row stays `FAILED` forever (no auto-retry). Call `ensure_parent_synced!(model, external_id_field, external_id, label)` on `Salesforce::SalesforceSyncJob` (the base class) before saving a child record; it checks the parent has a non-nil `sfid` in its Heroku Connect mirror and raises `SalesforceRecordNotFound` if not. The base job declares `retry_on SalesforceRecordNotFound, wait: :polynomially_longer, attempts: 10` so the job self-heals once parents land. See `Salesforce::RoleSyncJob` and `Salesforce::ClassTeacherSyncJob` for call-site examples. ## Where to Look First diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index 0a144fa11..674addc11 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -26,7 +26,7 @@ def show end def create - result = School::Create.call(school_params: create_params, creator_id: current_user.id, token: current_user.token) + result = School::Create.call(school_params: create_params, owner_id: current_user.id, token: current_user.token) if result.success? @school = result[:school] diff --git a/app/dashboards/school_dashboard.rb b/app/dashboards/school_dashboard.rb index 5629da864..eda78ff83 100644 --- a/app/dashboards/school_dashboard.rb +++ b/app/dashboards/school_dashboard.rb @@ -12,7 +12,6 @@ class SchoolDashboard < Administrate::BaseDashboard ATTRIBUTE_TYPES = { id: Field::String, code: Field::String, - creator: Field::BelongsTo.with_options(class_name: 'User'), postal_code: Field::String, creator_role: Field::String, creator_department: Field::String, @@ -64,7 +63,6 @@ class SchoolDashboard < Administrate::BaseDashboard name code user_origin - creator roles student_count creator_role diff --git a/app/jobs/salesforce/contact_sync_job.rb b/app/jobs/salesforce/contact_sync_job.rb deleted file mode 100644 index fad46a615..000000000 --- a/app/jobs/salesforce/contact_sync_job.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -module Salesforce - class ContactSyncJob < SalesforceSyncJob - MODEL_CLASS = Salesforce::Contact - - def perform(school_id:) - school = ::School.find(school_id) - - sf_contact = Salesforce::Contact.find_by(pi_accounts_unique_id__c: school.creator_id) - raise SalesforceRecordNotFound, "Contact not found for creator_id: #{school.creator_id}" unless sf_contact - - sf_contact.editor_consent_to_ux_contact__c = school.creator_agree_to_ux_contact - sf_contact.save! - end - - private - - def concurrency_key_id = arguments.first.with_indifferent_access[:school_id] - end -end diff --git a/app/jobs/salesforce/role_sync_job.rb b/app/jobs/salesforce/role_sync_job.rb index a6dc98fb6..c53e330ee 100644 --- a/app/jobs/salesforce/role_sync_job.rb +++ b/app/jobs/salesforce/role_sync_job.rb @@ -32,10 +32,18 @@ def perform(role_id:) sf_role.editor_type__c = role.school&.user_origin || ::School.new.user_origin sf_role.save! + + sync_ux_contact_consent(role:) if role.owner? end private + def sync_ux_contact_consent(role:) + sf_contact = Salesforce::Contact.find_by!(pi_accounts_unique_id__c: role.user_id) + sf_contact.editor_consent_to_ux_contact__c = role.school.creator_agree_to_ux_contact + sf_contact.save! + end + def sf_role_attributes(role:) mapped_attributes(role:).to_h do |sf_field, value| value = truncate_value(sf_field:, value:) if value.is_a?(String) diff --git a/app/jobs/school_import_job.rb b/app/jobs/school_import_job.rb index 76fe9866e..d372aa01d 100644 --- a/app/jobs/school_import_job.rb +++ b/app/jobs/school_import_job.rb @@ -71,7 +71,7 @@ def import_school(school_data) School.transaction do result = School::Create.call( school_params: school_params, - creator_id: proposed_owner[:id], + owner_id: proposed_owner[:id], token: @token ) diff --git a/app/models/ability.rb b/app/models/ability.rb index 15f03622f..f8546627f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -47,9 +47,6 @@ def define_authenticated_non_student_abilities(user) # Any authenticated user can create a school. They agree to become the school-owner. can :create, School - # An unverified school owner can read their own school. - can :read, School, creator_id: user.id, verified_at: nil - # Any authenticated user can create a lesson, to support a RPF library of public lessons. can :create, Lesson, school_id: nil, school_class_id: nil diff --git a/app/models/school.rb b/app/models/school.rb index 7591a18f8..c58aed310 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class School < ApplicationRecord + self.ignored_columns += [:creator_id] + has_many :classes, class_name: :SchoolClass, inverse_of: :school, dependent: :destroy has_many :lessons, dependent: :nullify has_many :projects, dependent: :nullify @@ -34,11 +36,6 @@ class School < ApplicationRecord uniqueness: { conditions: -> { active }, case_sensitive: false, allow_blank: true, message: I18n.t('validations.school.school_roll_number_exists') }, format: { with: /\A[0-9]+[A-Z]+\z/, allow_nil: true, message: I18n.t('validations.school.school_roll_number') }, presence: true, on: :create, if: :ireland?, unless: :hidden? - validates :creator_id, - presence: true, - uniqueness: { - conditions: -> { active } - }, unless: :hidden? validates :creator_agree_authority, presence: true, acceptance: true validates :creator_agree_terms_and_conditions, presence: true, acceptance: true validates :creator_agree_responsible_safeguarding, presence: true, acceptance: true @@ -63,16 +60,12 @@ class School < ApplicationRecord after_commit -> { do_salesforce_sync(is_create: false) }, on: :update, if: -> { FeatureFlags.salesforce_sync? } def self.find_for_user!(user) - school = Role.find_by(user_id: user.id)&.school || active.find_by(creator_id: user.id) + school = Role.find_by(user_id: user.id)&.school raise ActiveRecord::RecordNotFound unless school school end - def creator - User.from_userinfo(ids: creator_id).first - end - def verified? verified_at.present? end @@ -221,6 +214,5 @@ def format_uk_postal_code def do_salesforce_sync(is_create:) Salesforce::SchoolSyncJob.perform_later(school_id: id, is_create:) - Salesforce::ContactSyncJob.perform_later(school_id: id) end end diff --git a/app/services/school_onboarding_service.rb b/app/services/school_onboarding_service.rb index c2590b702..1c5e4c86b 100644 --- a/app/services/school_onboarding_service.rb +++ b/app/services/school_onboarding_service.rb @@ -7,10 +7,10 @@ def initialize(school) @school = school end - def onboard(token:) + def onboard(owner_id:, token:) School.transaction do - Role.owner.create!(user_id: school.creator_id, school:) - Role.teacher.create!(user_id: school.creator_id, school:) + Role.owner.create!(user_id: owner_id, school:) + Role.teacher.create!(user_id: owner_id, school:) ProfileApiClient.create_school(token:, id: school.id, code: school.code) end diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index df1ff5ad6..765025891 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -3,32 +3,51 @@ class School class Create class << self - def call(school_params:, creator_id:, token:) + def call(school_params:, owner_id:, token:) response = OperationResponse.new - response[:school] = build_school(school_params.merge!(creator_id:)) - School.transaction do - response[:school].save! + begin + response[:school] = nil + response[:school] = build_school(school_params) - SchoolOnboardingService.new(response[:school]).onboard(token:) - end + # Savepoint so failures roll back the school even when called inside an outer transaction (e.g. SchoolImportJob) + School.transaction(requires_new: true) do + acquire_advisory_lock_for_owner(owner_id) + response[:school].save! - response - rescue ProfileApiClient::UnauthorizedError => e - # Do not log noise to sentry. - # TODO: consider returning a separate error here to distinguish from other errors and return 401 from the API, not 422 - Rails.logger.warn { "Failed to onboard school #{response[:school].id}: user is unauthorized" } - failure(response, e) - rescue StandardError => e - Sentry.capture_exception(e) - failure(response, e) + SchoolOnboardingService.new(response[:school]).onboard(owner_id:, token:) + end + + response + rescue ProfileApiClient::UnauthorizedError => e + # Do not log noise to sentry. + # TODO: consider returning a separate error here to distinguish from other errors and return 401 from the API, not 422 + Rails.logger.warn { "Failed to onboard school #{response[:school].id}: user is unauthorized" } + failure(response, e) + rescue ActiveRecord::RecordInvalid => e + if e.record.is_a?(Role) + response[:school].errors.merge!(e.record.errors) + else + Sentry.capture_exception(e) + end + failure(response, e) + rescue StandardError => e + Sentry.capture_exception(e) + failure(response, e) + end end private + def acquire_advisory_lock_for_owner(owner_id) + lock_key = Zlib.crc32("#{owner_id}:#{name}") + School.connection.execute("SELECT pg_advisory_xact_lock(#{lock_key})") + end + def failure(response, error) - response[:error] = response[:school].errors.presence || [error.message] - response[:error_types] = response[:school].errors.details + school = response[:school] + response[:error] = school&.errors.presence || [error.message] + response[:error_types] = school&.errors&.details || {} response end diff --git a/lib/tasks/for_education.rake b/lib/tasks/for_education.rake index d222abe32..5c1edb138 100644 --- a/lib/tasks/for_education.rake +++ b/lib/tasks/for_education.rake @@ -17,7 +17,7 @@ namespace :for_education do task destroy_seed_data: :environment do ActiveRecord::Base.transaction do Rails.logger.info 'Destroying existing seeds...' - creator_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) + owner_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) teacher_id = ENV.fetch('SEEDING_TEACHER_ID', TEST_USERS[:john_doe]) # Hard coded as the student's school needs to match @@ -25,7 +25,7 @@ namespace :for_education do school_id = TEST_SCHOOL # Remove the roles first - Role.where(user_id: [creator_id, teacher_id] + student_ids).destroy_all + Role.where(user_id: [owner_id, teacher_id] + student_ids).destroy_all # Destroy the project and then the lesson itself (The lesson's `before_destroy` prevents us using destroy) lesson_ids = Lesson.where(school_id:).pluck(:id) @@ -53,8 +53,8 @@ namespace :for_education do ActiveRecord::Base.transaction do Rails.logger.info 'Attempting to seed data...' - creator_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) - create_school(creator_id, TEST_SCHOOL) + owner_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) + create_school(owner_id, TEST_SCHOOL) Rails.logger.info 'Done...' end @@ -69,9 +69,9 @@ namespace :for_education do ActiveRecord::Base.transaction do Rails.logger.info 'Attempting to seed data...' - creator_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) + owner_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) - school = create_school(creator_id, TEST_SCHOOL) + school = create_school(owner_id, TEST_SCHOOL) verify_school(school) Rails.logger.info 'Done...' end @@ -86,18 +86,18 @@ namespace :for_education do ActiveRecord::Base.transaction do Rails.logger.info 'Attempting to seed data...' - creator_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) + owner_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe]) teacher_id = ENV.fetch('SEEDING_TEACHER_ID', TEST_USERS[:john_doe]) - school = create_school(creator_id, TEST_SCHOOL) + school = create_school(owner_id, TEST_SCHOOL) verify_school(school) school.update!(scratch_enabled: true) assign_a_teacher(teacher_id, school) - school_class = create_school_class(creator_id, school) + school_class = create_school_class(owner_id, school) assign_students(school_class, school) - create_lessons(creator_id, school, school_class) + create_lessons(owner_id, school, school_class) Rails.logger.info 'Done...' end end diff --git a/lib/tasks/salesforce_sync.rake b/lib/tasks/salesforce_sync.rake index 8927c96d9..8f6e02fd4 100644 --- a/lib/tasks/salesforce_sync.rake +++ b/lib/tasks/salesforce_sync.rake @@ -15,13 +15,6 @@ namespace :salesforce_sync do end end - desc 'Sync creator_agree_to_ux_contact for all Schools to Salesforce Contact' - task contact: :environment do - School.find_each do |school| - Salesforce::ContactSyncJob.perform_later(school_id: school.id) - end - end - desc 'Sync all SchoolClasses to Salesforce' task school_class: :environment do SchoolClass.find_each do |school_class| diff --git a/lib/tasks/school_management.rake b/lib/tasks/school_management.rake index 67ac71707..f298328a3 100644 --- a/lib/tasks/school_management.rake +++ b/lib/tasks/school_management.rake @@ -21,18 +21,13 @@ namespace :school_management do next end - if School.exists?(creator_id: new_owner[:id]) - Rails.logger.error("User #{new_owner[:id]} is already the creator of a school") - next - end - school = Role.find_by(roles: { user_id: old_owner[:id], role: 'owner' }).school school.transaction do remove_old_owner(school, old_owner[:id], args[:keep_old_owner_as_teacher]) assign_roles_to_new_owner(school, new_owner[:id]) - school.update!(creator_id: new_owner[:id], creator_agree_to_ux_contact: false) + school.update!(creator_agree_to_ux_contact: false) end Rails.logger.info "Ownership transfered to #{new_owner[:email]} successfully." diff --git a/lib/tasks/seeds_helper.rb b/lib/tasks/seeds_helper.rb index 6353cbbb2..83f66f7fd 100644 --- a/lib/tasks/seeds_helper.rb +++ b/lib/tasks/seeds_helper.rb @@ -20,8 +20,8 @@ module SeedsHelper TEST_SCHOOL = 'e52de409-9210-4e94-b08c-dd11439e07d9' # e52de409-9210-4e94-b08c-dd11439e07d9 SCHOOL_CODE = '12-34-56' - def create_school(creator_id, school_id = nil) - School.find_or_create_by!(creator_id:, id: school_id) do |school| + def create_school(owner_id, school_id = nil) + seeded_school = School.find_or_create_by!(id: school_id) do |school| Rails.logger.info 'Seeding a school...' country_code = Faker::Address.country_code school.name = Faker::Educator.secondary_school @@ -31,7 +31,6 @@ def create_school(creator_id, school_id = nil) school.municipality = Faker::Address.city school.postal_code = Faker::Address.postcode school.country_code = country_code - school.creator_id = creator_id school.creator_agree_authority = true school.creator_agree_terms_and_conditions = true school.creator_agree_to_ux_contact = true @@ -43,6 +42,12 @@ def create_school(creator_id, school_id = nil) end school.school_roll_number = "#{rand(10_000..99_999)}#{('A'..'Z').to_a.sample}" if country_code == 'IE' end + + # Mirror SchoolOnboardingService: the owner gets their roles when the school is created + Role.owner.find_or_create_by!(user_id: owner_id, school: seeded_school) + Role.teacher.find_or_create_by!(user_id: owner_id, school: seeded_school) + + seeded_school end def verify_school(school) @@ -53,11 +58,7 @@ def verify_school(school) Rails.logger.info 'Verifying the school...' - School.transaction do - school.verify! - Role.owner.create!(user_id: school.creator_id, school:) - Role.teacher.create!(user_id: school.creator_id, school:) - end + school.verify! # rubocop:disable-next Rails/SkipsModelValidations school.update_column(:code, SCHOOL_CODE) # The code needs to match the one in the profile diff --git a/lib/tasks/test_seeds.rake b/lib/tasks/test_seeds.rake index 230be4925..642969516 100644 --- a/lib/tasks/test_seeds.rake +++ b/lib/tasks/test_seeds.rake @@ -20,7 +20,7 @@ namespace :test_seeds do student_ids = [TEST_USERS[:jane_smith], TEST_USERS[:john_smith], TEST_USERS[:emily_ssouser]] school_id = TEST_SCHOOL teacher_signup_school_id = - School.find_by(creator_id: teacher_signup_id)&.id + Role.owner.find_by(user_id: teacher_signup_id)&.school_id # Remove the roles first Role.where(user_id: teacher_signup_id).destroy_all diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index 54ccb7768..411818dda 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -21,37 +21,44 @@ end let(:token) { UserProfileMock::TOKEN } - let(:creator) { create(:user) } - let(:creator_id) { creator.id } + let(:owner) { create(:user) } + let(:owner_id) { owner.id } before do - authenticated_in_hydra_as(creator) + authenticated_in_hydra_as(owner) allow(ProfileApiClient).to receive(:create_school).and_return(true) stub_profile_api_create_safeguarding_flag end it 'returns a successful operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response.success?).to be(true) end it 'creates a school' do - expect { described_class.call(school_params:, creator_id:, token:) }.to change(School, :count).by(1) + expect { described_class.call(school_params:, owner_id:, token:) }.to change(School, :count).by(1) end it 'returns the school in the operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response[:school]).to be_a(School) end it 'assigns the name' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response[:school].name).to eq('Test School') end - it 'assigns the creator_id' do - response = described_class.call(school_params:, creator_id:, token:) - expect(response[:school].creator_id).to eq(creator_id) + it 'gives the owner the owner and teacher roles' do + response = described_class.call(school_params:, owner_id:, token:) + expect(response[:school].roles.map(&:role)).to contain_exactly('owner', 'teacher') + expect(response[:school].roles.pluck(:user_id).uniq).to eq([owner_id]) + end + + it 'acquires an advisory lock for the owner' do + allow(School.connection).to receive(:execute).and_call_original + described_class.call(school_params:, owner_id:, token:) + expect(School.connection).to have_received(:execute).with(/pg_advisory_xact_lock\(\d+\)/) end context 'when creation fails' do @@ -62,26 +69,26 @@ end it 'does not create a school' do - expect { described_class.call(school_params:, creator_id:, token:) }.not_to change(School, :count) + expect { described_class.call(school_params:, owner_id:, token:) }.not_to change(School, :count) end it 'returns a failed operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response.failure?).to be(true) end it 'returns the correct number of objects in the operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response[:error].count).to eq(11) end it 'returns the correct type of object in the operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response[:error].first).to be_a(ActiveModel::Error) end it 'sent the exception to Sentry' do - described_class.call(school_params:, creator_id:, token:) + described_class.call(school_params:, owner_id:, token:) expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) end end @@ -94,8 +101,8 @@ end it 'calls the onboarding service' do - described_class.call(school_params:, creator_id:, token:) - expect(onboarding_service).to have_received(:onboard).with(token:) + described_class.call(school_params:, owner_id:, token:) + expect(onboarding_service).to have_received(:onboard).with(owner_id:, token:) end end @@ -108,21 +115,21 @@ end it 'does not create a school' do - expect { described_class.call(school_params:, creator_id:, token:) }.not_to change(School, :count) + expect { described_class.call(school_params:, owner_id:, token:) }.not_to change(School, :count) end it 'returns a failed operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response.failure?).to be(true) end it 'sends the underlying error to Sentry rather than a generic error' do - described_class.call(school_params:, creator_id:, token:) + described_class.call(school_params:, owner_id:, token:) expect(Sentry).to have_received(:capture_exception).with(error) end it 'returns the underlying error message in the operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response[:error]).to eq([error.message]) end end @@ -134,17 +141,51 @@ end it 'does not create a school' do - expect { described_class.call(school_params:, creator_id:, token:) }.not_to change(School, :count) + expect { described_class.call(school_params:, owner_id:, token:) }.not_to change(School, :count) end it 'returns a failed operation response' do - response = described_class.call(school_params:, creator_id:, token:) + response = described_class.call(school_params:, owner_id:, token:) expect(response.failure?).to be(true) end it 'does not capture the error in Sentry' do - described_class.call(school_params:, creator_id:, token:) + described_class.call(school_params:, owner_id:, token:) expect(Sentry).not_to have_received(:capture_exception) end end + + context 'when the school owner already has a role in another school' do + let(:other_school) { create(:school) } + + before do + create(:owner_role, school: other_school, user_id: owner_id) + allow(Sentry).to receive(:capture_exception) + end + + it 'does not create a school' do + expect { described_class.call(school_params:, owner_id:, token:) }.not_to change(School, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(school_params:, owner_id:, token:) + expect(response.failure?).to be(true) + end + + it 'returns the role error on the school' do + response = described_class.call(school_params:, owner_id:, token:) + expect(response[:error][:base]).to eq(['Cannot create role as this user already has a role in a different school']) + end + + it 'does not capture the error in Sentry' do + described_class.call(school_params:, owner_id:, token:) + expect(Sentry).not_to have_received(:capture_exception) + end + + it 'does not create a school when called inside an outer transaction' do + expect do + School.transaction { described_class.call(school_params:, owner_id:, token:) } + end.not_to change(School, :count) + end + end end diff --git a/spec/factories/school.rb b/spec/factories/school.rb index 9378df265..9bef22879 100644 --- a/spec/factories/school.rb +++ b/spec/factories/school.rb @@ -11,7 +11,6 @@ country_code { 'GB' } sequence(:reference) { |n| format('%06d', 100_000 + n) } school_roll_number { nil } - creator_id { SecureRandom.uuid } creator_agree_authority { true } creator_agree_terms_and_conditions { true } creator_agree_responsible_safeguarding { true } diff --git a/spec/features/admin/schools_spec.rb b/spec/features/admin/schools_spec.rb index 2df754e35..7fac15598 100644 --- a/spec/features/admin/schools_spec.rb +++ b/spec/features/admin/schools_spec.rb @@ -17,14 +17,12 @@ end describe 'GET #show' do - let(:creator) { create(:user) } let(:verified_at) { nil } let(:rejected_at) { nil } let(:code) { nil } - let(:school) { create(:school, creator_id: creator.id, verified_at:, rejected_at:, code:) } + let(:school) { create(:school, verified_at:, rejected_at:, code:) } before do - stub_user_info_api_for(creator) get admin_school_path(school) end @@ -82,8 +80,7 @@ end describe 'GET #show with roles' do - let(:creator) { create(:user) } - let(:school) { create(:school, creator_id: creator.id) } + let(:school) { create(:school) } let(:owner) { create(:user, name: 'Olivia Owner', email: 'owner@example.com') } let(:teacher) { create(:user, name: 'Tariq Teacher', email: 'teacher@example.com') } let(:student) { create(:user, name: 'Sam Student', email: 'student@example.com') } @@ -99,7 +96,6 @@ create(:teacher_role, school:, user_id: teacher.id) create(:student_role, school:, user_id: student.id) - allow(User).to receive(:from_userinfo).with(ids: creator.id).and_return([creator]) allow(User).to receive(:from_userinfo).with(ids: contain_exactly(owner.id, teacher.id)).and_return(role_users) get admin_school_path(school) @@ -120,14 +116,12 @@ end describe 'POST #verify' do - let(:creator) { create(:user) } let(:verified_at) { nil } - let(:school) { create(:school, creator_id: creator.id, verified_at:) } + let(:school) { create(:school, verified_at:) } let(:verification_result) { nil } let(:verification_service) { instance_double(SchoolVerificationService, verify: verification_result) } before do - stub_user_info_api_for(creator) allow(SchoolVerificationService).to receive(:new).with(school).and_return(verification_service) post verify_admin_school_path(school) @@ -163,13 +157,11 @@ end describe 'PUT #reopen' do - let(:creator) { create(:user) } - let(:school) { create(:verified_school, creator_id: creator.id) } + let(:school) { create(:verified_school) } let(:reopen_result) { nil } let(:verification_service) { instance_double(SchoolVerificationService, reopen: reopen_result) } before do - stub_user_info_api_for(creator) allow(SchoolVerificationService).to receive(:new).with(school).and_return(verification_service) patch reopen_admin_school_path(school) @@ -206,12 +198,7 @@ end describe 'PUT #archive' do - let(:creator) { create(:user) } - let(:school) { create(:school, creator_id: creator.id) } - - before do - stub_user_info_api_for(creator) - end + let(:school) { create(:school) } it 'marks the school as archived' do patch archive_admin_school_path(school) diff --git a/spec/jobs/salesforce/contact_sync_job_spec.rb b/spec/jobs/salesforce/contact_sync_job_spec.rb deleted file mode 100644 index 2345f2a59..000000000 --- a/spec/jobs/salesforce/contact_sync_job_spec.rb +++ /dev/null @@ -1,68 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe Salesforce::ContactSyncJob, :requires_salesforce_db do - subject(:perform_job) { described_class.perform_now(school_id: school.id) } - - let(:school) { create(:school, creator_agree_to_ux_contact: true) } - let!(:sf_contact) { create(:salesforce_contact, pi_accounts_unique_id__c: school.creator_id) } - - around do |example| - ClimateControl.modify(SALESFORCE_ENABLED: 'true') { example.run } - end - - it 'sets editor_consent_to_ux_contact__c from school.creator_agree_to_ux_contact' do - perform_job - expect(sf_contact.reload.editor_consent_to_ux_contact__c).to be(true) - end - - it 'saves the contact' do - expect { perform_job }.not_to raise_error - end - - context 'when the Contact is not found in Salesforce' do - before { sf_contact.destroy } - - it 'retries the job' do - expect { perform_job }.to have_enqueued_job(described_class).with(school_id: school.id) - end - end - - context 'when the Salesforce contact fails to save' do - let(:sf_contact_double) { instance_double(Salesforce::Contact) } - - before do - allow(Salesforce::Contact).to receive(:find_by) - .with(pi_accounts_unique_id__c: school.creator_id) - .and_return(sf_contact_double) - allow(sf_contact_double).to receive(:editor_consent_to_ux_contact__c=) - allow(sf_contact_double).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) - end - - it 'raises an error' do - expect { perform_job }.to raise_error(ActiveRecord::RecordInvalid) - end - end - - context 'when SALESFORCE_ENABLED is false' do - around do |example| - ClimateControl.modify(SALESFORCE_ENABLED: 'false') do - example.run - end - end - - it 'discards the job without syncing' do - sf_contact.update!(editor_consent_to_ux_contact__c: false) - perform_job - expect(sf_contact.reload.editor_consent_to_ux_contact__c).to be(false) - end - end - - describe '#concurrency_key_id' do - it 'returns the school_id' do - job = described_class.new(school_id: school.id) - expect(job.send(:concurrency_key_id)).to eq(school.id) - end - end -end diff --git a/spec/jobs/salesforce/role_sync_job_spec.rb b/spec/jobs/salesforce/role_sync_job_spec.rb index db0cf834d..e3084785a 100644 --- a/spec/jobs/salesforce/role_sync_job_spec.rb +++ b/spec/jobs/salesforce/role_sync_job_spec.rb @@ -33,6 +33,22 @@ expect(sf_role.editor_type__c).to eq(role.school.user_origin) end + it 'sets editor_consent_to_ux_contact__c on the owner contact from school.creator_agree_to_ux_contact' do + role.school.update!(creator_agree_to_ux_contact: true) + perform_job + expect(sf_contact.reload.editor_consent_to_ux_contact__c).to be(true) + end + + context 'when the role is a teacher role' do + let(:role) { create(:teacher_role) } + + it 'does not update editor_consent_to_ux_contact__c on the contact' do + role.school.update!(creator_agree_to_ux_contact: true) + perform_job + expect(sf_contact.reload.editor_consent_to_ux_contact__c).to be_nil + end + end + it 'syncs archived roles' do role.update!(archived_at: Time.zone.now) perform_job diff --git a/spec/lib/tasks/for_education_spec.rb b/spec/lib/tasks/for_education_spec.rb index 9189baab1..3522243ec 100644 --- a/spec/lib/tasks/for_education_spec.rb +++ b/spec/lib/tasks/for_education_spec.rb @@ -4,7 +4,7 @@ require 'rake' RSpec.describe 'for_education', type: :task do - let(:creator_id) { '583ba872-b16e-46e1-9f7d-df89d267550d' } # jane.doe@example.com + let(:owner_id) { '583ba872-b16e-46e1-9f7d-df89d267550d' } # jane.doe@example.com let(:teacher_id) { 'bbb9b8fd-f357-4238-983d-6f87b99bdbb2' } # john.doe@example.com let(:student_1) { 'e52de409-9210-4e94-b08c-dd11439e07d9' } # student let(:student_2) { '0d488bec-b10d-46d3-b6f3-4cddf5d90c71' } # student @@ -12,24 +12,24 @@ describe ':destroy_seed_data' do let(:task) { Rake::Task['for_education:destroy_seed_data'] } - let(:school) { create(:school, creator_id:, id: school_id) } + let(:school) { create(:school, id: school_id) } before do - create(:role, user_id: creator_id, school:) + create(:role, user_id: owner_id, school:) create(:student_role, user_id: student_1, school:) - create(:teacher_role, user_id: creator_id, school:) - school_class = create(:school_class, school_id: school.id, teacher_ids: [creator_id]) + create(:teacher_role, user_id: owner_id, school:) + school_class = create(:school_class, school_id: school.id, teacher_ids: [owner_id]) create(:class_student, student_id: student_1, school_class_id: school_class.id) - create(:lesson, school_id: school.id, user_id: creator_id) + create(:lesson, school_id: school.id, user_id: owner_id) end it 'destroys all seed data' do task.invoke - expect(Role.where(user_id: [creator_id, teacher_id, student_1, student_2])).not_to exist - expect(School.where(creator_id:)).not_to exist + expect(Role.where(user_id: [owner_id, teacher_id, student_1, student_2])).not_to exist + expect(School.where(id: school_id)).not_to exist expect(ClassStudent.where(student_id: student_1)).not_to exist expect(SchoolClass.where(school_id: school.id)).not_to exist - expect(ClassTeacher.where(teacher_id: creator_id)).not_to exist + expect(ClassTeacher.where(teacher_id: owner_id)).not_to exist expect(Lesson.where(school_id: school.id)).not_to exist expect(Project.where(school_id: school.id)).not_to exist end @@ -40,7 +40,12 @@ it 'creates an unverified school' do task.invoke - expect(School.find_by(creator_id:).verified_at).to be_nil + expect(School.find_by(id: school_id).verified_at).to be_nil + end + + it 'gives the owner the owner and teacher roles' do + task.invoke + expect(Role.where(user_id: owner_id, school_id:).map(&:role)).to contain_exactly('owner', 'teacher') end end @@ -49,13 +54,13 @@ it 'creates a verified school' do task.invoke - expect(School.find_by(creator_id:).verified_at).to be_truthy + expect(School.find_by(id: school_id).verified_at).to be_truthy end end describe ':seed_a_school_with_lessons_and_students' do let(:task) { Rake::Task['for_education:seed_a_school_with_lessons_and_students'] } - let(:school) { School.find_by(creator_id:) } + let(:school) { School.find_by(id: school_id) } before do Rake::Task['for_education:destroy_seed_data'].invoke @@ -110,8 +115,8 @@ expect(Role.teacher.where(user_id: teacher_id, school_id: school.id)).to exist end - it 'creates a class teacher association for the creator' do - expect(ClassTeacher.where(teacher_id: creator_id).length).to eq(1) + it 'creates a class teacher association for the owner' do + expect(ClassTeacher.where(teacher_id: owner_id).length).to eq(1) end it 'assigns students' do diff --git a/spec/lib/tasks/remove_teacher_spec.rb b/spec/lib/tasks/remove_teacher_spec.rb index 8470a9b36..7f7518b4f 100644 --- a/spec/lib/tasks/remove_teacher_spec.rb +++ b/spec/lib/tasks/remove_teacher_spec.rb @@ -8,7 +8,7 @@ let(:task) { Rake::Task['remove_teacher:run'] } let(:owner_id) { SecureRandom.uuid } let(:student_id) { SecureRandom.uuid } - let(:school) { create(:school, creator_id: owner_id) } + let(:school) { create(:school) } let(:teacher_id) { SecureRandom.uuid } before do diff --git a/spec/lib/tasks/school_management_spec.rb b/spec/lib/tasks/school_management_spec.rb index 8dd669c71..08b5e9df1 100644 --- a/spec/lib/tasks/school_management_spec.rb +++ b/spec/lib/tasks/school_management_spec.rb @@ -8,7 +8,7 @@ let(:task) { Rake::Task['school_management:transfer_ownership'] } let(:old_user_id) { SecureRandom.uuid } let(:new_user_id) { SecureRandom.uuid } - let(:school) { create(:school, creator_id: old_user_id) } + let(:school) { create(:school) } before do stub_user_info_api_find_by_email( @@ -32,7 +32,7 @@ task.invoke('old_owner@example.com', 'not_real_owner@example.com') - expect(school.creator_id).to eq(old_user_id) + expect(school.roles.owner.pluck(:user_id)).to include(old_user_id) end it "exits early if old owner doesn't exist" do @@ -40,9 +40,9 @@ .with('not_real_owner@example.com') .and_return(nil) - task.invoke('old_owner@example.com', 'new_owner@example.com') + task.invoke('not_real_owner@example.com', 'new_owner@example.com') - expect(school.creator_id).to eq(old_user_id) + expect(school.roles.owner.pluck(:user_id)).to include(old_user_id) end it 'exits early if new owner is already owner of a school' do @@ -50,15 +50,7 @@ task.invoke('old_owner@example.com', 'new_owner@example.com') - expect(school.creator_id).to eq(old_user_id) - end - - it 'exits early if new owner is already creator of a school' do - create(:school, creator_id: new_user_id) - - task.invoke('old_owner@example.com', 'new_owner@example.com') - - expect(school.creator_id).to eq(old_user_id) + expect(school.roles.owner.pluck(:user_id)).to include(old_user_id) end it 'creates owner and teacher roles for the new owner' do @@ -99,20 +91,5 @@ teacher_user_ids = teachers.map(&:user_id) expect(teacher_user_ids).not_to include(old_user_id) end - - it 'switches creator to the new owner' do - task.invoke('old_owner@example.com', 'new_owner@example.com') - school.reload - expect(school.creator_id).to eq(new_user_id) - end - - it 'sets the school UX contact flag to false' do - school.update!(creator_agree_to_ux_contact: true) - - task.invoke('old_owner@example.com', 'new_owner@example.com') - school.reload - - expect(school.creator_agree_to_ux_contact).to be(false) - end end end diff --git a/spec/lib/test_seeds_spec.rb b/spec/lib/test_seeds_spec.rb index e5228a642..773fdc054 100644 --- a/spec/lib/test_seeds_spec.rb +++ b/spec/lib/test_seeds_spec.rb @@ -12,7 +12,7 @@ describe ':destroy' do let(:task) { Rake::Task['test_seeds:destroy'] } - let(:school) { create(:school, creator_id:, id: school_id) } + let(:school) { create(:school, id: school_id) } let(:scratch_project_id) { ScratchAsset.first.project_id } before do @@ -31,7 +31,7 @@ task.invoke expect(Role.where(user_id: [creator_id, teacher_id, student_1, student_2])).not_to exist - expect(School.where(creator_id:)).not_to exist + expect(School.where(id: school_id)).not_to exist expect(ClassStudent.where(student_id: student_1)).not_to exist expect(SchoolClass.where(school_id: school.id)).not_to exist expect(Lesson.where(school_id: school.id)).not_to exist @@ -93,7 +93,7 @@ end it 'creates a verified school' do - expect(School.find_by(creator_id:).verified_at).to be_truthy + expect(School.find(school_id).verified_at).to be_truthy end it 'creates a public Scratch preview project' do @@ -165,7 +165,7 @@ end it 'creates lessons with projects, one per language, for each class' do - school = School.find_by(creator_id:) + school = School.find(school_id) expect(SchoolClass.where(school_id: school.id)).to exist lesson = Lesson.where(school_id: school.id) expect(lesson.length).to eq(6) @@ -181,12 +181,11 @@ end it 'assigns a teacher' do - school = School.find_by(creator_id:) + school = School.find(school_id) expect(Role.teacher.where(user_id: teacher_id, school_id: school.id)).to exist end it 'creates class with lessons for the owner' do - school_id = School.find_by(creator_id:).id school_class = SchoolClass.joins(:teachers).find_by(school_id:, teachers: { teacher_id: creator_id }) expect(school_class).not_to be_nil @@ -198,7 +197,6 @@ end it 'creates class with lessons for the teacher' do - school_id = School.find_by(creator_id:).id school_class = SchoolClass.joins(:teachers).find_by(school_id:, teachers: { teacher_id: }) expect(school_class).not_to be_nil expect(Lesson.where(school_id:, school_class_id: school_class.id).length).to eq(3) @@ -209,7 +207,7 @@ end it 'is idempotent' do - school = School.find_by!(creator_id:) + school = School.find(school_id) owner_class = SchoolClass.joins(:teachers).find_by!(school_id: school.id, teachers: { teacher_id: creator_id }) teacher_class = SchoolClass.joins(:teachers).find_by!(school_id: school.id, teachers: { teacher_id: }) @@ -231,7 +229,7 @@ end it 'enables scratch for the school' do - school = School.find_by(creator_id:) + school = School.find(school_id) expect(school.scratch_enabled?).to be true end @@ -239,7 +237,7 @@ let(:seed_country_code) { 'US' } it 'creates a valid school and owner lessons' do - school = School.find_by(creator_id:) + school = School.find(school_id) school_class = SchoolClass.joins(:teachers).find_by(school_id: school.id, teachers: { teacher_id: creator_id }) expect(school).to be_valid diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 319c7c848..a46cbf32c 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -522,15 +522,6 @@ let(:school) { create(:school) } let(:user) { build(:user) } - context 'when user is not a school-owner but is the creator of the school' do - before do - user.id = user_id - school.update(creator_id: user_id, verified_at: nil) - end - - it { is_expected.to be_able_to(:read, school) } - end - context 'when user is a school owner' do before do create(:owner_role, user_id: user.id, school:) diff --git a/spec/models/school_email_domain_spec.rb b/spec/models/school_email_domain_spec.rb index d8695eb60..42b4c73cb 100644 --- a/spec/models/school_email_domain_spec.rb +++ b/spec/models/school_email_domain_spec.rb @@ -5,7 +5,7 @@ RSpec.describe SchoolEmailDomain do subject(:school_email_domain) { described_class.create!(school:, domain:) } - let(:school) { create(:school, creator_id: SecureRandom.uuid) } + let(:school) { create(:school) } let(:domain) { 'example.edu' } describe 'associations' do @@ -68,7 +68,7 @@ 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_school = create(:school) other_school_email_domain = described_class.new(school: other_school, domain: 'example.edu') expect(other_school_email_domain).to be_valid diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 409b8ff7d..9e8aed75b 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -5,9 +5,9 @@ RSpec.describe School do let(:student) { create(:student, school:) } let(:teacher) { create(:teacher, school:) } - let(:school) { create(:school, creator_id: SecureRandom.uuid) } - let!(:us_school) { create(:school, country_code: 'US', district_name: 'Some District', district_nces_id: '0100000', creator_id: SecureRandom.uuid) } - let!(:ireland_school) { create(:school, country_code: 'IE', school_roll_number: '01572D', creator_id: SecureRandom.uuid) } + let(:school) { create(:school) } + let!(:us_school) { create(:school, country_code: 'US', district_name: 'Some District', district_nces_id: '0100000') } + let!(:ireland_school) { create(:school, country_code: 'IE', school_roll_number: '01572D') } describe 'associations' do it 'has many classes' do @@ -158,25 +158,6 @@ expect(school).not_to be_valid end - it 'requires a creator_id' do - school.creator_id = nil - expect(school).not_to be_valid - end - - it 'requires a unique creator_id' do - school.save! - another_school = build(:school, creator_id: school.creator_id) - another_school.valid? - expect(another_school.errors[:creator_id]).to include('has already been taken') - end - - it 'schools can re-use creator_ids if the original school is rejected' do - rejected_school = create(:school, creator_id: SecureRandom.uuid, rejected_at: Time.zone.now) - other_school = build(:school, creator_id: rejected_school.creator_id) - expect(rejected_school).to be_valid - expect(other_school).to be_valid - end - it 'rejects a badly formed url for website' do school.website = 'http://.example.com' expect(school).not_to be_valid @@ -512,23 +493,6 @@ end end - describe '#creator' do - let(:creator) { create(:owner, school:) } - - before do - school.update!(creator_id: creator.id) - stub_user_info_api_for(creator) - end - - it 'returns a User instance' do - expect(school.creator).to be_instance_of(User) - end - - it 'returns the creator from the UserInfo API matching the creator_id' do - expect(school.creator.id).to eq(creator.id) - end - end - describe '.find_for_user!' do before do stub_user_info_api_for(teacher) @@ -539,24 +503,10 @@ expect(described_class.find_for_user!(user)).to eq(school) end - it "returns the school that the user created if they don't have a role in any school" do - creator = create(:user) - school.update!(creator_id: creator.id) - expect(described_class.find_for_user!(creator)).to eq(school) - end - it "raises ActiveRecord::RecordNotFound if the user doesn't have a role in a school" do user = build(:user) expect { described_class.find_for_user!(user) }.to raise_error(ActiveRecord::RecordNotFound) end - - it('raises ActiveRecord::RecordNotFound if the user is the creator of a rejected school') do - creator = create(:user) - school.update!(creator_id: creator.id) - school.update!(rejected_at: Time.zone.now) - - expect { described_class.find_for_user!(creator) }.to raise_error(ActiveRecord::RecordNotFound) - end end describe '#verified?' do @@ -695,20 +645,11 @@ .with(hash_including(is_create: true)) end - it 'enqueues Salesforce::ContactSyncJob on create' do - expect { create(:school) }.to have_enqueued_job(Salesforce::ContactSyncJob) - end - it 'enqueues Salesforce::SchoolSyncJob on update' do school = create(:school) expect { school.update!(name: 'Updated Name') }.to have_enqueued_job(Salesforce::SchoolSyncJob).with(hash_including(is_create: false)) end - it 'enqueues Salesforce::ContactSyncJob on update' do - school = create(:school) - expect { school.update!(name: 'Updated Name') }.to have_enqueued_job(Salesforce::ContactSyncJob) - end - context 'when SALESFORCE_ENABLED is false' do around do |example| ClimateControl.modify(SALESFORCE_ENABLED: 'false') { example.run } @@ -717,10 +658,6 @@ it 'does not enqueue Salesforce::SchoolSyncJob on create' do expect { create(:school) }.not_to have_enqueued_job(Salesforce::SchoolSyncJob) end - - it 'does not enqueue Salesforce::ContactSyncJob on create' do - expect { create(:school) }.not_to have_enqueued_job(Salesforce::ContactSyncJob) - end end end diff --git a/spec/services/school_onboarding_service_spec.rb b/spec/services/school_onboarding_service_spec.rb index 942319e88..c5160b3b6 100644 --- a/spec/services/school_onboarding_service_spec.rb +++ b/spec/services/school_onboarding_service_spec.rb @@ -4,41 +4,41 @@ RSpec.describe SchoolOnboardingService do let(:token) { UserProfileMock::TOKEN } - let(:school) { create(:verified_school, creator_id: school_creator.id) } - let(:school_creator) { create(:user) } + let(:school) { create(:verified_school) } + let(:owner) { create(:user) } let(:service) { described_class.new(school) } before do - authenticated_in_hydra_as(school_creator) + authenticated_in_hydra_as(owner) allow(ProfileApiClient).to receive(:create_school) stub_profile_api_create_safeguarding_flag end describe '#onboard' do describe 'when onboarding is successful' do - it 'grants the creator the owner role for the school' do - service.onboard(token:) - expect(school_creator).to be_school_owner(school) + it 'grants the owner the owner role for the school' do + service.onboard(owner_id: owner.id, token:) + expect(owner).to be_school_owner(school) end - it 'grants the creator the teacher role for the school' do - service.onboard(token:) - expect(school_creator).to be_school_teacher(school) + it 'grants the owner the teacher role for the school' do + service.onboard(owner_id: owner.id, token:) + expect(owner).to be_school_teacher(school) end it 'creates the school in Profile API' do - service.onboard(token:) + service.onboard(owner_id: owner.id, token:) expect(ProfileApiClient).to have_received(:create_school).with(token:, id: school.id, code: school.code) end - it 'creates the owner safeguarding flag for the creator' do - service.onboard(token:) - expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token:, flag: 'school:owner', email: school_creator.email, school_id: school.id) + it 'creates the owner safeguarding flag for the owner' do + service.onboard(owner_id: owner.id, token:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token:, flag: 'school:owner', email: owner.email, school_id: school.id) end - it 'creates the teacher safeguarding flag for the creator' do - service.onboard(token:) - expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token:, flag: 'school:teacher', email: school_creator.email, school_id: school.id) + it 'creates the teacher safeguarding flag for the owner' do + service.onboard(owner_id: owner.id, token:) + expect(ProfileApiClient).to have_received(:create_safeguarding_flag).with(token:, flag: 'school:teacher', email: owner.email, school_id: school.id) end it 'creates the school in Profile API before the safeguarding flags' do @@ -46,7 +46,7 @@ allow(ProfileApiClient).to receive(:create_school) { profile_api_calls << :create_school } allow(ProfileApiClient).to receive(:create_safeguarding_flag) { profile_api_calls << :create_safeguarding_flag } - service.onboard(token:) + service.onboard(owner_id: owner.id, token:) expect(profile_api_calls.first).to eq(:create_school) end end @@ -57,17 +57,17 @@ end it 'does not create owner role' do - suppress(RuntimeError) { service.onboard(token:) } - expect(school_creator).not_to be_school_owner(school) + suppress(RuntimeError) { service.onboard(owner_id: owner.id, token:) } + expect(owner).not_to be_school_owner(school) end it 'does not create teacher role' do - suppress(RuntimeError) { service.onboard(token:) } - expect(school_creator).not_to be_school_teacher(school) + suppress(RuntimeError) { service.onboard(owner_id: owner.id, token:) } + expect(owner).not_to be_school_teacher(school) end it 'raises the underlying error' do - expect { service.onboard(token:) }.to raise_error(RuntimeError) + expect { service.onboard(owner_id: owner.id, token:) }.to raise_error(RuntimeError) end end @@ -77,17 +77,17 @@ end it 'does not create owner role' do - suppress(ProfileApiClient::UnauthorizedError) { service.onboard(token:) } - expect(school_creator).not_to be_school_owner(school) + suppress(ProfileApiClient::UnauthorizedError) { service.onboard(owner_id: owner.id, token:) } + expect(owner).not_to be_school_owner(school) end it 'does not create teacher role' do - suppress(ProfileApiClient::UnauthorizedError) { service.onboard(token:) } - expect(school_creator).not_to be_school_teacher(school) + suppress(ProfileApiClient::UnauthorizedError) { service.onboard(owner_id: owner.id, token:) } + expect(owner).not_to be_school_teacher(school) end it 'raises the underlying error' do - expect { service.onboard(token:) }.to raise_error(ProfileApiClient::UnauthorizedError) + expect { service.onboard(owner_id: owner.id, token:) }.to raise_error(ProfileApiClient::UnauthorizedError) end end @@ -98,22 +98,22 @@ end it 'keeps the owner role' do - service.onboard(token:) - expect(school_creator).to be_school_owner(school) + service.onboard(owner_id: owner.id, token:) + expect(owner).to be_school_owner(school) end it 'keeps the teacher role' do - service.onboard(token:) - expect(school_creator).to be_school_teacher(school) + service.onboard(owner_id: owner.id, token:) + expect(owner).to be_school_teacher(school) end it 'reports the error to Sentry' do - service.onboard(token:) + service.onboard(owner_id: owner.id, token:) expect(Sentry).to have_received(:capture_exception) end it 'does not raise' do - expect { service.onboard(token:) }.not_to raise_error + expect { service.onboard(owner_id: owner.id, token:) }.not_to raise_error end end @@ -121,26 +121,26 @@ let(:another_school) { create(:school) } before do - create(:role, user_id: school.creator_id, school: another_school) + create(:role, user_id: owner.id, school: another_school) end it 'does not create owner role' do - suppress(ActiveRecord::RecordInvalid) { service.onboard(token:) } - expect(school_creator).not_to be_school_owner(school) + suppress(ActiveRecord::RecordInvalid) { service.onboard(owner_id: owner.id, token:) } + expect(owner).not_to be_school_owner(school) end it 'does not create teacher role' do - suppress(ActiveRecord::RecordInvalid) { service.onboard(token:) } - expect(school_creator).not_to be_school_teacher(school) + suppress(ActiveRecord::RecordInvalid) { service.onboard(owner_id: owner.id, token:) } + expect(owner).not_to be_school_teacher(school) end it 'does not create school in Profile API' do - suppress(ActiveRecord::RecordInvalid) { service.onboard(token:) } + suppress(ActiveRecord::RecordInvalid) { service.onboard(owner_id: owner.id, token:) } expect(ProfileApiClient).not_to have_received(:create_school) end it 'raises the underlying error' do - expect { service.onboard(token:) }.to raise_error(ActiveRecord::RecordInvalid) + expect { service.onboard(owner_id: owner.id, token:) }.to raise_error(ActiveRecord::RecordInvalid) end end end diff --git a/spec/services/school_verification_service_spec.rb b/spec/services/school_verification_service_spec.rb index fcd225da6..56ca2bdd5 100644 --- a/spec/services/school_verification_service_spec.rb +++ b/spec/services/school_verification_service_spec.rb @@ -4,8 +4,7 @@ RSpec.describe SchoolVerificationService do let(:website) { 'http://example.com' } - let(:school) { build(:school, creator_id: school_creator.id, website:) } - let(:school_creator) { create(:user) } + let(:school) { build(:school, website:) } let(:service) { described_class.new(school) } before do