Fix bugs in SchoolVerificationService

We were previously ignoring the result of the call to `School#update`
which meant that if the update failed we'd still create the owner and
teacher roles, and return true from `#verify`. Calling
`School#update!` means that if the update fails then a
`ActiveRecord::RecordInvalid` exception will be raised, the transaction
will be rolled back and the `rescue` block will be invoked.

Fixing the first problem revealed that the `rescue` block was attempting
to access `school_id` which doesn't exist.

Author: chrisroos

app/services/school_verification_service.rb

@@ -8,13 +8,14 @@ def initialize(school_id)
   def verify
     School.transaction do
       school = School.find(@school_id)
-      school.update(verified_at: Time.zone.now, rejected_at: nil)
+      school.update!(verified_at: Time.zone.now, rejected_at: nil)
+
       Role.owner.create(user_id: school.creator_id, school:)
       Role.teacher.create(user_id: school.creator_id, school:)
     end
   rescue StandardError => e
     Sentry.capture_exception(e)
-    Rails.logger.error { "Failed to verify school #{school_id}: #{e.message}" }
+    Rails.logger.error { "Failed to verify school #{@school_id}: #{e.message}" }
     false
   else
     true

spec/services/school_verification_service_spec.rb

@@ -10,21 +10,62 @@
   let(:organisation_id) { SecureRandom.uuid }
 
   describe '#verify' do
-    before do
-      service.verify
-      school.reload
-    end
+    describe 'when school can be saved' do
+      it 'sets verified_at to a date' do
+        service.verify
+        expect(school.reload.verified_at).to be_a(ActiveSupport::TimeWithZone)
+      end
+
+      it 'grants the creator the owner role for the school' do
+        service.verify
+        expect(school_creator).to be_school_owner(school)
+      end
+
+      it 'grants the creator the teacher role for the school' do
+        service.verify
+        expect(school_creator).to be_school_teacher(school)
+      end
 
-    it 'sets verified_at to a date' do
-      expect(school.verified_at).to be_a(ActiveSupport::TimeWithZone)
+      it 'returns true' do
+        expect(service.verify).to be(true)
+      end
     end
 
-    it 'grants the creator the owner role for the school' do
-      expect(school_creator).to be_school_owner(school)
+    describe 'when school cannot be saved' do
+      before do
+        school.update_attribute(:website, 'invalid') # rubocop:disable Rails/SkipsModelValidations
+      end
+
+      it 'does not set verified_at' do
+        service.verify
+        expect(school.reload.verified_at).to be_nil
+      end
+
+      it 'does not create owner role' do
+        service.verify
+        expect(school_creator).not_to be_school_owner(school)
+      end
+
+      it 'does not create teacher role' do
+        service.verify
+        expect(school_creator).not_to be_school_teacher(school)
+      end
+
+      it 'returns false' do
+        expect(service.verify).to be(false)
+      end
     end
 
-    it 'grants the creator the teacher role for the school' do
-      expect(school_creator).to be_school_teacher(school)
+    describe 'when the school cannot be found' do
+      before do
+        allow(Sentry).to receive(:capture_exception)
+        allow(School).to receive(:find).with(school.id).and_raise(ActiveRecord::RecordNotFound)
+        service.verify
+      end
+
+      it 'reports the error in Sentry' do
+        expect(Sentry).to have_received(:capture_exception)
+      end
     end
   end