diff --git a/.env.example b/.env.example index 7d94cf136..ba56008f3 100644 --- a/.env.example +++ b/.env.example @@ -55,5 +55,3 @@ GOOGLE_CLIENT_SECRET=changeme # E2E tests can supply this to enable test utility endpoints RESEED_API_KEY=changeme -# Enable immediate onboarding for schools -ENABLE_IMMEDIATE_SCHOOL_ONBOARDING=true diff --git a/app/models/school.rb b/app/models/school.rb index 2f14500b0..d57530e1a 100644 --- a/app/models/school.rb +++ b/app/models/school.rb @@ -49,8 +49,7 @@ class School < ApplicationRecord before_save :format_uk_postal_code, if: :should_format_uk_postal_code? - # TODO: Remove the conditional once the feature flag is retired - after_create :generate_code!, if: -> { FeatureFlags.immediate_school_onboarding? } + after_create :generate_code! def self.find_for_user!(user) school = Role.find_by(user_id: user.id)&.school || find_by(creator_id: user.id) @@ -72,9 +71,7 @@ def rejected? end def verify! - # TODO: Remove this line once the feature flag is retired - generate_code! unless FeatureFlags.immediate_school_onboarding? - + generate_code! update!(verified_at: Time.zone.now) end diff --git a/app/models/teacher_invitation.rb b/app/models/teacher_invitation.rb index 68576b5b7..c93a67195 100644 --- a/app/models/teacher_invitation.rb +++ b/app/models/teacher_invitation.rb @@ -6,7 +6,6 @@ class TeacherInvitation < ApplicationRecord belongs_to :school validates :email_address, format: { with: EmailValidator.regexp, message: I18n.t('validations.invitation.email_address') } - validate :school_is_verified, unless: -> { FeatureFlags.immediate_school_onboarding? } after_create_commit :send_invitation_email encrypts :email_address @@ -16,12 +15,6 @@ class TeacherInvitation < ApplicationRecord private - def school_is_verified - return if school.verified? - - errors.add(:school, 'is not verified') - end - def send_invitation_email InvitationMailer.with(invitation: self).invite_teacher.deliver_later end diff --git a/app/services/school_verification_service.rb b/app/services/school_verification_service.rb index aaa3b7559..b469ad565 100644 --- a/app/services/school_verification_service.rb +++ b/app/services/school_verification_service.rb @@ -7,24 +7,20 @@ def initialize(school) @school = school end - def verify(token: nil) + def verify(token:) success = false School.transaction do school.verify! - # TODO: Remove this line, once the feature flag is retired - success = FeatureFlags.immediate_school_onboarding? || SchoolOnboardingService.new(school).onboard(token: token) - - # TODO: Remove this line, once the feature flag is retired + success = SchoolOnboardingService.new(school).onboard(token: token) raise ActiveRecord::Rollback unless success end + + success rescue StandardError => e Sentry.capture_exception(e) Rails.logger.error { "Failed to verify school #{@school.id}: #{e.message}" } false - else - # TODO: Return 'true', once the feature flag is retired - success end delegate :reject, to: :school diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index ac6cdf96c..ff8e2521c 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -10,11 +10,8 @@ def call(school_params:, creator_id:, token:) School.transaction do response[:school].save! - # TODO: Remove this conditional once the feature flag is retired - if FeatureFlags.immediate_school_onboarding? - onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:) - raise 'School onboarding failed' unless onboarded - end + onboarded = SchoolOnboardingService.new(response[:school]).onboard(token:) + raise 'School onboarding failed' unless onboarded end response diff --git a/lib/concepts/school_student/create.rb b/lib/concepts/school_student/create.rb index 1c0745d00..2954c4fb4 100644 --- a/lib/concepts/school_student/create.rb +++ b/lib/concepts/school_student/create.rb @@ -25,8 +25,7 @@ def create_student(school, school_student_params, token) validate( username:, password:, - name:, - school: (FeatureFlags.immediate_school_onboarding? ? nil : school) + name: ) response = ProfileApiClient.create_school_student(token:, username:, password:, name:, school_id:) @@ -35,13 +34,10 @@ def create_student(school, school_student_params, token) user_id end - def validate(username:, password:, name:, school: nil) + def validate(username:, password:, name:) raise ArgumentError, "username '#{username}' is invalid" if username.blank? raise ArgumentError, "password '#{password}' is invalid" if password.size < 8 raise ArgumentError, "name '#{name}' is invalid" if name.blank? - - return unless school - raise ArgumentError, 'school must be verified' unless school.verified? end end end diff --git a/lib/feature_flags.rb b/lib/feature_flags.rb deleted file mode 100644 index 86a76d637..000000000 --- a/lib/feature_flags.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -module FeatureFlags - def self.immediate_school_onboarding? - ENV['ENABLE_IMMEDIATE_SCHOOL_ONBOARDING'] == 'true' - end -end diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index fd098540d..7b9927c3e 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -83,14 +83,7 @@ end end - describe 'when immediate onboarding is enabled' do - # TODO: Remove this block once the feature flag is retired - around do |example| - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - example.run - end - end - + describe 'immediate onboarding' do let(:onboarding_service) { instance_spy(SchoolOnboardingService, onboard: true) } before do @@ -102,24 +95,4 @@ expect(onboarding_service).to have_received(:onboard).with(token:) end end - - # TODO: Remove these examples once the feature flag is retired - describe 'when immediate onboarding is disabled' do - around do |example| - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: nil) do - example.run - end - end - - let(:onboarding_service) { instance_spy(SchoolOnboardingService) } - - before do - allow(SchoolOnboardingService).to receive(:new).and_return(onboarding_service) - end - - it 'does not call the onboarding service' do - described_class.call(school_params:, creator_id:, token:) - expect(onboarding_service).not_to have_received(:onboard) - end - end end diff --git a/spec/concepts/school_student/create_spec.rb b/spec/concepts/school_student/create_spec.rb index 1981f0a00..202b3815b 100644 --- a/spec/concepts/school_student/create_spec.rb +++ b/spec/concepts/school_student/create_spec.rb @@ -76,29 +76,6 @@ end end - context 'when the school is not verified' do - let(:school) { create(:school) } # not verified by default - - context 'when immediate_school_onboarding is FALSE' do - it 'returns the error message in the operation response' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'false') do - response = described_class.call(school:, school_student_params:, token:) - expect(response[:error]).to match(/school must be verified/) - end - end - end - - context 'when immediate_school_onboarding is TRUE' do - it 'does not error due to school verification' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - response = described_class.call(school:, school_student_params:, token:) - - expect(response[:error]).to be_nil - end - end - end - end - context 'when the student cannot be created in profile api because of a 422 response' do let(:error) { { 'message' => "something's up with the username" } } let(:exception) { ProfileApiClient::Student422Error.new(error) } diff --git a/spec/concepts/school_teacher/invite_spec.rb b/spec/concepts/school_teacher/invite_spec.rb index 231d8cbda..31f851c3f 100644 --- a/spec/concepts/school_teacher/invite_spec.rb +++ b/spec/concepts/school_teacher/invite_spec.rb @@ -49,26 +49,4 @@ expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) end end - - context 'when the school is not verified' do - let(:school) { create(:school) } - - context 'when immediate_school_onboarding is FALSE' do - it 'does return an error message in the operation response' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'false') do - response = described_class.call(school:, school_teacher_params:, token:) - expect(response[:error]).to match(/is not verified/) - end - end - end - - context 'when immediate_school_onboarding is TRUE' do - it 'does not return an error message in the operation response' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - response = described_class.call(school:, school_teacher_params:, token:) - expect(response[:error]).to be_blank - end - end - end - end end diff --git a/spec/lib/feature_flags_spec.rb b/spec/lib/feature_flags_spec.rb deleted file mode 100644 index b7fa7b085..000000000 --- a/spec/lib/feature_flags_spec.rb +++ /dev/null @@ -1,37 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe FeatureFlags do - describe '.immediate_school_onboarding?' do - it 'returns true when ENV is set to "true"' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - expect(described_class.immediate_school_onboarding?).to be(true) - end - end - - it 'returns false when ENV is not set' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: nil) do - expect(described_class.immediate_school_onboarding?).to be(false) - end - end - - it 'returns false when ENV is set to empty string' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: '') do - expect(described_class.immediate_school_onboarding?).to be(false) - end - end - - it 'returns false when ENV is set to "false"' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'false') do - expect(described_class.immediate_school_onboarding?).to be(false) - end - end - - it 'returns false when ENV is set to any other value' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: '1') do - expect(described_class.immediate_school_onboarding?).to be(false) - end - end - end -end diff --git a/spec/models/school_spec.rb b/spec/models/school_spec.rb index 06846060c..81bdbc06f 100644 --- a/spec/models/school_spec.rb +++ b/spec/models/school_spec.rb @@ -417,12 +417,6 @@ end describe 'code validations' do - around do |example| - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - example.run - end - end - it 'requires #code to be unique' do school # ensure existing school has a code another_school = build(:school) @@ -532,12 +526,6 @@ end describe 'code generation' do - around do |example| - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - example.run - end - end - it 'automatically generates a code after creation' do new_school = described_class.create!(build(:school).attributes.except('id', 'created_at', 'updated_at')) expect(new_school.reload.code).to be_present diff --git a/spec/models/teacher_invitation_spec.rb b/spec/models/teacher_invitation_spec.rb index d1706e08e..0fdb4af94 100644 --- a/spec/models/teacher_invitation_spec.rb +++ b/spec/models/teacher_invitation_spec.rb @@ -18,24 +18,6 @@ expect(invitation).not_to be_valid end - it 'is valid with an unverified school and immediate_school_onboarding is TRUE' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - school = build(:school, verified_at: nil) - invitation = build(:teacher_invitation, school:) - - expect(invitation).to be_valid - end - end - - it 'is invalid with an unverified school and immediate_school_onboarding is FALSE' do - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'false') do - school = build(:school, verified_at: nil) - invitation = build(:teacher_invitation, school:) - - expect(invitation).not_to be_valid - end - end - it 'sends an invitation email after create' do school = create(:verified_school) diff --git a/spec/services/school_verification_service_spec.rb b/spec/services/school_verification_service_spec.rb index 93ea0289b..e571ff714 100644 --- a/spec/services/school_verification_service_spec.rb +++ b/spec/services/school_verification_service_spec.rb @@ -14,173 +14,32 @@ end describe '#verify' do - describe 'when immediate onboarding is enabled' do - # TODO: Remove this block once the feature flag is retired - around do |example| - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: 'true') do - example.run - end + describe 'when school can be saved' do + it 'saves the school' do + service.verify(token:) + expect(school).to be_persisted end - describe 'when school can be saved' do - it 'saves the school' do - service.verify - expect(school).to be_persisted - end - - it 'sets verified_at to a date' do - service.verify - expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone) - end - - it 'returns true' do - expect(service.verify).to be(true) - end + it 'sets verified_at to a date' do + service.verify(token:) + expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone) end - describe 'when school cannot be saved' do - let(:website) { 'invalid' } - - it 'does not save the school' do - service.verify - expect(school).not_to be_persisted - end - - it 'returns false' do - expect(service.verify).to be(false) - end + it 'returns true' do + expect(service.verify(token:)).to be(true) end end - # TODO: Remove these examples once the feature flag is retired - describe 'when immediate onboarding is disabled' do - around do |example| - ClimateControl.modify(ENABLE_IMMEDIATE_SCHOOL_ONBOARDING: nil) do - example.run - end - end - - describe 'when school can be saved' do - it 'saves the school' do - service.verify(token:) - expect(school).to be_persisted - end - - it 'sets verified_at to a date' do - service.verify(token:) - expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone) - end - - it 'generates school code' do - service.verify(token:) - expect(school.reload.code).to be_present - end + describe 'when school cannot be saved' do + let(:website) { 'invalid' } - it 'grants the creator the owner role for the school' do - service.verify(token:) - expect(school_creator).to be_school_owner(school) - end - - it 'grants the creator the teacher role for the school' do - service.verify(token:) - expect(school_creator).to be_school_teacher(school) - end - - it 'creates the school in Profile API' do - service.verify(token:) - expect(ProfileApiClient).to have_received(:create_school).with(token:, id: school.id, code: school.code) - end - - it 'returns true' do - expect(service.verify(token:)).to be(true) - end + it 'does not save the school' do + service.verify(token:) + expect(school).not_to be_persisted end - describe 'when school cannot be saved' do - let(:website) { 'invalid' } - - it 'does not save the school' do - service.verify(token:) - expect(school).not_to be_persisted - end - - it 'does not create owner role' do - service.verify(token:) - expect(school_creator).not_to be_school_owner(school) - end - - it 'does not create teacher role' do - service.verify(token:) - expect(school_creator).not_to be_school_teacher(school) - end - - it 'does not create school in Profile API' do - expect(ProfileApiClient).not_to have_received(:create_school) - end - - it 'returns false' do - expect(service.verify(token:)).to be(false) - end - end - - describe 'when the school cannot be created in Profile API' do - before do - allow(ProfileApiClient).to receive(:create_school).and_raise(RuntimeError) - end - - it 'does not save the school' do - service.verify(token:) - expect { school.reload }.to raise_error(ActiveRecord::RecordNotFound) - end - - it 'does not create owner role' do - service.verify(token:) - expect(school_creator).not_to be_school_owner(school) - end - - it 'does not create teacher role' do - service.verify(token:) - expect(school_creator).not_to be_school_teacher(school) - end - - it 'does not create school in Profile API' do - expect(ProfileApiClient).not_to have_received(:create_school) - end - - it 'returns false' do - expect(service.verify(token:)).to be(false) - end - end - - describe 'when teacher and owner roles cannot be created because they already have a role in another school' do - let(:another_school) { create(:school) } - - before do - create(:role, user_id: school.creator_id, school: another_school) - end - - it 'does not save the school' do - service.verify(token:) - expect { school.reload }.to raise_error(ActiveRecord::RecordNotFound) - end - - it 'does not create owner role' do - service.verify(token:) - expect(school_creator).not_to be_school_owner(school) - end - - it 'does not create teacher role' do - service.verify(token:) - expect(school_creator).not_to be_school_teacher(school) - end - - it 'does not create school in Profile API' do - expect(ProfileApiClient).not_to have_received(:create_school) - end - - it 'returns false' do - expect(service.verify(token:)).to be(false) - end + it 'returns false' do + expect(service.verify(token:)).to be(false) end end end @@ -202,7 +61,7 @@ describe 'when the school was previously verified' do before do - service.verify + service.verify(token:) service.reject school.reload end