From b86638faaca2c4e838cece4deb62e39475e07415 Mon Sep 17 00:00:00 2001 From: Tejaswini Chile Date: Wed, 6 Jul 2022 00:37:43 +0530 Subject: [PATCH] fix: last incoming message sort (#4972) --- app/models/concerns/sort_handler.rb | 10 ++-- app/models/conversation.rb | 2 +- app/models/mention.rb | 2 +- spec/models/conversation_spec.rb | 88 ++++++++++++++++------------- spec/models/mention_spec.rb | 2 +- 5 files changed, 56 insertions(+), 48 deletions(-) diff --git a/app/models/concerns/sort_handler.rb b/app/models/concerns/sort_handler.rb index 01e124d1b..135912aad 100644 --- a/app/models/concerns/sort_handler.rb +++ b/app/models/concerns/sort_handler.rb @@ -11,14 +11,14 @@ module SortHandler end def self.last_messaged_conversations - Message.except(:order).select('DISTINCT ON (conversation_id) *').order('conversation_id, created_at DESC') + 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 - where( - 'grouped_conversations.message_type = 0' - ).order( - 'grouped_conversations.created_at ASC' + order( + 'grouped_conversations.message_type', 'grouped_conversations.created_at ASC' ) end end diff --git a/app/models/conversation.rb b/app/models/conversation.rb index 49b827b68..e3791a9d3 100644 --- a/app/models/conversation.rb +++ b/app/models/conversation.rb @@ -72,7 +72,7 @@ class Conversation < ApplicationRecord scope :last_user_message_at, lambda { joins( - "INNER JOIN (#{last_messaged_conversations.to_sql}) grouped_conversations + "INNER JOIN (#{last_messaged_conversations.to_sql}) AS grouped_conversations ON grouped_conversations.conversation_id = conversations.id" ).sort_on_last_user_message_at } diff --git a/app/models/mention.rb b/app/models/mention.rb index baa7acad4..00a3a859d 100644 --- a/app/models/mention.rb +++ b/app/models/mention.rb @@ -46,7 +46,7 @@ class Mention < ApplicationRecord # Then select only latest incoming message from the conversations which doesn't have last message as outgoing # Order by message created_at Mention.joins( - "INNER JOIN (#{last_messaged_conversations.to_sql}) grouped_conversations + "INNER JOIN (#{last_messaged_conversations.to_sql}) AS grouped_conversations ON grouped_conversations.conversation_id = mentions.conversation_id" ).sort_on_last_user_message_at end diff --git a/spec/models/conversation_spec.rb b/spec/models/conversation_spec.rb index 95b205541..1a80a55fb 100644 --- a/spec/models/conversation_spec.rb +++ b/spec/models/conversation_spec.rb @@ -477,6 +477,16 @@ RSpec.describe Conversation, type: :model do it 'returns true' do expect(conversation.can_reply?).to eq true end + + it 'return true for facebook channels' do + stub_request(:post, /graph.facebook.com/) + facebook_channel = create(:channel_facebook_page) + facebook_inbox = create(:inbox, channel: facebook_channel, account: facebook_channel.account) + fb_conversation = create(:conversation, inbox: facebook_inbox, account: facebook_channel.account) + + expect(fb_conversation.can_reply?).to eq true + expect(facebook_channel.messaging_window_enabled?).to eq false + end end describe 'on channels with 24 hour restriction' do @@ -488,31 +498,6 @@ RSpec.describe Conversation, type: :model do let!(:facebook_inbox) { create(:inbox, channel: facebook_channel, account: facebook_channel.account) } let!(:conversation) { create(:conversation, inbox: facebook_inbox, account: facebook_channel.account) } - it 'returns false if there are no incoming messages' do - expect(conversation.can_reply?).to eq true - end - - it 'return false if last incoming message is outside of 24 hour window' do - create( - :message, - account: conversation.account, - inbox: facebook_inbox, - conversation: conversation, - created_at: Time.now - 25.hours - ) - expect(conversation.can_reply?).to eq true - end - - it 'return true if last incoming message is inside 24 hour window' do - create( - :message, - account: conversation.account, - inbox: facebook_inbox, - conversation: conversation - ) - expect(conversation.can_reply?).to eq true - end - context 'when instagram channel' do it 'return true with HUMAN_AGENT if it is outside of 24 hour window' do InstallationConfig.where(name: 'ENABLE_MESSENGER_CHANNEL_HUMAN_AGENT').first_or_create(value: true) @@ -637,22 +622,45 @@ RSpec.describe Conversation, type: :model do expect(records.last.id).to eq(conversation_2.id) end - it 'Sort conversations based on last_user_message_at' 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) + 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) + end - records = described_class.last_user_message_at + # 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[0]['id']).to eq(conversation_2.id) - expect(records[1]['id']).to eq(conversation_3.id) - expect(records.pluck(:id)).not_to include(conversation_4.id) + 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) + 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) + end end context 'when last_activity_at updated by some actions' do @@ -674,7 +682,7 @@ RSpec.describe Conversation, type: :model do account_id: conversation_1.account_id, inbox_id: conversation_1.inbox_id, message_type: :activity, - content: 'Conversation was marked resolved by system due to days of inactivity' + content: 'Conversation was marked resolved by system due to days of inactivity' ) end records = described_class.latest diff --git a/spec/models/mention_spec.rb b/spec/models/mention_spec.rb index 5d4c71984..fe732ea03 100644 --- a/spec/models/mention_spec.rb +++ b/spec/models/mention_spec.rb @@ -45,7 +45,7 @@ RSpec.describe Mention, type: :model do expect(records.first.id).to eq(mention_2.id) expect(records.first.conversation_id).to eq(conversation_2.id) - expect(records.last.conversation_id).to eq(conversation_3.id) + expect(records.last.conversation_id).to eq(conversation_1.id) expect(records.pluck(:id)).not_to include(conversation_4.id) end