Fix notification sending in unpublished module

Test Plan:
-specs pass
-create an unpublished module
-publish a discussion topic in the module
-notification should not appear on dashboard course card

-publish module
-add discussion topic
-dashboard card should have notification

Fixes KNO-489

flag=none

Change-Id: I6a24073a2c586a568cd91695a25997fb3edb1be7
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/239234
Tested-by: Service Cloud Jenkins <[email protected]>
Reviewed-by: Rob Orton <[email protected]>
QA-Review: Rob Orton <[email protected]>
Product-Review: Rob Orton <[email protected]>

app/models/content_tag.rb

@@ -43,6 +43,8 @@ class LastLinkToOutcomeNotDestroyed < StandardError
   belongs_to :learning_outcome_content, :class_name => 'LearningOutcome', :foreign_key => :content_id
   has_many :learning_outcome_results
 
+  after_create :clear_stream_items_if_module_is_unpublished
+
   # This allows bypassing loading context for validation if we have
   # context_id and context_type set, but still allows validating when
   # context is not yet saved.
@@ -55,6 +57,8 @@ class LastLinkToOutcomeNotDestroyed < StandardError
   after_save :touch_context_module_after_transaction
   after_save :touch_context_if_learning_outcome
   after_save :run_due_date_cacher_for_quizzes_next
+  after_save :clear_discussion_stream_items
+  after_save :send_items_to_stream
 
   include CustomValidations
   validates_as_url :url
@@ -373,6 +377,28 @@ def available_for?(user, opts={})
     self.context_module.available_for?(user, opts.merge({:tag => self}))
   end
 
+  def send_items_to_stream
+    if self.content_type == "DiscussionTopic" && self.saved_change_to_workflow_state? && self.workflow_state == 'active'
+      content.send_items_to_stream
+    end
+  end
+
+  def clear_discussion_stream_items
+    if self.content_type == "DiscussionTopic"
+      if self.saved_change_to_workflow_state? &&
+        ['active', nil].include?(self.workflow_state_before_last_save) &&
+        self.workflow_state == 'unpublished'
+          content.clear_stream_items
+      end
+    end
+  end
+
+  def clear_stream_items_if_module_is_unpublished
+    if self.content_type == "DiscussionTopic" && context_module.workflow_state == 'unpublished'
+      content.clear_stream_items
+    end
+  end
+
   def self.update_for(asset, exclude_tag: nil)
     tags = ContentTag.where(:content_id => asset, :content_type => asset.class.to_s).not_deleted
     tags = tags.where('content_tags.id<>?', exclude_tag.id) if exclude_tag

app/models/context_module.rb

@@ -41,6 +41,8 @@ class ContextModule < ActiveRecord::Base
   after_save :touch_context
   after_save :invalidate_progressions
   after_save :relock_warning_check
+  after_save :clear_discussion_stream_items
+  after_save :send_items_to_stream
   validates_presence_of :workflow_state, :context_id, :context_type
   validates_presence_of :name, :if => :require_presence_of_name
   attr_accessor :require_presence_of_name
@@ -191,6 +193,24 @@ def can_be_duplicated?
     end
   end
 
+  def send_items_to_stream
+    if self.saved_change_to_workflow_state? && self.workflow_state == 'active'
+      self.content_tags.where(content_type: "DiscussionTopic", workflow_state: 'active').preload(:content).each do |ct|
+        ct.content.send_items_to_stream
+      end
+    end
+  end
+
+  def clear_discussion_stream_items
+    if self.saved_change_to_workflow_state? &&
+      ['active', nil].include?(self.workflow_state_before_last_save) &&
+      self.workflow_state == 'unpublished'
+      self.content_tags.where(content_type: "DiscussionTopic", workflow_state: 'active').preload(:content).each do |ct|
+        ct.content.clear_stream_items
+      end
+    end
+  end
+
   # This is intended for duplicating a content tag when we are duplicating a module
   # Not intended for duplicating a content tag to keep in the original module
   def duplicate_content_tag_base_model(original_content_tag)

app/models/discussion_topic.rb

@@ -901,6 +901,8 @@ def should_send_to_stream
       false
     elsif self.root_topic_id && self.has_group_category?
       false
+    elsif self.in_unpublished_module?
+      false
     else
       true
     end
@@ -920,12 +922,24 @@ def should_send_to_stream
     end
   end
 
+  # This is manually called for module publishing
+  def send_items_to_stream
+    if should_send_to_stream
+      queue_create_stream_items
+    end
+  end
+
   def clear_streams_if_not_published
-    if !self.published?
+    unless self.published?
       self.clear_stream_items
     end
   end
 
+  def in_unpublished_module?
+    return true if ContentTag.where(content_type: "DiscussionTopic", content_id: self, workflow_state: "unpublished").exists?
+    ContextModule.joins(:content_tags).where(content_tags: { content_type: "DiscussionTopic", content_id: self }, workflow_state: 'unpublished').exists?
+  end
+
   def clear_non_applicable_stream_items_for_sections
     # either changed sections or made section specificness
     return unless self.is_section_specific? ? @sections_changed : self.is_section_specific_before_last_save

spec/models/discussion_topic_spec.rb

@@ -2341,33 +2341,45 @@ def basic_announcement_model(opts = {})
     end
   end
 
-  describe "locked by context module" do
+  describe 'context modules' do
     before(:once) do
       discussion_topic_model(context: @course)
       @module = @course.context_modules.create!(name: 'some module')
-      @module.add_item(type: 'discussion_topic', id: @topic.id)
-      @module.unlock_at = 2.months.from_now
+      @tag = @module.add_item(type: 'discussion_topic', id: @topic.id)
       @module.save!
       @topic.reload
     end
-
-    it "stays visible_for? student even when locked by module" do
-      expect(@topic.visible_for?(@student)).to be_truthy
+    it 'should clear stream items when unpublishing a module' do
+      expect { @module.unpublish! }.to change { @student.stream_item_instances.count }.by(-1)
     end
-
-    it "is locked_for? students when locked by module" do
-      expect(@topic.locked_for?(@student, deep_check_if_needed: true)).to be_truthy
+    it 'should remove stream items when the module item is changed to unpublished' do
+      expect { @tag.unpublish! }.to change { @student.stream_item_instances.count }.by(-1)
     end
-
-    describe "reject_context_module_locked_topics" do
-      it "filters module locked topics for students" do
-        topics = DiscussionTopic.reject_context_module_locked_topics([@topic], @student)
-        expect(topics).to be_empty
+    it 'should clear stream items when added to unpublished module items' do
+      expect {
+        @module.content_tags.create!(workflow_state: 'unpublished', content: @topic, context: @course)
+      }.to change { @student.stream_item_instances.count }.by(-1)
+    end
+    describe 'unpublished context module' do
+      before(:once) do
+        @module.unpublish!
+        @tag.unpublish!
       end
-
-      it "does not filter module locked topics for teachers" do
-        topics = DiscussionTopic.reject_context_module_locked_topics([@topic], @teacher)
-        expect(topics).not_to be_empty
+      it 'should not create stream items for unpublished modules' do
+        @topic.unpublish!
+        expect { @topic.publish! }.to change { @student.stream_item_instances.count }.by 0
+      end
+      it 'should remove stream items from published topic when added to an unpublished module' do
+        topic = discussion_topic_model(context: @course)
+        expect { @module.add_item(type: 'discussion_topic', id: topic.id) }.to change { @student.stream_item_instances.count }.by(-1)
+      end
+      it 'should  create stream items when module is published' do
+        @tag.publish!
+        expect { @module.publish! }.to change { @student.stream_item_instances.count }.by 1
+      end
+      it 'should  create stream items when module item is published' do
+        @module.publish!
+        expect { @tag.publish! }.to change { @student.stream_item_instances.count }.by 1
       end
     end
   end