fix viewing grades for deactivated students

closes EVAL-2180
closes EVAL-2176
flag=none

Test Plan 1 (no grading period):
1. Give a student a few grades, and then (still as a teacher) go to the
   individual grades page for that student
   (/courses/:course_id/grades/:student_id). Notice that you can see
   their grades
2. Deactivate the student
3. Still as the teacher, go back to the individual grades page for that
   student. Notice that you can still see their grades.

Test Plan 2 (grading periods):
1. In a course using grading periods, give a student a few grades, and
   then (still as a teacher) go to the individual grades page for that
   student (/courses/:course_id/grades/:student_id). Select a grading
   period from the dropdown on the page, and then click "Apply". Notice
   that you can see their grades
2. Deactivate the student
3. Still as the teacher, go back to the individual grades page for that
   student. Select a grading period from the dropdown on the page, and
   then click "Apply". Notice that you can see their grades

Edge case testing:
- Make sure you can still see grades at that page for active and
  concluded stuents
- Students themselves should be able to see grades at that page if
  they are active or concluded (but not if they are deactivated)

Change-Id: I62421aae369a9a1257c212359d94d4df35f75f4f
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/282544
QA-Review: Eduardo Escobar <eduardo.escobar@instructure.com>
Product-Review: Jody Sailor
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Eduardo Escobar <eduardo.escobar@instructure.com>
Reviewed-by: Dustin Cowles <dustin.cowles@instructure.com>

app/controllers/gradebooks_controller.rb

@@ -124,7 +124,7 @@ class GradebooksController < ApplicationController
 
     grading_period = @grading_periods&.find { |period| period[:id] == gp_id }
 
-    ags_json = light_weight_ags_json(@presenter.groups, { student: @presenter.student })
+    ags_json = light_weight_ags_json(@presenter.groups)
     root_account = @context.root_account
 
     js_hash = {
@@ -180,31 +180,31 @@ class GradebooksController < ApplicationController
     end
   end
 
-  def light_weight_ags_json(assignment_groups, opts = {})
+  def light_weight_ags_json(assignment_groups)
-    assignment_groups.map do |ag|
+    assignments_by_group = @presenter.assignments.each_with_object({}) do |assignment, assignments|
-      visible_assignments = ag.visible_assignments(opts[:student] || @current_user).to_a
+      # Pseudo-assignment objects with a "special_class" set are created for
+      # assignment group totals, grading period totals, and course totals. We
+      # only care about real assignments here, so we'll ignore those
+      # pseudo-assignment objects.
+      next if assignment.special_class
 
-      if grading_periods? && @current_grading_period_id && !view_all_grading_periods?
+      assignments[assignment.assignment_group_id] ||= []
-        current_period = GradingPeriod.for(@context).find_by(id: @current_grading_period_id)
+      assignments[assignment.assignment_group_id] << {
-        visible_assignments = current_period.assignments_for_student(@context, visible_assignments, opts[:student])
+        id: assignment.id,
-      end
+        submission_types: assignment.submission_types_array,
-
+        points_possible: assignment.points_possible,
-      visible_assignments.map! do |a|
+        due_at: assignment.due_at,
-        {
+        omit_from_final_grade: assignment.omit_from_final_grade?,
-          id: a.id,
+        muted: assignment.muted?
-          submission_types: a.submission_types_array,
+      }
-          points_possible: a.points_possible,
+    end
-          due_at: a.due_at,
-          omit_from_final_grade: a.omit_from_final_grade?,
-          muted: a.muted?
-        }
-      end
 
+    assignment_groups.map do |group|
       {
-        id: ag.id,
+        id: group.id,
-        rules: ag.rules_hash({ stringify_json_ids: true }),
+        rules: group.rules_hash({ stringify_json_ids: true }),
-        group_weight: ag.group_weight,
+        group_weight: group.group_weight,
-        assignments: visible_assignments,
+        assignments: assignments_by_group.fetch(group.id, [])
       }
     end
   end

app/models/assignment.rb

@@ -128,6 +128,7 @@ class Assignment < ActiveRecord::Base
   has_many :conditional_release_rules, class_name: "ConditionalRelease::Rule", dependent: :destroy, foreign_key: "trigger_assignment_id", inverse_of: :trigger_assignment
   has_many :conditional_release_associations, class_name: "ConditionalRelease::AssignmentSetAssociation", dependent: :destroy, inverse_of: :assignment
 
+  scope :assigned_to_student, ->(student_id) { joins(:submissions).where(submissions: { user_id: student_id }) }
   scope :anonymous, -> { where(anonymous_grading: true) }
   scope :moderated, -> { where(moderated_grading: true) }
   scope :auditable, -> { anonymous.or(moderated) }

app/models/grading_period.rb

@@ -103,8 +103,8 @@ class GradingPeriod < ActiveRecord::Base
     grading_period_group.course_id.present?
   end
 
-  def assignments_for_student(course, assignments, student)
+  def assignments_for_student(course, assignments, student, includes: [])
-    assignment_ids = GradebookGradingPeriodAssignments.new(course, student: student).to_h.fetch(id, [])
+    assignment_ids = GradebookGradingPeriodAssignments.new(course, student: student, includes: includes).to_h.fetch(id, [])
     if assignment_ids.empty?
       []
     else

app/presenters/grade_summary_presenter.rb

@@ -141,19 +141,31 @@ class GradeSummaryPresenter
 
   def assignments
     @assignments ||= begin
-      visible_assignments = assignments_visible_to_student
+      visible_assignments = assignments_for_student
       overridden_assignments = assignments_overridden_for_student(visible_assignments)
       sorted_assignments(overridden_assignments)
     end
   end
 
-  def assignments_visible_to_student
+  def assignments_for_student
     includes = [:assignment_overrides, :post_policy]
     includes << :assignment_group if @assignment_order == :assignment_group
-    AssignmentGroup
+
-      .visible_assignments(student, @context, all_groups, includes: includes)
+    # AssignmentGroup#visible_assignments returns all published assignments if you pass it
-      .where.not(submission_types: %w[not_graded wiki_page])
+    # a nil user. On the other hand, if you pass it a student, it returns only assignments
-      .except(:order)
+    # visible to that student.
+    #
+    # The logic here is needed in order to ensure that teachers can view assignment grades
+    # for deactivated students (who themselves can not view those assignments).
+    assignments = if user_has_elevated_permissions?
+                    AssignmentGroup
+                      .visible_assignments(nil, @context, all_groups, includes: includes)
+                      .assigned_to_student(student.id)
+                  else
+                    AssignmentGroup.visible_assignments(student, @context, all_groups, includes: includes)
+                  end
+
+    assignments.where.not(submission_types: %w[not_graded wiki_page]).except(:order)
   end
 
   def assignments_overridden_for_student(assignments)

app/presenters/grading_period_grade_summary_presenter.rb

@@ -25,9 +25,11 @@ class GradingPeriodGradeSummaryPresenter < GradeSummaryPresenter
     @grading_period_id = grading_period_id
   end
 
-  def assignments_visible_to_student
+  def assignments_for_student
+    includes = ["completed"]
+    includes << "inactive" if user_has_elevated_permissions?
     grading_period = GradingPeriod.for(@context).where(id: grading_period_id).first
-    grading_period.assignments_for_student(@context, super, student)
+    grading_period.assignments_for_student(@context, super, student, includes: includes)
   end
 
   def groups

lib/gradebook_grading_period_assignments.rb

@@ -18,7 +18,7 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 
 class GradebookGradingPeriodAssignments
-  def initialize(course, student: nil, course_settings: nil)
+  def initialize(course, student: nil, course_settings: nil, includes: [])
     raise "Context must be a course" unless course.is_a?(Course)
     raise "Context must have an id" unless course.id
 
@@ -28,6 +28,7 @@ class GradebookGradingPeriodAssignments
       "show_concluded_enrollments" => "false",
       "show_inactive_enrollments" => "false"
     }
+    @includes = includes
   end
 
   def to_h
@@ -42,8 +43,14 @@ class GradebookGradingPeriodAssignments
 
   def excluded_workflow_states
     excluded_workflow_states = ["deleted"]
-    excluded_workflow_states << "completed" if @settings_for_course["show_concluded_enrollments"] != "true"
+    if @settings_for_course["show_concluded_enrollments"] != "true" && @includes.exclude?("completed")
-    excluded_workflow_states << "inactive" if @settings_for_course["show_inactive_enrollments"] != "true"
+      excluded_workflow_states << "completed"
+    end
+
+    if @settings_for_course["show_inactive_enrollments"] != "true" && @includes.exclude?("inactive")
+      excluded_workflow_states << "inactive"
+    end
+
     excluded_workflow_states
   end
 

spec/controllers/gradebooks_controller_spec.rb

@@ -351,7 +351,7 @@ describe GradebooksController do
       expect(submission[:workflow_state]).to eq "graded"
     end
 
-    it "returns submissions of even inactive students" do
+    it "returns submissions for inactive students" do
       user_session(@teacher)
       assignment = @course.assignments.create!(points_possible: 10)
       assignment.grade_student(@student, grade: 6.6, grader: @teacher)
@@ -361,6 +361,17 @@ describe GradebooksController do
       expect(assigns.fetch(:js_env).fetch(:submissions).first.fetch(:score)).to be 6.6
     end
 
+    it "returns assignments for inactive students" do
+      user_session(@teacher)
+      assignment = @course.assignments.create!(points_possible: 10)
+      assignment.grade_student(@student, grade: 6.6, grader: @teacher)
+      enrollment = @course.enrollments.find_by(user: @student)
+      enrollment.deactivate
+      get :grade_summary, params: { course_id: @course.id, id: @student.id }
+      assignment_id = assigns.dig(:js_env, :assignment_groups, 0, :assignments, 0, :id)
+      expect(assignment_id).to eq assignment.id
+    end
+
     context "assignment sorting" do
       let!(:teacher_session) { user_session(@teacher) }
       let!(:assignment1) { @course.assignments.create(title: "Banana", position: 2) }
@@ -2952,6 +2963,7 @@ describe GradebooksController do
       AssignmentGroup.add_never_drop_assignment(ag, a)
       @controller.instance_variable_set(:@context, @course)
       @controller.instance_variable_set(:@current_user, @user)
+      @controller.instance_variable_set(:@presenter, @controller.send(:grade_summary_presenter))
       expect(@controller.light_weight_ags_json([ag])).to eq [
         {
           id: ag.id,
@@ -2989,6 +3001,7 @@ describe GradebooksController do
 
       @controller.instance_variable_set(:@context, @course)
       @controller.instance_variable_set(:@current_user, @user)
+      @controller.instance_variable_set(:@presenter, @controller.send(:grade_summary_presenter))
       expect(@controller.light_weight_ags_json([ag])).to eq [
         {
           id: ag.id,

spec/lib/gradebook_grading_period_assignments_spec.rb

@@ -124,9 +124,16 @@ describe GradebookGradingPeriodAssignments do
           @student_enrollment = student_in_course(course: @course, active_all: true)
           @assignment = @course.assignments.create!(due_at: @period2.end_date)
           @settings = {}
+          @includes = []
         end
 
-        let(:hash) { GradebookGradingPeriodAssignments.new(@course, course_settings: @settings).to_h }
+        let(:hash) do
+          GradebookGradingPeriodAssignments.new(
+            @course,
+            course_settings: @settings,
+            includes: @includes
+          ).to_h
+        end
 
         describe "concluded students" do
           before(:once) do
@@ -154,12 +161,17 @@ describe GradebookGradingPeriodAssignments do
             expect(hash[@period2.id]).to be_nil
           end
 
-          it "optionally includes assignments assigned exclusively to concluded students" do
+          it "can optionally include assignments assigned exclusively to concluded students" do
+            @includes = ["completed"]
+            expect(hash[@period2.id]).to include @assignment.id.to_s
+          end
+
+          it "can be passed course settings to include assignments assigned exclusively to concluded students" do
             @settings = { "show_concluded_enrollments" => "true" }
             expect(hash[@period2.id]).to include @assignment.id.to_s
           end
 
-          it "optionally excludes assignments assigned exclusively to concluded students" do
+          it "can be passed course settings to exclude assignments assigned exclusively to concluded students" do
             @settings = { "show_concluded_enrollments" => "false" }
             expect(hash[@period2.id]).to be_nil
           end
@@ -191,12 +203,17 @@ describe GradebookGradingPeriodAssignments do
             expect(hash[@period2.id]).to be_nil
           end
 
-          it "optionally includes assignments assigned exclusively to deactivated students" do
+          it "can optionally include assignments assigned exclusively to deactivated students" do
+            @includes = ["inactive"]
+            expect(hash[@period2.id]).to include @assignment.id.to_s
+          end
+
+          it "can be passed course settings to include assignments assigned exclusively to deactivated students" do
             @settings = { "show_inactive_enrollments" => "true" }
             expect(hash[@period2.id]).to include @assignment.id.to_s
           end
 
-          it "optionally excludes assignments assigned exclusively to deactivated students" do
+          it "can be passed course settings to exclude assignments assigned exclusively to deactivated students" do
             @settings = { "show_inactive_enrollments" => "false" }
             expect(hash[@period2.id]).to be_nil
           end

spec/models/assignment_spec.rb

@@ -418,6 +418,30 @@ describe Assignment do
       end
     end
 
+    describe "#assigned_to_student" do
+      it "returns assignments assigned to the given student" do
+        assignment = @course.assignments.create!
+        expect(@course.assignments.assigned_to_student(@initial_student.id)).to include assignment
+      end
+
+      it "does not return assignments not assigned to the given student" do
+        new_student = student_in_course(course: @course, active_all: true, user_name: "new student").user
+        assignment = @course.assignments.create!(only_visible_to_overrides: true)
+        create_adhoc_override_for_assignment(assignment, new_student)
+        aggregate_failures do
+          expect(@course.assignments.assigned_to_student(new_student.id)).to include assignment
+          expect(@course.assignments.assigned_to_student(@initial_student.id)).not_to include assignment
+        end
+      end
+
+      it "returns assignments for a which a student does not have visibility but is assigned" do
+        assignment = @course.assignments.create!
+        # deactivated students can not view assignments they are assigned to
+        @course.enrollments.find_by(user: @initial_student).deactivate
+        expect(@course.assignments.assigned_to_student(@initial_student.id)).to include assignment
+      end
+    end
+
     describe "#update_submittable" do
       before do
         Timecop.freeze(1.day.ago) do

spec/presenters/grade_summary_presenter_spec.rb

@@ -356,6 +356,12 @@ describe GradeSummaryPresenter do
       p = GradeSummaryPresenter.new(@course, @teacher, @student.id)
       expect(p.assignments).to eq [published_assignment]
     end
+
+    it "includes assignments for deactivated students when a teacher is viewing" do
+      @course.enrollments.find_by(user: @student).deactivate
+      presenter = GradeSummaryPresenter.new(@course, @teacher, @student.id)
+      expect(presenter.assignments).to include published_assignment
+    end
   end
 
   describe "#sort_options" do

spec/presenters/grading_period_grade_summary_presenter_spec.rb

@@ -74,19 +74,32 @@ describe GradingPeriodGradeSummaryPresenter do
     end
   end
 
-  describe "#assignments_visible_to_student" do
+  describe "#assignments_for_student" do
     it "excludes assignments that are not due for the student in the given grading period" do
-      expect(presenter.assignments_visible_to_student).not_to include(@assignment_not_due_in_period)
+      expect(presenter.assignments_for_student).not_to include(@assignment_not_due_in_period)
     end
 
     it "includes assignments that are due for the student in the given grading period" do
-      expect(presenter.assignments_visible_to_student).to include(@assignment_due_in_period)
+      expect(presenter.assignments_for_student).to include(@assignment_due_in_period)
     end
 
     it "includes overridden assignments that are due for the student in the given grading period" do
       student_override = @assignment_not_due_in_period.assignment_overrides.create!(due_at: @now, due_at_overridden: true)
       student_override.assignment_override_students.create!(user: @student)
-      expect(presenter.assignments_visible_to_student).to include(@assignment_not_due_in_period)
+      expect(presenter.assignments_for_student).to include(@assignment_not_due_in_period)
+    end
+
+    it "includes assignments for deactivated students when a teacher is viewing" do
+      teacher = User.create!
+      @course.enroll_teacher(teacher, active_all: true)
+      @course.enrollments.find_by(user: @student).deactivate
+      presenter = GradingPeriodGradeSummaryPresenter.new(
+        @course,
+        teacher,
+        @student.id,
+        grading_period_id: @period.id
+      )
+      expect(presenter.assignments_for_student).to include(@assignment_due_in_period)
     end
   end