From cf91e9eb58fcb98f7c2cd2fa4331c8e9cb63a6b4 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Tue, 25 Apr 2023 09:32:35 +0530 Subject: [PATCH] chore: Use find_each instead of .all.each (#6975) - Enable the rubocop Rails/FindEach - Replace the .all.each with .find_each This should let us avoid potential memory usage. Motivation from the speedshop newsletter by Nate Berkopec ref: https://www.rubyinrails.com/2017/11/16/use-find-each-instead-of-all-each-in-rails/ ref: https://linear.app/chatwoot/issue/CW-1480/chore-run-all-sidekiq-jobs-async --- .rubocop.yml | 4 ++++ app/jobs/account/conversations_resolution_scheduler_job.rb | 2 +- app/jobs/conversations/reopen_snoozed_conversations_job.rb | 2 +- app/jobs/inboxes/fetch_imap_email_inboxes_job.rb | 2 +- .../update_first_response_time_in_reporting_events_job.rb | 2 +- app/jobs/trigger_scheduled_items_job.rb | 3 ++- .../api/v1/accounts/bulk_actions_controller_spec.rb | 2 +- spec/jobs/bulk_actions_job_spec.rb | 2 +- spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb | 4 ---- 9 files changed, 12 insertions(+), 11 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 8b41dd5fd..e66d0bc16 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -71,6 +71,10 @@ Rails/ApplicationController: - 'app/controllers/platform_controller.rb' - 'app/controllers/public_controller.rb' - 'app/controllers/survey/responses_controller.rb' +Rails/FindEach: + Enabled: true + Include: + - 'app/**/*.rb' Rails/CompactBlank: Enabled: false Rails/EnvironmentVariableAccess: diff --git a/app/jobs/account/conversations_resolution_scheduler_job.rb b/app/jobs/account/conversations_resolution_scheduler_job.rb index d98604c27..37848db49 100644 --- a/app/jobs/account/conversations_resolution_scheduler_job.rb +++ b/app/jobs/account/conversations_resolution_scheduler_job.rb @@ -2,7 +2,7 @@ class Account::ConversationsResolutionSchedulerJob < ApplicationJob queue_as :scheduled_jobs def perform - Account.where.not(auto_resolve_duration: nil).all.each do |account| + Account.where.not(auto_resolve_duration: nil).all.find_each(batch_size: 100) do |account| Conversations::ResolutionJob.perform_later(account: account) end end diff --git a/app/jobs/conversations/reopen_snoozed_conversations_job.rb b/app/jobs/conversations/reopen_snoozed_conversations_job.rb index c11298585..6a2c0d370 100644 --- a/app/jobs/conversations/reopen_snoozed_conversations_job.rb +++ b/app/jobs/conversations/reopen_snoozed_conversations_job.rb @@ -2,6 +2,6 @@ class Conversations::ReopenSnoozedConversationsJob < ApplicationJob queue_as :low def perform - Conversation.where(status: :snoozed).where(snoozed_until: 3.days.ago..Time.current).all.each(&:open!) + Conversation.where(status: :snoozed).where(snoozed_until: 3.days.ago..Time.current).all.find_each(batch_size: 100, &:open!) end end diff --git a/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb b/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb index c9b847575..190714c1e 100644 --- a/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb +++ b/app/jobs/inboxes/fetch_imap_email_inboxes_job.rb @@ -2,7 +2,7 @@ class Inboxes::FetchImapEmailInboxesJob < ApplicationJob queue_as :low def perform - Inbox.where(channel_type: 'Channel::Email').all.each do |inbox| + Inbox.where(channel_type: 'Channel::Email').all.find_each(batch_size: 100) do |inbox| ::Inboxes::FetchImapEmailsJob.perform_later(inbox.channel) if inbox.channel.imap_enabled end end diff --git a/app/jobs/migration/update_first_response_time_in_reporting_events_job.rb b/app/jobs/migration/update_first_response_time_in_reporting_events_job.rb index 0c43a0e7a..32f73910f 100644 --- a/app/jobs/migration/update_first_response_time_in_reporting_events_job.rb +++ b/app/jobs/migration/update_first_response_time_in_reporting_events_job.rb @@ -6,7 +6,7 @@ class Migration::UpdateFirstResponseTimeInReportingEventsJob < ApplicationJob def perform(account) get_conversations_with_bot_handoffs(account) - account.reporting_events.where(name: 'first_response').each do |event| + account.reporting_events.where(name: 'first_response').find_each do |event| conversation = event.conversation # if the conversation has a bot handoff event, we don't need to update the response_time diff --git a/app/jobs/trigger_scheduled_items_job.rb b/app/jobs/trigger_scheduled_items_job.rb index a3a502ec0..f51d9df9a 100644 --- a/app/jobs/trigger_scheduled_items_job.rb +++ b/app/jobs/trigger_scheduled_items_job.rb @@ -3,7 +3,8 @@ class TriggerScheduledItemsJob < ApplicationJob def perform # trigger the scheduled campaign jobs - Campaign.where(campaign_type: :one_off, campaign_status: :active).where(scheduled_at: 3.days.ago..Time.current).all.each do |campaign| + Campaign.where(campaign_type: :one_off, + campaign_status: :active).where(scheduled_at: 3.days.ago..Time.current).all.find_each(batch_size: 100) do |campaign| Campaigns::TriggerOneoffCampaignJob.perform_later(campaign) end diff --git a/spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb b/spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb index 8306ec539..ec6af61be 100644 --- a/spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/bulk_actions_controller_spec.rb @@ -12,7 +12,7 @@ RSpec.describe 'Api::V1::Accounts::BulkActionsController', type: :request do create(:conversation, account_id: account.id, status: :open, team_id: team_1.id) create(:conversation, account_id: account.id, status: :open) create(:conversation, account_id: account.id, status: :open) - Conversation.all.each do |conversation| + Conversation.all.find_each do |conversation| create(:inbox_member, inbox: conversation.inbox, user: agent_1) create(:inbox_member, inbox: conversation.inbox, user: agent_2) end diff --git a/spec/jobs/bulk_actions_job_spec.rb b/spec/jobs/bulk_actions_job_spec.rb index 2051c69b2..5e8ee8f9e 100644 --- a/spec/jobs/bulk_actions_job_spec.rb +++ b/spec/jobs/bulk_actions_job_spec.rb @@ -16,7 +16,7 @@ RSpec.describe BulkActionsJob, type: :job do let!(:conversation_3) { create(:conversation, account_id: account.id, status: :open) } before do - Conversation.all.each do |conversation| + Conversation.all.find_each do |conversation| create(:inbox_member, inbox: conversation.inbox, user: agent) end end diff --git a/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb b/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb index 0b23ef9c4..df0b34aca 100644 --- a/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb +++ b/spec/jobs/inboxes/fetch_imap_email_inboxes_job_spec.rb @@ -15,10 +15,6 @@ RSpec.describe Inboxes::FetchImapEmailInboxesJob, type: :job do context 'when called' do it 'fetch all the email channels' do - imap_email_inboxes = double - allow(imap_email_inboxes).to receive(:all).and_return([email_inbox]) - allow(Inbox).to receive(:where).and_return(imap_email_inboxes) - expect(Inboxes::FetchImapEmailsJob).to receive(:perform_later).with(imap_email_channel).once described_class.perform_now