Return 404 when scheme not found (#804)

* Return not found for schemes when... not found

* Redundant check

* Apply to all member actions

* Add not found checks for location

app/controllers/locations_controller.rb

@@ -39,13 +39,18 @@ class LocationsController < ApplicationController
     end
   end
 
-  def edit; end
+  def edit
+    render_not_found and return unless @location && @scheme
+  end
 
-  def edit_name; end
+  def edit_name
+    render_not_found and return unless @location && @scheme
+  end
 
   def update
-    page = params[:location][:page]
+    render_not_found and return unless @location && @scheme
 
+    page = params[:location][:page]
     if @location.update(location_params)
       case page
       when "edit"
@@ -77,12 +82,12 @@ private
     @scheme = if %w[new create index edit_name].include?(action_name)
                 Scheme.find(params[:id])
               else
-                @location.scheme
+                @location&.scheme
               end
   end
 
   def find_location
-    @location = params[:location_id].present? ? Location.find(params[:location_id]) : Location.find(params[:id])
+    @location = params[:location_id].present? ? Location.find_by(id: params[:location_id]) : Location.find_by(id: params[:id])
   end
 
   def authenticate_scope!
@@ -90,7 +95,7 @@ private
   end
 
   def authenticate_action!
-    if %w[new edit update create index edit_name].include?(action_name) && !((current_user.organisation == @scheme.owning_organisation) || current_user.support?)
+    if %w[new edit update create index edit_name].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?)
       render_not_found and return
     end
   end

app/controllers/schemes_controller.rb

@@ -18,6 +18,7 @@ class SchemesController < ApplicationController
 
   def show
     @scheme = Scheme.find_by(id: params[:id])
+    render_not_found and return unless @scheme
   end
 
   def new
@@ -42,7 +43,13 @@ class SchemesController < ApplicationController
     end
   end
 
+  def edit
+    render_not_found and return unless @scheme
+  end
+
   def update
+    render_not_found and return unless @scheme
+
     check_answers = params[:scheme][:check_answers]
     page = params[:scheme][:page]
 
@@ -64,34 +71,50 @@ class SchemesController < ApplicationController
   end
 
   def primary_client_group
+    render_not_found and return unless @scheme
+
     render "schemes/primary_client_group"
   end
 
   def confirm_secondary_client_group
+    render_not_found and return unless @scheme
+
     render "schemes/confirm_secondary"
   end
 
   def secondary_client_group
+    render_not_found and return unless @scheme
+
     render "schemes/secondary_client_group"
   end
 
   def support
+    render_not_found and return unless @scheme
+
     render "schemes/support"
   end
 
   def details
+    render_not_found and return unless @scheme
+
     render "schemes/details"
   end
 
   def check_answers
+    render_not_found and return unless @scheme
+
     render "schemes/check_answers"
   end
 
   def edit_name
+    render_not_found and return unless @scheme
+
     render "schemes/edit_name"
   end
 
   def support_services_provider
+    render_not_found and return unless @scheme
+
     render "schemes/support_services_provider"
   end
 
@@ -193,11 +216,15 @@ private
   end
 
   def arrangement_type_set_to_same_org?(required_params)
-    arrangement_type_value(required_params[:arrangement_type]) == "D" || (required_params[:arrangement_type].blank? && @scheme.present? && @scheme.arrangement_type_same?)
+    return unless @scheme
+
+    arrangement_type_value(required_params[:arrangement_type]) == "D" || (required_params[:arrangement_type].blank? && @scheme.arrangement_type_same?)
   end
 
   def arrangement_type_changed_to_different_org?(required_params)
-    @scheme.present? && @scheme.arrangement_type_same? && arrangement_type_value(required_params[:arrangement_type]) != "D" && required_params[:managing_organisation_id].blank?
+    return unless @scheme
+
+    @scheme.arrangement_type_same? && arrangement_type_value(required_params[:arrangement_type]) != "D" && required_params[:managing_organisation_id].blank?
   end
 
   def arrangement_type_value(key)
@@ -215,7 +242,7 @@ private
   def authenticate_scope!
     head :unauthorized and return unless current_user.data_coordinator? || current_user.support?
 
-    if %w[show locations primary_client_group confirm_secondary_client_group secondary_client_group support details check_answers edit_name].include?(action_name) && !((current_user.organisation == @scheme.owning_organisation) || current_user.support?)
+    if %w[show locations primary_client_group confirm_secondary_client_group secondary_client_group support details check_answers edit_name].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?)
       render_not_found and return
     end
   end

spec/requests/locations_controller_spec.rb

@@ -454,7 +454,7 @@ RSpec.describe LocationsController, type: :request do
         expect(page).to have_content("Add a location to this scheme")
       end
 
-      context "when trying to new location to a scheme that belongs to another organisation" do
+      context "when trying to edit a location that belongs to another organisation" do
         let(:another_scheme)  { FactoryBot.create(:scheme) }
         let(:another_location)  { FactoryBot.create(:location, scheme: another_scheme) }
 
@@ -463,6 +463,14 @@ RSpec.describe LocationsController, type: :request do
           expect(response).to have_http_status(:not_found)
         end
       end
+
+      context "when the requested location does not exist" do
+        let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1) }
+
+        it "returns not found" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
     end
 
     context "when signed in as a support user" do
@@ -635,6 +643,15 @@ RSpec.describe LocationsController, type: :request do
           expect(page).to have_content(I18n.t("activerecord.errors.models.location.attributes.type_of_unit.blank"))
         end
       end
+
+      context "when the requested location does not exist" do
+        let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1) }
+        let(:params) { {} }
+
+        it "returns not found" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
     end
 
     context "when signed in as a support user" do
@@ -1060,6 +1077,14 @@ RSpec.describe LocationsController, type: :request do
         expect(response).to have_http_status(:ok)
         expect(page).to have_content("Location name for #{location.postcode}")
       end
+
+      context "when the requested location does not exist" do
+        let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1) }
+
+        it "returns not found" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
     end
   end
 end

spec/requests/schemes_controller_spec.rb

@@ -250,6 +250,13 @@ RSpec.describe SchemesController, type: :request do
           expect(response).to have_http_status(:not_found)
         end
       end
+
+      context "when the requested scheme does not exist" do
+        it "returns not found" do
+          get "/schemes/#{Scheme.maximum(:id) + 1}"
+          expect(response).to have_http_status(:not_found)
+        end
+      end
     end
 
     context "when signed in as a support user" do
@@ -864,6 +871,15 @@ RSpec.describe SchemesController, type: :request do
           expect(scheme_to_update.reload.sensitive).to eq("Yes")
         end
       end
+
+      context "when the requested scheme does not exist" do
+        let(:scheme_to_update) { OpenStruct.new(id: Scheme.maximum(:id) + 1) }
+        let(:params) { {} }
+
+        it "returns not found" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
     end
 
     context "when signed in as a support" do