From 6cd46ae2f952685226ffc829a9615003429f62c2 Mon Sep 17 00:00:00 2001 From: rhymes Date: Fri, 7 Feb 2020 15:02:22 +0100 Subject: [PATCH] Enable bullet during tests to catch N+1 early (and some cleanups) (#5930) [deploy] * Remove unused methods * Small refactorings * Enable bullet in testing mode * Remove unused eager loading for notes * Skip bullet on rolify method * Use .present? * Fix method name --- app/controllers/internal/mods_controller.rb | 16 +++++++--- app/controllers/podcasts_controller.rb | 25 +++++++++++++-- app/models/comment.rb | 9 ------ app/models/html_variant.rb | 35 +++++++++++++-------- app/models/message.rb | 3 +- app/models/reaction.rb | 8 ----- app/models/tweet.rb | 2 ++ config/environments/test.rb | 5 +-- 8 files changed, 62 insertions(+), 41 deletions(-) diff --git a/app/controllers/internal/mods_controller.rb b/app/controllers/internal/mods_controller.rb index 79460f0cb..abab279f3 100644 --- a/app/controllers/internal/mods_controller.rb +++ b/app/controllers/internal/mods_controller.rb @@ -3,18 +3,26 @@ class Internal::ModsController < Internal::ApplicationController def index @mods = if params[:state] == "tag" - User.with_role(:tag_moderator, :any).page(params[:page]).per(50).includes(:notes) + User.with_role(:tag_moderator, :any).page(params[:page]).per(50) elsif params[:state] == "potential" - User.order("comments_count DESC").page(params[:page]).per(100).includes(:notes) + User.order("comments_count DESC").page(params[:page]).per(100) else - User.with_role(:trusted).page(params[:page]).per(50).includes(:notes) + User.with_role(:trusted).page(params[:page]).per(50) end - @mods = @mods.where("users.username ILIKE :search OR users.name ILIKE :search", search: "%#{params[:search]}%") if params[:search].present? + + return if params[:search].blank? + + @mods = @mods.where( + "users.username ILIKE :search OR users.name ILIKE :search", + search: "%#{params[:search]}%", + ) end def update @user = User.find(params[:id]) + AssignTagModerator.add_trusted_role(@user) + render body: nil # No response needed at the moment end end diff --git a/app/controllers/podcasts_controller.rb b/app/controllers/podcasts_controller.rb index 1bdb186b9..68794b105 100644 --- a/app/controllers/podcasts_controller.rb +++ b/app/controllers/podcasts_controller.rb @@ -1,6 +1,10 @@ class PodcastsController < ApplicationController before_action :authenticate_user! + # skip Bullet on create as it currently triggers an error inside the rolify + # method "current_user.add_role()" we have no control of + around_action :skip_bullet, only: %i[create], if: -> { defined?(Bullet) } + def new @podcast = Podcast.new @podcasts = Podcast.available.order("title asc") @@ -10,13 +14,16 @@ class PodcastsController < ApplicationController def create @podcast = Podcast.new(podcast_params) @podcast.creator = current_user + if @podcast.save current_user.add_role(:podcast_admin, @podcast) if added_by_owner? flash[:global_notice] = "Podcast suggested" - redirect_to "/pod" + + redirect_to pod_path else - @podcasts = Podcast.available.order("title asc") + @podcasts = Podcast.available.order(title: :asc) @podcast_index = true + render :new end end @@ -28,7 +35,19 @@ class PodcastsController < ApplicationController end def podcast_params - allowed_params = %i[android_url image itunes_url main_color_hex overcast_url pattern_image slug soundcloud_url twitter_username website_url title feed_url description] + allowed_params = %i[ + android_url image itunes_url main_color_hex overcast_url pattern_image + slug soundcloud_url twitter_username website_url title feed_url description + ] params.require(:podcast).permit(allowed_params) end + + def skip_bullet + previous_value = Bullet.enable? + Bullet.enable = false + + yield + ensure + Bullet.enable = previous_value + end end diff --git a/app/models/comment.rb b/app/models/comment.rb index b38ffb186..1075f8915 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -97,15 +97,6 @@ class Comment < ApplicationRecord end end - def self.users_with_number_of_comments(user_ids, before_date) - joins(:user). - select("users.username, COUNT(comments.user_id) AS number_of_comments"). - where(user_id: user_ids). - where(arel_table[:created_at].gt(before_date)). - group(User.arel_table[:username]). - order("number_of_comments DESC") - end - def self.tree_for(commentable, limit = 0) commentable.comments.includes(:user).arrange(order: "score DESC").to_a[0..limit - 1].to_h end diff --git a/app/models/html_variant.rb b/app/models/html_variant.rb index 48c867ba2..923071ea5 100644 --- a/app/models/html_variant.rb +++ b/app/models/html_variant.rb @@ -6,9 +6,11 @@ class HtmlVariant < ApplicationRecord validates :group, inclusion: { in: %w[article_show_sidebar_cta article_show_below_article_cta badge_landing_page] } validates :success_rate, presence: true validate :no_edits + belongs_to :user, optional: true has_many :html_variant_trials has_many :html_variant_successes + before_save :prefix_all_images def calculate_success_rate! @@ -16,27 +18,34 @@ class HtmlVariant < ApplicationRecord save! end - def self.find_for_test(tags = [], group = "article_show_sidebar_cta") - tags_array = tags + ["", nil] - if rand(10) == 1 # 10% return completely random - find_random_for_test(tags_array, group) - else # 90% chance return one in top 10 - find_top_for_test(tags_array, group) + class << self + def find_for_test(tags = [], group = "article_show_sidebar_cta") + tags_array = tags + ["", nil] + if rand(10) == 1 # 10% return completely random + find_random_for_test(tags_array, group) + else # 90% chance return one in top 10 + find_top_for_test(tags_array, group) + end end - end - def self.find_top_for_test(tags_array, group) - where(group: group, approved: true, published: true, target_tag: tags_array).order("success_rate DESC").limit(rand(1..15)).sample - end + private - def self.find_random_for_test(tags_array, group) - where(group: group, approved: true, published: true, target_tag: tags_array).order(Arel.sql("RANDOM()")).first + def find_top_for_test(tags_array, group) + where(group: group, approved: true, published: true, target_tag: tags_array). + order("success_rate DESC").limit(rand(1..15)).sample + end + + def find_random_for_test(tags_array, group) + where(group: group, approved: true, published: true, target_tag: tags_array). + order(Arel.sql("RANDOM()")).first + end end private def no_edits - errors.add(:base, "cannot change once published and approved") if (approved && (html_changed? || name_changed? || group_changed?)) && persisted? + published_and_approved = (approved && (html_changed? || name_changed? || group_changed?)) && persisted? + errors.add(:base, "cannot change once published and approved") if published_and_approved end def prefix_all_images diff --git a/app/models/message.rb b/app/models/message.rb index 43952400a..e5790024d 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -35,8 +35,7 @@ class Message < ApplicationRecord chat_channel. chat_channel_memberships. where("last_opened_at < ?", 10.seconds.ago). - where. - not(user_id: user_id). + where.not(user_id: user_id). update_all(has_unopened_messages: true) end diff --git a/app/models/reaction.rb b/app/models/reaction.rb index e6c56254e..2668e48bb 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -54,14 +54,6 @@ class Reaction < ApplicationRecord end end - def for_display(user) - includes(:reactable). - where(reactable_type: "Article", user: user). - where("created_at > ?", 5.days.ago). - select("distinct on (reactable_id) *"). - take(15) - end - def cached_any_reactions_for?(reactable, user, category) class_name = reactable.class.name == "ArticleDecorator" ? "Article" : reactable.class.name cache_name = "any_reactions_for-#{class_name}-#{reactable.id}-#{user.updated_at&.rfc3339}-#{category}" diff --git a/app/models/tweet.rb b/app/models/tweet.rb index 88baf3628..c21941ae1 100644 --- a/app/models/tweet.rb +++ b/app/models/tweet.rb @@ -61,6 +61,8 @@ class Tweet < ApplicationRecord make_tweet_from_api_object(tweet) end + private + def make_tweet_from_api_object(tweeted) twitter_bot = TwitterBot.new(random_identity) tweeted = if tweeted.attrs[:retweeted_status] diff --git a/config/environments/test.rb b/config/environments/test.rb index 80e5a4063..724719819 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -61,8 +61,9 @@ Rails.application.configure do # enable Bullet in testing mode only if requested config.after_initialize do - Bullet.enable = ENV["BULLET"] - Bullet.raise = ENV["BULLET"] + Bullet.enable = true + Bullet.raise = true + Bullet.add_whitelist(type: :unused_eager_loading, class_name: "ApiSecret", association: :user) # acts-as-taggable-on has super weird eager loading problems: Bullet.add_whitelist(type: :n_plus_one_query, class_name: "ActsAsTaggableOn::Tagging", association: :tag)