include observers in assignment notifications

fixes CNVS-8728

test plan:
  - DA off
    - set up an observer following a student
    - set up an observer in course without a student
    - create an assignment in this course
    - both observers and the student should be
      notified of the assignments creation
    - remove an enrollment from an observer user
      - create another assignment and this user
        should not be notified
  - DA on
    - set up an observer following a student
      (we'll call him observer A)
    - set up an observer following a student
      in a different section (observer B)
    - set up an observer in course without a student
      (observer C)
    - create an assignment in this course for only
      the section that observer A's student is in
    - observer A and observer C should be notified of
      the assignment, observer B should not be

Change-Id: I5ff01cc9a6f19e8c7dca4f806fe48b9d6d8dbe36
Reviewed-on: https://gerrit.instructure.com/45853
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Sean Lewis <slewis@instructure.com>
Product-Review: Hilary Scharton <hilary@instructure.com>

app/models/assignment.rb

@@ -487,7 +487,8 @@ class Assignment < ActiveRecord::Base
     p.to {
       # everyone who is _not_ covered by an assignment override affecting due_at
       # (the AssignmentOverride records will take care of notifying those users)
-      participants - participants_with_overridden_due_at
+      excluded_ids = participants_with_overridden_due_at.map(&:id).to_set
+      participants(include_observers: true, excluded_user_ids: excluded_ids)
     }
     p.whenever { |assignment|
       policy = BroadcastPolicies::AssignmentPolicy.new( assignment )
@@ -495,14 +496,14 @@ class Assignment < ActiveRecord::Base
     }
 
     p.dispatch :assignment_changed
-    p.to { participants }
+    p.to { participants(include_observers: true) }
     p.whenever { |assignment|
       policy = BroadcastPolicies::AssignmentPolicy.new( assignment )
       policy.should_dispatch_assignment_changed?
     }
 
     p.dispatch :assignment_created
-    p.to { participants }
+    p.to { participants(include_observers: true) }
     p.whenever { |assignment|
       policy = BroadcastPolicies::AssignmentPolicy.new( assignment )
       policy.should_dispatch_assignment_created?
@@ -512,7 +513,7 @@ class Assignment < ActiveRecord::Base
     }
 
     p.dispatch :assignment_unmuted
-    p.to { participants }
+    p.to { participants(include_observers: true) }
     p.whenever { |assignment|
       assignment.recently_unmuted
     }
@@ -855,14 +856,27 @@ class Assignment < ActiveRecord::Base
     end
   end
 
-  def participants
-    return context.participants unless differentiated_assignments_applies?
-    participants_with_visibility
+  def participants(opts={})
+    return context.participants(opts[:include_observers], excluded_user_ids: opts[:excluded_user_ids]) unless differentiated_assignments_applies?
+    participants_with_visibility(opts)
   end
 
-  def participants_with_visibility
+  def participants_with_visibility(opts={})
     users = context.participating_admins
-    users += students_with_visibility
+
+    applicable_students = if opts[:excluded_user_ids]
+                            students_with_visibility.reject{|s| opts[:excluded_user_ids].include?(s.id)}
+                          else
+                            students_with_visibility
+                          end
+
+    users += applicable_students
+
+    if opts[:include_observers]
+      users += User.observing_students_in_course(applicable_students.map(&:id), self.context_id)
+      users += User.observing_full_course(context.id)
+    end
+
     users.uniq
   end
 

app/models/course.rb

@@ -1609,8 +1609,28 @@ class Course < ActiveRecord::Base
     end
   end
 
-  def participants(include_observers=false)
-    (participating_admins + participating_students + (include_observers ? participating_observers : [])).uniq
+  def active_course_level_observers
+    participating_observers.observing_full_course(self.id)
+  end
+
+  def participants(include_observers=false, opts={})
+    participants = []
+    participants += participating_admins
+
+    applicable_students = if opts[:excluded_user_ids]
+                 participating_students.reject{|p| opts[:excluded_user_ids].include?(p.id)}
+               else
+                 participating_students
+               end
+
+    participants += applicable_students
+
+    if include_observers
+      participants += User.observing_students_in_course(applicable_students.map(&:id), self.id)
+      participants += User.observing_full_course(self.id)
+    end
+
+    participants.uniq
   end
 
   def enroll_user(user, type='StudentEnrollment', opts={})

app/models/user.rb

@@ -205,6 +205,24 @@ class User < ActiveRecord::Base
     where(:quiz_student_visibilities => { :quiz_id => quiz_id, :course_id => course_id })
   }
 
+  scope :observing_students_in_course, lambda {|observee_ids, course_ids|
+    joins(:enrollments).where(enrollments: {type: 'ObserverEnrollment', associated_user_id: observee_ids, course_id: course_ids, workflow_state: 'active'})
+  }
+
+  # when an observer is added to a course they get an enrollment where associated_user_id is nil. when they are linked to
+  # a student, this first enrollment stays the same, but a new one with an associated_user_id is added. thusly to find
+  # course observers, you take the difference between all active observers and active observers with associated users
+  scope :observing_full_course, lambda {|course_ids|
+    active_observer_scope = joins(:enrollments).where(enrollments: {type: 'ObserverEnrollment', course_id: course_ids, workflow_state: 'active'})
+    users_observing_students = active_observer_scope.where("enrollments.associated_user_id IS NOT NULL").pluck(:id)
+
+    if users_observing_students == [] || users_observing_students == nil
+      active_observer_scope
+    else
+      active_observer_scope.where("users.id NOT IN (?)", users_observing_students)
+    end
+  }
+
   def assignment_and_quiz_visibilities(opts = {})
      opts = {user_id: self.id}.merge(opts)
      {assignment_ids: AssignmentStudentVisibility.where(opts).order('assignment_id desc').pluck(:assignment_id),

spec/models/assignment_spec.rb

@@ -1426,6 +1426,38 @@ describe Assignment do
         expect(@assignment.participants.include?(@teacher)).to be_truthy
         expect(@assignment.participants.include?(@ta)).to be_truthy
       end
+
+      context "including observers" do
+        before do
+          oe = @assignment.context.enroll_user(user_with_pseudonym(active_all: true), 'ObserverEnrollment',:enrollment_state => 'active')
+          @course_level_observer = oe.user
+
+          oe = @assignment.context.enroll_user(user_with_pseudonym(active_all: true), 'ObserverEnrollment',:enrollment_state => 'active')
+          oe.associated_user_id = @student1.id
+          oe.save!
+          @student1_observer = oe.user
+
+          oe = @assignment.context.enroll_user(user_with_pseudonym(active_all: true), 'ObserverEnrollment',:enrollment_state => 'active')
+          oe.associated_user_id = @student2.id
+          oe.save!
+          @student2_observer = oe.user
+        end
+
+        it "should include course_level observers" do
+          expect(@assignment.participants(include_observers: true).include?(@course_level_observer)).to be_truthy
+        end
+
+        it "should exclude student observers if their student does not have visibility" do
+          expect(@assignment.participants(include_observers: true).include?(@student1_observer)).to be_truthy
+          expect(@assignment.participants(include_observers: true).include?(@student2_observer)).to be_falsey
+        end
+
+        it "should exclude all observers unless opt is given" do
+          expect(@assignment.participants.include?(@student1_observer)).to be_falsey
+          expect(@assignment.participants.include?(@student2_observer)).to be_falsey
+          expect(@assignment.participants.include?(@course_level_observer)).to be_falsey
+        end
+      end
     end
 
     describe "with differentiated_assignments off" do
@@ -1434,9 +1466,37 @@ describe Assignment do
         @course.disable_feature!(:differentiated_assignments)
       end
 
-      it 'returns all users in the course' do
+      it 'normally returns all users in the course' do
         expect(@assignment.participants.length).to eq 4
       end
+
+      it "should exclude students when given the option" do
+        expect( @assignment.participants(excluded_user_ids: [@student1.id]) ).to_not be_include(@student1)
+        expect( @assignment.participants(excluded_user_ids: [@student1.id]) ).to be_include(@student2)
+      end
+
+      context "including observers" do
+        before :once do
+          oe = @assignment.context.enroll_user(user_with_pseudonym, 'ObserverEnrollment',:enrollment_state => 'active')
+          @course_level_observer = oe.user
+
+          oe = @assignment.context.enroll_user(user_with_pseudonym, 'ObserverEnrollment',:enrollment_state => 'active')
+          oe.associated_user_id = @student1.id
+          oe.save!
+          @student1_observer = oe.user
+        end
+        it "should include course_level observers" do
+          expect( @assignment.participants(include_observers: true) ).to be_include(@course_level_observer)
+        end
+        it "should include student observers if their student is participating" do
+          expect( @assignment.participants(include_observers: true) ).to be_include(@student1)
+          expect( @assignment.participants(include_observers: true) ).to be_include(@student1_observer)
+        end
+        it "should exclude student observers if their student is explicitly excluded" do
+          expect( @assignment.participants(include_observers: true, excluded_user_ids: [@student1.id]) ).to_not be_include(@student1)
+          expect( @assignment.participants(include_observers: true, excluded_user_ids: [@student1.id]) ).to_not be_include(@student1_observer)
+        end
+      end
     end
   end
 

spec/models/course_spec.rb

@@ -679,6 +679,59 @@ describe Course do
   end
 end
 
+
+describe Course, "participants" do
+  before :once do
+    @course = Course.create(:name => "some_name")
+    se = @course.enroll_student(user_with_pseudonym,:enrollment_state => 'active')
+    tae = @course.enroll_ta(user_with_pseudonym,:enrollment_state => 'active')
+    te = @course.enroll_teacher(user_with_pseudonym,:enrollment_state => 'active')
+    @student, @ta, @teach = [se, tae, te].map(&:user)
+  end
+
+  context "vanilla usage" do
+    it "should return participating_admins and participating_students" do
+      [@student, @ta, @teach].each { |usr| expect(@course.participants).to be_include(usr) }
+    end
+  end
+
+  context "including obervers" do
+    before :once  do
+      oe = @course.enroll_user(user_with_pseudonym, 'ObserverEnrollment',:enrollment_state => 'active')
+      @course_level_observer = oe.user
+
+      oe = @course.enroll_user(user_with_pseudonym, 'ObserverEnrollment',:enrollment_state => 'active')
+      oe.associated_user_id = @student.id
+      oe.save!
+      @student_following_observer = oe.user
+    end
+
+    it "should return participating_admins, participating_students, and observers" do
+      participants = @course.participants(true)
+      [@student, @ta, @teach, @course_level_observer, @student_following_observer].each do |usr|
+        expect(participants).to be_include(usr)
+      end
+    end
+
+    context "excluding specific students" do
+      it "should reject observers only following one of the excluded students" do
+        partic = @course.participants(true, excluded_user_ids: [@student.id, @student_following_observer.id])
+        [@student, @student_following_observer].each { |usr| expect(partic).to_not be_include(usr) }
+      end
+      it "should include admins and course level observers" do
+        partic = @course.participants(true, excluded_user_ids: [@student.id, @student_following_observer.id])
+        [@ta, @teach, @course_level_observer].each { |usr| expect(partic).to be_include(usr) }
+      end
+    end
+  end
+
+  it "should exclude some student when passed their id" do
+    partic = @course.participants(false, excluded_user_ids: [@student.id])
+    [@ta, @teach].each { |usr| expect(partic).to be_include(usr) }
+    expect(partic).to_not be_include(@student)
+  end
+end
+
 describe Course, "enroll" do
 
   before :once do
@@ -1353,6 +1406,16 @@ describe Course, "tabs_available" do
       tab_ids = @course.tabs_available(@user).map{|t| t[:id] }
       expect(tab_ids).not_to be_include(Course::TAB_DISCUSSIONS)
     end
+
+    it "should recognize active_course_level_observers" do
+      user = user_with_pseudonym
+      observer_enrollment = @course.enroll_user(user, 'ObserverEnrollment',:enrollment_state => 'active')
+      @course_level_observer = observer_enrollment.user
+
+      course_observers = @course.active_course_level_observers
+      expect(course_observers).to be_include(@course_level_observer)
+      expect(course_observers).to_not be_include(@oe.user)
+    end
   end
 
   context "a public course" do