fix differentiated assignments discussion stream items

fixes COMMS-246 COMMS-500

test plan:
- create a graded discussion in a course with at least two students
- post in the discussion as the teacher
- without viewing the notification, check the dashboard as each of the
  students and make sure each have 1 discussion notification
- as the teacher, assign that discussion only to the first student
- as the first student check your dashboard and note you still have
  that notification
- as the second student check your dashboard and note you no longer
  see that discussion notification or the discussion itself
- as the teacher make another reply on that discussion
- as the second student note that you don't see a new discussion
  notification or the discussion itself on the dashboard

Change-Id: If5af3878d9e995479b959d799f6ebd7e82a1d14a
Reviewed-on: https://gerrit.instructure.com/132037
Tested-by: Jenkins
Reviewed-by: Cody Cutrer <cody@instructure.com>
Reviewed-by: Venk Natarajan <vnatarajan@instructure.com>
QA-Review: Heath Hales <hhales@instructure.com>
Product-Review: Felix Milea-Ciobanu <fmileaciobanu@instructure.com>

app/models/discussion_topic.rb

@@ -89,6 +89,7 @@ class DiscussionTopic < ActiveRecord::Base
   after_save :touch_context
   after_save :schedule_delayed_transitions
   after_save :update_materialized_view_if_changed
+  after_update :update_stream_items
   after_update :clear_streams_if_not_published
   after_create :create_participant
   after_create :create_materialized_view
@@ -212,6 +213,22 @@ class DiscussionTopic < ActiveRecord::Base
     end
   end
 
+  def unsubscribe_users(users)
+    users.each { |user| self.unsubscribe(user) }
+    self.stream_item.remove_users(users)
+  end
+
+  def update_stream_items
+    return if self.stream_item&.users.nil?
+    return if self.deleted?
+    return unless self.assignment&.context_type == "Course"
+
+    active_users = self.active_participants_with_visibility
+    users_to_remove = self.stream_item.users - active_users
+    self.unsubscribe_users(users_to_remove)
+  end
+  protected :update_stream_items
+
   attr_accessor :saved_by
   def update_assignment
     return if self.deleted?

app/models/stream_item.rb

@@ -113,6 +113,10 @@ class StreamItem < ActiveRecord::Base
     res
   end
 
+  def remove_users(users)
+    self.stream_item_instances.where(user_id: users).try(:destroy_all)
+  end
+
   def prepare_conversation(conversation)
     res = conversation.attributes.slice('id', 'has_attachments', 'updated_at')
     res['private'] = conversation.private?

db/migrate/20171114172137_fix_assignment_overrides_enrollment_order.rb

@@ -0,0 +1,143 @@
+#
+# Copyright (C) 2017 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+class FixAssignmentOverridesEnrollmentOrder < ActiveRecord::Migration[5.0]
+  tag :postdeploy
+
+  def up
+    self.connection.execute "DROP VIEW IF EXISTS #{connection.quote_table_name('assignment_student_visibilities')}"
+
+    # update the view so it looks at overrides first before it looks at enrollments
+    # this fixes some edge cases where adhoc overrides did not work as intended because
+    # they were returning all students in the course instead of just the adhoc students
+    # if the assignment was only visible to overrides and only had adhoc overrides
+    self.connection.execute %(CREATE VIEW #{connection.quote_table_name('assignment_student_visibilities')} AS
+    SELECT DISTINCT
+      a.id as assignment_id,
+      e.user_id as user_id,
+      c.id as course_id
+
+    FROM #{Assignment.quoted_table_name} a
+
+    JOIN #{Course.quoted_table_name} c
+      ON a.context_id = c.id
+      AND a.context_type = 'Course'
+
+    LEFT JOIN #{AssignmentOverride.quoted_table_name} ao
+      ON ao.assignment_id = a.id
+      AND ao.workflow_state = 'active'
+
+    LEFT JOIN #{AssignmentOverrideStudent.quoted_table_name} aos
+      ON aos.assignment_id = a.id
+
+    LEFT JOIN #{Group.quoted_table_name} g
+      ON g.context_type = 'Course'
+      AND g.context_id = c.id
+      AND g.workflow_state = 'available'
+      AND (
+        ao.set_type::text = 'Group'::text AND g.id = ao.set_id
+      )
+
+    LEFT JOIN #{GroupMembership.quoted_table_name} gm
+      ON gm.group_id = g.id
+      AND gm.workflow_state = 'accepted'
+
+    LEFT JOIN #{Submission.quoted_table_name} s
+      ON s.assignment_id = a.id
+      AND s.workflow_state != 'deleted'
+
+    JOIN #{Enrollment.quoted_table_name} e
+      ON e.course_id = c.id
+      AND e.type IN ('StudentEnrollment', 'StudentViewEnrollment')
+      AND e.workflow_state != 'deleted'
+      AND (
+        a.only_visible_to_overrides = false
+        OR s.user_id = e.user_id
+        OR (ao.set_type::text = 'CourseSection'::text AND e.course_section_id = ao.set_id)
+        OR (ao.set_type::text = 'ADHOC'::text AND e.user_id = aos.user_id)
+        OR (ao.set_type::text = 'Group'::text AND e.user_id = gm.user_id)
+      )
+
+    WHERE a.workflow_state NOT IN ('deleted','unpublished')
+      AND(
+        ( a.only_visible_to_overrides = 'true' AND (ao.id IS NOT NULL OR s.id IS NOT NULL))
+        OR (COALESCE(a.only_visible_to_overrides, 'false') = 'false')
+      )
+    )
+  end
+
+  def down
+    self.connection.execute "DROP VIEW IF EXISTS #{connection.quote_table_name('assignment_student_visibilities')}"
+
+    # Recreate the old view so that this migration is reversible
+    self.connection.execute %(CREATE VIEW #{connection.quote_table_name('assignment_student_visibilities')} AS
+      SELECT DISTINCT a.id as assignment_id,
+      e.user_id as user_id,
+      c.id as course_id
+
+      FROM #{Assignment.quoted_table_name} a
+
+      JOIN #{Course.quoted_table_name} c
+        ON a.context_id = c.id
+        AND a.context_type = 'Course'
+
+      JOIN #{Enrollment.quoted_table_name} e
+        ON e.course_id = c.id
+        AND e.type IN ('StudentEnrollment', 'StudentViewEnrollment')
+        AND e.workflow_state != 'deleted'
+
+      JOIN #{CourseSection.quoted_table_name} cs
+        ON cs.course_id = c.id
+        AND e.course_section_id = cs.id
+
+      LEFT JOIN #{GroupMembership.quoted_table_name} gm
+        ON gm.user_id = e.user_id
+        AND gm.workflow_state = 'accepted'
+
+      LEFT JOIN #{Group.quoted_table_name} g
+        ON g.context_type = 'Course'
+        AND g.context_id = c.id
+        AND g.workflow_state = 'available'
+        AND gm.group_id = g.id
+
+      LEFT JOIN #{AssignmentOverrideStudent.quoted_table_name} aos
+        ON aos.assignment_id = a.id
+        AND aos.user_id = e.user_id
+
+      LEFT JOIN #{AssignmentOverride.quoted_table_name} ao
+        ON ao.assignment_id = a.id
+        AND ao.workflow_state = 'active'
+        AND (
+          (ao.set_type = 'CourseSection' AND ao.set_id = cs.id)
+          OR (ao.set_type = 'ADHOC' AND ao.set_id IS NULL AND ao.id = aos.assignment_override_id)
+          OR (ao.set_type = 'Group' AND ao.set_id = g.id)
+        )
+
+      LEFT JOIN #{Submission.quoted_table_name} s
+        ON s.user_id = e.user_id
+        AND s.assignment_id = a.id
+        AND s.workflow_state != 'deleted'
+
+      WHERE a.workflow_state NOT IN ('deleted','unpublished')
+        AND(
+          ( a.only_visible_to_overrides = 'true' AND (ao.id IS NOT NULL OR s.id IS NOT NULL))
+          OR (COALESCE(a.only_visible_to_overrides, 'false') = 'false')
+        )
+    )
+  end
+end

spec/models/discussion_topic_spec.rb

@@ -358,6 +358,89 @@ describe DiscussionTopic do
         expect(@topic.check_policy(@student1)).to include :reply
         expect(@topic.check_policy(@student2)).not_to include :reply
       end
+
+      context "when override changes" do
+        context "with sections" do
+          before :each do
+            @course = course_factory(active_course: true)
+            discussion_topic_model(:user => @teacher, :context => @course)
+            @course.enroll_teacher(@teacher).accept!
+            @section1 = @course.course_sections.create
+            @section2 = @course.course_sections.create
+            @student1, @student2 = create_users(2, return_type: :record)
+            @course.enroll_student(@student1, :enrollment_state => 'active')
+            @course.enroll_student(@student2, :enrollment_state => 'active')
+            student_in_section(@section1, user: @student1)
+            student_in_section(@section2, user: @student2)
+
+            @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!
+            @topic.reply_from(:user => @teacher, :text => 'hello world')
+
+            create_section_override_for_assignment(@assignment, {course_section: @section1})
+            @topic.touch
+            @course.reload
+          end
+
+          it "should should unsubscribe students in removed section" do
+            expect(@topic.subscribed?(@student2)).to be_falsey
+          end
+
+          it "should delete notifications for students in removed section" do
+            expect(@topic.stream_item.stream_item_instances.where(user_id: @student2.id)).to be_blank
+          end
+
+          it "prevent removed students in removed sections from receiving new notifications when new replies are made" do
+            @topic.reply_from(:user => @teacher, :text => 'hello world')
+            expect(@topic.stream_item.stream_item_instances.where(user_id: @student2.id)).to be_blank
+          end
+        end
+
+        context "with students" do
+          before :each do
+            @course = course_factory(active_course: true)
+            discussion_topic_model(:user => @teacher, :context => @course)
+            @course.enroll_teacher(@teacher).accept!
+            @student1, @student2 = create_users(2, return_type: :record)
+            @course.enroll_student(@student1, :enrollment_state => 'active')
+            @course.enroll_student(@student2, :enrollment_state => 'active')
+
+            @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!
+            @topic.reply_from(:user => @teacher, :text => 'hello world')
+
+            create_adhoc_override_for_assignment(@assignment, @student1)
+            @topic.touch
+            @course.reload
+          end
+
+          it "should should unsubscribe removed students" do
+            expect(@topic.subscribed?(@student2)).to be_falsey
+          end
+
+          it "should delete notifications for removed students" do
+            expect(@topic.stream_item.stream_item_instances.where(user_id: @student2.id)).to be_blank
+          end
+
+          it "prevent removed students from receiving new notifications when new replies are made" do
+            @topic.reply_from(:user => @teacher, :text => 'hello world')
+            expect(@topic.stream_item.stream_item_instances.where(user_id: @student2.id)).to be_blank
+          end
+        end
+      end
+
       context "active_participants_with_visibility" do
         it "should filter participants by visibility" do
           [@student1, @teacher].each do |user|

spec/models/stream_item_spec.rb

@@ -65,6 +65,21 @@ describe StreamItem do
     end
   end
 
+  describe "remove_users" do
+    it "should remove users" do
+      course = Course.create!
+      students = n_students_in_course(3, course: course)
+      dt = DiscussionTopic.create!(:context => course, :require_initial_post => true)
+      dt.generate_stream_items(students)
+      si = dt.stream_item
+
+      si.remove_users(students[0, 2])
+      expect(si.stream_item_instances.count).to be_equal 1
+      expect(si.users.count).to be_equal 1
+      expect(si.users.first.id).to be_equal students.last.id
+    end
+  end
+
   context "across shards" do
     specs_require_sharding