fix bug where cancellations can take place without a reason

app/services/request_destroy_service.rb

@@ -30,6 +30,8 @@ def valid?
       errors.add(:base, 'request_id is invalid')
     elsif request.discarded_at.present?
       errors.add(:base, 'request already cancelled')
+    elsif reason.blank?
+      errors.add(:base, 'a cancellation reason is required')
     end
 
     errors.none?

app/views/requests/cancelation/new.html.erb

@@ -28,9 +28,9 @@
         <div class="card card-primary">
           <div class="card-body">
             <div class="box w-1/2">
-              <%= simple_form_for :cancelation, url: request_cancelation_path(organization: @organization, request_id: @request.id), method: :post do |f| %>
+              <%= simple_form_for :cancelation, url: request_cancelation_path(organization: @organization, request_id: @request.id), method: :post, html: { novalidate: false } do |f| %>
                 <div class='flex flex-col'>
-                  <%= f.input :reason, label: "Cancellation reason" %>
+                  <%= f.input :reason, label: "Cancellation reason", required: true %>
                   <div class='font-light text-md text-yellow-600'>This will be included in the email notification we send to the partner</div>
                   <div class="font-light text-sm text-red">Note: cancellation emails will not be sent to deactivated partners</div>
                 </div>

spec/requests/requests_requests_spec.rb

@@ -261,5 +261,33 @@
         end
       end
     end
+
+    describe 'POST #create for cancelation' do
+      let(:request) { create(:request, organization: organization) }
+
+      context 'when a cancellation reason is given' do
+        it 'cancels the request and redirects to the index', :aggregate_failures do
+          expect do
+            post request_cancelation_path(request_id: request.id), params: { cancelation: { reason: 'Partner closed for the season' } }
+          end.to change { request.reload.status }.from('pending').to('cancelled')
+
+          expect(request.reload.discard_reason).to eq('Partner closed for the season')
+          expect(flash[:notice]).to eq("Request #{request.id} has been removed!")
+          expect(response).to redirect_to(requests_path)
+        end
+      end
+
+      context 'when the cancellation reason is blank' do
+        it 'does not cancel the request and redirects back with an error', :aggregate_failures do
+          expect do
+            post request_cancelation_path(request_id: request.id), params: { cancelation: { reason: '' } }
+          end.not_to change { request.reload.status }
+
+          expect(request.reload.discarded?).to be false
+          expect(flash[:error]).to eq("Request #{request.id} could not be removed because a cancellation reason is required")
+          expect(response).to redirect_to(new_request_cancelation_path(request_id: request.id))
+        end
+      end
+    end
   end
 end

spec/services/request_destroy_service_spec.rb

@@ -1,8 +1,9 @@
 RSpec.describe RequestDestroyService, type: :service do
   describe '#call' do
-    subject { described_class.new(request_id: request_id).call }
+    subject { described_class.new(request_id: request_id, reason: reason).call }
     let(:request_id) { request.id }
     let(:request) { create(:request) }
+    let(:reason) { 'Partner no longer needs these items' }
 
     it 'should return an instance of itself' do
       expect(subject).to be_a_kind_of(RequestDestroyService)
@@ -26,6 +27,30 @@
       end
     end
 
+    context 'when the cancellation reason is blank' do
+      let(:reason) { '   ' }
+
+      it 'should not be successful and have errors indicating a reason is required' do
+        expect(subject.errors.full_messages).to eq(['a cancellation reason is required'])
+      end
+
+      it 'should have the same errors when no reason is given at all' do
+        svc = described_class.new(request_id: request_id).call
+
+        expect(svc.errors.full_messages).to eq(['a cancellation reason is required'])
+      end
+
+      it 'should not cancel the request' do
+        expect { subject }.not_to change { request.reload.discarded? }
+        expect(request.reload).to be_status_pending
+      end
+
+      it 'should not send a email notification to the partner' do
+        expect(RequestMailer).not_to receive(:request_cancel_partner_notification)
+        subject
+      end
+    end
+
     context 'when there are no validation errors' do
       let(:fake_mailer) { double('fake_mailer', deliver_later: -> {}) }
       before do
@@ -40,6 +65,10 @@
         expect { subject }.to change { request.reload.status_cancelled? }.from(false).to(true)
       end
 
+      it 'should store the cancellation reason on the request' do
+        expect { subject }.to change { request.reload.discard_reason }.from(nil).to(reason)
+      end
+
       it 'should send a email notification to the partner' do
         subject
         expect(fake_mailer).to have_received(:deliver_later)

spec/system/request_system_spec.rb

@@ -264,6 +264,24 @@
         expect(request.reload.discard_reason).to eq(reason)
       end
 
+      it 'should not submit the form until a reason is given' do
+        click_on 'Cancel'
+
+        # the browser blocks the submission while the required reason is empty
+        expect(page).to have_field('Cancellation reason *', valid: false)
+
+        click_on 'Yes. Cancel Request'
+
+        expect(page).to have_field('Cancellation reason *')
+        expect(request.reload.discarded_at).to eq(nil)
+
+        fill_in 'Cancellation reason *', with: reason
+        click_on 'Yes. Cancel Request'
+
+        expect(page).to have_content("Request #{request.id} has been removed")
+        expect(request.reload.discard_reason).to eq(reason)
+      end
+
       it 'should show the partners name, requesters email, request date, comments' do
         click_on 'Cancel'