autosubscribe users to a topic when they post to it

this preserves legacy behavior: students who posted to a topic
before this feature was introduced should be automatially
subscribed to the topics they have posted to.

when a user posts to the topic, the subscription button should
immediately turn green.

fixes CNVS-6826

test plan:
 - before updating code, create a topic as a teacher, and post
   to it as a student. student should not be subscribed.
 - before updating code, create a graded group discussion and
   post to the discussion as a student in one of the groups.
 - update the code and look at the previous topics as the
   student who posted. the student should now be subscribed to
   the normal topic and the graded group topic.
 - post to the topic as another student that has not posted and
   is not subscribed. the student should be subscribed
   automatically. the subscribe button should turn green
   immediately when the post is made.
 - test this with both root level replies, and sub-replies.
 - unsubscribe from the topic and then post to the topic. user
   should be automatically resubscribed to the topic.
 - announcements should still not have the ability to subscribe.

Change-Id: I32062b018ba0f85b3a9bc6c65c0b64c2d379b868
Reviewed-on: https://gerrit.instructure.com/22255
Reviewed-by: Mark Ericksen <marke@instructure.com>
QA-Review: Cam Theriault <cam@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
Product-Review: Jon Willesen <jonw@instructure.com>

app/coffeescripts/views/DiscussionTopic/EntryView.coffee

@@ -213,6 +213,7 @@ define [
         @treeView.collection.add entry
         @treeView.collection.fullCollection.add entry
         @trigger 'addReply'
+        EntryView.trigger 'addReply', entry
 
     addReplyAttachment: (event, $el) ->
       event.preventDefault()

app/coffeescripts/views/DiscussionTopic/TopicView.coffee

@@ -48,6 +48,9 @@ define [
       # set initial subscribed state
       @topic.set 'subscribed', ENV.DISCUSSION.TOPIC.IS_SUBSCRIBED
 
+      # catch when non-root replies are added so we can twiddle the subscribed button
+      EntryView.on('addReply', => @setSubscribed(true))
+
     hideIfFiltering: =>
       if @filterModel.hasFilter()
         @$topic.addClass 'hidden'
@@ -111,10 +114,18 @@ define [
         @reply = new Reply this, topLevel: true, focus: true
         @reply.on 'edit', => @$addRootReply?.hide()
         @reply.on 'hide', => @$addRootReply?.show()
-        @reply.on 'save', (entry) => @trigger 'addReply', entry
+        @reply.on 'save', (entry) =>
+          @setSubscribed(true)
+          @trigger 'addReply', entry
       @model.set 'notification', ''
       @reply.edit()
 
+    # Update subscribed state without posted. Done when replies are posted and
+    # user is auto-subscribed.
+    setSubscribed: (newValue) ->
+      @topic.set('subscribed', true)
+      @subscriptionStatusChanged()
+
     addReplyAttachment: EntryView::addReplyAttachment
 
     removeReplyAttachment: EntryView::removeReplyAttachment

app/models/discussion_entry.rb

@@ -392,6 +392,7 @@ class DiscussionEntry < ActiveRecord::Base
                                                                                          :workflow_state => "unread",
                                                                                          :subscribed => self.discussion_topic.subscribed?(self.user))
         end
+        self.discussion_topic.subscribe(self.user)
       end
     end
   end

app/models/discussion_topic.rb

@@ -306,12 +306,16 @@ class DiscussionTopic < ActiveRecord::Base
     end
     participant ||= discussion_topic_participants.where(:user_id => current_user.id).first
 
-    if participant.try(:subscribed).nil?
-      # if there is no explicit subscription, assume the author is subscribed
-      # assume non-authors are not subscribed
-      current_user == user
+    if participant
+      if participant.subscribed.nil?
+        # if there is no explicit subscription, assume the author and posters
+        # are subscribed, everyone else is not subscribed
+        current_user == user || participant.discussion_topic.posters.include?(current_user)
+      else
+        participant.subscribed
+      end
     else
-      participant.subscribed
+      current_user == user
     end
   end
 
@@ -358,7 +362,7 @@ class DiscussionTopic < ActiveRecord::Base
         topic_participant.workflow_state = opts[:new_state] if opts[:new_state]
         topic_participant.unread_entry_count += opts[:offset] if opts[:offset] && opts[:offset] != 0
         topic_participant.unread_entry_count = opts[:new_count] if opts[:new_count]
-        topic_participant.subscribed = opts[:subscribed] if !opts[:subscribed].nil?
+        topic_participant.subscribed = opts[:subscribed] if opts.has_key?(:subscribed)
         topic_participant.save
       end
     end
@@ -703,14 +707,18 @@ class DiscussionTopic < ActiveRecord::Base
   def participating_users(user_ids)
     context.respond_to?(:participating_users) ? context.participating_users(user_ids) : User.find(user_ids)
   end
-  
+
   def subscribers
-    user_ids = discussion_topic_participants.where(:subscribed => true).pluck(:user_id)
-    user_ids.push(user_id) if subscribed?(user)
-    user_ids.uniq!
-    participating_users(user_ids)
+    # this duplicates some logic from #subscribed? so we don't have to call 
+    # #posters for each legacy subscriber.
+    sub_ids = discussion_topic_participants.where(:subscribed => true).pluck(:user_id)
+    legacy_sub_ids = discussion_topic_participants.where(:subscribed => nil).pluck(:user_id)
+    poster_ids = posters.map(&:id)
+    legacy_sub_ids &= poster_ids + [user.id]
+    sub_ids += legacy_sub_ids
+    participating_users(sub_ids)
   end
-    
+
   def posters
     user_ids = discussion_entries.map(&:user_id).push(self.user_id).uniq
     participating_users(user_ids)

spec/factories/discussion_topic_factory.rb

@@ -58,5 +58,5 @@ def topic_with_nested_replies(opts = {})
   @reply1.destroy
   @all_entries = [@root1, @root2, @reply1, @reply2, @reply_reply1, @reply_reply2, @reply3]
   @all_entries.each &:reload
-  @topic
+  @topic.reload
 end

spec/models/discussion_entry_spec.rb

@@ -59,6 +59,8 @@ describe DiscussionEntry do
     before do
       course_with_teacher(:active_all => true)
       student_in_course(:active_all => true)
+      @non_posting_student = @student
+      student_in_course(:active_all => true)
 
       @notification_name = "New Discussion Entry"
       n = Notification.create(:name => @notification_name, :category => "TestImmediately")
@@ -69,21 +71,24 @@ describe DiscussionEntry do
       topic = @course.discussion_topics.create!(:user => @teacher, :message => "Hi there")
       entry = topic.discussion_entries.create!(:user => @student, :message => "Hi I'm a student")
 
-      to_users = entry.messages_sent[@notification_name].map(&:user)
-      to_users.should include(@teacher) # teacher is auto-subscribed
-      to_users.should_not include(@student)
+      to_users = entry.messages_sent[@notification_name].map(&:user).map(&:id)
+      to_users.should include(@teacher.id) # teacher is auto-subscribed
+      to_users.should_not include(@student.id) # posters are auto-subscribed, but student is not notified of his own post
+      to_users.should_not include(@non_posting_student.id)
 
       entry = topic.discussion_entries.create!(:user => @teacher, :message => "Nice to meet you")
-      # student not subscribed so no notification to student
-      # teacher is subscribed, but does not get notification of her own posts
-      entry.messages_sent[@notification_name].should be_blank
+      to_users = entry.messages_sent[@notification_name].map(&:user).map(&:id)
+      to_users.should_not include(@teacher.id) # author
+      to_users.should include(@student.id)
+      to_users.should_not include(@non_posting_student.id)
 
-      topic.subscribe(@student)
+      topic.subscribe(@non_posting_student)
       entry = topic.discussion_entries.create!(:user => @teacher, :message => "Welcome to the class")
-      # now that the student is subscribed, he should gets notified of posts
-      to_users = entry.messages_sent[@notification_name].map(&:user)
-      to_users.should_not include(@teacher)
-      to_users.should include(@student)
+      # now that the non_posting_student is subscribed, he should get notified of posts
+      to_users = entry.messages_sent[@notification_name].map(&:user).map(&:id)
+      to_users.should_not include(@teacher.id)
+      to_users.should include(@student.id)
+      to_users.should include(@non_posting_student.id)
     end
 
     it "should send them for group discussion topics" do
@@ -100,15 +105,16 @@ describe DiscussionEntry do
       topic = @group.discussion_topics.create!(:user => @teacher, :message => "Hi there")
       entry = topic.discussion_entries.create!(:user => s1, :message => "Hi I'm a student")
       # teacher is subscribed but is not in the "participating_users" for this group
+      # s1 is the author, s2 is not subscribed
       entry.messages_sent[@notification_name].should be_blank
 
-      topic.subscribe(s1)
+      # s1 should be subscribed from posting to the topic
       topic.subscribe(s2)
       entry = topic.discussion_entries.create!(:user => s2, :message => "Hi I'm a student")
       to_users = entry.messages_sent[@notification_name].map(&:user)
       to_users.should_not include(@teacher)
       to_users.should include(s1)
-      to_users.should_not include(s2)
+      to_users.should_not include(s2) # s2 not notified of own post
     end
 
     it "should not send them to irrelevant users" do

spec/models/discussion_topic_spec.rb

@@ -614,7 +614,7 @@ describe DiscussionTopic do
       @context = @course
       discussion_topic_model(:user => @teacher)
     end
-    
+
     it "should automatically include the author" do
       @topic.subscribers.should include(@teacher)
     end
@@ -624,11 +624,31 @@ describe DiscussionTopic do
       @topic.subscribers.should_not include(@teacher)
     end
 
-    it "should not automatically include posters" do
+    it "should automatically include posters" do
       @topic.reply_from(:user => @student, :text => "entry")
+      @topic.subscribers.should include(@student)
+    end
+
+    it "should include users that have posted entries before subscriptions were added" do
+      @topic.reply_from(:user => @student, :text => "entry")
+      participant = @topic.update_or_create_participant(current_user: @student, subscribed: nil)
+      participant.subscribed.should be_nil
+      @topic.subscribers.map(&:id).should include(@student.id)
+    end
+
+    it "should not include posters if they unsubscribe" do
+      @topic.reply_from(:user => @student, :text => "entry")
+      @topic.unsubscribe(@student)
       @topic.subscribers.should_not include(@student)
     end
 
+    it "should resubscribe unsubscribed users if they post" do
+      @topic.reply_from(:user => @student, :text => "entry")
+      @topic.unsubscribe(@student)
+      @topic.reply_from(:user => @student, :text => "another entry")
+      @topic.subscribers.should include(@student)
+    end
+
     it "should include users who subscribe" do
       @topic.subscribe(@student)
       @topic.subscribers.should include(@student)
@@ -968,9 +988,9 @@ describe DiscussionTopic do
       @topic.subscribed?(@teacher).should be_true
     end
 
-    it "should not assume posters are subscribed" do
+    it "should assume posters are subscribed" do
       @topic.reply_from(:user => @student, :text => 'first post!')
-      @topic.subscribed?(@student).should be_false
+      @topic.subscribed?(@student).should be_true
     end
 
     context "when initial_post_required" do
@@ -978,7 +998,6 @@ describe DiscussionTopic do
         @topic.require_initial_post = true
         @topic.save!
         @entry = @topic.reply_from(:user => @student, :text => 'first post!')
-        @topic.subscribe(@student)
         @topic.subscribed?(@student).should be_true
         @entry.destroy
         @topic.subscribed?(@student).should be_false
@@ -999,6 +1018,15 @@ describe DiscussionTopic do
       @topic.subscribed?(@student).should be_true
     end
 
+    it "should return true if the user has posted to a child topic" do
+      child_topic = @topic.child_topics.first
+      child_topic.context.add_user(@student)
+      child_topic.reply_from(:user => @student, :text => "post")
+      child_topic_participant = child_topic.update_or_create_participant(:current_user => @student, :subscribed => nil)
+      child_topic_participant.subscribed.should be_nil
+      @topic.subscribed?(@student).should be_true
+    end
+
     it "should subscribe a group user to the child topic" do
       child_one, child_two = @topic.child_topics
       child_one.context.add_user(@student)

spec/selenium/discussions_spec.rb

@@ -1221,9 +1221,18 @@ describe "discussions" do
       # now the subscribe button should be available.
       get "/courses/#{@course.id}/discussion_topics/#{@topic.id}"
       wait_for_ajax_requests
-      f('.topic-subscribe-button').displayed?.should be_true
+      # already subscribed because they posted
+      f('.topic-unsubscribe-button').displayed?.should be_true
     end
 
+    it "should updated subscribed button when user posts to a topic" do
+      course_with_student_logged_in(:course => @course)
+      get "/courses/#{@course.id}/discussion_topics/#{@topic.id}"
+      wait_for_ajax_requests
+      f('.topic-subscribe-button').displayed?.should be_true
+      add_reply "student posting"
+      f('.topic-unsubscribe-button').displayed?.should be_true
+    end
   end
 
 end