performance improvements for enrollments and state_based_on_date

test plan:
* regression test enrollment dates and
 the course/account settings to restrict students
 to from viewing past and future courses

closes #CNVS-21602

Change-Id: Ifc2fd8f6fd279839bd8ba520442e2c0b9cc91da6
Reviewed-on: https://gerrit.instructure.com/57535
Reviewed-by: Cody Cutrer <cody@instructure.com>
Tested-by: Jenkins
QA-Review: Clare Strong <clare@instructure.com>
Product-Review: James Williams  <jamesw@instructure.com>

app/controllers/application_controller.rb

@@ -34,6 +34,7 @@ class ApplicationController < ActionController::Base
   include Api::V1::WikiPage
   include LegalInformationHelper
   around_filter :set_locale
+  around_filter :enable_request_cache
 
   helper :all
 
@@ -255,6 +256,12 @@ class ApplicationController < ActionController::Base
     I18n.localizer = nil
   end
 
+  def enable_request_cache
+    RequestCache.enable do
+      yield
+    end
+  end
+
   def store_session_locale
     return unless locale = params[:session_locale]
     supported_locales = I18n.available_locales.map(&:to_s)

app/models/account.rb

@@ -123,8 +123,8 @@ class Account < ActiveRecord::Base
   before_save :ensure_defaults
   after_save :update_account_associations_if_changed
 
-  before_save :setup_quota_cache_invalidation
-  after_save :invalidate_quota_caches_if_changed
+  before_save :setup_cache_invalidation
+  after_save :invalidate_caches_if_changed
 
   after_create :default_enrollment_term
 
@@ -230,6 +230,7 @@ class Account < ActiveRecord::Base
   BRANDING_SETTINGS.each { |setting| add_setting(setting, root_only: true) }
 
   def settings=(hash)
+    @invalidate_settings_cache = true
     if hash.is_a?(Hash)
       hash.each do |key, val|
         if account_settings_options && account_settings_options[key.to_sym]
@@ -524,29 +525,38 @@ class Account < ActiveRecord::Base
     nil
   end
 
-  def setup_quota_cache_invalidation
-    @quota_invalidations = []
+  def setup_cache_invalidation
+    @invalidations = []
     unless self.new_record?
-      @quota_invalidations += ['default_storage_quota', 'current_quota'] if self.try_rescue(:default_storage_quota_changed?)
-      @quota_invalidations << 'default_group_storage_quota' if self.try_rescue(:default_group_storage_quota_changed?)
+      @invalidations += ['default_storage_quota', 'current_quota'] if self.try_rescue(:default_storage_quota_changed?)
+      @invalidations << 'default_group_storage_quota' if self.try_rescue(:default_group_storage_quota_changed?)
     end
   end
 
-  def invalidate_quota_caches_if_changed
-    Account.send_later_if_production(:invalidate_quota_caches, self.id, @quota_invalidations) if @quota_invalidations.present?
+  def invalidate_caches_if_changed
+    @invalidations ||= []
+    @invalidations += Account.inheritable_settings if @invalidate_settings_cache
+    if @invalidations.present?
+      @invalidations.each do |key|
+        Rails.cache.delete([key, self.global_id].cache_key)
+      end
+      Account.send_later_if_production(:invalidate_inherited_caches, self, @invalidations)
+    end
   end
 
-  def self.invalidate_quota_caches(account_id, keys)
-    account_ids = Account.sub_account_ids_recursive(account_id) + [account_id]
-    keys.each do |quota_key|
+  def self.invalidate_inherited_caches(parent_account, keys)
+    parent_account.shard.activate do
+      account_ids = Account.sub_account_ids_recursive(parent_account.id).map{|id| Shard.global_id_for(id)}
       account_ids.each do |id|
-        Rails.cache.delete([quota_key, id].cache_key)
+        keys.each do |key|
+          Rails.cache.delete([key, id].cache_key)
+        end
       end
     end
   end
 
   def quota
-    Rails.cache.fetch(['current_quota', self.id].cache_key) do
+    Rails.cache.fetch(['current_quota', self.global_id].cache_key) do
       read_attribute(:storage_quota) ||
         (self.parent_account.default_storage_quota rescue nil) ||
         Setting.get('account_default_quota', 500.megabytes.to_s).to_i
@@ -554,7 +564,7 @@ class Account < ActiveRecord::Base
   end
 
   def default_storage_quota
-    Rails.cache.fetch(['default_storage_quota', self.id].cache_key) do
+    Rails.cache.fetch(['default_storage_quota', self.global_id].cache_key) do
       read_attribute(:default_storage_quota) ||
         (self.parent_account.default_storage_quota rescue nil) ||
         Setting.get('account_default_quota', 500.megabytes.to_s).to_i
@@ -600,7 +610,7 @@ class Account < ActiveRecord::Base
   end
 
   def default_group_storage_quota
-    Rails.cache.fetch(['default_group_storage_quota', self.id].cache_key) do
+    Rails.cache.fetch(['default_group_storage_quota', self.global_id].cache_key) do
       read_attribute(:default_group_storage_quota) ||
         (self.parent_account.default_group_storage_quota rescue nil) ||
         Group.default_storage_quota

app/models/account/settings.rb

@@ -5,7 +5,7 @@ module Account::Settings
         opts[:hash] = true
         opts[:values] = [:value, :locked]
 
-        self.class_eval "def #{setting}; calculate_inherited_setting(:#{setting}); end"
+        self.class_eval "def #{setting}; cached_inherited_setting(:#{setting}); end"
       elsif (opts && opts[:boolean] && opts.has_key?(:default))
         if opts[:default]
           # if the default is true, we want a nil result to evaluate to true.
@@ -19,6 +19,10 @@ module Account::Settings
       end
       self.account_settings_options[setting.to_sym] = opts || {}
     end
+
+    def inheritable_settings
+      self.account_settings_options.select{|k, v| v[:inheritable]}.keys
+    end
   end
 
   def self.included(klass)
@@ -27,6 +31,12 @@ module Account::Settings
     klass.account_settings_options ||= {}
   end
 
+  def cached_inherited_setting(setting)
+    Rails.cache.fetch([setting, self.global_id].cache_key) do
+      calculate_inherited_setting(setting)
+    end
+  end
+
   # should continue down the account chain until it reaches a locked value
   # otherwise use the last explicitly set value
   def calculate_inherited_setting(setting)

app/models/course.rb

@@ -2620,7 +2620,9 @@ class Course < ActiveRecord::Base
     class_eval <<-CODE
       def #{setting}
         if Course.settings_options[#{setting.inspect}][:inherited]
-          inherited = self.account.send(#{setting.inspect})
+          inherited = RequestCache.cache('inherited_course_setting', #{setting.inspect}, self.global_account_id) do
+            self.account.send(#{setting.inspect})
+          end
           if inherited[:locked] || settings_frd[#{setting.inspect}].nil?
             inherited[:value]
           else

app/models/enrollment.rb

@@ -619,6 +619,12 @@ class Enrollment < ActiveRecord::Base
   end
 
   def state_based_on_date
+    RequestCache.cache('enrollment_state_based_on_date', self) do
+      calculated_state_based_on_date
+    end
+  end
+
+  def calculated_state_based_on_date
     if state == :completed || ([:invited, :active].include?(state) && self.course.completed?)
       if self.restrict_past_view?
         return :inactive
@@ -656,11 +662,15 @@ class Enrollment < ActiveRecord::Base
   end
 
   def restrict_past_view?
-    self.view_restrictable? && self.course.restrict_student_past_view?
+    self.view_restrictable? && RequestCache.cache('restrict_student_past_view', self.global_course_id) do
+      self.course.restrict_student_past_view?
+    end
   end
 
   def restrict_future_view?
-    self.view_restrictable? && self.course.restrict_student_future_view?
+    self.view_restrictable? && RequestCache.cache('restrict_student_future_view', self.global_course_id) do
+      self.course.restrict_student_future_view?
+    end
   end
 
   def active?

app/models/user.rb

@@ -1708,27 +1708,31 @@ class User < ActiveRecord::Base
 
   # this method takes an optional {:include_enrollment_uuid => uuid}   so that you can pass it the session[:enrollment_uuid] and it will include it.
   def cached_current_enrollments(opts={})
-    enrollments = self.shard.activate do
-      res = Rails.cache.fetch([self, 'current_enrollments3', opts[:include_future], ApplicationController.region ].cache_key) do
-        scope = (opts[:include_future] ? self.enrollments.current_and_future : self.enrollments.current_and_invited)
-        scope.shard(in_region_associated_shards).to_a
+    RequestCache.cache('cached_current_enrollments', self, opts) do
+      enrollments = self.shard.activate do
+        res = Rails.cache.fetch([self, 'current_enrollments3', opts[:include_future], ApplicationController.region ].cache_key) do
+          scope = (opts[:include_future] ? self.enrollments.current_and_future : self.enrollments.current_and_invited)
+          scope.shard(in_region_associated_shards).to_a
+        end
+        if opts[:include_enrollment_uuid] && !res.find { |e| e.uuid == opts[:include_enrollment_uuid] } &&
+            (pending_enrollment = Enrollment.where(uuid: opts[:include_enrollment_uuid], workflow_state: "invited").first)
+          res << pending_enrollment
+        end
+        res
+      end + temporary_invitations
+      if opts[:preload_courses]
+        ActiveRecord::Associations::Preloader.new(enrollments, :course).run
       end
-      if opts[:include_enrollment_uuid] && !res.find { |e| e.uuid == opts[:include_enrollment_uuid] } &&
-          (pending_enrollment = Enrollment.where(uuid: opts[:include_enrollment_uuid], workflow_state: "invited").first)
-        res << pending_enrollment
-      end
-      res
-    end + temporary_invitations
-    if opts[:preload_courses]
-      ActiveRecord::Associations::Preloader.new(enrollments, :course).run
+      enrollments
     end
-    enrollments
   end
 
   def cached_not_ended_enrollments
-    self.shard.activate do
-      @cached_all_enrollments = Rails.cache.fetch([self, 'not_ended_enrollments2'].cache_key) do
-        self.not_ended_enrollments.to_a
+    RequestCache.cache("not_ended_enrollments", self) do
+      self.shard.activate do
+        Rails.cache.fetch([self, 'not_ended_enrollments2'].cache_key) do
+          self.not_ended_enrollments.to_a
+        end
       end
     end
   end
@@ -1738,8 +1742,8 @@ class User < ActiveRecord::Base
   end
 
   def cached_current_group_memberships
-    self.shard.activate do
-      @cached_current_group_memberships = Rails.cache.fetch(group_membership_key) do
+    @cached_current_group_memberships ||= self.shard.activate do
+      Rails.cache.fetch(group_membership_key) do
         self.current_group_memberships.shard(self.in_region_associated_shards).to_a
       end
     end

lib/canvas/builders/enrollment_date_builder.rb

@@ -45,7 +45,9 @@ class EnrollmentDateBuilder
   end
 
   def self.fetch(enrollment)
-    result = Rails.cache.fetch(cache_key(enrollment))
+    result = RequestCache.cache('enrollment_dates', enrollment) do
+      Rails.cache.read(cache_key(enrollment))
+    end
     enrollment.instance_variable_set(:@enrollment_dates, result)
   end
 
@@ -72,6 +74,10 @@ class EnrollmentDateBuilder
       @enrollment_dates << default_dates
     end
 
+    RequestCache.cache('enrollment_dates', self) do
+      @enrollment_dates
+    end
+
     Rails.cache.write(cache_key, @enrollment_dates)
     @enrollment.instance_variable_set(:@enrollment_dates, @enrollment_dates)
   end

lib/request_cache.rb

@@ -0,0 +1,3 @@
+# try to use for values that are small and likely to be re-used often within the lifetime of a request
+class RequestCache < TempCache
+end

lib/temp_cache.rb

@@ -5,6 +5,7 @@ class TempCache
     clear
     @enabled = true
     yield
+  ensure
     @enabled = false
     clear
   end

spec/models/account_spec.rb

@@ -1266,7 +1266,7 @@ describe Account do
     it "should only clear the quota cache if something changes" do
       account = account_model
 
-      Account.expects(:invalidate_quota_caches).once
+      Account.expects(:invalidate_inherited_caches).once
 
       account.default_storage_quota = 10.megabytes
       account.save! # clear here