From b1fe4b85e787e2a2b74a4d8f3b11040a2b823486 Mon Sep 17 00:00:00 2001
From: kevinrobinson <kevin.robinson.0@gmail.com>
Date: Wed, 25 Sep 2019 16:20:47 -0400
Subject: [PATCH 1/2] Cleanup: Add notes about validations across models

---
 app/config_objects/per_district.rb | 10 ++++++++++
 app/models/ed_plan.rb              |  4 ++++
 app/models/student.rb              |  2 --
 lib/tasks/validations.rake         | 29 +++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/app/config_objects/per_district.rb b/app/config_objects/per_district.rb
index 83e2b7f83f..39a142c54f 100644
--- a/app/config_objects/per_district.rb
+++ b/app/config_objects/per_district.rb
@@ -62,6 +62,16 @@ def try_star_filename(key, fallback = nil)
   # the ed plan).
   def patched_plan_504(student)
     if @district_key == SOMERVILLE
+      # When run across a `Student` collection, this approach trigers n+1 queries
+      # even if `ed_plans` was eagerly loaded and there are no records.
+      #
+      # To avoid this, we could filter in memory, but that would trigger n+1 when
+      # not eagerly loaded.
+      #
+      #  eg: student.ed_plans.select(&:active?).size > 0 ? '504' : nil
+      # 
+      # Because this method is called from a validation, it's hard for callers
+      # to control this, and changing may require a new PerfTest.
       student.ed_plans.active.size > 0 ? '504' : nil
     else
       student.plan_504(force: true)
diff --git a/app/models/ed_plan.rb b/app/models/ed_plan.rb
index eed2fb12f7..a075060d0a 100644
--- a/app/models/ed_plan.rb
+++ b/app/models/ed_plan.rb
@@ -11,6 +11,10 @@ def self.active
     where(sep_status: SEP_STATUS_MAP[:active])
   end
 
+  def active?
+    sep_status == SEP_STATUS_MAP[:active]
+  end
+
   def specific_disability
     sep_fieldd_006
   end
diff --git a/app/models/student.rb b/app/models/student.rb
index 38b603f8a8..0a2d2af102 100644
--- a/app/models/student.rb
+++ b/app/models/student.rb
@@ -55,9 +55,7 @@ class Student < ApplicationRecord
   validates :sped_placement, exclusion: { in: ['']}
   validates :disability, exclusion: { in: ['']}
   validates :sped_level_of_need, exclusion: { in: ['']}
-  validates :plan_504, exclusion: { in: ['']}
   validates :student_address, exclusion: { in: ['']}
-  validates :free_reduced_lunch, exclusion: { in: ['']}
   validates :race, exclusion: { in: ['']}
   validates :hispanic_latino, exclusion: { in: ['']}
   validates :gender, exclusion: { in: ['']}
diff --git a/lib/tasks/validations.rake b/lib/tasks/validations.rake
index b9a20266b1..6551b974bb 100644
--- a/lib/tasks/validations.rake
+++ b/lib/tasks/validations.rake
@@ -1,6 +1,35 @@
 namespace :validations do
   desc 'Check all validations manually across all models and records in the database'
   task run_all_manually: :environment do
+    # This still has lots of unnecessary queries, but could be optimized
+    # further to run more frequently.  FOr the `Educator` model the `plan_504`
+    # path is complicated and triggers queries, and for others uniqueness
+    # validations trigger a query for every validation check, even if the
+    # whole table is eagerly loaded.  Those could be removed and moved into
+    # db uniqueness constraints (which are stricter anyway).
+    def optimized_check_for_sections!
+      model_classes = {
+        Educator => [],
+        Student => [:school, :ed_plans],
+        Course => [],
+        Section => [],
+        EducatorSectionAssignment => [],
+        StudentSectionAssignment => []
+      }
+
+      out = {}
+      model_classes.each do |model_class, includes|
+        puts "Checking #{model_class.name}..."
+        relation = includes.size == 0 ? model_class.all : model_class.includes(*includes).all
+        records = relation.to_a # forces eager loading
+        invalids = records.select {|record, index| !record.valid? }
+        puts "  invalids.size: #{invalids.size} / #{records.size}"
+        out[model_class.name] = invalids
+      end
+      out
+    end
+
+
     def invalid_ids_for_class(model_class)
       invalid_ids = []
       counter = 0

From 71aff1607111c47debba8b98bb053a312c03c6a3 Mon Sep 17 00:00:00 2001
From: kevinrobinson <kevin.robinson.0@gmail.com>
Date: Wed, 25 Sep 2019 16:21:03 -0400
Subject: [PATCH 2/2] Fix whitespace

---
 app/config_objects/per_district.rb | 2 +-
 lib/tasks/validations.rake         | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/config_objects/per_district.rb b/app/config_objects/per_district.rb
index 39a142c54f..bb7d8bf243 100644
--- a/app/config_objects/per_district.rb
+++ b/app/config_objects/per_district.rb
@@ -69,7 +69,7 @@ def patched_plan_504(student)
       # not eagerly loaded.
       #
       #  eg: student.ed_plans.select(&:active?).size > 0 ? '504' : nil
-      # 
+      #
       # Because this method is called from a validation, it's hard for callers
       # to control this, and changing may require a new PerfTest.
       student.ed_plans.active.size > 0 ? '504' : nil
diff --git a/lib/tasks/validations.rake b/lib/tasks/validations.rake
index 6551b974bb..2f5d6fba83 100644
--- a/lib/tasks/validations.rake
+++ b/lib/tasks/validations.rake
@@ -29,7 +29,6 @@ namespace :validations do
       out
     end
 
-
     def invalid_ids_for_class(model_class)
       invalid_ids = []
       counter = 0