Merge pull request #6158 from rubyforgood/case-contacts-controller-n1

Improve speed of CaseContactsController

Author: compwron

.gitignore

@@ -61,3 +61,4 @@ npm-debug.log
 
 # In case we want to keep something in the compiled assets directory
 !/app/assets/builds/.keep
+.aider*

.standard_todo.yml

@@ -675,6 +675,8 @@ ignore:
   - RSpec/NamedSubject
 - spec/policies/learning_hour_policy_spec.rb:
   - RSpec/NamedSubject
+- spec/policies/nil_class_policy_spec.rb:
+  - RSpec/NamedSubject
 - spec/policies/notification_policy_spec.rb:
   - RSpec/NamedSubject
 - spec/policies/other_duty_policy_spec.rb:
@@ -804,6 +806,7 @@ ignore:
 - spec/requests/case_contacts_spec.rb:
   - RSpec/MultipleMemoizedHelpers
   - RSpec/MultipleExpectations
+  - RSpec/PendingWithoutReason
 - spec/requests/case_court_reports_spec.rb:
   - RSpec/ContextWording
   - RSpec/NestedGroups

app/controllers/case_contacts_controller.rb

@@ -107,7 +107,8 @@ def all_case_contacts
       :creator,
       :followups,
       :contact_types,
-      contact_topic_answers: [:contact_topic]
+      contact_topic_answers: [:contact_topic],
+      casa_case: [:volunteers]
     )
   end
 
@@ -116,25 +117,16 @@ def additional_expense_params
   end
 
   def set_case_contact
-    if current_organization.case_contacts.exists?(params[:id])
-      @case_contact = authorize(current_organization.case_contacts.find(params[:id]))
-    else
-      redirect_to authenticated_user_root_path
-    end
+    @case_contact = authorize(current_organization.case_contacts.find_by(id: params[:id]))
+    redirect_to authenticated_user_root_path unless @case_contact
   end
 
   def build_draft_case_ids(params, casa_cases)
     # Use case(s) from params if present
-    if params[:draft_case_ids].present?
-      params[:draft_case_ids]
-    elsif params.dig(:case_contact, :casa_case_id).present?
-      casa_cases.where(id: params.dig(:case_contact, :casa_case_id)).pluck(:id)
-    elsif casa_cases.count == 1
-      # If there is only one case for user, select that case
-      [casa_cases.first.id]
-    else
-      # Otherwise, let user select cases
-      []
-    end
+    return params[:draft_case_ids] if params[:draft_case_ids].present?
+    return casa_cases.where(id: params.dig(:case_contact, :casa_case_id)).pluck(:id) if params.dig(:case_contact, :casa_case_id).present?
+    return [casa_cases.first.id] if casa_cases.count == 1
+
+    []
   end
 end

app/policies/nil_class_policy.rb

@@ -0,0 +1,9 @@
+class NilClassPolicy < ApplicationPolicy
+  def method_missing(*)
+    false
+  end
+
+  def respond_to_missing?(*)
+    true
+  end
+end

app/presenters/case_contact_presenter.rb

@@ -25,9 +25,6 @@ def boolean_select_options
   private
 
   def org_cases
-    CasaOrg.includes(:casa_cases)
-      .references(:casa_cases)
-      .find_by(id: current_user.casa_org_id)
-      .casa_cases
+    CasaOrg.find(current_user.casa_org_id).casa_cases.active
   end
 end

lib/tasks/check_controller_tests.rake

@@ -0,0 +1,12 @@
+namespace :test do
+  desc "Check for controller tests in spec/controllers and fail if any are found"
+  task check_controller_tests: :environment do
+    controller_tests = Dir.glob("spec/controllers/**/*_spec.rb")
+    if controller_tests.any?
+      puts "controller tests should be in spec/requests"
+      exit 1
+    else
+      puts "No controller tests found in spec/controllers"
+    end
+  end
+end

spec/controllers/README.md

@@ -0,0 +1 @@
+Tests for controllers should be in spec/requests

spec/policies/nil_class_policy_spec.rb

@@ -0,0 +1,33 @@
+require "rails_helper"
+
+RSpec.describe NilClassPolicy do
+  subject { described_class.new(nil, nil) }
+
+  it "doesn't permit index action" do
+    expect(subject).not_to be_index
+  end
+
+  it "doesn't permit show action" do
+    expect(subject).not_to be_show
+  end
+
+  it "doesn't permit create action" do
+    expect(subject).not_to be_create
+  end
+
+  it "doesn't permit new action" do
+    expect(subject).not_to be_new
+  end
+
+  it "doesn't permit update action" do
+    expect(subject).not_to be_update
+  end
+
+  it "doesn't permit edit action" do
+    expect(subject).not_to be_edit
+  end
+
+  it "doesn't permit destroy action" do
+    expect(subject).not_to be_destroy
+  end
+end

spec/requests/case_contacts_spec.rb

@@ -145,4 +145,19 @@
       expect { request }.to change { case_contact.reload.deleted? }.from(true).to(false)
     end
   end
+
+  xdescribe "GET /leave" do
+    subject(:request) do
+      get leave_case_contact_path
+
+      response
+    end
+
+    it { is_expected.to redirect_to(case_contacts_path) }
+
+    it "redirects back to referer or fallback location" do
+      request
+      expect(response).to redirect_to(case_contacts_path)
+    end
+  end
 end