fix submission -> conversation regression, fixes #9165

fix issue where new submission comments cause conversations to be marked
as unread for everyone, including the comment author.

slight cleanup, and also make no_submission_comments_inbox compatible with
assignment muting.

test plan:
1. as a teacher, send a message to the student
2. the conversation should be unread for the student, but read for the
   teacher
3. as a student, submit an assignment
4. as the teacher add a comment to the submission
5. as the teacher, confirm that the submission appears in the
   conversation, but is NOT marked as unread
6. as the student, confirm that the submission appears in the
   conversation, and IS marked as unread
7. as the student, add a comment to the submission
8. confirm that it is NOT marked as unread for the student
9. confirm that it IS marked as unread for the teacher

Change-Id: I73376e563ccbaea092cf1d22d5d16f186bf5dfac
Reviewed-on: https://gerrit.instructure.com/11848
Reviewed-by: Mark Ericksen <marke@instructure.com>
Tested-by: Jon Jensen <jon@instructure.com>

app/models/submission.rb

@@ -840,24 +840,22 @@ class Submission < ActiveRecord::Base
   #
   # ==== Overrides
   # * <tt>:skip_ids</tt> - Gets passed through to <tt>Conversation</tt>.<tt>update_all_for_asset</tt>.
+  #                        nil by default, which means mark-as-unread for
+  #                        everyone but the author.
   def create_or_update_conversations!(trigger, overrides={})
     options = {}
     case trigger
     when :create
       options[:update_participants] = true
-      options[:skip_ids] = overrides[:skip_ids] || []
+      options[:skip_ids] = overrides[:skip_ids] || [conversation_message_data[:author_id]] # don't mark-as-unread for the author
       if commenting_instructors.empty?
         # until the first instructor comments, we don't want the submitter to see
-        # the message (whether the submitter is the author, or someone in the group is)
+        # the message in existing conversations with anyone
         options[:update_for_skips] = false
-        options[:skip_ids] = [user_id]
       end
-      if overrides[:respect_submission_comment_pref]
-        # How to identify the teachers?
-        self.assignment.context.participating_instructors.each do |t|
-          # Check their settings and add to :skip_ids if set to suppress.
-          options[:skip_ids] << t.id if t.preferences[:no_submission_comments_inbox] == true
-        end
+      participating_instructors.each do |t|
+        # Check their settings and add to :skip_ids if set to suppress.
+        options[:skip_ids] << t.id if t.preferences[:no_submission_comments_inbox] == true
       end
     when :destroy
       options[:delete_all] = visible_submission_comments.empty?

app/models/submission_comment.rb

@@ -39,7 +39,7 @@ class SubmissionComment < ActiveRecord::Base
   after_save :update_submission
   after_destroy :delete_other_comments_in_this_group
   after_create :update_participants
-  after_create { |c| c.submission.create_or_update_conversations!(:create, :respect_submission_comment_pref => true) if c.send_to_conversations? }
+  after_create { |c| c.submission.create_or_update_conversations!(:create) if c.send_to_conversations? }
   after_destroy { |c| c.submission.create_or_update_conversations!(:destroy) if c.send_to_conversations? }
 
   serialize :cached_attachments

spec/models/submission_comment_spec.rb

@@ -237,13 +237,11 @@ This text has a http://www.google.com link in it...
         @submission1.add_comment(:author => @student1, :comment => "hello")
         tconvo = @teacher1.conversations.first
         tconvo.should be_unread
+        tconvo.update_attribute :workflow_state, 'read' 
         @submission1.add_comment(:author => @teacher1, :comment => "hi")
         sconvo = @student1.conversations.first
         sconvo.should be_unread
-
-        tconvo.remove_messages(:all)
-        sconvo.workflow_state = :read
-        sconvo.save!
+        tconvo.reload.should be_read
       end
 
       context "with no_submission_comments_inbox" do
@@ -280,15 +278,20 @@ This text has a http://www.google.com link in it...
             @teacher1.preferences[:no_submission_comments_inbox] = true
             @teacher1.save!
           end
-          it "should not show up in conversations" do
+          it "should not create new conversations" do
             @teacher1.conversations.count.should == 0
-            # Disable notification with existing conversation
-            @teacher1.preferences[:no_submission_comments_inbox] = true
-            @teacher1.save!
-            # Student adds another comment
             @submission1.add_comment(:author => @student1, :comment => 'New comment')
             @teacher1.conversations.count.should == 0
           end
+          it "should create conversations after re-enabling the notification" do
+            @submission1.add_comment(:author => @student1, :comment => 'New comment')
+            @teacher1.conversations.count.should == 0
+            @teacher1.preferences[:no_submission_comments_inbox] = false
+            @teacher1.save!
+            # Student adds another comment
+            @submission1.add_comment(:author => @student1, :comment => 'Another comment')
+            @teacher1.conversations.count.should == 1
+          end
           it "should show teacher comment as new to student" do
             @submission1.add_comment(:author => @student1, :comment => 'Test comment')
             @submission1.add_comment(:author => @teacher1, :comment => 'Test response')
@@ -299,6 +302,17 @@ This text has a http://www.google.com link in it...
             convo.add_message(@student1, 'My direct message')
             @teacher.conversations.unread.count.should == 1
           end
+          it "should add submission comments to existing conversations" do
+            convo = Conversation.initiate([@student1.id, @teacher1.id], true)
+            convo.add_message(@student1, 'My direct message')
+            c = @teacher1.conversations.unread.first
+            c.should_not be_nil
+            c.update_attribute(:workflow_state, 'read')
+            @submission1.add_comment(:author => @student1, :comment => 'A comment')
+            c.reload
+            c.should be_read # still read, since we don't care to be notified
+            c.messages.size.should eql 2 # but the submission is visible
+          end
         end
       end
     end
@@ -357,6 +371,29 @@ This text has a http://www.google.com link in it...
         @student1.conversations.first.should be_unread
         @student1.conversations.last.should be_unread
       end
+
+      it "should respect the no_submission_comments_inbox setting" do
+        @teacher1.preferences[:no_submission_comments_inbox] = true
+        @teacher1.save!
+        c1 = @submission1.add_comment(:author => @student1, :comment => "help!")
+        c2 = @submission1.add_comment(:author => @teacher1, :comment => "ok", :hidden => true)
+        c3 = @submission1.add_comment(:author => @teacher2, :comment => "no", :hidden => true)
+        @student1.conversations.size.should eql 0
+        @teacher1.conversations.size.should eql 0
+        t2convo = @teacher2.conversations.first
+        t2convo.workflow_state = :read
+        t2convo.save!
+
+        @assignment.unmute!
+
+        t1convo = @teacher1.reload.conversations.first
+        t1convo.should_not be_nil
+        t1convo.should be_read
+        t2convo.reload.should be_unread
+        @student1.reload.conversations.size.should eql 2
+        @student1.conversations.first.should be_unread
+        @student1.conversations.last.should be_unread
+      end
     end
 
     context "deletion" do