From 0fad3e1db6354d40b17073db168c86ab6ac4d013 Mon Sep 17 00:00:00 2001 From: Marc Alan Phillips Date: Thu, 18 Feb 2016 02:24:07 -0600 Subject: [PATCH] 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 QA-Review: Michael Hargiss Product-Review: Jason Sparks --- .../assignment_overrides_controller.rb | 2 +- app/controllers/assignments_controller.rb | 3 +- app/controllers/quizzes/quizzes_controller.rb | 9 +- app/models/assignment_override.rb | 16 ++- lib/api/v1/assignment.rb | 5 +- lib/api/v1/assignment_override.rb | 70 ++++++++++-- lib/assignment_override_applicator.rb | 18 ++- lib/dates_overridable.rb | 4 +- lib/user_search.rb | 2 +- spec/lib/api/v1/assignment_override_spec.rb | 103 +++++++++++++++++- spec/lib/dates_overridable_spec.rb | 96 ++++++++++++++++ 11 files changed, 301 insertions(+), 27 deletions(-) diff --git a/app/controllers/assignment_overrides_controller.rb b/app/controllers/assignment_overrides_controller.rb index 272f1fd2300..622d78a304b 100644 --- a/app/controllers/assignment_overrides_controller.rb +++ b/app/controllers/assignment_overrides_controller.rb @@ -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 diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index 5e3526dfd39..c1e86b0d5a3 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -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), diff --git a/app/controllers/quizzes/quizzes_controller.rb b/app/controllers/quizzes/quizzes_controller.rb index 38a8bd96725..e6d481a7643 100644 --- a/app/controllers/quizzes/quizzes_controller.rb +++ b/app/controllers/quizzes/quizzes_controller.rb @@ -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) diff --git a/app/models/assignment_override.rb b/app/models/assignment_override.rb index 7b0655d0a3d..9539d80ac75 100644 --- a/app/models/assignment_override.rb +++ b/app/models/assignment_override.rb @@ -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 diff --git a/lib/api/v1/assignment.rb b/lib/api/v1/assignment.rb index ed495f6cb8a..9e92f3bad32 100644 --- a/lib/api/v1/assignment.rb +++ b/lib/api/v1/assignment.rb @@ -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 diff --git a/lib/api/v1/assignment_override.rb b/lib/api/v1/assignment_override.rb index ea36c4fa6b4..342bd84ff55 100644 --- a/lib/api/v1/assignment_override.rb +++ b/lib/api/v1/assignment_override.rb @@ -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 diff --git a/lib/assignment_override_applicator.rb b/lib/assignment_override_applicator.rb index ff30f1a9654..8227a5b58f8 100644 --- a/lib/assignment_override_applicator.rb +++ b/lib/assignment_override_applicator.rb @@ -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 diff --git a/lib/dates_overridable.rb b/lib/dates_overridable.rb index 3428d50d7d2..98793ebd857 100644 --- a/lib/dates_overridable.rb +++ b/lib/dates_overridable.rb @@ -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 diff --git a/lib/user_search.rb b/lib/user_search.rb index 386c13d4fe7..288431f0a16 100644 --- a/lib/user_search.rb +++ b/lib/user_search.rb @@ -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 diff --git a/spec/lib/api/v1/assignment_override_spec.rb b/spec/lib/api/v1/assignment_override_spec.rb index fe5105b9bac..95c0b2c81e4 100644 --- a/spec/lib/api/v1/assignment_override_spec.rb +++ b/spec/lib/api/v1/assignment_override_spec.rb @@ -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 diff --git a/spec/lib/dates_overridable_spec.rb b/spec/lib/dates_overridable_spec.rb index 8cd628b7d8f..6c79d817ad2 100644 --- a/spec/lib/dates_overridable_spec.rb +++ b/spec/lib/dates_overridable_spec.rb @@ -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? }