From 3af42f8ec7f5f7025bc735a202aff6b465d62273 Mon Sep 17 00:00:00 2001 From: Spencer Olson Date: Tue, 11 Jan 2022 11:07:44 -0600 Subject: [PATCH] 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 Product-Review: Jody Sailor Tested-by: Service Cloud Jenkins Reviewed-by: Eduardo Escobar Reviewed-by: Dustin Cowles --- app/controllers/gradebooks_controller.rb | 46 +++++++++---------- app/models/assignment.rb | 1 + app/models/grading_period.rb | 4 +- app/presenters/grade_summary_presenter.rb | 24 +++++++--- .../grading_period_grade_summary_presenter.rb | 6 ++- lib/gradebook_grading_period_assignments.rb | 13 ++++-- .../controllers/gradebooks_controller_spec.rb | 15 +++++- ...adebook_grading_period_assignments_spec.rb | 27 +++++++++-- spec/models/assignment_spec.rb | 24 ++++++++++ .../grade_summary_presenter_spec.rb | 6 +++ ...ing_period_grade_summary_presenter_spec.rb | 21 +++++++-- 11 files changed, 141 insertions(+), 46 deletions(-) diff --git a/app/controllers/gradebooks_controller.rb b/app/controllers/gradebooks_controller.rb index b74404c28ea..141527a65db 100644 --- a/app/controllers/gradebooks_controller.rb +++ b/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 = {}) - assignment_groups.map do |ag| - visible_assignments = ag.visible_assignments(opts[:student] || @current_user).to_a + def light_weight_ags_json(assignment_groups) + assignments_by_group = @presenter.assignments.each_with_object({}) do |assignment, assignments| + # 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? - current_period = GradingPeriod.for(@context).find_by(id: @current_grading_period_id) - visible_assignments = current_period.assignments_for_student(@context, visible_assignments, opts[:student]) - end - - visible_assignments.map! do |a| - { - id: a.id, - submission_types: a.submission_types_array, - points_possible: a.points_possible, - due_at: a.due_at, - omit_from_final_grade: a.omit_from_final_grade?, - muted: a.muted? - } - end + assignments[assignment.assignment_group_id] ||= [] + assignments[assignment.assignment_group_id] << { + id: assignment.id, + submission_types: assignment.submission_types_array, + points_possible: assignment.points_possible, + due_at: assignment.due_at, + omit_from_final_grade: assignment.omit_from_final_grade?, + muted: assignment.muted? + } + end + assignment_groups.map do |group| { - id: ag.id, - rules: ag.rules_hash({ stringify_json_ids: true }), - group_weight: ag.group_weight, - assignments: visible_assignments, + id: group.id, + rules: group.rules_hash({ stringify_json_ids: true }), + group_weight: group.group_weight, + assignments: assignments_by_group.fetch(group.id, []) } end end diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 1d8abd32f3f..e05711e7c6e 100644 --- a/app/models/assignment.rb +++ b/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) } diff --git a/app/models/grading_period.rb b/app/models/grading_period.rb index a3ae79740f5..23d1ee76cf6 100644 --- a/app/models/grading_period.rb +++ b/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) - assignment_ids = GradebookGradingPeriodAssignments.new(course, student: student).to_h.fetch(id, []) + def assignments_for_student(course, assignments, student, includes: []) + assignment_ids = GradebookGradingPeriodAssignments.new(course, student: student, includes: includes).to_h.fetch(id, []) if assignment_ids.empty? [] else diff --git a/app/presenters/grade_summary_presenter.rb b/app/presenters/grade_summary_presenter.rb index b85bc3a7a83..db1b3437ba1 100644 --- a/app/presenters/grade_summary_presenter.rb +++ b/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) - .where.not(submission_types: %w[not_graded wiki_page]) - .except(:order) + + # AssignmentGroup#visible_assignments returns all published assignments if you pass it + # a nil user. On the other hand, if you pass it a student, it returns only assignments + # 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) diff --git a/app/presenters/grading_period_grade_summary_presenter.rb b/app/presenters/grading_period_grade_summary_presenter.rb index ccc3346211f..3bfbe33691e 100644 --- a/app/presenters/grading_period_grade_summary_presenter.rb +++ b/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 diff --git a/lib/gradebook_grading_period_assignments.rb b/lib/gradebook_grading_period_assignments.rb index ad95aca8887..207aa65f849 100644 --- a/lib/gradebook_grading_period_assignments.rb +++ b/lib/gradebook_grading_period_assignments.rb @@ -18,7 +18,7 @@ # with this program. If not, see . 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" - excluded_workflow_states << "inactive" if @settings_for_course["show_inactive_enrollments"] != "true" + if @settings_for_course["show_concluded_enrollments"] != "true" && @includes.exclude?("completed") + 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 diff --git a/spec/controllers/gradebooks_controller_spec.rb b/spec/controllers/gradebooks_controller_spec.rb index 2051f96b86b..a13df38472e 100644 --- a/spec/controllers/gradebooks_controller_spec.rb +++ b/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, diff --git a/spec/lib/gradebook_grading_period_assignments_spec.rb b/spec/lib/gradebook_grading_period_assignments_spec.rb index 34d400e6410..83a24af627c 100644 --- a/spec/lib/gradebook_grading_period_assignments_spec.rb +++ b/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 diff --git a/spec/models/assignment_spec.rb b/spec/models/assignment_spec.rb index 7dd5e81ee62..2cd37ff5c88 100644 --- a/spec/models/assignment_spec.rb +++ b/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 diff --git a/spec/presenters/grade_summary_presenter_spec.rb b/spec/presenters/grade_summary_presenter_spec.rb index dc2617fd4c6..48318260398 100644 --- a/spec/presenters/grade_summary_presenter_spec.rb +++ b/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 diff --git a/spec/presenters/grading_period_grade_summary_presenter_spec.rb b/spec/presenters/grading_period_grade_summary_presenter_spec.rb index 4eb5ff1a3ab..3e0aa769c14 100644 --- a/spec/presenters/grading_period_grade_summary_presenter_spec.rb +++ b/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