feat: Add support for ascending and descending options in sort (#8542)

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

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

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

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