From d1c0dda299accb124f5f4e147b0dd4b6ec6760e7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Mon, 22 Apr 2024 22:07:59 +0200
Subject: [PATCH] Remove Bullet from Gemfile

We've been ignoring what the Bullet gem reports for at least 6 years
(maybe more), but we were still updating the gem and maintaining the
code in `config/environments/` (which caused conflicts every time we run
`rails app:update` to upgrade to a new Rails version). Maintaining it
isn't a huge effort, but it's infinitely bigger than the benefits we get
from it, which are zero.

Adding `includes` everywhere we query for records would be a huge
maintenance effort and would make the code less readable, so I don't
think it's worth it. We might do it occasionally if we detect a
performance bottleneck.

We could also use a gem to automatically avoid the N+1 queries problem,
like Goldiloader [1], ArLazyPreload [2] or JitPreload [3]. Benchmarks
show that the performance improvements obtained by using these gems is
about less than 10% (it depends a lot on the page being loaded, though),
which IMHO doesn't justify adding yet another gem that patches
ActiveRecord and that could be incompatible with other gems doing so.

There are a couple of open pull requests (at the time of writing,
they've been open for about two years) in the Rails repository [4][5] to
automatically avoid N+1 queries as well. For now, we'll hope something
similar is integrated in Rails itself in the future.

[1] https://github.com/salsify/goldiloader
[2] https://github.com/DmitryTsepelev/ar_lazy_preload
[3] https://github.com/clio/jit_preloader/
[4] Pull request 45231 in https://github.com/rails/rails/
[5] Pull request 45413 in https://github.com/rails/rails/
---
 Gemfile                            | 1 -
 Gemfile.lock                       | 5 -----
 config/environments/development.rb | 9 ---------
 config/environments/test.rb        | 8 --------
 spec/spec_helper.rb                | 6 ------
 5 files changed, 29 deletions(-)

diff --git a/Gemfile b/Gemfile
index 9602992cf..11d8bbdfd 100644
--- a/Gemfile
+++ b/Gemfile
@@ -64,7 +64,6 @@ gem "wicked_pdf", "~> 2.8.0"
 gem "wkhtmltopdf-binary", "~> 0.12.6"
 
 group :development, :test do
-  gem "bullet", "~> 7.1.6"
   gem "debug", "~> 1.9.2"
   gem "factory_bot_rails", "~> 6.4.3"
   gem "faker", "~> 3.3.1"
diff --git a/Gemfile.lock b/Gemfile.lock
index 3ef79bb65..c2d3169c1 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -109,9 +109,6 @@ GEM
     bing_translator (6.2.0)
       json
     builder (3.2.4)
-    bullet (7.1.6)
-      activesupport (>= 3.0.0)
-      uniform_notifier (~> 1.11)
     cancancan (3.5.0)
     capistrano (3.18.1)
       airbrussh (>= 1.0.0)
@@ -658,7 +655,6 @@ GEM
     uglifier (4.2.0)
       execjs (>= 0.3.0, < 3)
     unicode-display_width (2.5.0)
-    uniform_notifier (1.16.0)
     uri (0.13.0)
     uuidtools (2.2.0)
     version_gem (1.1.3)
@@ -703,7 +699,6 @@ DEPENDENCIES
   audited (~> 5.4.3)
   autoprefixer-rails (~> 10.4.16)
   bing_translator (~> 6.2.0)
-  bullet (~> 7.1.6)
   cancancan (~> 3.5.0)
   capistrano (~> 3.18.1)
   capistrano-bundler (~> 2.1.0)
diff --git a/config/environments/development.rb b/config/environments/development.rb
index ab669e80b..337ba77e0 100644
--- a/config/environments/development.rb
+++ b/config/environments/development.rb
@@ -79,15 +79,6 @@ Rails.application.configure do
   logger.formatter = config.log_formatter
   config.logger = ActiveSupport::TaggedLogging.new(logger)
 
-  config.after_initialize do
-    Bullet.enable = true
-    Bullet.bullet_logger = true
-    if ENV["BULLET"]
-      Bullet.rails_logger = true
-      Bullet.add_footer = true
-    end
-  end
-
   # Uncomment if you wish to allow Action Cable access from any origin.
   # config.action_cable.disable_request_forgery_protection = true
 end
diff --git a/config/environments/test.rb b/config/environments/test.rb
index 5c9b0b58d..17f91bad8 100644
--- a/config/environments/test.rb
+++ b/config/environments/test.rb
@@ -72,14 +72,6 @@ Rails.application.configure do
   logger.formatter = config.log_formatter
   config.logger = ActiveSupport::TaggedLogging.new(logger)
 
-  config.after_initialize do
-    Bullet.enable = true
-    Bullet.bullet_logger = true
-    if ENV["BULLET"]
-      Bullet.raise = true # raise an error if n+1 query occurs
-    end
-  end
-
   # Allow managing different tenants using the same application
   config.multitenancy = true
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 15c49772e..61738381b 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -69,15 +69,9 @@ RSpec.configure do |config|
   end
 
   config.before(:each, type: :system) do
-    Bullet.start_request
     allow(InvisibleCaptcha).to receive(:timestamp_threshold).and_return(0)
   end
 
-  config.after(:each, type: :system) do
-    Bullet.perform_out_of_channel_notifications if Bullet.notification?
-    Bullet.end_request
-  end
-
   config.before(:each, :admin, type: :system) do
     login_as(create(:administrator).user)
   end