remove N+1 queries on term index page

This will make the page a little faster, but unfortunately the bulk of
the time comes from counting active users in each term, and whether we
do that as N queries or 1, the overall time is about the same.  To
really get this page to perform well, we need to look into storing these
counts somewhere that don't have to be calculated every time.

fixes CNVS-21524

test plan:
- set up several enrollment terms with different numbers of courses and
  users in each.
- to go the root account enrollment terms page, and make sure the counts
  of active courses and users is accurate.
- try deleting some of the courses and users and make sure counts update
  appropriately.
- basic regression test of functionality around saving/editing
  enrollment term start/end date overrides

Change-Id: I35fb637ddd91db64c4a5890d58a622f5affa3923
Reviewed-on: https://gerrit.instructure.com/56439
Tested-by: Jenkins
Reviewed-by: Cody Cutrer <cody@instructure.com>
QA-Review: August Thornton <august@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>

app/controllers/terms_controller.rb

@@ -24,7 +24,11 @@ class TermsController < ApplicationController
   def index
     @root_account = @context.root_account
     @context.default_enrollment_term
-    @terms = @context.enrollment_terms.active.preload(:enrollment_dates_overrides).order("COALESCE(start_at, created_at) DESC").to_a
+    @terms = @context.enrollment_terms.active.
+      preload(:enrollment_dates_overrides).
+      order("COALESCE(start_at, created_at) DESC").to_a
+    @course_counts_by_term = EnrollmentTerm.course_counts(@terms)
+    @user_counts_by_term = EnrollmentTerm.user_counts(@root_account, @terms)
   end
   
   # @API Create enrollment term

app/models/enrollment_term.rb

@@ -125,10 +125,20 @@ class EnrollmentTerm < ActiveRecord::Base
     false
   end
   
-  def users_count
-    scope = Enrollment.active.joins(:course).
-      where(root_account_id: root_account_id, courses: {enrollment_term_id: self})
-    scope.select(:user_id).uniq.count
+  def self.user_counts(root_account, terms)
+    # Warning: returns keys as strings, I think because of the join
+    Enrollment.active.joins(:course).
+      where(root_account_id: root_account, courses: {enrollment_term_id: terms}).
+      group(:enrollment_term_id).
+      uniq.
+      count(:user_id)
+  end
+
+  def self.course_counts(terms)
+    Course.active.
+      where(enrollment_term_id: terms).
+      group(:enrollment_term_id).
+      count
   end
   
   workflow do

app/views/terms/_term.html.erb

@@ -5,13 +5,13 @@
       <span class="name"><%= term.name rescue nbsp %></span>
     </div>
     <div class="show_term" style="font-size: 0.8em;">
-      <% if term && (term.sis_source_id && can_do(term.root_account, @current_user, :read_sis) || can_do(term.root_account, @current_user, :manage_sis)) %>
+      <% if term && (term.sis_source_id && can_do(@root_account, @current_user, :read_sis) || can_do(@root_account, @current_user, :manage_sis)) %>
         <div style="font-size: 0.8em;">
           <%= before_label :sis_id, "SIS ID" %> <span class="sis_source_id"><%= term.sis_source_id %></span>
         </div>
       <% end %>
-      <%= term && t(:course_count, "Course", :count => term.courses.active.count) %><br/>
-      <%= term && t(:user_count, "User", :count => term.users_count) %>
+      <%= term && t(:course_count, "Course", :count => @course_counts_by_term[term.id] || 0) %><br/>
+      <%= term && t(:user_count, "User", :count => @user_counts_by_term[term.id.to_s] || 0) %>
     </div>
     <%= form_for (term || EnrollmentTerm.new), :url => (term ? context_url(@context, :context_term_url, term.id) : context_url(@context, :context_terms_url)), :html => {:class => "enrollment_term_form", :method => (term ? :put : :post)} do |f| %>
         <div class="control-group edit_term">
@@ -45,7 +45,11 @@
                                                         :end_tooltip => t('titles.term_ends', "Term ends"), :whenever => [true, true] } %>
       </tr>
     <% types.each do |type| %>
-      <% override = term.enrollment_dates_overrides.where(context_type: @context.class.to_s, context_id: @context, enrollment_type: type.to_s).first if term %>
+      <% override = term.enrollment_dates_overrides.detect{|edo|
+           edo.context_type == @context.class.to_s &&
+             edo.context_id == @context.id &&
+             edo.enrollment_type == type.to_s
+         } if term %>
       <% override ||= EnrollmentDatesOverride.new %>
       <% type_string = type.to_s.underscore %>
       <% start_whenever = true %>
@@ -83,7 +87,7 @@
   </td>
   <td class="links">
     <a href="#" class="edit_term_link no-hover"><i class="icon-edit standalone-icon"></i></a>
-    <% if term && term.courses.active.exists? %>
+    <% if term && @course_counts_by_term[term.id].to_i > 0 %>
       <a href="#" class="cant_delete_term_link no-hover"><i class="icon-end standalone-icon"></i></a>
     <% elsif term != @context.default_enrollment_term %>
       <a href="#" class="delete_term_link no-hover"><i class="icon-trash standalone-icon"></i></a>

spec/models/enrollment_term_spec.rb

@@ -105,4 +105,46 @@ describe EnrollmentTerm do
     end
   end
 
+  describe "counts" do
+    before(:once) do
+      @t1 = Account.default.enrollment_terms.create!
+      course_with_teacher(active_course: true, active_enrollment: true)
+      @course.enrollment_term_id = @t1.id
+      @course.save!
+
+      @t2 = Account.default.enrollment_terms.create!
+      course_with_teacher(active_course: true, active_enrollment: true)
+      @course.enrollment_term_id = @t2.id
+      @course.save!
+
+      @t3 = Account.default.enrollment_terms.create!
+      course_with_teacher(active_course: true, active_enrollment: true)
+      @course.enrollment_term_id = @t3.id
+      @course.save!
+    end
+
+    describe ".users_counts" do
+      it "returns user counts" do
+        student_in_course(active_all: true, course: @t1.courses.first)
+
+        counts = {}
+        counts[@t1.id.to_s] = 2
+        counts[@t2.id.to_s] = 1
+        expect(EnrollmentTerm.user_counts(Account.default, [@t1, @t2])).to eq counts
+      end
+    end
+
+    describe ".courses_counts" do
+      it "returns course counts" do
+        course_with_teacher(active_all: true)
+        @course.enrollment_term_id = @t1.id
+        @course.save!
+
+        counts = {}
+        counts[@t1.id] = 2
+        counts[@t2.id] = 1
+        expect(EnrollmentTerm.course_counts([@t1, @t2])).to eq counts
+      end
+    end
+  end
 end

spec/views/enrollment_terms/index.html.erb_spec.rb

@@ -29,7 +29,8 @@ describe "terms/_term.html.erb" do
       assigns[:context] = @account
       assigns[:account] = @account
       assigns[:root_account] = @account
-      
+      assigns[:course_counts_by_term] = EnrollmentTerm.course_counts([@term])
+      assigns[:user_counts_by_term] = EnrollmentTerm.user_counts(@account, [@term])
     end
 
     it "should show to sis admin" do
@@ -49,4 +50,4 @@ describe "terms/_term.html.erb" do
       expect(response).to have_tag("span.sis_source_id", @term.sis_source_id)
     end
   end
-end
+end

spec/views/terms/index.html.erb_spec.rb

@@ -25,6 +25,8 @@ describe "/terms/index" do
     term = Account.default.enrollment_terms.create!
     term.courses.create! { |c| c.workflow_state = 'deleted' }
     assigns[:terms] = Account.default.enrollment_terms.active.sort_by{|t| t.start_at || t.created_at }.reverse
+    assigns[:course_counts_by_term] = EnrollmentTerm.course_counts(assigns[:terms])
+    assigns[:user_counts_by_term] = EnrollmentTerm.user_counts(Account.default, assigns[:terms])
     render "terms/index"
     page = Nokogiri('<document>' + response.body + '</document>')
     expect(page.css(".delete_term_link")[0]['href']).to eq '#'