refactor: centralize controller rescues, enable index policy scoping, fix Docker mount perms

.github/workflows/docker.yml

@@ -33,7 +33,10 @@ jobs:
           mkdir -p tmp/downloads
           chmod 777 tmp tmp/downloads
       - name: docker UP
-        run: docker compose up -d
+        run: |
+          docker compose up -d database selenium_chrome
+          docker compose run --rm --no-deps --user 0:0 --entrypoint sh web -c "chown -R app:app /usr/src/app /usr/local/bundle/gems"
+          docker compose up -d web
       - name: db:setup
         run: docker compose exec -T web rails db:setup
       - name: compile assets

app/controllers/all_casa_admins_controller.rb

@@ -4,6 +4,7 @@ class AllCasaAdminsController < ApplicationController
   before_action :set_custom_error_heading, only: [:update_password]
   after_action :reset_custom_error_heading, only: [:update_password]
   skip_after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def new
     @all_casa_admin = AllCasaAdmin.new

app/controllers/android_app_associations_controller.rb

@@ -1,5 +1,6 @@
 class AndroidAppAssociationsController < ApplicationController
   skip_before_action :authenticate_user!
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     android_asset_link_data = [

app/controllers/application_controller.rb

@@ -12,13 +12,12 @@ class ApplicationController < ActionController::Base
   before_action :set_current_organization
   before_action :set_active_banner
   after_action :verify_authorized, except: :index, unless: :devise_controller?
-  # after_action :verify_policy_scoped, only: :index
+  after_action :verify_policy_scoped, only: :index, unless: :devise_controller?
 
-  KNOWN_ERRORS = [Pundit::NotAuthorizedError, Organizational::UnknownOrganization]
-  rescue_from StandardError, with: :log_and_reraise
   rescue_from Pundit::NotAuthorizedError, with: :not_authorized
   rescue_from Organizational::UnknownOrganization, with: :not_authorized
   rescue_from ActionController::UnknownFormat, with: :unsupported_media_type
+  rescue_from ActiveRecord::RecordNotFound, with: :record_not_found
 
   impersonates :user
 
@@ -157,6 +156,13 @@ def not_authorized
     end
   end
 
+  def record_not_found
+    respond_to do |format|
+      format.json { render json: {error: "Record not found"}, status: :not_found }
+      format.any { render file: Rails.public_path.join("404.html"), status: :not_found, layout: false }
+    end
+  end
+
   def unsupported_media_type
     respond_to do |format|
       format.json do
@@ -169,13 +175,6 @@ def unsupported_media_type
     end
   end
 
-  def log_and_reraise(error)
-    unless KNOWN_ERRORS.include?(error.class)
-      Bugsnag.notify(error)
-    end
-    raise
-  end
-
   def check_unconfirmed_email_notice(user)
     notice = "#{user.role} was successfully updated."
     if user.saved_changes.include?("unconfirmed_email")

app/controllers/banners_controller.rb

@@ -1,5 +1,6 @@
 class BannersController < ApplicationController
   after_action :verify_authorized, except: %i[dismiss]
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
   before_action :set_banner, only: %i[edit update destroy dismiss]
 
   def index

app/controllers/case_contact_reports_controller.rb

@@ -2,6 +2,7 @@
 
 class CaseContactReportsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize :application, :see_reports_page?

app/controllers/case_court_reports_controller.rb

@@ -3,6 +3,7 @@
 class CaseCourtReportsController < ApplicationController
   before_action :set_casa_case, only: %i[show]
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize CaseCourtReport

app/controllers/contact_type_groups_controller.rb

@@ -2,8 +2,6 @@ class ContactTypeGroupsController < ApplicationController
   before_action :set_contact_type_group, except: [:new, :create]
   after_action :verify_authorized
 
-  rescue_from ActiveRecord::RecordNotFound, with: :record_not_found
-
   def new
     authorize ContactTypeGroup
     @contact_type_group = ContactTypeGroup.new
@@ -35,13 +33,6 @@ def update
 
   private
 
-  def record_not_found
-    respond_to do |format|
-      format.json { render json: {error: "Record not found"}, status: :not_found }
-      format.any { render file: Rails.public_path.join("404.html"), status: :not_found, layout: false }
-    end
-  end
-
   def contact_type_group_params
     params.require(:contact_type_group).permit(:name, :active)
   end

app/controllers/error_controller.rb

@@ -3,6 +3,7 @@
 class ErrorController < ApplicationController
   skip_before_action :authenticate_user!
   skip_after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
   end

app/controllers/followup_reports_controller.rb

@@ -2,6 +2,7 @@
 
 class FollowupReportsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize :application, :see_reports_page?

app/controllers/health_controller.rb

@@ -3,6 +3,7 @@
 class HealthController < ApplicationController
   skip_before_action :authenticate_user!
   skip_after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
   before_action :verify_token_for_old_object_stats, only: [:old_objects]
 
   def index

app/controllers/imports_controller.rb

@@ -4,6 +4,7 @@ class ImportsController < ApplicationController
   include ActionView::Helpers::UrlHelper
   before_action :failed_csv_service, only: [:create, :download_failed]
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   ERR_FAILED_IMPORT_NOTE = "Note: An additional 'error' column has been added to the file. " \
     "Please note the failure reason and remove the column when resubmitting."

app/controllers/learning_hours_reports_controller.rb

@@ -1,5 +1,6 @@
 class LearningHoursReportsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize :application, :see_reports_page?

app/controllers/mileage_rates_controller.rb

@@ -1,5 +1,6 @@
 class MileageRatesController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
   before_action :set_mileage_rate, only: %i[edit update]
 
   def index

app/controllers/mileage_reports_controller.rb

@@ -4,6 +4,7 @@
 
 class MileageReportsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize :application, :see_reports_page?

app/controllers/missing_data_reports_controller.rb

@@ -1,5 +1,6 @@
 class MissingDataReportsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize :application, :see_reports_page?

app/controllers/notifications_controller.rb

@@ -1,5 +1,6 @@
 class NotificationsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
   before_action :set_notification, only: %i[mark_as_read]
 
   def index

app/controllers/other_duties_controller.rb

@@ -1,6 +1,7 @@
 class OtherDutiesController < ApplicationController
   before_action :set_other_duty, except: [:new, :create, :index]
   before_action :convert_duration_minutes, only: [:update, :create]
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize OtherDuty

app/controllers/placement_reports_controller.rb

@@ -1,5 +1,6 @@
 class PlacementReportsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize :application, :see_reports_page?

app/controllers/reports_controller.rb

@@ -1,5 +1,6 @@
 class ReportsController < ApplicationController
   after_action :verify_authorized
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   def index
     authorize :application, :see_reports_page?

app/controllers/static_controller.rb

@@ -3,6 +3,7 @@ class StaticController < ApplicationController
   skip_before_action :authenticate_user!
   skip_before_action :set_current_user
   skip_before_action :set_current_organization
+  skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
 
   layout false
 

spec/controllers/application_controller_spec.rb

@@ -5,6 +5,8 @@
   let(:volunteer) { create(:volunteer) }
 
   controller do
+    skip_after_action :verify_policy_scoped # TODO: index should call policy_scope; remove this skip once it does
+
     def index
       render plain: "hello there..."
     end
@@ -26,6 +28,10 @@ def not_authorized_error
     def unknown_organization
       raise Organizational::UnknownOrganization
     end
+
+    def missing_record
+      raise ActiveRecord::RecordNotFound
+    end
   end
 
   before do
@@ -82,6 +88,24 @@ def unknown_organization
     end
   end
 
+  describe "rescue_from ActiveRecord::RecordNotFound" do
+    before do
+      routes.draw { get :missing_record, to: "anonymous#missing_record" }
+    end
+
+    it "renders the static 404 page for HTML requests" do
+      get :missing_record
+      expect(response).to have_http_status(:not_found)
+      expect(response.body).to eq(File.read(Rails.public_path.join("404.html")))
+    end
+
+    it "renders a JSON error for JSON requests" do
+      get :missing_record, format: :json
+      expect(response).to have_http_status(:not_found)
+      expect(response.parsed_body).to eq("error" => "Record not found")
+    end
+  end
+
   describe "After signin path" do
     it "is equal to initial path" do
       routes.draw { get :index, to: "anonymous#index" }

spec/controllers/emancipations_controller_spec.rb

@@ -30,10 +30,9 @@
     end
 
     context "when case does not exist" do
-      it "raises a record not found error" do
-        expect {
-          get :show, params: {casa_case_id: "nonexistent-case"}
-        }.to raise_error(ActiveRecord::RecordNotFound)
+      it "responds with 404" do
+        get :show, params: {casa_case_id: "nonexistent-case"}
+        expect(response).to have_http_status(:not_found)
       end
     end
 

spec/requests/bulk_court_dates_spec.rb

@@ -56,8 +56,9 @@
     context "when different casa org's case group" do
       let(:case_group) { create :case_group, case_count:, casa_org: build(:casa_org) }
 
-      it "raises ActiveRecord::RecordNotFound" do
-        expect { subject }.to raise_error(ActiveRecord::RecordNotFound)
+      it "responds with 404" do
+        subject
+        expect(response).to have_http_status(:not_found)
       end
     end
 

spec/requests/case_contacts/followups_spec.rb

@@ -52,8 +52,9 @@
     end
 
     context "with invalid case_contact" do
-      it "raises ActiveRecord::RecordNotFound" do
-        expect { post case_contact_followups_path(444444) }.to raise_error(ActiveRecord::RecordNotFound)
+      it "responds with 404" do
+        post case_contact_followups_path(444444)
+        expect(response).to have_http_status(:not_found)
       end
     end
   end
@@ -116,8 +117,9 @@
     end
 
     context "followup doesn't exists" do
-      it "raises ActiveRecord::RecordNotFound" do
-        expect { patch resolve_followup_path(444444) }.to raise_error(ActiveRecord::RecordNotFound)
+      it "responds with 404" do
+        patch resolve_followup_path(444444)
+        expect(response).to have_http_status(:not_found)
       end
     end
   end

spec/requests/users_spec.rb

@@ -508,8 +508,9 @@
         }
       end
 
-      it "raises error when Language do not exist" do
-        expect { delete remove_language_users_path(999) }.to raise_error(ActiveRecord::RecordNotFound)
+      it "responds with 404 when Language does not exist" do
+        delete remove_language_users_path(999)
+        expect(response).to have_http_status(:not_found)
       end
     end
   end