DA - quiz/submission/discussion notifications

fixes CNVS-15156

test plan:
  - with DA on:
    - submission:
      - make a differentiated assignment
      - turn in a submission as a student
      - give the student a grade
        - as the student, ensure you have the notification, and then
          delete it
      - as a teacher, take away visibility and delete their grade
        - as the student, you should not have a new notification for grade
          changing
      - as a teacher, comment on this submission
        - as the student, you get notified about the comment
    - quiz:
      - as a teacher make a differentiated quiz
        - students without visibility dont get notified about it
    - discussion:
      - discussion notifications are broken so we cant QA
  - with DA off:
    - submission:
      - comment on a submission as a teacher and make sure the
        student is notified
      - grade a student as a teacher and make sure the student
        is notified
    - quiz:
      - making & assignment a quiz sends out notifications properly
    - discussion:
      - discussion notifications are broken so we cant QA

Change-Id: I891f9782b5ae70252e7949a6858896b73ac28871
Reviewed-on: https://gerrit.instructure.com/41025
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Cameron Sutter <csutter@instructure.com>
QA-Review: Sean Lewis <slewis@instructure.com>
Product-Review: Hilary Scharton <hilary@instructure.com>

Author: Michael Nomitch

app/models/assignment_override.rb

@@ -219,6 +219,17 @@ def applies_to_students
     end
   end
 
+  def applies_to_students_with_visibility
+    return applies_to_students unless self.assignment.try(:differentiated_assignments_applies?) || self.quiz.try(:differentiated_assignments_applies?)
+    if self.quiz
+      applies_to_students.able_to_see_quiz_in_course_with_da(self.quiz_id, context.id)
+    elsif self.assignment
+      applies_to_students.able_to_see_assignment_in_course_with_da(self.assignment_id, context.id)
+    else
+      applies_to_students
+    end
+  end
+
   def applies_to_admins
     case set_type
     when 'CourseSection'
@@ -242,7 +253,7 @@ def notify_change?
   has_a_broadcast_policy
   set_broadcast_policy do |p|
     p.dispatch :assignment_due_date_changed
-    p.to { applies_to_students }
+    p.to { applies_to_students_with_visibility }
     p.whenever { |record| record.notify_change? }
     p.filter_asset_by_recipient { |record, user|
       # note that our asset for this message is an Assignment, not an AssignmentOverride

app/models/broadcast_policies/quiz_submission_policy.rb

@@ -15,13 +15,15 @@ def should_dispatch_submission_graded?
     def should_dispatch_submission_grade_changed?
       quiz_is_accepting_messages? &&
       quiz_submission.submission.graded_at &&
-      quiz_submission.changed_in_state(:complete, :fields => [:score])
+      quiz_submission.changed_in_state(:complete, :fields => [:score]) &&
+      user_has_visibility?
     end
 
     def should_dispatch_submission_needs_grading?
       !quiz.survey? &&
       quiz_is_accepting_messages? &&
-      quiz_submission.pending_review?
+      quiz_submission.pending_review? &&
+      user_has_visibility?
     end
 
     private
@@ -40,5 +42,10 @@ def quiz_is_accepting_messages?
     def manually_graded
       quiz_submission.changed_in_state(:pending_review, :fields => [:fudge_points])
     end
+
+    def user_has_visibility?
+      return true unless quiz_submission.context.feature_enabled?(:differentiated_assignments)
+      Quizzes::QuizStudentVisibility.where(quiz_id: quiz.id, user_id: quiz_submission.user_id).any?
+    end
   end
 end

app/models/broadcast_policies/submission_policy.rb

@@ -43,14 +43,16 @@ def should_dispatch_group_assignment_submitted_late?
 
     def should_dispatch_submission_graded?
       broadcasting_grades? &&
+      user_has_visibility? &&
       (submission.changed_state_to(:graded) || (grade_updated? && graded_recently?))
     end
 
     def should_dispatch_submission_grade_changed?
       broadcasting_grades? &&
       submission.graded_at &&
       !graded_recently? &&
-      grade_updated?
+      grade_updated? &&
+      user_has_visibility?
     end
 
     private
@@ -94,5 +96,9 @@ def graded_recently?
       submission.assignment_graded_in_the_last_hour?
     end
 
+    def user_has_visibility?
+      return true unless submission.context.feature_enabled?(:differentiated_assignments)
+      AssignmentStudentVisibility.where(assignment_id: submission.assignment_id, user_id: submission.user_id).any?
+    end
   end
 end

app/models/discussion_topic.rb

@@ -837,7 +837,7 @@ def ensure_submission(user)
 
   set_broadcast_policy do |p|
     p.dispatch :new_discussion_topic
-    p.to { active_participants - [user] }
+    p.to { active_participants_with_visibility - [user] }
     p.whenever { |record|
       record.context.available? and
         !record.context.concluded? and
@@ -862,6 +862,15 @@ def active_participants(include_observers=false)
     end
   end
 
+  def active_participants_with_visibility
+    return active_participants unless context.feature_enabled?(:differentiated_assignments)
+    users_with_visibility = AssignmentStudentVisibility.where(assignment_id: self.assignment_id).pluck(:user_id)
+    # observers will not be returned, which is okay for the functions current use cases (but potentially not others)
+    instructor_ids = context.participating_instructors.pluck(:id)
+    users_with_visibility.concat(instructor_ids)
+    active_participants.select{|p| users_with_visibility.include?(p.id)}
+  end
+
   def participating_users(user_ids)
     context.respond_to?(:participating_users) ? context.participating_users(user_ids) : User.find(user_ids)
   end
@@ -874,7 +883,23 @@ def subscribers
     poster_ids = posters.map(&:id)
     legacy_sub_ids &= poster_ids
     sub_ids += legacy_sub_ids
-    participating_users(sub_ids)
+
+    subscribed_users = participating_users(sub_ids)
+
+    if context.feature_enabled?(:differentiated_assignments) && self.for_assignment?
+      students_with_visibility = AssignmentStudentVisibility.where(course_id: context_id, assignment_id: assignment_id).pluck(:user_id)
+      admin_ids = self.context.participating_admins.pluck(:id)
+      observer_ids = self.context.participating_observers.pluck(:id)
+      observed_students = ObserverEnrollment.observed_student_ids_by_observer_id(self.context,observer_ids)
+
+      subscribed_users.select!{ |user|
+        students_with_visibility.include?(user.id) || admin_ids.include?(user.id) ||
+        # an observer with no students or one with students who have visibility
+        (observed_students[user.id] && (observed_students[user.id] == [] || (observed_students[user.id] & students_with_visibility).any?))
+      }
+    end
+
+    subscribed_users
   end
 
   def posters

app/models/observer_enrollment.rb

@@ -43,4 +43,16 @@ def self.observed_student_ids(context, current_user)
       context.observer_enrollments.where("user_id=? AND associated_user_id IS NOT NULL", current_user).pluck(:associated_user_id)
     end
   end
+
+  def self.observed_student_ids_by_observer_id(course, observers)
+    # select_all allows plucking multiplecolumns without instantiating AR objects
+    obs_hash = connection.select_all( ObserverEnrollment.where(course_id: course, user_id: observers).select([:user_id, :associated_user_id])).group_by{|record| record["user_id"]}
+    obs_hash.keys.each{ |key|
+      obs_hash[key.to_i] = obs_hash.delete(key).map{|v|
+        v["associated_user_id"].try(:to_i)
+      }.compact
+    }
+    # should look something like this: {10 => [11,12,13], 20 => [11,24,32]}
+    obs_hash
+  end
 end

lib/dates_overridable.rb

@@ -73,7 +73,7 @@ def all_due_dates
   def differentiated_assignments_applies?
     return false if !context.feature_enabled?(:differentiated_assignments)
 
-    if self.is_a?(Assignment) || self.is_a?(Quizzes::Quiz)
+    if self.is_a?(Assignment) || Quizzes::Quiz.class_names.include?(self.class_name)
       self.only_visible_to_overrides
     elsif self.assignment
       self.assignment.only_visible_to_overrides

spec/models/broadcast_policies/quiz_submission_policy_spec.rb

@@ -6,6 +6,7 @@ module BroadcastPolicies
     let(:course) do
       mock("Course").tap do |c|
         c.stubs(:available?).returns(true)
+        c.stubs(:feature_enabled?).with(:differentiated_assignments).returns(false)
       end
     end
     let(:assignment) do
@@ -41,6 +42,7 @@ module BroadcastPolicies
         qs.stubs(:quiz).returns(quiz)
         qs.stubs(:submission).returns(submission)
         qs.stubs(:user).returns(user)
+        qs.stubs(:context).returns(course)
       end
     end
     let(:policy) { QuizSubmissionPolicy.new(quiz_submission) }
@@ -92,6 +94,7 @@ def wont_send_when
       specify { wont_send_when { quiz.stubs(:deleted?).returns true } }
       specify { wont_send_when { submission.stubs(:graded_at).returns nil }}
       specify { wont_send_when { submission.stubs(:pending_review?).returns false }}
+      specify { wont_send_when { submission.stubs(:user_has_visibility).returns false }}
     end
 
 
@@ -115,6 +118,7 @@ def wont_send_when
       specify { wont_send_when { course.stubs(:available?).returns false} }
       specify { wont_send_when { quiz.stubs(:deleted?).returns true } }
       specify { wont_send_when { submission.stubs(:graded_at).returns nil }}
+      specify { wont_send_when { submission.stubs(:user_has_visibility).returns false }}
 
       specify do
         wont_send_when do

spec/models/broadcast_policies/submission_policy_spec.rb

@@ -7,6 +7,7 @@ module BroadcastPolicies
       stub("Course").tap do |c|
         c.stubs(:available?).returns(true)
         c.stubs(:id).returns(1)
+        c.stubs(:feature_enabled?).with(:differentiated_assignments).returns(false)
       end
     end
     let(:assignment) do
@@ -48,6 +49,7 @@ module BroadcastPolicies
         s.stubs(:late?).returns(false)
         s.stubs(:quiz_submission).returns(nil)
         s.stubs(:user).returns(user)
+        s.stubs(:context).returns(course)
       end
     end
 
@@ -177,6 +179,7 @@ def wont_send_when
       specify { wont_send_when{ submission.stubs(:quiz_submission).returns stub }}
       specify { wont_send_when{ course.stubs(:available?).returns false }}
       specify { wont_send_when{ assignment.stubs(:published?).returns false }}
+      specify { wont_send_when{ SubmissionPolicy.any_instance.stubs(:user_has_visibility?).returns(false)}}
     end
 
   end

spec/models/discussion_topic_spec.rb

@@ -191,7 +191,8 @@
     context "differentiated assignements" do
       before do
         @course = course(:active_course => true)
-        discussion_topic_model(:user => @teacher)
+        discussion_topic_model(:user => @teacher, :context => @course)
+        @course.enroll_teacher(@teacher).accept!
         @course_section = @course.course_sections.create
         @student1, @student2, @student3 = create_users(3, return_type: :record)
 
@@ -223,6 +224,14 @@
         it "should be visible to a teacher" do
           @topic.visible_for?(@teacher).should be_true
         end
+        context "active_participants_with_visibility" do
+          it "should filter participants by visibility" do
+            [@student1, @teacher].each do |user|
+              @topic.active_participants_with_visibility.include?(user).should be_true
+            end
+            @topic.active_participants_with_visibility.include?(@student2).should be_false
+          end
+        end
       end
 
       context "feature flag off" do
@@ -232,6 +241,13 @@
             @topic.visible_for?(user).should be_true
           end
         end
+        context "active_participants_with_visibility" do
+          it "should not filter any participants" do
+            [@student1,@student2].each do |user|
+              @topic.active_participants_with_visibility.include?(user).should be_true
+            end
+          end
+        end
       end
     end
   end
@@ -738,6 +754,52 @@ def delayed_discussion_topic(opts = {})
       @topic.subscribers.should_not include(@student)
       @topic2.subscribers.should_not include(@student)
     end
+
+    context "differentiated_assignments" do
+      before do
+        @assignment = @course.assignments.create!(:title => "some discussion assignment",only_visible_to_overrides: true)
+        @assignment.submission_types = 'discussion_topic'
+        @assignment.save!
+        @topic.assignment_id = @assignment.id
+        @topic.save!
+        @section = @course.course_sections.create!(name: "test section")
+        create_section_override_for_assignment(@topic.assignment, {course_section: @section})
+      end
+      context "enabled" do
+        before{@course.enable_feature!(:differentiated_assignments)}
+        it "should filter subscribers based on visibility" do
+          @topic.subscribe(@student)
+          @topic.subscribers.should_not include(@student)
+          student_in_section(@section, user: @student)
+          @topic.subscribers.should include(@student)
+        end
+
+        it "filters observers if their student cant see" do
+          @observer = user(:active_all => true, :name => "Observer")
+          observer_enrollment = @course.enroll_user(@observer, 'ObserverEnrollment', :section => @section, :enrollment_state => 'active')
+          observer_enrollment.update_attribute(:associated_user_id, @student.id)
+          @topic.subscribe(@observer)
+          @topic.subscribers.include?(@observer).should be_false
+          student_in_section(@section, user: @student)
+          @topic.subscribers.include?(@observer).should be_true
+        end
+
+        it "doesnt filter for observers with no student" do
+          @observer = user(:active_all => true)
+          observer_enrollment = @course.enroll_user(@observer, 'ObserverEnrollment', :section => @section, :enrollment_state => 'active')
+          @topic.subscribe(@observer)
+          @topic.subscribers.should include(@observer)
+        end
+
+      end
+      context "disabled" do
+        before{@course.disable_feature!(:differentiated_assignments)}
+        it "should not filter subscribers based on visibility" do
+          @topic.subscribe(@student)
+          @topic.subscribers.should include(@student)
+        end
+      end
+    end
   end
 
   context "posters" do

spec/models/observer_enrollment_spec.rb

@@ -19,17 +19,26 @@
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
 
 describe ObserverEnrollment do
+  before do
+    course(:active_all => 1)
+    @student = user
+    @observer = user
+    @student_enrollment = @course.enroll_student(@student)
+    @observer_enrollment = @course.enroll_user(@observer, 'ObserverEnrollment')
+    @observer_enrollment.update_attribute(:associated_user_id, @student.id)
+  end
   describe 'observed_students' do
     it "should not fail if the observed has been deleted" do
-      course(:active_all => 1)
-      @student = user
-      @observer = user
-      @student_enrollment = @course.enroll_student(@student)
-      @observer_enrollment = @course.enroll_user(@observer, 'ObserverEnrollment')
-      @observer_enrollment.update_attribute(:associated_user_id, @student.id)
       ObserverEnrollment.observed_students(@course, @observer).should == { @student => [@student_enrollment]}
       @student_enrollment.destroy
       ObserverEnrollment.observed_students(@course, @observer).should == {}
     end
   end
+  describe 'observed_student_ids_by_observer_id' do
+    it "should return a properly formatted hash" do
+      @observer_two = user
+      @observer_enrollment_two = @course.enroll_user(@observer_two, 'ObserverEnrollment')
+      ObserverEnrollment.observed_student_ids_by_observer_id(@course, [@observer.id,@observer_two.id]).should == {@observer.id => [@student.id], @observer_two.id => []}
+    end
+  end
 end