From 7851d0f45ac57ed21a12fdc484398315333abddd Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Wed, 23 Jul 2025 07:57:27 +0300 Subject: [PATCH 1/4] minor cleanup of the comments controller --- app/controllers/comments_controller.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index de2bfede1..358f23271 100644 --- a/app/controllers/comments_controller.rb +++ b/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 @comment_thread.save! @@ -49,7 +49,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(', ')}" @@ -59,7 +59,7 @@ 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) @@ -67,7 +67,7 @@ def create 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 @@ -98,13 +98,13 @@ 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', @@ -372,12 +372,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] list of pinged user ids def check_for_pings(thread, content) pingable = helpers.get_pingable(thread) matches = content.scan(/@#(\d+)/) matches.flatten.select { |m| pingable.include?(m.to_i) }.map(&:to_i) end + # @param pings [Array] list of pinged user ids def apply_pings(pings) pings.each do |p| user = User.where(id: p).first From 409b762846dd7b96176a4f1eecdabc63d3458d8a Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Wed, 23 Jul 2025 08:03:25 +0300 Subject: [PATCH 2/4] fixed updating a comment temporarily showing pings as 'unpingable' --- app/controllers/comments_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 358f23271..f6aaaad78 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -108,7 +108,8 @@ def update 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(', ')})" }, From a8d4a69a20fb0b5352817de88be1720bb1cbd8ed Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Wed, 23 Jul 2025 08:09:16 +0300 Subject: [PATCH 3/4] moved warning that unrelated pings users are not notified to a locale string --- app/helpers/comments_helper.rb | 2 +- config/locales/strings/en.comments.yml | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/helpers/comments_helper.rb b/app/helpers/comments_helper.rb index 57e29a55b..45e870ae1 100644 --- a/app/helpers/comments_helper.rb +++ b/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 diff --git a/config/locales/strings/en.comments.yml b/config/locales/strings/en.comments.yml index 11427e9e4..044ee1a4b 100644 --- a/config/locales/strings/en.comments.yml +++ b/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. From 07cb550d358f332a38af3b2a161371d235649527 Mon Sep 17 00:00:00 2001 From: Oleg Valter Date: Wed, 23 Jul 2025 08:51:12 +0300 Subject: [PATCH 4/4] fixed other cases of comments not accounting for pingable state --- app/controllers/comments_controller.rb | 5 ++-- app/helpers/comments_helper.rb | 28 -------------------- app/models/comment.rb | 6 +++++ app/models/comment_thread.rb | 27 +++++++++++++++++-- app/views/comment_threads/_expanded.html.erb | 2 +- app/views/flags/_flag.html.erb | 2 +- app/views/moderator/recent_comments.html.erb | 10 ++++++- 7 files changed, 44 insertions(+), 36 deletions(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index f6aaaad78..46c365bc1 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -273,8 +273,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 @@ -377,7 +376,7 @@ def check_if_target_post_locked # @param content [String] content to extract pings from # @return [Array] 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 diff --git a/app/helpers/comments_helper.rb b/app/helpers/comments_helper.rb index 45e870ae1..1f803bd2e 100644 --- a/app/helpers/comments_helper.rb +++ b/app/helpers/comments_helper.rb @@ -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] - 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. diff --git a/app/models/comment.rb b/app/models/comment.rb index dce66c8fb..7164cdd34 100644 --- a/app/models/comment.rb +++ b/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 diff --git a/app/models/comment_thread.rb b/app/models/comment_thread.rb index 512a8e02a..e657c4606 100644 --- a/app/models/comment_thread.rb +++ b/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] + 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 diff --git a/app/views/comment_threads/_expanded.html.erb b/app/views/comment_threads/_expanded.html.erb index 0c14c50b0..b422ac7db 100644 --- a/app/views/comment_threads/_expanded.html.erb +++ b/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 %> <% diff --git a/app/views/flags/_flag.html.erb b/app/views/flags/_flag.html.erb index 8745e885d..0cd32d05f 100644 --- a/app/views/flags/_flag.html.erb +++ b/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 %> <% 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 %>

diff --git a/app/views/moderator/recent_comments.html.erb b/app/views/moderator/recent_comments.html.erb index 9901ee5bb..c9923ee3d 100644 --- a/app/views/moderator/recent_comments.html.erb +++ b/app/views/moderator/recent_comments.html.erb @@ -9,8 +9,16 @@ communities; consider using activity logs on user profiles instead.

+<% + 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 %>