From 6a626e819d4b8a5d8460fe8719118d654aeecaf2 Mon Sep 17 00:00:00 2001 From: rhymes Date: Tue, 28 May 2019 23:32:53 +0200 Subject: [PATCH] Refactor: fix issues raised by bullet (#2965) * Preload organization, not user * Preload users only when needed * Pre-load podcasts for podcast episodes in API * Avoid eager loading error by loading rating votes separately * Preload associations for moderation * Preload user comments in trees * Preload organization for non org dashboard and cleanup queries * Optimize ArticleSuggester to only load N articles at need * Remove eager loading and pass variables to partials for easier debug * Reorganize tags validation code and ignore actsastaggableon eager loading issues * Remove unused eager loading and bring up comments relation * Preload podcasts when loading podcast episodes * Fix views specs * Make sure ArticleSuggester never returns duplicates * Remove commented code * Re-trigger build * Move suggested articles back to view to respect fragment caching --- app/controllers/api/v0/comments_controller.rb | 4 +-- .../api/v0/podcast_episodes_controller.rb | 4 ++- app/controllers/articles_controller.rb | 5 ++- app/controllers/comments_controller.rb | 2 +- app/controllers/dashboards_controller.rb | 14 ++++++-- app/controllers/moderations_controller.rb | 3 +- .../podcast_episodes_controller.rb | 2 +- app/labor/article_suggester.rb | 33 +++++++++++++----- app/labor/cache_buster.rb | 2 +- app/models/article.rb | 23 +++++++------ app/models/comment.rb | 6 ++-- app/services/article_api_index_service.rb | 2 +- app/services/articles/updater.rb | 11 +++--- app/services/moderator/delete_user.rb | 3 +- .../moderator/manage_activity_and_roles.rb | 2 +- app/views/api/v0/comments/index.json.jbuilder | 9 +++-- app/views/articles/_bottom_content.html.erb | 4 ++- app/views/articles/_v2_form.html.erb | 34 +++++++++---------- app/views/articles/_video_player.html.erb | 8 ++--- app/views/articles/edit.html.erb | 5 +-- app/views/articles/new.html.erb | 2 +- app/views/articles/show.html.erb | 6 ++-- app/views/comments/index.html.erb | 2 +- .../dashboards/_dashboard_article.html.erb | 2 +- app/views/dashboards/show.html.erb | 3 +- app/views/moderations/index.html.erb | 8 +++-- config/environments/test.rb | 3 ++ spec/labor/article_suggester_spec.rb | 24 +++++++------ spec/requests/articles_spec.rb | 26 ++++++++------ spec/requests/moderations_spec.rb | 2 ++ 30 files changed, 148 insertions(+), 106 deletions(-) diff --git a/app/controllers/api/v0/comments_controller.rb b/app/controllers/api/v0/comments_controller.rb index 2175cb3e1..024651a30 100644 --- a/app/controllers/api/v0/comments_controller.rb +++ b/app/controllers/api/v0/comments_controller.rb @@ -13,8 +13,8 @@ module Api respond_to :json def index - @commentable = Article.find(params[:a_id]) - @commentable_type = "Article" + article = Article.find(params[:a_id]) + @comments = Comment.rooted_on(article, "Article").order(score: :desc) end def show diff --git a/app/controllers/api/v0/podcast_episodes_controller.rb b/app/controllers/api/v0/podcast_episodes_controller.rb index 37ac4b36d..924a9ba4f 100644 --- a/app/controllers/api/v0/podcast_episodes_controller.rb +++ b/app/controllers/api/v0/podcast_episodes_controller.rb @@ -19,7 +19,9 @@ module Api page(@page). per(30) else - @podcast_episodes = PodcastEpisode.order("published_at desc").page(@page).per(30) + @podcast_episodes = PodcastEpisode. + includes(:podcast). + order("published_at desc").page(@page).per(30) end end end diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 946f473b4..479771210 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -73,9 +73,8 @@ class ArticlesController < ApplicationController def edit authorize @article - @user = @article.user + @organization = @article.user&.organization @version = @article.has_frontmatter? ? "v1" : "v2" - @organization = @user&.organization end def manage @@ -203,7 +202,7 @@ class ArticlesController < ApplicationController def set_article owner = User.find_by(username: params[:username]) || Organization.find_by(slug: params[:username]) found_article = if params[:slug] - owner.articles.includes(:user).find_by(slug: params[:slug]) + owner.articles.find_by(slug: params[:slug]) else Article.includes(:user).find(params[:id]) end diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index f089aeeb8..2f38cf72e 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -27,8 +27,8 @@ class CommentsController < ApplicationController @article = @commentable not_found unless @commentable.published end + @commentable_type = @commentable.class.name - @root_comment = Comment.find(params[:id_code].to_i(26)) if params[:id_code].present? set_surrogate_key_header "comments-for-#{@commentable.id}-#{@commentable_type}" end diff --git a/app/controllers/dashboards_controller.rb b/app/controllers/dashboards_controller.rb index 86c9973a6..e092efbdb 100644 --- a/app/controllers/dashboards_controller.rb +++ b/app/controllers/dashboards_controller.rb @@ -6,9 +6,17 @@ class DashboardsController < ApplicationController def show fetch_and_authorize_user - target = @user - target = @user.organization if @user&.organization && @user&.org_admin && params[:which] == "organization" - @articles = target.articles.sorting(params[:sort]).decorate + @org_dashboard = @user&.organization && @user&.org_admin && params[:which] == "organization" + + if @org_dashboard + articles = @user.organization.articles + @org_members = @user.organization.users.pluck(:name, :id) + else + articles = @user.articles.includes(:organization) + end + + @articles = articles.sorting(params[:sort]).decorate + # Updates analytics in background if appropriate: ArticleAnalyticsFetcher.new.delay.update_analytics(current_user.id) if @articles && ApplicationConfig["GA_FETCH_RATE"] < 50 # Rate limit concerned, sometimes we throttle down. end diff --git a/app/controllers/moderations_controller.rb b/app/controllers/moderations_controller.rb index bab82a395..d68552b96 100644 --- a/app/controllers/moderations_controller.rb +++ b/app/controllers/moderations_controller.rb @@ -6,13 +6,12 @@ class ModerationsController < ApplicationController return unless current_user&.trusted @articles = Article.published. - includes(:rating_votes). where("rating_votes_count < 3"). where("score > -5"). order("hotness_score DESC").limit(100) - @articles = @articles.cached_tagged_with(params[:tag]) if params[:tag].present? + @rating_votes = RatingVote.where(article: @articles.pluck(:id), user: current_user) @articles = @articles.decorate end diff --git a/app/controllers/podcast_episodes_controller.rb b/app/controllers/podcast_episodes_controller.rb index 38c2db167..6b40e9ee7 100644 --- a/app/controllers/podcast_episodes_controller.rb +++ b/app/controllers/podcast_episodes_controller.rb @@ -5,7 +5,7 @@ class PodcastEpisodesController < ApplicationController def index @podcast_index = true @podcasts = Podcast.order("title asc") - @podcast_episodes = PodcastEpisode.order("published_at desc").first(20) + @podcast_episodes = PodcastEpisode.includes(:podcast).order("published_at desc").first(20) if params[:q].blank? set_surrogate_key_header("podcast_episodes_all " + params[:q].to_s, @podcast_episodes.map { |e| e["record_key"] }) diff --git a/app/labor/article_suggester.rb b/app/labor/article_suggester.rb index 949850b90..8bdc2872e 100644 --- a/app/labor/article_suggester.rb +++ b/app/labor/article_suggester.rb @@ -1,33 +1,48 @@ class ArticleSuggester - attr_accessor :article def initialize(article) @article = article end - def articles(num = 4) + def articles(max: 4) if article.tag_list.any? - (suggestions_by_tag + other_suggestions(num)).flatten.first(num).to_a + # avoid loading more data if we don't need to + tagged_suggestions = suggestions_by_tag(max: max) + return tagged_suggestions if tagged_suggestions.size == max + + # if there are not enough tagged articles, load other suggestions + # ignoring tagged articles that might be relevant twice, hence avoiding duplicates + num_remaining_needed = max - tagged_suggestions.size + other_articles = other_suggestions( + max: num_remaining_needed, + ids_to_ignore: tagged_suggestions.pluck(:id), + ) + tagged_suggestions.union(other_articles) else - other_suggestions(num).to_a + other_suggestions(max: max) end end - def other_suggestions(num = 4) + private + + attr_reader :article + + def other_suggestions(max: 4, ids_to_ignore: []) + ids_to_ignore << article.id Article.published.where(featured: true). - where.not(id: article.id). + where.not(id: ids_to_ignore). order("hotness_score DESC"). includes(:user). offset(rand(0..offsets[1])). - first(num) + first(max) end - def suggestions_by_tag + def suggestions_by_tag(max: 4) Article.published.tagged_with(article.tag_list, any: true). where.not(id: article.id). order("hotness_score DESC"). includes(:user). offset(rand(0..offsets[0])). - first(4) + first(max) end def offsets diff --git a/app/labor/cache_buster.rb b/app/labor/cache_buster.rb index c3f966fb5..7617588e2 100644 --- a/app/labor/cache_buster.rb +++ b/app/labor/cache_buster.rb @@ -22,7 +22,7 @@ class CacheBuster end bust("#{commentable.path}/comments/") bust(commentable.path.to_s) - commentable.comments.find_each do |c| + commentable.comments.includes(:user).find_each do |c| bust(c.path) bust(c.path + "?i=i") end diff --git a/app/models/article.rb b/app/models/article.rb index 197cc1544..cbe4f9597 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -62,8 +62,6 @@ class Article < ApplicationRecord after_save :detect_human_language before_save :update_cached_user after_update :update_notifications, if: proc { |article| article.notifications.any? && !article.saved_changes.empty? } - # after_save :send_to_moderator - # turned off for now before_destroy :before_destroy_actions serialize :ids_for_suggested_articles @@ -466,11 +464,9 @@ class Article < ApplicationRecord self.title = front_matter["title"] if front_matter["title"].present? if front_matter["tags"].present? ActsAsTaggableOn::Taggable::Cache.included(Article) - self.tag_list = [] + self.tag_list = [] # overwrite any existing tag with those from the front matter tag_list.add(front_matter["tags"], parser: ActsAsTaggableOn::TagParser) - TagAdjustment.where(article_id: id, adjustment_type: "removal", status: "committed").pluck(:tag_name).each do |name| - tag_list.remove(name, parser: ActsAsTaggableOn::TagParser) - end + remove_tag_adjustments_from_tag_list end self.published = front_matter["published"] if %w[true false].include?(front_matter["published"].to_s) self.published_at = parsed_date(front_matter["date"]) if published @@ -494,17 +490,22 @@ class Article < ApplicationRecord def validate_tag # remove adjusted tags - TagAdjustment.where(article_id: id, adjustment_type: "removal", status: "committed").pluck(:tag_name).each do |name| - tag_list.remove(name, parser: ActsAsTaggableOn::TagParser) - self.tag_list = tag_list - end - return errors.add(:tag_list, "exceed the maximum of 4 tags") if tag_list.length > 4 + remove_tag_adjustments_from_tag_list + # check there are not too many tags + return errors.add(:tag_list, "exceed the maximum of 4 tags") if tag_list.size > 4 + + # check tags names aren't too long tag_list.each do |tag| errors.add(:tag, "\"#{tag}\" is too long (maximum is 20 characters)") if tag.length > 20 end end + def remove_tag_adjustments_from_tag_list + tags_to_remove = TagAdjustment.where(article_id: id, adjustment_type: "removal", status: "committed").pluck(:tag_name) + tag_list.remove(tags_to_remove, parser: ActsAsTaggableOn::TagParser) if tags_to_remove + end + def validate_video return errors.add(:published, "cannot be set to true if video is still processing") if published && video_state == "PROGRESSING" return errors.add(:video, "cannot be added member without permission") if video.present? && user.created_at > 2.weeks.ago diff --git a/app/models/comment.rb b/app/models/comment.rb index 02df2423b..4636f5a08 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -122,12 +122,10 @@ class Comment < ApplicationRecord end def self.rooted_on(commentable_id, commentable_type) - includes(:user, :commentable). + includes(:user). select(:id, :user_id, :commentable_type, :commentable_id, :deleted, :created_at, :processed_html, :ancestry, :updated_at, :score). - where(commentable_id: commentable_id, - ancestry: nil, - commentable_type: commentable_type) + where(commentable_id: commentable_id, ancestry: nil, commentable_type: commentable_type) end def self.tree_for(commentable, limit = 0) diff --git a/app/services/article_api_index_service.rb b/app/services/article_api_index_service.rb index 23d609591..fac6d50b2 100644 --- a/app/services/article_api_index_service.rb +++ b/app/services/article_api_index_service.rb @@ -36,7 +36,7 @@ class ArticleApiIndexService if (user = User.find_by(username: username)) user.articles.published. - includes(:user). + includes(:organization). order("published_at DESC"). page(page). per(num) diff --git a/app/services/articles/updater.rb b/app/services/articles/updater.rb index f04c0d950..0800f59e8 100644 --- a/app/services/articles/updater.rb +++ b/app/services/articles/updater.rb @@ -11,11 +11,7 @@ module Articles end def call - article = if user.has_role?(:super_admin) - Article.includes(:user).find(article_id) - else - user.articles.find(article_id) - end + article = load_article # the client can change the series the article belongs to if article_params.key?(:series) @@ -51,5 +47,10 @@ module Articles attr_reader :user, :article_id attr_accessor :article_params + + def load_article + relation = user.has_role?(:super_admin) ? Article.includes(:user) : user.articles + relation.find(article_id) + end end end diff --git a/app/services/moderator/delete_user.rb b/app/services/moderator/delete_user.rb index e6c94ebd3..8e63a2863 100644 --- a/app/services/moderator/delete_user.rb +++ b/app/services/moderator/delete_user.rb @@ -51,7 +51,8 @@ module Moderator def reassign_articles return unless user.articles.any? - user.articles.find_each do |article| + # preload associations that are going to be used during indexing + user.articles.preload(:taggings, :organization).find_each do |article| path = "/#{@ghost.username}/#{article.slug}" article.update_columns(user_id: @ghost.id, path: path) article.index! diff --git a/app/services/moderator/manage_activity_and_roles.rb b/app/services/moderator/manage_activity_and_roles.rb index 2a7fd64d4..6d5c927d9 100644 --- a/app/services/moderator/manage_activity_and_roles.rb +++ b/app/services/moderator/manage_activity_and_roles.rb @@ -28,7 +28,7 @@ module Moderator user.articles.find_each do |article| article.reactions.delete_all - article.comments.find_each do |comment| + article.comments.includes(:user).find_each do |comment| comment.reactions.delete_all CacheBuster.new.bust_comment(comment.commentable, comment.user.username) comment.delete diff --git a/app/views/api/v0/comments/index.json.jbuilder b/app/views/api/v0/comments/index.json.jbuilder index f53ec613e..746b7d211 100644 --- a/app/views/api/v0/comments/index.json.jbuilder +++ b/app/views/api/v0/comments/index.json.jbuilder @@ -1,5 +1,6 @@ -json.array! Comment.rooted_on(@commentable.id, @commentable_type).order("score DESC") do |comment| +json.array! @comments do |comment| json.type_of "comment" + json.id_code comment.id_code_generated json.body_html comment.processed_html json.user do @@ -11,7 +12,8 @@ json.array! Comment.rooted_on(@commentable.id, @commentable_type).order("score D json.profile_image ProfileImage.new(comment.user).get(640) json.profile_image_90 ProfileImage.new(comment.user).get(90) end - json.children comment.children.order("score DESC").each do |children_comment| + + json.children comment.children.order(score: :desc).each do |children_comment| json.id_code children_comment.id_code_generated json.body_html children_comment.processed_html json.user do @@ -23,7 +25,8 @@ json.array! Comment.rooted_on(@commentable.id, @commentable_type).order("score D json.profile_image ProfileImage.new(children_comment.user).get(640) json.profile_image_90 ProfileImage.new(children_comment.user).get(90) end - json.children children_comment.children.order("score DESC").each do |grandchild_comment| + + json.children children_comment.children.order(score: :desc).each do |grandchild_comment| json.id_code grandchild_comment.id_code_generated json.body_html grandchild_comment.processed_html json.user do diff --git a/app/views/articles/_bottom_content.html.erb b/app/views/articles/_bottom_content.html.erb index 5358fa1eb..973e3b794 100644 --- a/app/views/articles/_bottom_content.html.erb +++ b/app/views/articles/_bottom_content.html.erb @@ -1,5 +1,6 @@ +<% if articles %>
- <% ArticleSuggester.new(@article).articles(4).each do |article| %> + <% articles.each do |article| %>
@@ -25,3 +26,4 @@ <% end %>
+<% end %> diff --git a/app/views/articles/_v2_form.html.erb b/app/views/articles/_v2_form.html.erb index 53ccf6cfa..bb09b2c4c 100644 --- a/app/views/articles/_v2_form.html.erb +++ b/app/views/articles/_v2_form.html.erb @@ -1,36 +1,36 @@
-
- <% if @organization %> + data-article="<%= article.to_json(only: %i[id title cached_tag_list published body_markdown main_image organization_id canonical_url], methods: %i[series all_series]) %>" + data-organization="<%= organization&.to_json(only: %i[name bg_color_hex text_color_hex], methods: [:profile_image_90]) %>" + data-version="<%= version %>"> + + <% if organization %>
- " alt="<%= @organization.name %> profile image"> - <%= @organization.name %> -
<% end %> - <% if @version == "v2" %> - <% if @article.main_image.present? %> -
+ <% if version == "v2" %> + <% if article.main_image.present? %> +
<% end %> - +
- +
<% end %> - +
- <% if @version == "v2" %> + <% if version == "v2" %> <% end %>
@@ -42,7 +42,7 @@
- <% if @version == "v2" %> + <% if version == "v2" %> <%= render "pages/editor_guide_text", version: "2" %> <% else %> <%= render "pages/editor_guide_text", version: "1" %> diff --git a/app/views/articles/_video_player.html.erb b/app/views/articles/_video_player.html.erb index f9076c7cb..288b1b249 100644 --- a/app/views/articles/_video_player.html.erb +++ b/app/views/articles/_video_player.html.erb @@ -1,11 +1,11 @@
<% if meta_tags %> - - - + + + - <% minutes, seconds = @article.video_duration_in_minutes.split(":") %> + <% minutes, seconds = article.video_duration_in_minutes.split(":") %> sM%sS", minutes: minutes, seconds: seconds) %>" /> <% end %> diff --git a/app/views/articles/edit.html.erb b/app/views/articles/edit.html.erb index 3a2416b37..491c7779c 100644 --- a/app/views/articles/edit.html.erb +++ b/app/views/articles/edit.html.erb @@ -2,7 +2,6 @@ <% if @article.video.present? %>
- <% if @article.video_state == "PROGRESSING" %>

⏳ Video Transcoding In Progress ⏳

Bored kid incessantly tapping his fingers @@ -19,16 +18,14 @@

<%= render "articles/video_player", meta_tags: false, article: @article %>
- <%= form_for(@article) do |f| %> Preview Image URL: <%= f.text_field :video_thumbnail_url, placeholder: "New Thumbnail URL" %> <%= f.submit "Submit Change", class: "cta" %> <% end %>
- <% end %>
<% end %> <%= javascript_pack_tag "articleForm", defer: true %> -<%= render "articles/v2_form" %> +<%= render "articles/v2_form", article: @article, organization: @organization, version: @version %> diff --git a/app/views/articles/new.html.erb b/app/views/articles/new.html.erb index 7228dd472..52486c036 100644 --- a/app/views/articles/new.html.erb +++ b/app/views/articles/new.html.erb @@ -2,7 +2,7 @@ <% if user_signed_in? %> <%= javascript_pack_tag "articleForm", defer: true %> - <%= render "articles/v2_form" %> + <%= render "articles/v2_form", article: @article, organization: @organization, version: @version %> <% else %> <% @new_article_not_logged_in = true %> <%= render "devise/registrations/registration_form" %> diff --git a/app/views/articles/show.html.erb b/app/views/articles/show.html.erb index 448e5b59d..3e0cf5b69 100644 --- a/app/views/articles/show.html.erb +++ b/app/views/articles/show.html.erb @@ -2,7 +2,7 @@ <%= javascript_pack_tag "clipboardCopy", defer: true %> -<% cache("content-related-optional-scripts-#{@article.id}-#{@article.updated_at}", :expires_in => 30.hours) do %> +<% cache("content-related-optional-scripts-#{@article.id}-#{@article.updated_at}", expires_in: 30.hours) do %> <% if @article.processed_html.include? "ltag_gist-liquid-tag" %> <%= javascript_include_tag "https://cdnjs.cloudflare.com/ajax/libs/postscribe/2.0.8/postscribe.min.js", defer: true %>