From 531e5dbc58a6fa98b48a3149a4034a78316da715 Mon Sep 17 00:00:00 2001 From: Derek Bender Date: Mon, 24 Jun 2019 15:23:48 -0500 Subject: [PATCH] Scope speed grader to new setting Closes: GRADE-2245 Test Plan: - Given a course with new gradebook enabled - Given Filter by Student Group is enabled - Given at least two groups with students in each - Given the new gradebook has "Group Filters" selected in the View -> Filters menu - When the new filter has the first group selected - Then the Speed Grader page for any assignment will only display students from the first group - When the second group filter is selected - Then the Speed Grader page for any assignment will only display students from the second group - When the filter is changed to "All Student Groups" - Then the Speed Grader page for any assignment will display all students like when this feature is disabled - Given a course with new gradebook enabled - Given Filter by Student Group is disabled - Given at least two groups with students in each - Given the new gradebook has "Group Filters" selected in the View -> Filters menu - When the filter is changed to the first group - Then the Speed Grader page for any assignment will display all students - When the filter is changed to the second group - Then the Speed Grader page for any assignment will display all students - When the filter is changed to "All Student Groups" - Then the Speed Grader page for any assignment will display all students Change-Id: I530f12bbef0cdd799c15f159366cbae3d9b0eb1e Reviewed-on: https://gerrit.instructure.com/198809 Tested-by: Jenkins Reviewed-by: Adrian Packel Reviewed-by: Keith Garner QA-Review: James Butters Product-Review: Keith Garner --- app/controllers/assignments_controller.rb | 2 +- app/controllers/submissions_api_controller.rb | 4 +- app/models/assignment.rb | 33 ++++-- app/models/course.rb | 1 + app/models/group_membership.rb | 1 - app/models/speed_grader/assignment.rb | 7 +- lib/api/v1/submission_comment.rb | 2 +- lib/content_zipper.rb | 2 +- spec/models/assignment_spec.rb | 10 +- spec/models/speed_grader/assignment_spec.rb | 105 +++++++++++++++--- 10 files changed, 128 insertions(+), 39 deletions(-) diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index d34abae5ce9..2d8cce1a597 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -247,7 +247,7 @@ class AssignmentsController < ApplicationController student_ids = if assignment.grade_as_group? - assignment.representatives(current_user).map(&:id) + assignment.representatives(user: current_user).map(&:id) else context.apply_enrollment_visibility(context.student_enrollments, current_user).pluck(:user_id) end diff --git a/app/controllers/submissions_api_controller.rb b/app/controllers/submissions_api_controller.rb index aee08424c8c..11dcb450946 100644 --- a/app/controllers/submissions_api_controller.rb +++ b/app/controllers/submissions_api_controller.rb @@ -235,7 +235,7 @@ class SubmissionsApiController < ApplicationController student_ids = if value_to_boolean(params[:grouped]) # this provides one assignment object(and # submission object within), per user group - @assignment.representatives(@current_user).map(&:id) + @assignment.representatives(user: @current_user).map(&:id) else @context.apply_enrollment_visibility(@context.student_enrollments, @current_user, section_ids). @@ -1103,7 +1103,7 @@ class SubmissionsApiController < ApplicationController if authorized_action(@context, @current_user, [:manage_grades, :view_all_grades]) @assignment = @context.assignments.active.find(params[:assignment_id]) student_ids = if should_group? - @assignment.representatives(@current_user).map(&:id) + @assignment.representatives(user: @current_user).map(&:id) else student_scope = @context.students_visible_to(@current_user). where("enrollments.type<>'StudentViewEnrollment' AND enrollments.workflow_state = 'active'").distinct diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 345449207d7..a62b56d719d 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -2071,7 +2071,7 @@ class Assignment < ActiveRecord::Base def sections_with_visibility(user) return context.active_course_sections unless self.differentiated_assignments_applies? - visible_student_ids = visible_students_for_speed_grader(user).map(&:id) + visible_student_ids = visible_students_for_speed_grader(user: user).map(&:id) context.active_course_sections.joins(:student_enrollments). where(:enrollments => {:user_id => visible_student_ids, :type => "StudentEnrollment"}).distinct.reorder("name") end @@ -2116,8 +2116,8 @@ class Assignment < ActiveRecord::Base # for group assignments, returns a single "student" for each # group's submission. the students name will be changed to the group's # name. for non-group assignments this just returns all visible users - def representatives(user, includes: [:inactive]) - return visible_students_for_speed_grader(user, includes: includes) unless grade_as_group? + def representatives(user:, includes: [:inactive], group_id: nil) + return visible_students_for_speed_grader(user: user, includes: includes, group_id: group_id) unless grade_as_group? submissions = self.submissions.to_a user_ids_with_submissions = submissions.select(&:has_submission?).map(&:user_id).to_set @@ -2143,7 +2143,7 @@ class Assignment < ActiveRecord::Base enrollment_priority = { 'active' => 1, 'inactive' => 2 } enrollment_priority.default = 100 - visible_student_ids = visible_students_for_speed_grader(user, includes: includes).map(&:id).to_set + visible_student_ids = visible_students_for_speed_grader(user: user, includes: includes).map(&:id).to_set reps_and_others = groups_and_ungrouped(user, includes: includes).map do |group_name, group_info| group_students = group_info[:users] @@ -2180,7 +2180,7 @@ class Assignment < ActiveRecord::Base groups.active.preload(group_memberships: :user). map { |g| [g.name, { sortable_name: g.name, users: g.users}] } users_in_group = groups_and_users.flat_map { |_, group_info| group_info[:users] } - groupless_users = visible_students_for_speed_grader(user, includes: includes) - users_in_group + groupless_users = visible_students_for_speed_grader(user: user, includes: includes) - users_in_group phony_groups = groupless_users.map do |u| sortable_name = users_in_group.empty? ? u.sortable_name : u.name [u.name, { sortable_name: sortable_name, users: [u] }] @@ -2190,14 +2190,25 @@ class Assignment < ActiveRecord::Base private :groups_and_ungrouped # using this method instead of students_with_visibility so we - # can add the includes and students_visible_to/participating_students scopes - def visible_students_for_speed_grader(user, includes: [:inactive]) + # can add the includes and students_visible_to/participating_students scopes. + # a group_id filter can optionally be supplied. + def visible_students_for_speed_grader(user:, includes: [:inactive], group_id: nil) @visible_students_for_speed_grader ||= {} - @visible_students_for_speed_grader[[user.global_id, includes]] ||= ( - student_scope = user ? context.students_visible_to(user, include: includes) : context.participating_students - students_with_visibility(student_scope) - ).order_by_sortable_name.distinct.to_a + @visible_students_for_speed_grader[[user.global_id, includes, group_id]] ||= begin + student_scope = if user.present? + context.students_visible_to(user, include: includes) + else + context.participating_students + end + students = students_with_visibility(student_scope).order_by_sortable_name.distinct + if group_id.present? + students = students.joins(:group_memberships). + where(group_memberships: {group_id: group_id, workflow_state: :accepted}) + end + students.to_a + end end + private :visible_students_for_speed_grader def visible_rubric_assessments_for(user, opts={}) return [] unless user && self.rubric_association diff --git a/app/models/course.rb b/app/models/course.rb index 7c870b3fd83..9f7def266f6 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -2448,6 +2448,7 @@ class Course < ActiveRecord::Base end # can apply to user scopes as well if through enrollments (e.g. students, teachers) + # returns a scope for enrollments def apply_enrollment_visibility(scope, user, section_ids=nil, include: []) include = Array(include) if section_ids diff --git a/app/models/group_membership.rb b/app/models/group_membership.rb index 902c04881aa..65e083c95ce 100644 --- a/app/models/group_membership.rb +++ b/app/models/group_membership.rb @@ -17,7 +17,6 @@ # class GroupMembership < ActiveRecord::Base - include Workflow belongs_to :group diff --git a/app/models/speed_grader/assignment.rb b/app/models/speed_grader/assignment.rb index 35a830a6e44..02a1ea8000f 100644 --- a/app/models/speed_grader/assignment.rb +++ b/app/models/speed_grader/assignment.rb @@ -73,7 +73,7 @@ module SpeedGrader # valid submission object, which means no inactive or concluded students # even if the user has elected to show them in gradebook includes = assignment.anonymize_students? ? [] : gradebook_includes(user: current_user, course: course) - students = assignment.representatives(current_user, includes: includes) do |rep, others| + students = assignment.representatives(user: current_user, includes: includes, group_id: group_id_filter) do |rep, others| others.each { |s| res[:context][:rep_for_student][s.id] = rep.id } end @@ -404,5 +404,10 @@ module SpeedGrader submissions: submissions ) end + + def group_id_filter + return nil unless course.filter_speed_grader_by_student_group? + current_user.preferences.dig(:gradebook_settings, course.id, 'filter_rows_by', 'student_group_id') + end end end diff --git a/lib/api/v1/submission_comment.rb b/lib/api/v1/submission_comment.rb index 1c21cfcf727..381955bfa76 100644 --- a/lib/api/v1/submission_comment.rb +++ b/lib/api/v1/submission_comment.rb @@ -141,7 +141,7 @@ module Api::V1::SubmissionComment def students(course:, assignment:, current_user:) @students ||= begin includes = gradebook_includes(user: current_user, course: course) - assignment.representatives(current_user, includes: includes) + assignment.representatives(user: current_user, includes: includes) end end diff --git a/lib/content_zipper.rb b/lib/content_zipper.rb index 05134e4d6fc..74c1e973193 100644 --- a/lib/content_zipper.rb +++ b/lib/content_zipper.rb @@ -85,7 +85,7 @@ class ContentZipper # This neglects the complexity of group assignments students = User.where(id: submissions.pluck(:user_id)).index_by(&:id) else - students = assignment.representatives(user).index_by(&:id) + students = assignment.representatives(user: user).index_by(&:id) submissions = assignment.submissions.where(user_id: students.keys, submission_type: downloadable_submissions) end diff --git a/spec/models/assignment_spec.rb b/spec/models/assignment_spec.rb index 1f26b385289..cd68235e37e 100644 --- a/spec/models/assignment_spec.rb +++ b/spec/models/assignment_spec.rb @@ -1293,7 +1293,7 @@ describe Assignment do expect(User).to receive(:best_unicode_collation_key).with('sortable_name').and_call_original assignment = @course.assignments.create!(assignment_valid_attributes) - representatives = assignment.representatives(@teacher) + representatives = assignment.representatives(user: @teacher) expect(representatives[0].name).to eql(student_three.name) expect(representatives[1].name).to eql(student_one.name) @@ -1333,7 +1333,7 @@ describe Assignment do expect(Canvas::ICU).to receive(:collate_by).and_call_original - representatives = assignment.representatives(@teacher) + representatives = assignment.representatives(user: @teacher) expect(representatives[0].name).to eql(group_two.name) expect(representatives[1].name).to eql(group_one.name) @@ -1364,7 +1364,7 @@ describe Assignment do expect(Canvas::ICU).to receive(:collate_by).and_call_original - representatives = assignment.representatives(@teacher) + representatives = assignment.representatives(user: @teacher) expect(representatives[0].name).to eql(student_three.name) expect(representatives[1].name).to eql(student_one.name) @@ -1402,7 +1402,7 @@ describe Assignment do expect(Canvas::ICU).to receive(:collate_by).and_call_original - representatives = assignment.representatives(@teacher) + representatives = assignment.representatives(user: @teacher) expect(representatives[0].name).to eql(student_three.name) expect(representatives[1].name).to eql(group_two.name) @@ -1444,7 +1444,7 @@ describe Assignment do expect(User).to receive(:best_unicode_collation_key).with('sortable_name').and_call_original - representatives = assignment.representatives(@teacher) + representatives = assignment.representatives(user: @teacher) expect(representatives[0].name).to eql(student_three.name) expect(representatives[1].name).to eql(student_one.name) diff --git a/spec/models/speed_grader/assignment_spec.rb b/spec/models/speed_grader/assignment_spec.rb index a7ed17ca1af..50079aa4244 100644 --- a/spec/models/speed_grader/assignment_spec.rb +++ b/spec/models/speed_grader/assignment_spec.rb @@ -555,11 +555,84 @@ describe SpeedGrader::Assignment do end end - it "is not in group mode for non-group assignments" do - assignment = @course.assignments.create! - assignment.submit_homework(@student, {submission_type: 'online_text_entry', body: 'blah'}) - json = SpeedGrader::Assignment.new(assignment, @teacher).json - expect(json["GROUP_GRADING_MODE"]).to be false + context 'given an assignment' do + before(:once) do + @assignment = @course.assignments.create! + end + + it "is not in group mode for non-group assignments" do + @assignment.submit_homework(@student, {submission_type: 'online_text_entry', body: 'blah'}) + json = SpeedGrader::Assignment.new(@assignment, @teacher).json + expect(json["GROUP_GRADING_MODE"]).to be false + end + + context 'when a course has new gradeook and filter by student group enabled' do + before(:once) do + @course.enable_feature!(:new_gradebook) + @course.update!(filter_speed_grader_by_student_group: true) + end + + context 'when no group filter is present' do + it 'returns all students' do + @teacher.preferences.deep_merge!(gradebook_settings: { + @course.id => {'filter_rows_by' => {'student_group_id' => nil}} + }) + json = SpeedGrader::Assignment.new(@assignment, @teacher).json + json_students = json.fetch(:context).fetch(:students).map {|s| s.except(:rubric_assessments)} + students = @course.students.as_json(include_root: false, only: [:id, :name, :sortable_name]) + StringifyIds.recursively_stringify_ids(students) + expect(json_students).to match_array(students) + end + end + + context 'when the first group filter is present' do + let(:group) { @first_group } + + before(:once) do + @teacher.preferences.deep_merge!(gradebook_settings: { + @course.id => {'filter_rows_by' => {'student_group_id' => group.id.to_s}} + }) + end + + it 'returns only students that belong to the first group' do + json = SpeedGrader::Assignment.new(@assignment, @teacher).json + json_students = json.fetch(:context).fetch(:students).map {|s| s.except(:rubric_assessments)} + group_students = group.users.as_json(include_root: false, only: [:id, :name, :sortable_name]) + StringifyIds.recursively_stringify_ids(group_students) + expect(json_students).to match_array(group_students) + end + + context 'when a student is removed from a group' do + let(:first_student) { group.users.first } + + before { group.group_memberships.find_by!(user: first_student).destroy! } + + it 'that student is no longer included' do + json = SpeedGrader::Assignment.new(@assignment, @teacher).json + json_students = json.fetch(:context).fetch(:students).map {|s| s.except(:rubric_assessments)} + group_students = group.users.where.not(id: first_student). + as_json(include_root: false, only: [:id, :name, :sortable_name]) + StringifyIds.recursively_stringify_ids(group_students) + expect(json_students).to match_array(group_students) + end + end + + context 'when the second group filter is present' do + let(:group) { @second_group } + + it 'returns only students that belong to the second group' do + @teacher.preferences.deep_merge!(gradebook_settings: { + @course.id => {'filter_rows_by' => {'student_group_id' => group.id.to_s}} + }) + json = SpeedGrader::Assignment.new(@assignment, @teacher).json + json_students = json.fetch(:context).fetch(:students).map {|s| s.except(:rubric_assessments)} + group_students = group.users.as_json(include_root: false, only: [:id, :name, :sortable_name]) + StringifyIds.recursively_stringify_ids(group_students) + expect(json_students).to match_array(group_students) + end + end + end + end end context 'given a group assignment' do @@ -606,7 +679,7 @@ describe SpeedGrader::Assignment do first_group_representative = @first_group.users.sample submission = @assignment.submission_for_student(first_group_representative) submission.update!(submission_type: 'online_upload') - expect(@assignment.representatives(@teacher)).to include first_group_representative + expect(@assignment.representatives(user: @teacher)).to include first_group_representative end it "prefers people who aren't excused when submission exists" do @@ -618,19 +691,19 @@ describe SpeedGrader::Assignment do everyone_else.each do |user| @assignment.grade_student(user, excuse: true, grader: @teacher) end - expect(@assignment.representatives(@teacher)).to include first_group_representative + expect(@assignment.representatives(user: @teacher)).to include first_group_representative end it "includes users who aren't in a group" do student_in_course active_all: true - expect(@assignment.representatives(@teacher)).to include @student + expect(@assignment.representatives(user: @teacher)).to include @student end it "includes groups" do student_in_course active_all: true group = @group_category.groups.create!(context: @course) group.add_user(@student) - expect(@assignment.representatives(@teacher).map(&:name)).to include group.name + expect(@assignment.representatives(user: @teacher).map(&:name)).to include group.name end it "doesn't include deleted groups" do @@ -638,7 +711,7 @@ describe SpeedGrader::Assignment do group = @group_category.groups.create!(context: @course) group.add_user(@student) group.destroy! - expect(@assignment.representatives(@teacher).map(&:name)).not_to include group.name + expect(@assignment.representatives(user: @teacher).map(&:name)).not_to include group.name end it 'prefers active users over other workflow states' do @@ -646,7 +719,7 @@ describe SpeedGrader::Assignment do enrollments.first.deactivate enrollments.second.conclude - reps = @assignment.representatives(@teacher, includes: %i[inactive completed]) + reps = @assignment.representatives(user: @teacher, includes: %i[inactive completed]) user = reps.find { |u| u.name == @first_group.name } expect(user).to eql(enrollments.third.user) end @@ -657,7 +730,7 @@ describe SpeedGrader::Assignment do enrollments.second.deactivate enrollments.third.conclude - reps = @assignment.representatives(@teacher, includes: %i[inactive completed]) + reps = @assignment.representatives(user: @teacher, includes: %i[inactive completed]) user = reps.find { |u| u.name == @first_group.name } expect(user).to eql(enrollments.second.user) end @@ -666,7 +739,7 @@ describe SpeedGrader::Assignment do enrollments = @first_group.all_real_student_enrollments enrollments.each(&:conclude) - reps = @assignment.representatives(@teacher, includes: [:completed]) + reps = @assignment.representatives(user: @teacher, includes: [:completed]) user = reps.find { |u| u.name == @first_group.name } expect(user).to eql(enrollments.first.user) end @@ -675,7 +748,7 @@ describe SpeedGrader::Assignment do enrollments = @first_group.all_real_student_enrollments enrollments.each(&:conclude) - reps = @assignment.representatives(@teacher, includes: []) + reps = @assignment.representatives(user: @teacher, includes: []) user = reps.find { |u| u.name == @first_group.name } expect(user).to be_nil end @@ -684,7 +757,7 @@ describe SpeedGrader::Assignment do enrollments = @first_group.all_real_student_enrollments enrollments.each(&:deactivate) - reps = @assignment.representatives(@teacher, includes: [:inactive]) + reps = @assignment.representatives(user: @teacher, includes: [:inactive]) user = reps.find { |u| u.name == @first_group.name } expect(user).to eql(enrollments.first.user) end @@ -693,7 +766,7 @@ describe SpeedGrader::Assignment do enrollments = @first_group.all_real_student_enrollments enrollments.each(&:deactivate) - reps = @assignment.representatives(@teacher, includes: []) + reps = @assignment.representatives(user: @teacher, includes: []) user = reps.find { |u| u.name == @first_group.name } expect(user).to be_nil end