Merge pull request #1705 from codidact/0valt/725/comments

User pings no longer temporarily become displayed as "unpingable" after comment edits

Author: ArtOfCode-

app/controllers/comments_controller.rb

@@ -29,7 +29,7 @@ def create_thread
     @comment_thread = CommentThread.new(title: title, post: @post)
     @comment = Comment.new(post: @post, content: body, user: current_user, comment_thread: @comment_thread)
 
-    pings = check_for_pings @comment_thread, body
+    pings = check_for_pings(@comment_thread, body)
 
     success = ActiveRecord::Base.transaction do
       thread_success = @comment_thread.save
@@ -56,7 +56,7 @@ def create_thread
         ThreadFollower.create(user: tf.user, comment_thread: @comment_thread)
       end
 
-      apply_pings pings
+      apply_pings(pings)
     else
       flash[:danger] = "Could not create comment thread: #{(@comment_thread.errors.full_messages \
                                                            + @comment.errors.full_messages).join(', ')}"
@@ -66,15 +66,15 @@ def create_thread
 
   def create
     body = params[:content]
-    pings = check_for_pings @comment_thread, body
+    pings = check_for_pings(@comment_thread, body)
 
     @comment = Comment.new(post: @post, content: body, user: current_user,
                            comment_thread: @comment_thread, has_reference: false)
 
     status = @comment.save
 
     if status
-      apply_pings pings
+      apply_pings(pings)
       @comment_thread.thread_follower.each do |follower|
         next if follower.user_id == current_user.id
         next if pings.include? follower.user_id
@@ -105,17 +105,18 @@ def update
     @post = @comment.post
     @comment_thread = @comment.comment_thread
     before = @comment.content
-    before_pings = check_for_pings @comment_thread, before
+    before_pings = check_for_pings(@comment_thread, before)
     if @comment.update comment_params
       unless current_user.id == @comment.user_id
         audit('comment_update', @comment, "from <<#{before}>>\nto <<#{@comment.content}>>")
       end
 
-      after_pings = check_for_pings @comment_thread, @comment.content
+      after_pings = check_for_pings(@comment_thread, @comment.content)
       apply_pings(after_pings - before_pings - @comment_thread.thread_follower.to_a)
 
       render json: { status: 'success',
-                     comment: render_to_string(partial: 'comments/comment', locals: { comment: @comment }) }
+                     comment: render_to_string(partial: 'comments/comment',
+                                               locals: { comment: @comment, pingable: after_pings }) }
     else
       render json: { status: 'failed',
                      message: "Comment failed to save (#{@comment.errors.full_messages.join(', ')})" },
@@ -279,8 +280,7 @@ def post_follow
 
   def pingable
     thread = params[:id] == '-1' ? CommentThread.new(post_id: params[:post]) : CommentThread.find(params[:id])
-    ids = helpers.get_pingable(thread)
-    users = User.where(id: ids)
+    users = User.where(id: thread.pingable)
     render json: users.to_h { |u| [u.username, u.id] }
   end
 
@@ -379,12 +379,16 @@ def check_if_target_post_locked
     check_if_locked(Post.find(params[:post_id]))
   end
 
+  # @param thread [CommentThread] thread to extract pings for
+  # @param content [String] content to extract pings from
+  # @return [Array<Integer>] list of pinged user ids
   def check_for_pings(thread, content)
-    pingable = helpers.get_pingable(thread)
+    pingable = thread.pingable
     matches = content.scan(/@#(\d+)/)
     matches.flatten.select { |m| pingable.include?(m.to_i) }.map(&:to_i)
   end
 
+  # @param pings [Array<Integer>] list of pinged user ids
   def apply_pings(pings)
     pings.each do |p|
       user = User.where(id: p).first

app/helpers/comments_helper.rb

@@ -58,7 +58,7 @@ def render_pings(content, pingable: nil)
         was_pung = pingable.present? && pingable.include?(user.id)
         classes = "ping #{'me' if user.same_as?(current_user)} #{'unpingable' unless was_pung}"
         user_link user, class: classes, dir: 'ltr',
-                  title: was_pung ? '' : 'This user was not notified because they have not participated in this thread.'
+                  title: was_pung ? '' : I18n.t('comments.warnings.unrelated_user_not_pinged')
       end
     end.html_safe
   end
@@ -149,34 +149,6 @@ def rate_limited_error_msg(user, post)
     I18n.t('comments.errors.rate_limited', count: comments_count)
   end
 
-  ##
-  # Get a list of user IDs who should be pingable in a specified comment thread. This combines the post author, answer
-  # authors, recent history event authors, recent comment authors on the post (in any thread), and all thread followers.
-  # @param thread [CommentThread]
-  # @return [Array<Integer>]
-  def get_pingable(thread)
-    post = thread.post
-
-    # post author +
-    # answer authors +
-    # last 500 history event users +
-    # last 500 comment authors +
-    # all thread followers
-    query = <<~END_SQL
-      SELECT posts.user_id FROM posts WHERE posts.id = #{post.id}
-      UNION DISTINCT
-      SELECT DISTINCT posts.user_id FROM posts WHERE posts.parent_id = #{post.id}
-      UNION DISTINCT
-      SELECT DISTINCT ph.user_id FROM post_histories ph WHERE ph.post_id = #{post.id}
-      UNION DISTINCT
-      SELECT DISTINCT comments.user_id FROM comments WHERE comments.post_id = #{post.id}
-      UNION DISTINCT
-      SELECT DISTINCT tf.user_id FROM thread_followers tf WHERE tf.comment_thread_id = #{thread.id || '-1'}
-    END_SQL
-
-    ActiveRecord::Base.connection.execute(query).to_a.flatten
-  end
-
   ##
   # Is the specified user comment rate limited for the specified post?
   # @param user [User] The user to check.

app/models/comment.rb

@@ -33,6 +33,12 @@ def content_length
     end
   end
 
+  def pings
+    pingable = thread.pingable
+    matches = content.scan(/@#(\d+)/)
+    matches.flatten.select { |m| pingable.include?(m.to_i) }.map(&:to_i)
+  end
+
   private
 
   def create_follower

app/models/comment_thread.rb

@@ -13,6 +13,10 @@ class CommentThread < ApplicationRecord
 
   after_create :create_follower
 
+  def self.post_followed?(post, user)
+    ThreadFollower.where(post: post, user: user).any?
+  end
+
   def read_only?
     locked? || archived? || deleted?
   end
@@ -26,8 +30,27 @@ def can_access?(user)
       post.can_access?(user)
   end
 
-  def self.post_followed?(post, user)
-    ThreadFollower.where(post: post, user: user).any?
+  # Gets a list of user IDs who should be pingable in the thread.
+  # @return [Array<Integer>]
+  def pingable
+    # post author +
+    # answer authors +
+    # last 500 history event users +
+    # last 500 comment authors +
+    # all thread followers
+    query = <<~END_SQL
+      SELECT posts.user_id FROM posts WHERE posts.id = #{post.id}
+      UNION DISTINCT
+      SELECT DISTINCT posts.user_id FROM posts WHERE posts.parent_id = #{post.id}
+      UNION DISTINCT
+      SELECT DISTINCT ph.user_id FROM post_histories ph WHERE ph.post_id = #{post.id}
+      UNION DISTINCT
+      SELECT DISTINCT comments.user_id FROM comments WHERE comments.post_id = #{post.id}
+      UNION DISTINCT
+      SELECT DISTINCT tf.user_id FROM thread_followers tf WHERE tf.comment_thread_id = #{id || '-1'}
+    END_SQL
+
+    ActiveRecord::Base.connection.execute(query).to_a.flatten
   end
 
   private

app/views/comment_threads/_expanded.html.erb

@@ -10,7 +10,7 @@
 
 <% max_shown_comments = 5 %>
 <% comment_id ||= defined?(comment_id) ? comment_id : nil %>
-<% pingable = get_pingable(thread) %>
+<% pingable = thread.pingable %>
 <% shown_comments_count = [max_shown_comments, thread.reply_count].min %>
 
 <%

app/views/flags/_flag.html.erb

@@ -16,7 +16,7 @@
       <%= render 'posts/type_agnostic', show_type_tag: true, show_category_tag: true, post: flag.post %>
     </div>
   <% elsif flag.post_type == 'Comment' %>
-    <%= render 'comments/comment', comment: flag.post, with_post_link: true %>
+    <%= render 'comments/comment', comment: flag.post, with_post_link: true, pingable: flag.post.comment_thread.pingable %>
   <% end %>
   <div class="widget--body">
     <p>

app/views/moderator/recent_comments.html.erb

@@ -9,8 +9,16 @@
   communities; consider using activity logs on user profiles instead.
 </p>
 
+<%
+  thread_pingables = @comments.map(&:comment_thread).to_set.to_h do |thread|
+    [thread, thread.pingable]
+  end
+%>
+
 <% @comments.each do |comment| %>
-  <%= render 'comments/comment', comment: comment, with_post_link: true %>
+  <%= render 'comments/comment', comment: comment,
+                                 pingable: thread_pingables[comment.comment_thread],
+                                 with_post_link: true %>
 <% end %>
 
 <div class="has-padding-top-4">

config/locales/strings/en.comments.yml

@@ -34,3 +34,6 @@ en:
         Start new comment thread
       reply_to_thread: >
         Reply to this thread
+    warnings:
+      unrelated_user_not_pinged: >
+        This user was not notified because they have not participated in this thread.