Merge pull request #2634 from studentinsights/patch/notes-about-valid…

…ations

Cleanup: Add notes about validations across models

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)

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

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: ['']}

lib/tasks/validations.rake

@@ -1,6 +1,34 @@
 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