Don't blow up if checking effective due dates for a non-assigned user

Fixes CNVS-35631

Test plan:
* Create a course with an assignment and a student
* Create another course with another student
* In rails console:
  edd = EffectiveDueDates.for_course(course1)
  edd.in_closed_grading_period?(assignment, student_in_other_course)

This should return false, rather than crash.

Change-Id: I6facdf0643b2eabc557ff2fc651534d5572bfc0b
Reviewed-on: https://gerrit.instructure.com/104965
Reviewed-by: Keith T. Garner <kgarner@instructure.com>
Tested-by: Jenkins
Reviewed-by: Derek Bender <djbender@instructure.com>
QA-Review: Spencer Olson <solson@instructure.com>
Product-Review: Keith T. Garner <kgarner@instructure.com>

lib/effective_due_dates.rb

@@ -68,11 +68,11 @@ class EffectiveDueDates
     # no need to check this one specifically
     return false if @any_in_closed_grading_period == false
 
-    assignment_due_dates = to_hash[assignment_id]
+    assignment_due_dates = find_effective_due_dates_for_assignment(assignment_id)
     student_id = student_or_student_id.try(:id) || student_or_student_id
 
-    if usable_student_id?(student_id)
-      assignment_due_dates[student_id][:in_closed_grading_period]
+    if student_id.to_i > 0
+      !!assignment_due_dates.dig(student_id, :in_closed_grading_period)
     else
       any_student_in_closed_grading_period?(assignment_due_dates)
     end
@@ -92,13 +92,6 @@ class EffectiveDueDates
 
   private
 
-  def usable_student_id?(student_id)
-    return false unless student_id.present?
-    return false unless student_id.to_i.present?
-
-    true
-  end
-
   def any_student_in_closed_grading_period?(assignment_due_dates)
     return false unless assignment_due_dates
     assignment_due_dates.any? { |_, student| student[:in_closed_grading_period] }

lib/gradebook_importer.rb

@@ -123,7 +123,7 @@ class GradebookImporter
       .select(['submissions.id', :assignment_id, :user_id, :score, :excused, :cached_due_date, 'submissions.updated_at'])
       .where(assignment_id: assignment_ids, user_id: user_ids)
       .map do |submission|
-        is_gradeable = gradeable?(submission: submission, periods: periods, is_admin: is_admin)
+        is_gradeable = gradeable?(submission: submission, is_admin: is_admin)
         score = submission.excused? ? "EX" : submission.score.to_s
         {
           user_id: submission.user_id,
@@ -160,7 +160,6 @@ class GradebookImporter
             effective_due_dates.find_effective_due_date(student.id, assignment.id).fetch(:due_at, nil)
           submission['gradeable'] = gradeable?(
             submission: new_submission,
-            periods: periods,
             is_admin: is_admin
           )
         end
@@ -321,7 +320,7 @@ class GradebookImporter
   end
 
   def prevent_new_assignment_creation?(periods, is_admin)
-    return false unless context.grading_periods?
+    return false unless @context.grading_periods?
     return false if is_admin
 
     GradingPeriod.date_in_closed_grading_period?(
@@ -500,16 +499,10 @@ class GradebookImporter
 
   private
 
-  def gradeable?(submission:, periods:, is_admin:)
-    user_can_grade_submission = submission.grants_right?(@user, :grade)
-    return user_can_grade_submission unless @context.grading_periods?
-
-    user_can_grade_submission &&
-      (is_admin || !GradingPeriod.date_in_closed_grading_period?(
-        course: submission.context,
-        date: submission.cached_due_date,
-        periods: periods
-      ))
+  def gradeable?(submission:, is_admin: false)
+    # `submission#grants_right?` will check if the user
+    # is an admin, but if we've pre-loaded that value already
+    # to avoid an N+1, check that first.
+    is_admin || submission.grants_right?(@user, :grade)
   end
-
 end

spec/lib/effective_due_dates_spec.rb

@@ -1424,12 +1424,25 @@ describe Course do
           expect(@edd.in_closed_grading_period?(@assignment1)).to eq(false)
         end
 
+        it 'returns true if the specified student has a due date for this assignment' do
+          expect(@edd.in_closed_grading_period?(@assignment2, @student2)).to be true
+          expect(@edd.in_closed_grading_period?(@assignment2, @student2.id)).to be true
+        end
+
         it 'returns false if the specified student has a due date in an open grading period' do
           override = @assignment2.assignment_overrides.create!(due_at: 1.day.from_now(@now), due_at_overridden: true)
           override.assignment_override_students.create!(user: @student1)
 
-          expect(@edd.in_closed_grading_period?(@assignment2, @student1)).to be_falsey
-          expect(@edd.in_closed_grading_period?(@assignment2, @student1.id)).to be_falsey
+          expect(@edd.in_closed_grading_period?(@assignment2, @student1)).to be false
+          expect(@edd.in_closed_grading_period?(@assignment2, @student1.id)).to be false
+        end
+
+        it 'returns false if the specified student does not have a due date for this assignment' do
+          @other_course = Course.create!
+          @student_in_other_course = student_in_course(course: @other_course, active_all: true).user
+
+          expect(@edd.in_closed_grading_period?(@assignment2, @student_in_other_course)).to be false
+          expect(@edd.in_closed_grading_period?(@assignment2, @student_in_other_course.id)).to be false
         end
       end
     end