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

fix assignment edit for multi-section course 

Fix the edit for a multi-section course where an assignment
override is made for a student in another section than the
teacher who is editing the assignment.

fixes CNVS-23944

Test Plan:
  -Create a course with two sections.
  -Add a teacher that can only grade their section.
  -Create a quiz or assignment
  -Override with only student(s) that the sub-teacher cant see
  -Masquerade as the teacher and go attempt to edit the assignment.
  -Make a change
  -Click save and see that it now saves..
  -Repeat with override with mix of students that the sub-teacher
     can/cannot see
  -Repeat with various combos not stated that you can think of
  -Make sure to also test that groups and section overrides still work

Change-Id: I360368bf3340a15452da5e60e734817841df139c
Reviewed-on: https://gerrit.instructure.com/71526
Tested-by: Jenkins
Reviewed-by: Mike Nomitch <mnomitch@instructure.com>
QA-Review: Michael Hargiss <mhargiss@instructure.com>
Product-Review: Jason Sparks <jsparks@instructure.com>
This commit is contained in:
Marc Alan Phillips 2016-02-18 02:24:07 -06:00
parent ae5b65f915
commit 0fad3e1db6
11 changed files with 301 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/assignment_overrides_controller.rb
View File

@ -102,7 +102,7 @@ class AssignmentOverridesController < ApplicationController
# @returns [AssignmentOverride]
def index
@overrides = assignment_override_collection(@assignment, true)
render :json => assignment_overrides_json(@overrides)
render :json => assignment_overrides_json(@overrides, @current_user)
end
# @API Get a single assignment override

3
app/controllers/assignments_controller.rb
View File

@ -422,7 +422,8 @@ class AssignmentsController < ApplicationController
}),
:ASSIGNMENT_OVERRIDES =>
(assignment_overrides_json(
@assignment.overrides_for(@current_user, ensure_set_not_empty: true)
@assignment.overrides_for(@current_user, ensure_set_not_empty: true),
@current_user
)),
:ASSIGNMENT_INDEX_URL => polymorphic_url([@context, :assignments]),
:DIFFERENTIATED_ASSIGNMENTS_ENABLED => @context.feature_enabled?(:differentiated_assignments),

9
app/controllers/quizzes/quizzes_controller.rb
View File

@ -279,7 +279,9 @@ class Quizzes::QuizzesController < ApplicationController
hash = {
:ASSIGNMENT_ID => @assignment.present? ? @assignment.id : nil,
:ASSIGNMENT_OVERRIDES => assignment_overrides_json(@quiz.overrides_for(@current_user, ensure_set_not_empty: true)),
:ASSIGNMENT_OVERRIDES => assignment_overrides_json(@quiz.overrides_for(@current_user,
ensure_set_not_empty: true),
@current_user),
:DIFFERENTIATED_ASSIGNMENTS_ENABLED => @context.feature_enabled?(:differentiated_assignments),
:QUIZ => quiz_json(@quiz, @context, @current_user, session),
:SECTION_LIST => sections.map { |section|
@ -335,7 +337,7 @@ class Quizzes::QuizzesController < ApplicationController
params[:quiz].delete(:only_visible_to_overrides) unless @context.feature_enabled?(:differentiated_assignments)
@quiz.transaction do
@quiz.update_attributes!(params[:quiz])
batch_update_assignment_overrides(@quiz,overrides) unless overrides.nil?
batch_update_assignment_overrides(@quiz, overrides, @current_user) unless overrides.nil?
end
if Assignment.sis_grade_export_enabled?(@context) && @quiz.assignment
post_to_sis = nil
@ -412,7 +414,7 @@ class Quizzes::QuizzesController < ApplicationController
@quiz.assignment.save
end
batch_update_assignment_overrides(@quiz,overrides) unless overrides.nil?
batch_update_assignment_overrides(@quiz, overrides, @current_user) unless overrides.nil?
# quiz.rb restricts all assignment broadcasts if notify_of_update is
# false, so we do the same here
@ -781,7 +783,6 @@ class Quizzes::QuizzesController < ApplicationController
def delete_override_params
# nil represents the fact that we don't want to update the overrides
return nil unless params[:quiz].has_key?(:assignment_overrides)
overrides = params[:quiz].delete(:assignment_overrides)
overrides = deserialize_overrides(overrides)

16
app/models/assignment_override.rb
View File

@ -74,7 +74,7 @@ class AssignmentOverride < ActiveRecord::Base
def set_not_empty?
overridable = assignment? ? assignment : quiz
['CourseSection', 'Group'].include?(self.set_type) ||
set.any? && overridable.context.current_enrollments.where(user_id: set).exists?
(set.any? && overridable.context.current_enrollments.where(user_id: set).exists?)
end
def update_cached_due_dates
@ -112,6 +112,14 @@ class AssignmentOverride < ActiveRecord::Base
scope :active, -> { where(:workflow_state => 'active') }
scope :visible_students_only, -> (visible_ids) do
select("#{AssignmentOverride.quoted_table_name}.*").
joins(:assignment_override_students).
where(
assignment_override_students: { user_id: visible_ids },
)
end
before_validation :default_values
def default_values
self.set_type ||= 'ADHOC'
@ -174,6 +182,12 @@ class AssignmentOverride < ActiveRecord::Base
scope "overriding_#{field}", -> { where("#{field}_overridden" => true) }
end
def visible_student_overrides(visible_student_ids)
assignment_override_students.any? do |aos|
visible_student_ids.include?(aos.user_id)
end
end
override :due_at
override :unlock_at
override :lock_at

5
lib/api/v1/assignment.rb
View File

@ -87,7 +87,7 @@ module Api::V1::Assignment
hash['has_submitted_submissions'] = assignment.has_submitted_submissions?
if !opts[:overrides].blank?
hash['overrides'] = assignment_overrides_json(opts[:overrides])
hash['overrides'] = assignment_overrides_json(opts[:overrides], user)
end
if !assignment.user_submitted.nil?
@ -329,7 +329,7 @@ module Api::V1::Assignment
if overrides
assignment.transaction do
assignment.save_without_broadcasting!
batch_update_assignment_overrides(assignment, overrides)
batch_update_assignment_overrides(assignment, overrides, user)
end
assignment.do_notifications!(old_assignment, assignment_params[:notify_of_update])
else
@ -337,6 +337,7 @@ module Api::V1::Assignment
end
return true
# some values need to be removed before sending the error
rescue ActiveRecord::RecordInvalid
return false
end

70
lib/api/v1/assignment_override.rb
View File

@ -19,7 +19,7 @@
module Api::V1::AssignmentOverride
include Api::V1::Json
def assignment_override_json(override)
def assignment_override_json(override, visible_users=nil)
fields = [:id, :assignment_id, :title]
fields.concat([:due_at, :all_day, :all_day_date]) if override.due_at_overridden
fields << :unlock_at if override.unlock_at_overridden
@ -27,7 +27,12 @@ module Api::V1::AssignmentOverride
api_json(override, @current_user, session, :only => fields).tap do |json|
case override.set_type
when 'ADHOC'
json[:student_ids] = override.assignment_override_students.map(&:user_id)
students = if visible_users
override.assignment_override_students.where(user_id: visible_users)
else
override.assignment_override_students
end
json[:student_ids] = students.map(&:user_id)
when 'Group'
json[:group_id] = override.set_id
when 'CourseSection'
@ -36,8 +41,12 @@ module Api::V1::AssignmentOverride
end
end
def assignment_overrides_json(overrides)
overrides.map{ |override| assignment_override_json(override) }
def assignment_overrides_json(overrides, user = nil)
visible_users = if user && !overrides.empty?
context = overrides.first.assignment.context
UserSearch.scope_for(context, user, { force_users_visible_to: true }).map(&:id)
end
overrides.map{ |override| assignment_override_json(override, visible_users) }
end
def assignment_override_collection(assignment, include_students=false)
@ -102,8 +111,10 @@ module Api::V1::AssignmentOverride
errors << "invalid student_ids #{student_ids.inspect}"
students = []
else
# look up the students
students = api_find_all(assignment.context.students_visible_to(@current_user, include: :priors), student_ids).uniq
# look up all the active students since the assignment will affect all
# active students in the course on this override and not just what the
# teacher can see that were sent in the request object
students = api_find_all(assignment.context.students.active, student_ids).uniq
# make sure they were all valid
found_ids = students.map{ |s| [
@ -232,18 +243,57 @@ module Api::V1::AssignmentOverride
return false
end
def batch_update_assignment_overrides(assignment, overrides_params)
override_param_ids = overrides_params.map{ |ov| ov[:id] }
existing_overrides = assignment.assignment_overrides.active
def invisible_users_and_overrides_for_user(context, user, existing_overrides)
# get the student overrides the user can't see and ensure those overrides are included
visible_user_ids = UserSearch.scope_for(context, user, { force_users_visible_to: true }).map(&:id)
invisible_user_ids = context.users.pluck(:id) - visible_user_ids
invisible_override_ids = existing_overrides.select{ |ov|
ov.set_type == 'ADHOC' &&
!ov.visible_student_overrides(visible_user_ids)
}.map(&:id)
return invisible_user_ids, invisible_override_ids
end
def overrides_after_defunct_removed(assignment, existing_overrides, overrides_params, invisible_override_ids)
override_param_ids = invisible_override_ids + overrides_params.map{ |ov| ov[:id] }
remaining_overrides = destroy_defunct_overrides(assignment, override_param_ids, existing_overrides)
ActiveRecord::Associations::Preloader.new.preload(remaining_overrides, :assignment_override_students)
remaining_overrides
end
def update_override_with_invisible_data(override_params, override, invisible_override_ids, invisible_user_ids)
return override_params = override if invisible_override_ids.include?(override.id)
# add back in the invisible students for this override if any found
hidden_ids = override.assignment_override_students.where(user_id: invisible_user_ids).pluck(:user_id)
unless hidden_ids.empty?
override_params[:student_ids] = (override_params[:student_ids] + hidden_ids)
overrides_size = override_params[:student_ids].size
override_params[:title] = t({ one: '1 student', other: "%{count} students" }, count: overrides_size)
end
end
def batch_update_assignment_overrides(assignment, overrides_params, user)
existing_overrides = assignment.assignment_overrides.active
invisible_user_ids, invisible_override_ids = invisible_users_and_overrides_for_user(
assignment.context,
user,
existing_overrides
)
remaining_overrides = overrides_after_defunct_removed(assignment,
existing_overrides,
overrides_params,
invisible_override_ids)
overrides = overrides_params.map do |override_params|
override = get_override_from_params(override_params, assignment, remaining_overrides)
update_override_with_invisible_data(override_params, override, invisible_override_ids, invisible_user_ids)
data, errors = interpret_assignment_override_data(assignment, override_params, override.set_type)
if errors
override.errors.add(errors)
# add the errors to the assignment so that they are caught on
# the Api::V1::Assignment#update_api_assignment
# to enact intended behavior
assignment.errors.add(:base, errors.join(','))
else
update_assignment_override_without_save(override, data)
end

18
lib/assignment_override_applicator.rb
View File

@ -78,20 +78,30 @@ module AssignmentOverrideApplicator
Rails.cache.fetch(cache_key) do
next [] if self.has_invalid_args?(assignment_or_quiz, user)
context = assignment_or_quiz.context
overrides = []
visible_user_ids = UserSearch.scope_for(context, user, { force_users_visible_to: true }).map(&:id)
if context.grants_right?(user, :read_as_admin)
overrides = assignment_or_quiz.assignment_overrides
if assignment_or_quiz.current_version?
overrides = overrides.loaded? ?
overrides.select{|o| o.workflow_state == 'active'} :
overrides.active.to_a
overrides = if overrides.loaded?
ovs = overrides.select do |ov|
ov.workflow_state == 'active' &&
ov.set_type != 'ADHOC'
end
ovs + overrides.select{ |ov| ov.visible_student_overrides(visible_user_ids) }
else
ovs = overrides.active.where.not(set_type: 'ADHOC').to_a
ovs + overrides.active.visible_students_only(visible_user_ids).to_a
end
else
overrides = current_override_version(assignment_or_quiz, overrides)
end
return overrides
end
overrides = []
# priority: adhoc, group, section (do not exclude deleted)
adhoc = adhoc_override(assignment_or_quiz, user)
overrides << adhoc.assignment_override if adhoc

4
lib/dates_overridable.rb
View File

@ -29,8 +29,8 @@ module DatesOverridable
# All overrides, not just dates
def overrides_for(user, opts={})
overrides = AssignmentOverrideApplicator.overrides_for_assignment_and_user(self, user)
if opts[:ensure_set_not_empty]
overrides.select(&:set_not_empty?)
if opts[:ensure_set_not_empty]
overrides.select(&:set_not_empty?)
else
overrides
end

2
lib/user_search.rb
View File

@ -53,7 +53,7 @@ module UserSearch
users = if context.is_a?(Account)
User.of_account(context).active
elsif context.is_a?(Course)
elsif context.is_a?(Course) && !options.fetch(:force_users_visible_to, false)
context.users_visible_to(searcher, include_prior_enrollments,
enrollment_state: enrollment_states, include_inactive: include_inactive_enrollments).uniq
else

103
spec/lib/api/v1/assignment_override_spec.rb
View File

@ -16,11 +16,112 @@ describe "Api::V1::AssignmentOverride" do
}
subj = Subject.new
subj.stubs(:api_find_all).returns []
assignment = stub(:context => stub(:students_visible_to) )
assignment = stub(:context => stub(:students => stub(:active)))
result = subj.interpret_assignment_override_data(assignment, override,'ADHOC')
expect(result.first[:due_at]).to eq nil
expect(result.first[:unlock_at]).to eq nil
expect(result.first[:lock_at]).to eq nil
end
end
describe 'overrides retrieved for teacher' do
before :once do
course_model
@override = assignment_override_model
@subj = Subject.new
end
context 'in restricted course section' do
before do
2.times{ @course.course_sections.create! }
@section_invisible = @course.active_course_sections[2]
@section_visible = @course.active_course_sections.second
@student_invisible = student_in_section(@section_invisible)
@student_visible = student_in_section(@section_visible, user: user)
@teacher = teacher_in_section(@section_visible, user: user)
enrollment = @teacher.enrollments.first
enrollment.limit_privileges_to_course_section = true
enrollment.save!
end
context '#invisble_users_and_overrides_for_user' do
before do
@override.set_type = "ADHOC"
@override_student = @override.assignment_override_students.build
@override_student.user = @student_visible
@override_student.save!
end
it "returns the invisible_student's id in first param" do
@override_student = @override.assignment_override_students.build
@override_student.user = @student_invisible
@override_student.save!
invisible_ids, _ = @subj.invisible_users_and_overrides_for_user(
@course, @teacher, @assignment.assignment_overrides.active
)
expect(invisible_ids).to include(@student_invisible.id)
end
it "returns the invisible_override in the second param" do
override_invisible = @override.assignment.assignment_overrides.create
override_invisible.set_type = "ADHOC"
override_student = override_invisible.assignment_override_students.build
override_student.user = @student_invisible
override_student.save!
_, invisible_overrides = @subj.invisible_users_and_overrides_for_user(
@course, @teacher, @assignment.assignment_overrides.active
)
expect(invisible_overrides.first).to eq override_invisible.id
end
end
end
context 'with no restrictions' do
before do
2.times do @course.course_sections.create! end
@section_invisible = @course.active_course_sections[2]
@section_visible = @course.active_course_sections.second
@student_invisible = student_in_section(@section_invisible)
@student_visible = student_in_section(@section_visible, user: user)
end
context '#invisble_users_and_overrides_for_user' do
before do
@override.set_type = "ADHOC"
@override_student = @override.assignment_override_students.build
@override_student.user = @student_visible
@override_student.save!
end
it "does not return the invisible student's param in first param" do
@override_student = @override.assignment_override_students.build
@override_student.user = @student_invisible
@override_student.save!
invisible_ids, _ = @subj.invisible_users_and_overrides_for_user(
@course, @teacher, @assignment.assignment_overrides.active
)
expect(invisible_ids).to_not include(@student_invisible.id)
end
it "returns no override ids in the second param" do
override_invisible = @override.assignment.assignment_overrides.create
override_invisible.set_type = "ADHOC"
override_student = override_invisible.assignment_override_students.build
override_student.user = @student_invisible
override_student.save!
_, invisible_overrides = @subj.invisible_users_and_overrides_for_user(
@course, @teacher, @assignment.assignment_overrides.active
)
expect(invisible_overrides).to be_empty
end
end
end
end
end

96
spec/lib/dates_overridable_spec.rb
View File

@ -98,6 +98,102 @@ shared_examples_for "an object whose dates are overridable" do
end
end
describe "override teacher visibility" do
context "when teacher restricted" do
before do
2.times{ course.course_sections.create! }
@section_invisible = course.active_course_sections[2]
@section_visible = course.active_course_sections.second
@student_invisible = student_in_section(@section_invisible)
@student_visible = student_in_section(@section_visible, user: user)
@teacher = teacher_in_section(@section_visible, user: user)
enrollment = @teacher.enrollments.first
enrollment.limit_privileges_to_course_section = true
enrollment.save!
end
it "returns empty for overrides of student in other section" do
override.set_type = "ADHOC"
@override_student = override.assignment_override_students.build
@override_student.user = @student_invisible
@override_student.save!
expect(overridable.overrides_for(@teacher)).to be_empty
end
it "returns not empty for overrides of student in same section" do
override.set_type = "ADHOC"
@override_student = override.assignment_override_students.build
@override_student.user = @student_visible
@override_student.save!
expect(overridable.overrides_for(@teacher)).to_not be_empty
end
it "returns the correct student for override with students in same and different section" do
override.set_type = "ADHOC"
@override_student = override.assignment_override_students.build
@override_student.user = @student_visible
@override_student.save!
@override_student = override.assignment_override_students.build
@override_student.user = @student_invisible
@override_student.save!
expect(overridable.overrides_for(@teacher).size).to eq 1
ov = overridable.overrides_for(@teacher).first
s_id = ov.assignment_override_students.first.user_id
expect(s_id).to eq @student_visible.id
end
end
context "when teacher not restricted" do
before do
course.course_sections.create!
course.course_sections.create!
@section_invisible = course.active_course_sections[2]
@section_visible = course.active_course_sections.second
@student_invisible = student_in_section(@section_invisible)
@student_visible = student_in_section(@section_visible, user: user)
@teacher = teacher_in_section(@section_visible, user: user)
end
it "returns not empty for overrides of student in other section" do
override.set_type = "ADHOC"
@override_student = override.assignment_override_students.build
@override_student.user = @student_invisible
@override_student.save!
expect(overridable.overrides_for(@teacher)).to_not be_empty
end
it "returns not empty for overrides of student in same section" do
override.set_type = "ADHOC"
@override_student = override.assignment_override_students.build
@override_student.user = @student_visible
@override_student.save!
expect(overridable.overrides_for(@teacher)).to_not be_empty
end
it "returns two for override of student in same section and different section" do
override.set_type = "ADHOC"
@override_student = override.assignment_override_students.build
@override_student.user = @student_visible
@override_student.save!
@override_student = override.assignment_override_students.build
@override_student.user = @student_invisible
@override_student.save!
expect(overridable.overrides_for(@teacher).size).to eq 2
end
end
end
describe "has_overrides?" do
subject { overridable.has_overrides? }