ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 <apackel@instructure.com>
Reviewed-by: Keith Garner <kgarner@instructure.com>
QA-Review: James Butters <jbutters@instructure.com>
Product-Review: Keith Garner <kgarner@instructure.com>
This commit is contained in:
Derek Bender 2019-06-24 15:23:48 -05:00
parent cbccd7faa8
commit 531e5dbc58
10 changed files with 128 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/assignments_controller.rb
View File

@ -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

4
app/controllers/submissions_api_controller.rb
View File

@ -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

33
app/models/assignment.rb
View File

@ -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

1
app/models/course.rb
View File

@ -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

1
app/models/group_membership.rb
View File

@ -17,7 +17,6 @@
#
class GroupMembership < ActiveRecord::Base
include Workflow
belongs_to :group

7
app/models/speed_grader/assignment.rb
View File

@ -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

2
lib/api/v1/submission_comment.rb
View File

@ -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

2
lib/content_zipper.rb
View File

@ -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

10
spec/models/assignment_spec.rb
View File

@ -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)

105
spec/models/speed_grader/assignment_spec.rb
View File

@ -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