From 8c9a351c8424ab44f879f5b1c205b357a5c3cb68 Mon Sep 17 00:00:00 2001 From: Pranav Raj S Date: Tue, 12 Dec 2023 17:15:48 -0800 Subject: [PATCH] feat: Add support for ascending and descending options in sort (#8542) --- app/finders/conversation_finder.rb | 25 ++-- app/models/concerns/sort_handler.rb | 42 +++--- app/services/conversations/filter_service.rb | 2 +- spec/models/conversation_spec.rb | 132 ++++++++++++------- 4 files changed, 118 insertions(+), 83 deletions(-) diff --git a/app/finders/conversation_finder.rb b/app/finders/conversation_finder.rb index 02c454e04..0cc5e52d8 100644 --- a/app/finders/conversation_finder.rb +++ b/app/finders/conversation_finder.rb @@ -3,13 +3,21 @@ class ConversationFinder DEFAULT_STATUS = 'open'.freeze SORT_OPTIONS = { - latest: 'latest', - sort_on_created_at: 'sort_on_created_at', - last_user_message_at: 'last_user_message_at', - sort_on_priority: 'sort_on_priority', - sort_on_waiting_since: 'sort_on_waiting_since' - }.with_indifferent_access + 'last_activity_at_asc' => %w[sort_on_last_activity_at asc], + 'last_activity_at_desc' => %w[sort_on_last_activity_at desc], + 'created_at_asc' => %w[sort_on_created_at asc], + 'created_at_desc' => %w[sort_on_created_at desc], + 'priority_asc' => %w[sort_on_priority asc], + 'priority_desc' => %w[sort_on_priority desc], + 'waiting_since_asc' => %w[sort_on_waiting_since asc], + 'waiting_since_desc' => %w[sort_on_waiting_since desc], + # To be removed in v3.5.0 + 'latest' => %w[sort_on_last_activity_at desc], + 'sort_on_created_at' => %w[sort_on_created_at asc], + 'sort_on_priority' => %w[sort_on_priority desc], + 'sort_on_waiting_since' => %w[sort_on_waiting_since asc] + }.with_indifferent_access # assumptions # inbox_id if not given, take from all conversations, else specific to inbox # assignee_type if not given, take 'all' @@ -159,7 +167,8 @@ class ConversationFinder @conversations = @conversations.includes( :taggings, :inbox, { assignee: { avatar_attachment: [:blob] } }, { contact: { avatar_attachment: [:blob] } }, :team, :contact_inbox ) - sort_by = SORT_OPTIONS[params[:sort_by]] || SORT_OPTIONS['latest'] - @conversations.send(sort_by).page(current_page).per(ENV.fetch('CONVERSATION_RESULTS_PER_PAGE', '25').to_i) + + sort_by, sort_order = SORT_OPTIONS[params[:sort_by]] || SORT_OPTIONS['last_activity_at_desc'] + @conversations.send(sort_by, sort_order).page(current_page).per(ENV.fetch('CONVERSATION_RESULTS_PER_PAGE', '25').to_i) end end diff --git a/app/models/concerns/sort_handler.rb b/app/models/concerns/sort_handler.rb index 5c4dde7c3..00eb73717 100644 --- a/app/models/concerns/sort_handler.rb +++ b/app/models/concerns/sort_handler.rb @@ -1,41 +1,37 @@ module SortHandler extend ActiveSupport::Concern - included do - def self.latest - order(last_activity_at: :desc) + class_methods do + def sort_on_last_activity_at(sort_direction = :desc) + order(last_activity_at: sort_direction) end - def self.sort_on_created_at - order(created_at: :asc) + def sort_on_created_at(sort_direction = :asc) + order(created_at: sort_direction) end - def self.last_messaged_conversations + def sort_on_priority(sort_direction = :desc) + order(generate_sql_query("priority #{sort_direction.to_s.upcase} NULLS LAST, last_activity_at DESC")) + end + + def sort_on_waiting_since(sort_direction = :asc) + order(generate_sql_query("waiting_since #{sort_direction.to_s.upcase} NULLS LAST, created_at ASC")) + end + + def last_messaged_conversations Message.except(:order).select( 'DISTINCT ON (conversation_id) conversation_id, id, created_at, message_type' ).order('conversation_id, created_at DESC') end - def self.sort_on_last_user_message_at - order( - 'grouped_conversations.message_type', 'grouped_conversations.created_at ASC' - ) + def sort_on_last_user_message_at + order('grouped_conversations.message_type', 'grouped_conversations.created_at ASC') end - def self.sort_on_priority - order( - Arel::Nodes::SqlLiteral.new( - sanitize_sql_for_order('CASE WHEN priority IS NULL THEN 0 ELSE priority END DESC, last_activity_at DESC') - ) - ) - end + private - def self.sort_on_waiting_since - order( - Arel::Nodes::SqlLiteral.new( - sanitize_sql_for_order('CASE WHEN waiting_since IS NULL THEN now() ELSE waiting_since END ASC, created_at ASC') - ) - ) + def generate_sql_query(query) + Arel::Nodes::SqlLiteral.new(sanitize_sql_for_order(query)) end end end diff --git a/app/services/conversations/filter_service.rb b/app/services/conversations/filter_service.rb index 5ec7b75fb..87384017c 100644 --- a/app/services/conversations/filter_service.rb +++ b/app/services/conversations/filter_service.rb @@ -64,6 +64,6 @@ class Conversations::FilterService < FilterService end def conversations - @conversations.latest.page(current_page) + @conversations.sort_on_last_activity_at.page(current_page) end end diff --git a/spec/models/conversation_spec.rb b/spec/models/conversation_spec.rb index f01806863..82dac1979 100644 --- a/spec/models/conversation_spec.rb +++ b/spec/models/conversation_spec.rb @@ -715,61 +715,52 @@ RSpec.describe Conversation do end end - describe 'Custom Sort' do + describe 'custom sort option' do include ActiveJob::TestHelper - let!(:conversation_7) { create(:conversation, created_at: DateTime.now - 10.days, last_activity_at: DateTime.now - 13.days) } - let!(:conversation_6) { create(:conversation, created_at: DateTime.now - 10.days, last_activity_at: DateTime.now - 10.days) } - let!(:conversation_5) { create(:conversation, created_at: DateTime.now - 10.days, last_activity_at: DateTime.now - 12.days, priority: :urgent) } - let!(:conversation_4) { create(:conversation, created_at: DateTime.now - 10.days, last_activity_at: DateTime.now - 10.days, priority: :urgent) } - let!(:conversation_3) { create(:conversation, created_at: DateTime.now - 9.days, last_activity_at: DateTime.now - 9.days, priority: :low) } - let!(:conversation_2) { create(:conversation, created_at: DateTime.now - 6.days, last_activity_at: DateTime.now - 6.days, priority: :high) } - let!(:conversation_1) { create(:conversation, created_at: DateTime.now - 8.days, last_activity_at: DateTime.now - 8.days, priority: :medium) } + let!(:conversation_7) { create(:conversation, created_at: DateTime.now - 6.days, last_activity_at: DateTime.now - 13.days) } + let!(:conversation_6) { create(:conversation, created_at: DateTime.now - 7.days, last_activity_at: DateTime.now - 10.days) } + let!(:conversation_5) { create(:conversation, created_at: DateTime.now - 8.days, last_activity_at: DateTime.now - 12.days, priority: :urgent) } + let!(:conversation_4) { create(:conversation, created_at: DateTime.now - 9.days, last_activity_at: DateTime.now - 11.days, priority: :urgent) } + let!(:conversation_3) { create(:conversation, created_at: DateTime.now - 5.days, last_activity_at: DateTime.now - 9.days, priority: :low) } + let!(:conversation_2) { create(:conversation, created_at: DateTime.now - 3.days, last_activity_at: DateTime.now - 6.days, priority: :high) } + let!(:conversation_1) { create(:conversation, created_at: DateTime.now - 4.days, last_activity_at: DateTime.now - 8.days, priority: :medium) } - it 'Sort conversations based on created_at' do - records = described_class.sort_on_created_at - expect(records.first.id).to eq(conversation_7.id) - expect(records.last.id).to eq(conversation_2.id) + describe 'sort_on_created_at' do + let(:created_desc_order) do + [ + conversation_2.id, conversation_1.id, conversation_3.id, conversation_7.id, conversation_6.id, + conversation_5.id, conversation_4.id + ] + end + + it 'returns the list in ascending order by default' do + records = described_class.sort_on_created_at + expect(records.map(&:id)).to eq created_desc_order.reverse + end + + it 'returns the list in descending order if desc is passed as sort direction' do + records = described_class.sort_on_created_at(:desc) + expect(records.map(&:id)).to eq created_desc_order + end end - context 'when sort on last_user_message_at' do - before do - create(:message, conversation_id: conversation_3.id, message_type: :outgoing, created_at: DateTime.now - 9.days) - create(:message, conversation_id: conversation_1.id, message_type: :incoming, created_at: DateTime.now - 8.days) - create(:message, conversation_id: conversation_1.id, message_type: :incoming, created_at: DateTime.now - 8.days) - create(:message, conversation_id: conversation_1.id, message_type: :outgoing, created_at: DateTime.now - 7.days) - create(:message, conversation_id: conversation_2.id, message_type: :incoming, created_at: DateTime.now - 6.days) - create(:message, conversation_id: conversation_2.id, message_type: :incoming, created_at: DateTime.now - 6.days) - create(:message, conversation_id: conversation_3.id, message_type: :incoming, created_at: DateTime.now - 6.days) - create(:message, conversation_id: conversation_3.id, message_type: :incoming, created_at: DateTime.now - 6.days) - create(:message, conversation_id: conversation_3.id, message_type: :incoming, created_at: DateTime.now - 2.days) + describe 'sort_on_last_activity_at' do + let(:last_activity_asc_order) do + [ + conversation_7.id, conversation_5.id, conversation_4.id, conversation_6.id, conversation_3.id, + conversation_1.id, conversation_2.id + ] end - # conversation_2 has last unanswered incoming message 6 days ago - # conversation_3 has last unanswered incoming message 2 days ago - # conversation_1 has incoming message 8 days ago but outgoing message on 7 days ago - # so we won't consider it to show it on top of the sort as it is answered/replied conversation - it 'Sort conversations with oldest unanswered incoming message first' do - conversation_with_message_count = described_class.joins(:messages).uniq.count - records = described_class.last_user_message_at - - expect(records.length).to eq(conversation_with_message_count) - expect(records[0]['id']).to eq(conversation_2.id) - expect(records[1]['id']).to eq(conversation_3.id) - expect(records[2]['id']).to eq(conversation_1.id) - expect(records.pluck(:id)).not_to include(conversation_4.id) + it 'returns the list in descending order by default' do + records = described_class.sort_on_last_activity_at + expect(records.map(&:id)).to eq last_activity_asc_order.reverse end - # Now we have no incoming message the sprt will happen on the created at - it 'Sort based on oldest message first when there are no incoming message' do - Message.where(message_type: :incoming).update(message_type: :template) - conversation_with_message_count = described_class.joins(:messages).uniq.count - records = described_class.last_user_message_at - - expect(records.length).to eq(conversation_with_message_count) - expect(records[0]['id']).to eq(conversation_1.id) - expect(records[1]['id']).to eq(conversation_2.id) - expect(records[2]['id']).to eq(conversation_3.id) + it 'returns the list in asc order if asc is passed as sort direction' do + records = described_class.sort_on_last_activity_at(:asc) + expect(records.map(&:id)).to eq last_activity_asc_order end end @@ -781,7 +772,7 @@ RSpec.describe Conversation do end it 'sort conversations with latest resolved conversation at first' do - records = described_class.latest + records = described_class.sort_on_last_activity_at expect(records.first.id).to eq(conversation_3.id) @@ -795,27 +786,48 @@ RSpec.describe Conversation do content: 'Conversation was marked resolved by system due to days of inactivity' ) end - records = described_class.latest + records = described_class.sort_on_last_activity_at expect(records.first.id).to eq(conversation_1.id) end it 'Sort conversations with latest message' do create(:message, conversation_id: conversation_3.id, message_type: :incoming, created_at: DateTime.now) - records = described_class.latest + records = described_class.sort_on_last_activity_at expect(records.first.id).to eq(conversation_3.id) end end - context 'when sort on priority' do - it 'Sort conversations with the following order high > medium > low > nil' do + describe 'sort_on_priority' do + it 'return list with the following order urgent > high > medium > low > nil by default' do # ensure they are not pre-sorted records = described_class.sort_on_created_at expect(records.pluck(:priority)).not_to eq(['urgent', 'urgent', 'high', 'medium', 'low', nil, nil]) records = described_class.sort_on_priority expect(records.pluck(:priority)).to eq(['urgent', 'urgent', 'high', 'medium', 'low', nil, nil]) + expect(records.pluck(:id)).to eq( + [ + conversation_4.id, conversation_5.id, conversation_2.id, conversation_1.id, conversation_3.id, + conversation_6.id, conversation_7.id + ] + ) + end + + it 'return list with the following order low > medium > high > urgent > nil by default' do + # ensure they are not pre-sorted + records = described_class.sort_on_created_at + expect(records.pluck(:priority)).not_to eq(['urgent', 'urgent', 'high', 'medium', 'low', nil, nil]) + + records = described_class.sort_on_priority(:asc) + expect(records.pluck(:priority)).to eq(['low', 'medium', 'high', 'urgent', 'urgent', nil, nil]) + expect(records.pluck(:id)).to eq( + [ + conversation_3.id, conversation_1.id, conversation_2.id, conversation_4.id, conversation_5.id, + conversation_6.id, conversation_7.id + ] + ) end it 'sorts conversation with last_activity for the same priority' do @@ -830,6 +842,24 @@ RSpec.describe Conversation do expect(records.pluck(:priority, :id)).to eq([[nil, conversation_6.id], [nil, conversation_7.id]]) end end + + describe 'sort_on_waiting_since' do + it 'returns the list in ascending order by default' do + records = described_class.sort_on_waiting_since + expect(records.map(&:id)).to eq [ + conversation_4.id, conversation_5.id, conversation_6.id, conversation_7.id, conversation_3.id, conversation_1.id, + conversation_2.id + ] + end + + it 'returns the list in desc order if asc is passed as sort direction' do + records = described_class.sort_on_waiting_since(:desc) + expect(records.map(&:id)).to eq [ + conversation_2.id, conversation_1.id, conversation_3.id, conversation_7.id, conversation_6.id, conversation_5.id, + conversation_4.id + ] + end + end end describe 'cached_label_list_array' do