From b283281e5e92dea8ac83cdc77838b2eeb5dbb268 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 20 Aug 2019 20:53:40 -0500 Subject: [PATCH] comments: minimize sql queries. Certain parts of comment rendering triggered sql queries that we didn't really need to do. Rework things to avoid this. * Preload comment creators in order to display commenter names with link_to_user. * Preload comment votes in order to display "undo vote" links. Only preload votes for members since anonymous users can't vote and don't have "undo vote" links. * Rework various conditionals to do the filtering in Ruby so that we avoid issuing any extra queries in sql. * Avoid issuing any queries at all when the post doesn't have any comments (when last_commented_at is blank). --- app/controllers/comments_controller.rb | 5 +++- app/controllers/posts_controller.rb | 4 +++ app/models/comment.rb | 27 +++++-------------- app/models/post.rb | 2 +- app/views/application/_update_notice.html.erb | 2 +- app/views/comments/_index_by_post.html.erb | 12 ++++----- .../comments/partials/index/_list.html.erb | 16 +++++------ app/views/posts/show.html.erb | 2 +- test/unit/comment_test.rb | 20 -------------- 9 files changed, 30 insertions(+), 60 deletions(-) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index b1f623d34..637d09a1c 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -74,7 +74,10 @@ private def index_by_post @posts = Post.where("last_comment_bumped_at IS NOT NULL").tag_match(params[:tags]).reorder("last_comment_bumped_at DESC NULLS LAST").paginate(params[:page], :limit => 5, :search_count => params[:search]) - @posts.each # hack to force rails to eager load + + @posts = @posts.includes(comments: [:creator]) + @posts = @posts.includes(comments: [:votes]) if CurrentUser.is_member? + respond_with(@posts) do |format| format.xml do render :xml => @posts.to_xml(:root => "posts") diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index fd57fb78d..5a7dea927 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -23,6 +23,10 @@ class PostsController < ApplicationController def show @post = Post.find(params[:id]) + @comments = @post.last_commented_at.present? ? @post.comments : Comment.none + @comments = @comments.includes(:creator) + @comments = @comments.includes(:votes) if CurrentUser.is_member? + include_deleted = @post.is_deleted? || (@post.parent_id.present? && @post.parent.is_deleted?) || CurrentUser.user.show_deleted_children? @parent_post_set = PostSets::PostRelationship.new(@post.parent_id, :include_deleted => include_deleted) @children_post_set = PostSets::PostRelationship.new(@post.id, :include_deleted => include_deleted) diff --git a/app/models/comment.rb b/app/models/comment.rb index 0f30aed63..68f66365c 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -22,26 +22,6 @@ class Comment < ApplicationRecord ) module SearchMethods - def recent - reorder("comments.id desc").limit(6) - end - - def hidden(user) - if user.is_moderator? - where("(score < ? and is_sticky = false) or is_deleted = true", user.comment_threshold) - else - where("score < ? and is_sticky = false", user.comment_threshold) - end - end - - def visible(user) - if user.is_moderator? - where("(score >= ? or is_sticky = true) and is_deleted = false", user.comment_threshold) - else - where("score >= ? or is_sticky = true", user.comment_threshold) - end - end - def deleted where("comments.is_deleted = true") end @@ -172,9 +152,16 @@ class Comment < ApplicationRecord end def voted_by?(user) + return false if user.is_anonymous? user.id.in?(votes.map(&:user_id)) end + def visible_by?(user, show_thresholded: false, show_deleted: false) + return false if is_deleted? && !show_deleted && !user.is_moderator? + return false if score < user.comment_threshold && !is_sticky? && !show_thresholded + true + end + def hidden_attributes super + [:body_index] end diff --git a/app/models/post.rb b/app/models/post.rb index 38264e126..507a13374 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -51,7 +51,7 @@ class Post < ApplicationRecord has_many :appeals, :class_name => "PostAppeal", :dependent => :destroy has_many :votes, :class_name => "PostVote", :dependent => :destroy has_many :notes, :dependent => :destroy - has_many :comments, -> {includes(:creator, :updater).order("comments.id")}, :dependent => :destroy + has_many :comments, -> {order("comments.id")}, :dependent => :destroy has_many :children, -> {order("posts.id")}, :class_name => "Post", :foreign_key => "parent_id" has_many :approvals, :class_name => "PostApproval", :dependent => :destroy has_many :disapprovals, :class_name => "PostDisapproval", :dependent => :destroy diff --git a/app/views/application/_update_notice.html.erb b/app/views/application/_update_notice.html.erb index 2f8e0a056..dcb6d474b 100644 --- a/app/views/application/_update_notice.html.erb +++ b/app/views/application/_update_notice.html.erb @@ -1,6 +1,6 @@ <%# record, interval %> -<% if record.respond_to?(:updater) && record.updater != record.creator %> +<% if record.respond_to?(:updater) && record.updater_id != record.creator_id %>

Updated by <%= link_to_user record.updater %> <%= time_ago_in_words_tagged(record.updated_at) %>

<% elsif record.updated_at - record.created_at > (local_assigns[:interval] || 5.minutes) %>

Updated <%= time_ago_in_words_tagged(record.updated_at) %>

diff --git a/app/views/comments/_index_by_post.html.erb b/app/views/comments/_index_by_post.html.erb index 640d3ef23..08faa93af 100644 --- a/app/views/comments/_index_by_post.html.erb +++ b/app/views/comments/_index_by_post.html.erb @@ -5,19 +5,17 @@ <% end %> - <% if @posts.empty? %> + <% if @posts.blank? %> <%= render "post_sets/blank" %> <% end %> - <% @posts.select {|x| x.visible?}.each do |post| %> - <% if CurrentUser.is_moderator? || post.comments.undeleted.exists? %> + <% @posts.select(&:visible?).each do |post| %> + <% if post.comments.any? { |c| c.visible_by?(CurrentUser.user, show_thresholded: true) } %> <%= content_tag(:div, { id: "post_#{post.id}", class: ["post", *PostPresenter.preview_class(post)].join(" ") }.merge(PostPresenter.data_attributes(post))) do %>
- <% if post.visible? %> - <%= link_to(image_tag(post.preview_file_url), post_path(post)) %> - <% end %> + <%= link_to(image_tag(post.preview_file_url), post_path(post)) %>
- <%= render "comments/partials/index/list", :post => post, :comments => post.comments.visible(CurrentUser.user).recent.reverse, :show_header => true %> + <%= render "comments/partials/index/list", post: post, comments: post.comments.select { |c| c.visible_by?(CurrentUser.user) }.last(6), page: :comments %>
<% end %> <% end %> diff --git a/app/views/comments/partials/index/_list.html.erb b/app/views/comments/partials/index/_list.html.erb index 20d1d299e..a5d7810dc 100644 --- a/app/views/comments/partials/index/_list.html.erb +++ b/app/views/comments/partials/index/_list.html.erb @@ -1,10 +1,10 @@
- <% if show_header %> + <% if page == :comments %> <%= render "comments/partials/index/header", :post => post %> <% end %>
- <% if post.comments.hidden(CurrentUser.user).count > 0 || (params[:controller] == "comments" && post.comments.count > 6) %> + <% if (post.last_commented_at.present? && post.comments.any? { |c| !c.visible_by?(CurrentUser.user) }) || (page == :comments && post.comments.size > 6) %> <%= link_to "Show all comments", comments_path(:post_id => post.id), :remote => true %> @@ -12,14 +12,12 @@
- <% if comments.empty? %> - <% if post.last_commented_at.present? %> -

There are no visible comments.

- <% else %> -

There are no comments.

- <% end %> + <% if comments.present? %> + <%= render partial: "comments/partials/show/comment", collection: comments %> + <% elsif post.last_commented_at.present? %> +

There are no visible comments.

<% else %> - <%= render :partial => "comments/partials/show/comment", :collection => comments %> +

There are no comments.

<% end %>
diff --git a/app/views/posts/show.html.erb b/app/views/posts/show.html.erb index cfc6350c9..a2c139ad6 100644 --- a/app/views/posts/show.html.erb +++ b/app/views/posts/show.html.erb @@ -120,7 +120,7 @@ <% if !CurrentUser.user.is_builder? %>

Before commenting, read the <%= link_to "how to comment guide", wiki_pages_path(:search => {:title => "howto:comment"}) %>.

<% end %> - <%= render "comments/partials/index/list", :comments => @post.comments.visible(CurrentUser.user).includes(:creator), :post => @post, :show_header => false %> + <%= render "comments/partials/index/list", comments: @comments, post: @post, page: :post %>