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

@@ -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

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

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

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

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
 

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}"

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]

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: <https://github.com/mbleigh/acts-as-taggable-on/issues/91>
     Bullet.add_whitelist(type: :n_plus_one_query, class_name: "ActsAsTaggableOn::Tagging", association: :tag)