Merge pull request #1779 from tactilenews/1778_reduce_db_queries

Optimize db queries for requests index page

Gemfile

@@ -26,6 +26,7 @@ group :development, :test do
 end
 
 group :development do
+  gem 'bullet'
   gem 'listen', '>= 3.0.5', '< 3.8'
   gem 'rubocop-rails', require: false
 
@@ -84,9 +85,11 @@ gem 'sentry-ruby'
 gem 'administrate'
 gem 'administrate_exportable'
 
+# Notifications
 gem 'noticed', '~> 1.6'
 
 # Charts
 gem 'groupdate'
 
+# Pagination
 gem 'kaminari', '~> 1.2'

Gemfile.lock

@@ -104,6 +104,9 @@ GEM
       msgpack (~> 1.2)
     browser (5.3.1)
     builder (3.2.4)
+    bullet (7.1.6)
+      activesupport (>= 3.0.0)
+      uniform_notifier (~> 1.11)
     byebug (11.1.3)
     capybara (3.37.1)
       addressable
@@ -405,6 +408,7 @@ GEM
       unf_ext
     unf_ext (0.0.8.2)
     unicode-display_width (2.1.0)
+    uniform_notifier (1.16.0)
     valid_email2 (4.0.4)
       activemodel (>= 3.2)
       mail (~> 2.5)
@@ -435,6 +439,7 @@ DEPENDENCIES
   administrate_exportable
   bootsnap (>= 1.4.2)
   browser
+  bullet
   byebug
   capybara (>= 2.15)
   clearance

app/models/message.rb

@@ -15,7 +15,13 @@ class Message < ApplicationRecord
   has_many :files, dependent: :destroy, class_name: 'Message::File'
   has_many :notifications_as_mentioned, class_name: 'ActivityNotification', dependent: :destroy
 
-  counter_culture :request, column_name: proc { |model| model.reply? ? 'replies_count' : nil }
+  counter_culture :request,
+                  column_name: proc { |model| model.reply? ? 'replies_count' : nil },
+                  column_names: lambda {
+                    {
+                      Message.replies => 'replies_count'
+                    }
+                  }
 
   scope :replies, -> { where(sender_type: Contributor.name) }
 

app/models/message/file.rb

@@ -2,10 +2,29 @@
 
 class Message::File < ApplicationRecord
   belongs_to :message
+  counter_culture :message,
+                  column_name: proc { |model| model.message.reply? && model.image_attachment? ? 'photos_count' : nil },
+                  column_names: lambda {
+                                  {
+                                    Message::File.photos_of_replies => 'photos_count'
+                                  }
+                                }
+
+  scope :photos_of_replies, lambda {
+    joins(:message)
+      .merge(Message.replies)
+      .joins(attachment_attachment: :blob)
+      .merge(ActiveStorage::Blob.where('"active_storage_blobs"."content_type" ~* ?', 'image'))
+  }
+
   has_one_attached :attachment
   validates :attachment, presence: true
 
   def thumbnail
     attachment
   end
+
+  def image_attachment?
+    attachment.blob.content_type.match?(/image/)
+  end
 end

app/models/request.rb

@@ -14,7 +14,7 @@ class Request < ApplicationRecord
   has_many :notifications_as_mentioned, class_name: 'ActivityNotification', dependent: :destroy
   has_many_attached :files
 
-  scope :include_associations, -> { preload(messages: :sender).includes(messages: :files).eager_load(:messages) }
+  scope :include_associations, -> { preload(messages: :sender).includes([:tags, { messages: :files }]).eager_load(:messages) }
   scope :planned, -> { where.not(schedule_send_for: nil).where('schedule_send_for > ?', Time.current) }
   scope :broadcasted, -> { where.not(broadcasted_at: nil) }
 
@@ -39,13 +39,8 @@ def stats
       counts: {
         recipients: messages.map(&:recipient_id).compact.uniq.size,
         contributors: messages.select(&:reply?).map(&:sender_id).compact.uniq.size,
-        photos: messages.replies.map do |message|
-          message.photos_count ||
-            message.files.joins(:attachment_blob).where(active_storage_blobs: { content_type: %w[image/jpg image/jpeg
-                                                                                                 image/png image/gif] }).size ||
-            0
-        end.sum,
-        replies: messages.count(&:reply?)
+        photos: messages.pluck(:photos_count).sum,
+        replies: replies_count
       }
     }
   end

config/environments/development.rb

@@ -3,6 +3,12 @@
 require 'active_support/core_ext/integer/time'
 require_relative '../../app/models/setting'
 Rails.application.configure do
+  config.after_initialize do
+    Bullet.enable        = true
+    Bullet.bullet_logger = true
+    Bullet.rails_logger  = true
+  end
+
   # Settings specified here will take precedence over those in config/application.rb.
 
   # In the development environment your application's code is reloaded on

db/migrate/20240201093310_change_replies_count_default_on_requests.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class ChangeRepliesCountDefaultOnRequests < ActiveRecord::Migration[6.1]
+  def up
+    change_column :requests, :replies_count, :integer, default: 0
+  end
+
+  def down
+    change_column :requests, :replies_count, :integer, default: nil
+  end
+end

db/migrate/20240201093729_change_photos_count_default_on_messages.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class ChangePhotosCountDefaultOnMessages < ActiveRecord::Migration[6.1]
+  def up
+    change_column :messages, :photos_count, :integer, default: 0
+  end
+
+  def down
+    change_column :messages, :photos_count, :integer, default: nil
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2023_12_05_194628) do
+ActiveRecord::Schema.define(version: 2024_02_01_093729) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -162,7 +162,7 @@
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false
     t.string "telegram_media_group_id"
-    t.integer "photos_count"
+    t.integer "photos_count", default: 0
     t.bigint "recipient_id"
     t.boolean "broadcasted", default: false
     t.boolean "unknown_content", default: false
@@ -213,7 +213,7 @@
     t.string "text"
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false
-    t.integer "replies_count"
+    t.integer "replies_count", default: 0
     t.bigint "user_id"
     t.datetime "schedule_send_for"
     t.datetime "broadcasted_at"

lib/tasks/temporary/add_photos_count_to_messages.rake

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+namespace :messages do
+  desc 'Add photos count to messages to remove nil default'
+  task add_photos_count_to_messages: :environment do
+    puts "Add photos_count to #{Message.count} messages"
+    ActiveRecord::Base.transaction do
+      Message.replies.find_each do |message|
+        message.photos_count = message.photos.count
+        message.photos_count += message.files.joins(:attachment_blob).where(active_storage_blobs: {
+                                                                              content_type: %w[image/jpg image/jpeg
+                                                                                               image/png image/gif]
+                                                                            }).size
+        message.save
+        print '.'
+      end
+    end
+  end
+end

lib/tasks/temporary/add_replies_count_to_requests.rake

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+namespace :requests do
+  desc 'Add replies count to requests to remove nil default'
+  task add_replies_count_to_requests: :environment do
+    puts "Add replies_count to #{Request.count} requests"
+    ActiveRecord::Base.transaction do
+      Request.find_each do |request|
+        request.replies_count = request.replies.count
+        request.save
+        print '.'
+      end
+    end
+  end
+end

spec/adapters/whats_app_adapter/outbound/file_spec.rb

@@ -17,7 +17,13 @@
   let(:contributor) do
     create(:contributor, whats_app_phone_number: whats_app_phone_number, email: nil)
   end
-  let(:message) { create(:message, recipient: contributor, files: [create(:file), create(:file)]) }
+
+  let(:message) do
+    create(:message, recipient: contributor) do |message|
+      create_list(:file, 2, message: message)
+    end
+  end
+
   let(:expected_params) do
     {
       from: "whatsapp:#{Setting.whats_app_server_phone_number}",

spec/adapters/whats_app_adapter/three_sixty_dialog_outbound_spec.rb

@@ -120,8 +120,7 @@
       end
 
       describe 'message with files' do
-        let(:file) { create(:file) }
-        before { message.update(files: [file]) }
+        before { create(:file, message: message) }
 
         context 'contributor has not sent a message within 24 hours' do
           it 'enqueues the Text job with WhatsApp template' do

spec/adapters/whats_app_adapter/twilio_outbound_spec.rb

@@ -88,8 +88,7 @@
       end
 
       describe 'message with files' do
-        let(:file) { create(:file) }
-        before { message.update(files: [file]) }
+        before { create(:file, message: message) }
 
         context 'contributor has not sent a message within 24 hours' do
           it 'enqueues the Text job with WhatsApp template' do

spec/models/message/file_spec.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Message::File, type: :model do
+  describe '::counter_culture_fix_counts' do
+    subject do
+      described_class.counter_culture_fix_counts
+      message.reload
+    end
+
+    context 'given the photos count has become invalid for whatever reason' do
+      let(:request) { create(:request) }
+
+      describe 'counts images of replies' do
+        let(:message) { create(:message, :with_file, request: request, attachment: fixture_file_upload('example-image.png')) }
+        before { message.update(photos_count: 4711) }
+        it { expect { subject }.to (change { message.photos_count }).from(4711).to(1) }
+      end
+
+      describe 'does not count other content types' do
+        let(:message) { create(:message, :with_file, request: request, attachment: fixture_file_upload('example-audio.oga')) }
+        before { message.update(photos_count: 4711) }
+        it { expect { subject }.to (change { message.photos_count }).from(4711).to(0) }
+      end
+
+      describe 'does not count non-replies' do
+        let(:message) { create(:message, :with_file, :outbound, request: request, attachment: fixture_file_upload('example-image.png')) }
+        before { message.update(photos_count: 4711) }
+        it { expect { subject }.to (change { message.photos_count }).from(4711).to(0) }
+      end
+
+      context 'if there is a photo model' do
+        let(:message) { create(:message, :with_file, request: request, attachment: fixture_file_upload('example-image.png')) }
+        subject do
+          Photo.counter_culture_fix_counts
+          described_class.counter_culture_fix_counts
+          message.reload
+        end
+
+        before do
+          create(:photo, message: message)
+          message.update(photos_count: 4711)
+        end
+
+        it 'both caches get combined' do
+          pending 'counter_culture does not like two models updating a single count'
+          expect { subject }.to (change { message.photos_count }).from(4711).to(2)
+        end
+      end
+    end
+  end
+end

spec/models/message_spec.rb

@@ -138,4 +138,24 @@
       it_behaves_like 'an ActivityNotification', 'MessageReceived'
     end
   end
+
+  describe '::counter_culture_fix_counts' do
+    let(:request) { create(:request) }
+
+    subject do
+      described_class.counter_culture_fix_counts
+      request.reload
+    end
+
+    describe 'fixes replies counter' do
+      before do
+        create(:message, :inbound, request: request)
+        create(:message, :outbound, request: request)
+        create(:message, :inbound) # another requst
+        request.update(replies_count: 4711)
+      end
+
+      it { expect { subject }.to (change { request.replies_count }).from(4711).to(1) }
+    end
+  end
 end

spec/models/request_spec.rb

@@ -199,26 +199,29 @@
       before(:each) do
         create_list(:message, 2)
         delivered_messages = create_list(:message, 7, :outbound, request: request, broadcasted: true)
+        create(:message, :with_file, :outbound, request: request, broadcasted: false, attachment: fixture_file_upload('example-image.png'))
         # _ is some unresponsive recipient
         responsive_recipient, _, *other_recipients = delivered_messages.map(&:recipient)
         create_list(:message, 3, request: request, sender: responsive_recipient)
         other_recipients.each do |recipient|
           create(:message, :with_a_photo, sender: recipient, request: request)
           create(:message, :with_file, sender: recipient, request: request, attachment: fixture_file_upload('example-image.png'))
+          create(:message, :with_file, sender: recipient, request: request, attachment: fixture_file_upload('invalid_profile_picture.pdf'))
         end
+        request.reload
       end
 
       describe '[:counts][:replies]' do
         subject { stats[:counts][:replies] }
-        it { should eq(13) } # unique contributors
+        it { should eq(18) }
 
         describe 'messages from us' do
           before(:each) do
             create(:message, request: request, sender: nil, broadcasted: true)
           end
 
           it 'are excluded' do
-            should eq(13)
+            should eq(18)
           end
         end
       end
@@ -240,7 +243,7 @@
 
       describe '[:counts][:recipients]' do
         subject { stats[:counts][:recipients] }
-        it { should eq(7) }
+        it { should eq(8) }
       end
 
       describe '[:counts][:photos]' do
@@ -250,11 +253,11 @@
 
       describe 'iterating through a list' do
         subject { -> { Request.find_each.map(&:stats) } }
-        it { should make_database_queries(count: 39) }
+        it { should make_database_queries(count: 32) }
 
-        describe 'preload(messages: :sender).eager_load(:messages)' do
-          subject { -> { Request.preload(messages: :sender).includes(messages: :files).eager_load(:messages).find_each.map(&:stats) } }
-          it { should make_database_queries(count: 17) } # better
+        describe '::include_associations' do
+          subject { -> { Request.include_associations.find_each.map(&:stats) } }
+          it { should make_database_queries(count: 4) } # better
         end
       end
     end