optimize get_all_pertinent_contexts, especially for /calendar

By scoping the finds to the user, we can avoid a lot of grants_right
lookups and the associated db queries. This is especially helpful for
users who belong to a lot of groups or courses.

Change-Id: I31e22ecddf9456734d6c1297d776a8353e43a526
Reviewed-on: https://gerrit.instructure.com/2486
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: Zach Wily <zach@instructure.com>

app/controllers/application_controller.rb

@@ -244,45 +244,48 @@ class ApplicationController < ActionController::Base
     end
   end
   
-  # This is used by both /calendar and /assignments.
-  # Retrieves a list of all contexts associated with
-  # the given context.  If the context is a user then 
-  # it will include all the user's current contexts.
-  # Assigns it to the variable @context
-  def get_all_pertinent_contexts(include_groups=false)
-    unless @already_ran_get_all_pertinent_contexts
-      @already_ran_get_all_pertinent_contexts = true
-      @contexts = [@context].compact
-      @included_contexts = []
-      if params[:only_contexts]
-        params[:only_contexts].split(",").each do |include_context|
-          context = Context.find_by_asset_string(include_context)
-          @included_contexts << context if context
-          @contexts << context if context
-        end
-      else
-        if (@context && @context.is_a?( User))
-          @contexts.concat @context.courses
-          @contexts.concat @context.groups if include_groups
-        end
-        if params[:include_contexts]
-          params[:include_contexts].split(",").each do |include_context|
-            context = Context.find_by_asset_string(include_context)
-            @included_contexts << context if context
-            @contexts << context if context
-          end
-        end
+  # This is used by a number of actions to retrieve a list of all contexts
+  # associated with the given context.  If the context is a user then it will
+  # include all the user's current contexts.
+  # Assigns it to the variable @contexts
+  def get_all_pertinent_contexts(include_groups = false)
+    return if @already_ran_get_all_pertinent_contexts
+    @already_ran_get_all_pertinent_contexts = true
+
+    raise(ArgumentError, "Need a starting context") if @context.nil?
+
+    @contexts = [@context]
+    only_contexts = ActiveRecord::Base.parse_asset_string_list(params[:only_contexts])
+    if @context && @context.is_a?(User)
+      # we already know the user can read these courses and groups, so skip
+      # the grants_right? check to avoid querying for the various memberships
+      # again.
+      courses = @context.courses.active
+      groups = include_groups ? @context.groups.active : []
+      if only_contexts.present?
+        # find only those courses and groups passed in the only_contexts
+        # parameter, but still scoped by user so we know they have rights to
+        # view them.
+        courses = courses.find_all_by_id(only_contexts.select { |c| c.first == "Course" }.map(&:last))
+        groups = groups.find_all_by_id(only_contexts.select { |c| c.first == "Group" }.map(&:last)) if include_groups
       end
-      @contexts = @contexts.uniq.select{|c| c == @current_user || c.grants_rights?(@current_user, nil, nil)[:read] }
-      Course.require_assignment_groups(@contexts)
-      @original_context = @context
-      @context = ((@contexts & @included_contexts).first rescue @context) if @included_contexts && !@included_contexts.empty?
-      @context ||= @contexts.first
-      @context_enrollment = @context.membership_for_user(@current_user) rescue nil
-      @context_membership = @context_enrollment
+      @contexts.concat courses
+      @contexts.concat groups
     end
+    if params[:include_contexts]
+      params[:include_contexts].split(",").each do |include_context|
+        # don't load it again if we've already got it
+        next if @contexts.any? { |c| c.asset_string == include_context }
+        context = Context.find_by_asset_string(include_context)
+        @contexts << context if context && context.grants_right?(@current_user, nil, :read)
+      end
+    end
+    @contexts = @contexts.uniq
+    Course.require_assignment_groups(@contexts)
+    @context_enrollment = @context.membership_for_user(@current_user) if @context.respond_to?(:membership_for_user)
+    @context_membership = @context_enrollment
   end
-  
+
   # Retrieves all assignments for all contexts held in the @contexts variable.
   # Also retrieves submissions and sorts the assignments based on
   # their due dates and submission status for the given user.

app/controllers/assignments_controller.rb

@@ -23,28 +23,30 @@ class AssignmentsController < ApplicationController
   before_filter { |c| c.active_tab = "assignments" }
   
   def index
-    get_all_pertinent_contexts
-    get_sorted_assignments
-    add_crumb("Assignments", (@just_viewing_one_course ? named_context_url(@context, :context_assignments_url) : "/assignments" ))
-    @context= (@just_viewing_one_course ? @context : @current_user)
-    return if @just_viewing_one_course && !tab_enabled?(@context.class::TAB_ASSIGNMENTS)
+    if @context == @current_user || authorized_action(@context, @current_user, :read)
+      get_all_pertinent_contexts
+      get_sorted_assignments
+      add_crumb("Assignments", (@just_viewing_one_course ? named_context_url(@context, :context_assignments_url) : "/assignments" ))
+      @context= (@just_viewing_one_course ? @context : @current_user)
+      return if @just_viewing_one_course && !tab_enabled?(@context.class::TAB_ASSIGNMENTS)
 
-    respond_to do |format|
-      if @contexts.empty?
-        if @context
-          format.html { redirect_to @context == @current_user ? dashboard_url : named_context_url(@context, :context_url) }
+      respond_to do |format|
+        if @contexts.empty?
+          if @context
+            format.html { redirect_to @context == @current_user ? dashboard_url : named_context_url(@context, :context_url) }
+          else
+            format.html { redirect_to root_url }
+          end
+        elsif @just_viewing_one_course && @context.assignments.new.grants_right?(@current_user, session, :update)
+          format.html
         else
-          format.html { redirect_to root_url }
+          @current_user_submissions ||= @current_user && @current_user.submissions.scoped(:select => 'id, assignment_id, score, workflow_state', :conditions => {:assignment_id => @upcoming_assignments.map(&:id)}) 
+          format.html { render :action => "student_index" }
         end
-      elsif @just_viewing_one_course && @context.assignments.new.grants_right?(@current_user, session, :update)
-        format.html
-      else
-        @current_user_submissions ||= @current_user && @current_user.submissions.scoped(:select => 'id, assignment_id, score, workflow_state', :conditions => {:assignment_id => @upcoming_assignments.map(&:id)}) 
-        format.html { render :action => "student_index" }
+        format.xml  { render :xml => @assignments.to_xml }
+        # TODO: eager load the rubric associations
+        format.json { render :json => @assignments.to_json(:include => [ :rubric_association, :rubric ]) }
       end
-      format.xml  { render :xml => @assignments.to_xml }
-      # TODO: eager load the rubric associations
-      format.json { render :json => @assignments.to_json(:include => [ :rubric_association, :rubric ]) }
     end
   end
   

app/controllers/calendars_controller.rb

@@ -20,6 +20,13 @@ class CalendarsController < ApplicationController
 
   def show
     get_context
+    if @context != @current_user
+      # we used to have calendar pages under contexts, like
+      # /courses/X/calendar, but these all redirect to /calendar now.
+      # we shouldn't have any of these URLs anymore, but let's leave in this
+      # fail-safe in case somebody has a bookmark or something.
+      return redirect_to(calendar_url_for([@context]))
+    end
     get_all_pertinent_contexts(true) # passing true has it return groups too.
     build_calendar_dates
     
@@ -28,23 +35,11 @@ class CalendarsController < ApplicationController
         @events = []
         @undated_events = []
         @show_left_side = false
-        # what is this doing?
-        if @contexts.empty? || (@original_context && @original_context != @current_user) #authorized_action(@context, @current_user, :read)
-          if @context == @current_user
-            redirect_to dashboard_url
-            return
-          else
-            @included_contexts << @original_context
-            redirect_to calendar_url_for(@included_contexts.uniq)
-            return
-          end
-        else
-          @calendar_event = @contexts[0].calendar_events.new
-          @contexts.each do |context|
-            log_asset_access("dashboard_calendar:#{context.asset_string}", "calendar", 'other')
-          end
+        @calendar_event = @contexts[0].calendar_events.new
+        @contexts.each do |context|
+          log_asset_access("dashboard_calendar:#{context.asset_string}", "calendar", 'other')
         end
-        render :action => "show" 
+        render :action => "show"
       end
       # this  unless @dont_render_again stuff is ugly but I wanted to send back a 304 but it started giving me "Double Render errors"
       format.json do

config/initializers/active_record.rb

@@ -29,7 +29,16 @@ class ActiveRecord::Base
     id = code.pop
     code.join("_").classify.constantize.find(id) rescue nil
   end
-  
+
+  # takes an asset string list, like "course_5,user_7" and turns it into an
+  # array of [class_name, id] like [ ["Course", 5], ["User", 7] ]
+  def self.parse_asset_string_list(asset_string_list)
+    asset_string_list.to_s.split(",").map do |str|
+      code = str.split("_", 2)
+      [code.first.classify, code.last.to_i]
+    end
+  end
+
   def self.initialize_by_asset_string(string, asset_types)
     code = string.split("_")
     id = code.pop

spec/controllers/assignments_controller_spec.rb

@@ -41,7 +41,7 @@ describe AssignmentsController do
     it "should return unauthorized without a valid session" do
       course_with_student(:active_all => true)
       get 'index', :course_id => @course.id
-      response.should be_redirect
+      assert_status(401)
     end
     
     it "should redirect 'disabled', if disabled by the teacher" do

spec/controllers/calendars_controller_spec.rb

@@ -29,7 +29,7 @@ describe CalendarsController do
       course_with_student(:active_all => true)
       course_event
       get 'show', :course_id => @course.id
-      assigns[:contexts].should be_empty
+      assigns[:contexts].should be_blank
       response.should be_redirect
     end