From da76537011cde5020ed558f43fdba18ec90f1137 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Mon, 13 Mar 2023 19:10:31 +0530 Subject: [PATCH] chore: Search optimisations (#6644) - Strip search term before searching - order messages by created_at desc - order contacts by last_activity_at desc - order conversations by created_at desc - Search only resolved contacts - Optimize resolved contacts query ref: #6583 --- app/models/contact.rb | 4 +-- app/models/installation_config.rb | 2 ++ app/models/message.rb | 4 +++ app/models/notification.rb | 2 ++ app/services/search_service.rb | 17 +++++++--- .../api/v1/accounts/search_controller_spec.rb | 2 +- spec/services/search_service_spec.rb | 34 +++++++++++-------- 7 files changed, 42 insertions(+), 23 deletions(-) diff --git a/app/models/contact.rb b/app/models/contact.rb index 83019b969..9a742c0b2 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -136,9 +136,7 @@ class Contact < ApplicationRecord end def self.resolved_contacts - where.not(email: [nil, '']).or( - Current.account.contacts.where.not(phone_number: [nil, '']) - ).or(Current.account.contacts.where.not(identifier: [nil, ''])) + where("COALESCE(NULLIF(contacts.email,''),NULLIF(contacts.phone_number,''),NULLIF(contacts.identifier,'')) IS NOT NULL") end def discard_invalid_attrs diff --git a/app/models/installation_config.rb b/app/models/installation_config.rb index 03e0b246a..f0e2ca4f1 100644 --- a/app/models/installation_config.rb +++ b/app/models/installation_config.rb @@ -24,6 +24,8 @@ class InstallationConfig < ApplicationRecord before_validation :set_lock validates :name, presence: true + # TODO: Get rid of default scope + # https://stackoverflow.com/a/1834250/939299 default_scope { order(created_at: :desc) } scope :editable, -> { where(locked: false) } diff --git a/app/models/message.rb b/app/models/message.rb index 7596dedaf..f45f2d8f9 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -81,6 +81,10 @@ class Message < ApplicationRecord scope :chat, -> { where.not(message_type: :activity).where(private: false) } scope :non_activity_messages, -> { where.not(message_type: :activity).reorder('id desc') } scope :today, -> { where("date_trunc('day', created_at) = ?", Date.current) } + + # TODO: Get rid of default scope + # https://stackoverflow.com/a/1834250/939299 + # if you want to change order, use `reorder` default_scope { order(created_at: :asc) } belongs_to :account diff --git a/app/models/notification.rb b/app/models/notification.rb index 681203710..eadb48032 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -42,6 +42,8 @@ class Notification < ApplicationRecord after_create_commit :process_notification_delivery, :dispatch_create_event + # TODO: Get rid of default scope + # https://stackoverflow.com/a/1834250/939299 default_scope { order(id: :desc) } PRIMARY_ACTORS = ['Conversation'].freeze diff --git a/app/services/search_service.rb b/app/services/search_service.rb index 77994c26f..add7938a0 100644 --- a/app/services/search_service.rb +++ b/app/services/search_service.rb @@ -20,24 +20,31 @@ class SearchService @accessable_inbox_ids ||= @current_user.assigned_inboxes.pluck(:id) end + def search_query + @search_query ||= params[:q].to_s.strip + end + def filter_conversations @conversations = current_account.conversations.where(inbox_id: accessable_inbox_ids) .joins('INNER JOIN contacts ON conversations.contact_id = contacts.id') .where("cast(conversations.display_id as text) ILIKE :search OR contacts.name ILIKE :search OR contacts.email - ILIKE :search OR contacts.phone_number ILIKE :search OR contacts.identifier ILIKE :search", search: "%#{params[:q]}%") + ILIKE :search OR contacts.phone_number ILIKE :search OR contacts.identifier ILIKE :search", search: "%#{search_query}%") + .order('conversations.created_at DESC') .limit(10) end def filter_messages @messages = current_account.messages.where(inbox_id: accessable_inbox_ids) - .where('messages.content ILIKE :search', search: "%#{params[:q]}%") - .where('created_at >= ?', 3.months.ago).limit(10) + .where('messages.content ILIKE :search', search: "%#{search_query}%") + .where('created_at >= ?', 3.months.ago) + .reorder('created_at DESC') + .limit(10) end def filter_contacts @contacts = current_account.contacts.where( "name ILIKE :search OR email ILIKE :search OR phone_number - ILIKE :search OR identifier ILIKE :search", search: "%#{params[:q]}%" - ).limit(10) + ILIKE :search OR identifier ILIKE :search", search: "%#{search_query}%" + ).resolved_contacts.order_on_last_activity_at('desc').limit(10) end end diff --git a/spec/controllers/api/v1/accounts/search_controller_spec.rb b/spec/controllers/api/v1/accounts/search_controller_spec.rb index d12898d9e..9081c0d4c 100644 --- a/spec/controllers/api/v1/accounts/search_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/search_controller_spec.rb @@ -32,7 +32,7 @@ RSpec.describe 'Search', type: :request do expect(response).to have_http_status(:success) response_data = JSON.parse(response.body, symbolize_names: true) - expect(response_data[:payload][:messages].first[:content]).to eq 'test1' + expect(response_data[:payload][:messages].first[:content]).to eq 'test2' expect(response_data[:payload].keys).to match_array [:contacts, :conversations, :messages] expect(response_data[:payload][:messages].length).to eq 2 expect(response_data[:payload][:conversations].length).to eq 1 diff --git a/spec/services/search_service_spec.rb b/spec/services/search_service_spec.rb index 9bea59568..3bc45e9ae 100644 --- a/spec/services/search_service_spec.rb +++ b/spec/services/search_service_spec.rb @@ -7,12 +7,12 @@ describe ::SearchService do let!(:account) { create(:account) } let!(:user) { create(:user, account: account) } let!(:inbox) { create(:inbox, account: account, enable_auto_assignment: false) } - let(:harry) { create(:contact, name: 'Harry Potter', account_id: account.id) } + let!(:harry) { create(:contact, name: 'Harry Potter', email: 'test@test.com', account_id: account.id) } + let!(:conversation) { create(:conversation, contact: harry, inbox: inbox, account: account) } + let!(:message) { create(:message, account: account, inbox: inbox, content: 'Harry Potter is a wizard') } before do create(:inbox_member, user: user, inbox: inbox) - create(:conversation, contact: harry, inbox: inbox, account: account) - create(:message, account: account, inbox: inbox, content: 'Harry Potter is a wizard') Current.account = account end @@ -50,38 +50,44 @@ describe ::SearchService do end context 'when contact search' do - it 'searches across name, email, phone_number and identifier' do + it 'searches across name, email, phone_number and identifier and returns in the order of contact last_activity_at' do # random contact create(:contact, account_id: account.id) - harry2 = create(:contact, email: 'HarryPotter@test.com', account_id: account.id) - harry3 = create(:contact, identifier: 'Potter123', account_id: account.id) + # unresolved contact -> no identifying info + # will not appear in search results + create(:contact, name: 'Harry Potter', account_id: account.id) + harry2 = create(:contact, email: 'HarryPotter@test.com', account_id: account.id, last_activity_at: 2.days.ago) + harry3 = create(:contact, identifier: 'Potter123', account_id: account.id, last_activity_at: 1.day.ago) + harry4 = create(:contact, identifier: 'Potter1235', account_id: account.id, last_activity_at: 2.minutes.ago) - params = { q: 'Potter' } + params = { q: 'Potter ' } search = described_class.new(current_user: user, current_account: account, params: params, search_type: 'Contact') - expect(search.perform[:contacts].map(&:id)).to match_array([harry.id, harry2.id, harry3.id]) + expect(search.perform[:contacts].map(&:id)).to eq([harry4.id, harry3.id, harry2.id, harry.id]) end end context 'when message search' do - it 'searches across message content' do - # random messages in another inbox - create(:message, account: account, inbox: create(:inbox, account: account), content: 'Harry Potter is a wizard') + it 'searches across message content and return in created_at desc' do + # random messages in another account create(:message, content: 'Harry Potter is a wizard') + # random messsage in inbox with out access + create(:message, account: account, inbox: create(:inbox, account: account), content: 'Harry Potter is a wizard') message2 = create(:message, account: account, inbox: inbox, content: 'harry is cool') params = { q: 'Harry' } search = described_class.new(current_user: user, current_account: account, params: params, search_type: 'Message') - expect(search.perform[:messages].map(&:id)).to match_array([Message.first.id, message2.id]) + expect(search.perform[:messages].map(&:id)).to eq([message2.id, message.id]) end end context 'when conversation search' do - it 'searches across conversations using contact information' do + it 'searches across conversations using contact information and order by created_at desc' do # random messages in another inbox random = create(:contact, account_id: account.id) create(:conversation, contact: random, inbox: inbox, account: account) + conv2 = create(:conversation, contact: harry, inbox: inbox, account: account) params = { q: 'Harry' } search = described_class.new(current_user: user, current_account: account, params: params, search_type: 'Conversation') - expect(search.perform[:conversations].map(&:id)).to match_array([Conversation.first.id]) + expect(search.perform[:conversations].map(&:id)).to eq([conv2.id, conversation.id]) end it 'searches across conversations with display id' do