From 24fbab94c37dc168556f82949cec8545ada5b17f Mon Sep 17 00:00:00 2001 From: Muhsin Keloth Date: Sat, 28 Oct 2023 03:51:39 +0530 Subject: [PATCH] chore: Refactor `MarkMessagesAsReadJob` based on conversation id and time stamp (#8217) - Mark all messages as read by providing the conversation ID and timestamp. - For Instagram, ensure all previous messages that weren't marked as failed are now marked as read. This is because the read events are only triggered for the most recent message and not for any previous ones. --- .../api/v1/widget/conversations_controller.rb | 2 +- .../api/v1/inboxes/conversations_controller.rb | 2 +- .../conversations/mark_messages_as_read_job.rb | 9 ++++++--- app/services/instagram/read_status_service.rb | 7 +------ .../v1/widget/conversations_controller_spec.rb | 5 ++++- .../v1/inbox/conversations_controller_spec.rb | 4 +++- .../mark_messages_as_read_job_spec.rb | 18 ++++++++++++------ .../instagram/read_status_service_spec.rb | 13 ++++++++----- 8 files changed, 36 insertions(+), 24 deletions(-) diff --git a/app/controllers/api/v1/widget/conversations_controller.rb b/app/controllers/api/v1/widget/conversations_controller.rb index df0ef64ff..69b85797c 100644 --- a/app/controllers/api/v1/widget/conversations_controller.rb +++ b/app/controllers/api/v1/widget/conversations_controller.rb @@ -28,7 +28,7 @@ class Api::V1::Widget::ConversationsController < Api::V1::Widget::BaseController conversation.contact_last_seen_at = DateTime.now.utc conversation.save! - ::Conversations::MarkMessagesAsReadJob.perform_later(conversation) + ::Conversations::MarkMessagesAsReadJob.perform_later(conversation.id, conversation.contact_last_seen_at) head :ok end diff --git a/app/controllers/public/api/v1/inboxes/conversations_controller.rb b/app/controllers/public/api/v1/inboxes/conversations_controller.rb index 1e20b2a1d..3242942f4 100644 --- a/app/controllers/public/api/v1/inboxes/conversations_controller.rb +++ b/app/controllers/public/api/v1/inboxes/conversations_controller.rb @@ -23,7 +23,7 @@ class Public::Api::V1::Inboxes::ConversationsController < Public::Api::V1::Inbox def update_last_seen @conversation.contact_last_seen_at = DateTime.now.utc @conversation.save! - ::Conversations::MarkMessagesAsReadJob.perform_later(@conversation) + ::Conversations::MarkMessagesAsReadJob.perform_later(@conversation.id, @conversation.contact_last_seen_at) head :ok end diff --git a/app/jobs/conversations/mark_messages_as_read_job.rb b/app/jobs/conversations/mark_messages_as_read_job.rb index ce828521f..212b8569f 100644 --- a/app/jobs/conversations/mark_messages_as_read_job.rb +++ b/app/jobs/conversations/mark_messages_as_read_job.rb @@ -1,12 +1,15 @@ class Conversations::MarkMessagesAsReadJob < ApplicationJob queue_as :low - def perform(conversation) + def perform(conversation_id, timestamp) + conversation = Conversation.find_by(id: conversation_id) + + return unless conversation + # Mark every message created before the user's viewing time as read. conversation.messages.where(status: %w[sent delivered]) .where.not(message_type: 'incoming') - .where('created_at <= ?', - conversation.contact_last_seen_at).find_each do |message| + .where('messages.created_at <= ?', timestamp).find_each do |message| message.update!(status: 'read') end end diff --git a/app/services/instagram/read_status_service.rb b/app/services/instagram/read_status_service.rb index 80f67d680..41ef7246a 100644 --- a/app/services/instagram/read_status_service.rb +++ b/app/services/instagram/read_status_service.rb @@ -4,12 +4,7 @@ class Instagram::ReadStatusService def perform return if instagram_channel.blank? - process_status if message.present? - end - - def process_status - @message.status = 'read' - @message.save! + ::Conversations::MarkMessagesAsReadJob.perform_later(message.conversation.id, message.created_at) if message.present? end def instagram_id diff --git a/spec/controllers/api/v1/widget/conversations_controller_spec.rb b/spec/controllers/api/v1/widget/conversations_controller_spec.rb index 26e525d3a..15bfb6855 100644 --- a/spec/controllers/api/v1/widget/conversations_controller_spec.rb +++ b/spec/controllers/api/v1/widget/conversations_controller_spec.rb @@ -168,9 +168,12 @@ RSpec.describe '/api/v1/widget/conversations/toggle_typing', type: :request do describe 'POST /api/v1/widget/conversations/update_last_seen' do context 'with a conversation' do it 'returns the correct conversation params' do + current_time = DateTime.now.utc + allow(DateTime).to receive(:now).and_return(current_time) + allow(Rails.configuration.dispatcher).to receive(:dispatch) expect(conversation.contact_last_seen_at).to be_nil - expect(Conversations::MarkMessagesAsReadJob).to receive(:perform_later).with(conversation) + expect(Conversations::MarkMessagesAsReadJob).to receive(:perform_later).with(conversation.id, current_time) post '/api/v1/widget/conversations/update_last_seen', headers: { 'X-Auth-Token' => token }, diff --git a/spec/controllers/public/api/v1/inbox/conversations_controller_spec.rb b/spec/controllers/public/api/v1/inbox/conversations_controller_spec.rb index 3001263bf..7e1779b4a 100644 --- a/spec/controllers/public/api/v1/inbox/conversations_controller_spec.rb +++ b/spec/controllers/public/api/v1/inbox/conversations_controller_spec.rb @@ -80,8 +80,10 @@ RSpec.describe 'Public Inbox Contact Conversations API', type: :request do end it 'updates the last seen of the conversation contact' do + current_time = DateTime.now.utc + allow(DateTime).to receive(:now).and_return(current_time) contact_last_seen_at = conversation.contact_last_seen_at - expect(Conversations::MarkMessagesAsReadJob).to receive(:perform_later).with(conversation) + expect(Conversations::MarkMessagesAsReadJob).to receive(:perform_later).with(conversation.id, current_time) post update_last_seen_path expect(response).to have_http_status(:success) diff --git a/spec/jobs/conversations/mark_messages_as_read_job_spec.rb b/spec/jobs/conversations/mark_messages_as_read_job_spec.rb index 18ddb41f5..e4eff0f0a 100644 --- a/spec/jobs/conversations/mark_messages_as_read_job_spec.rb +++ b/spec/jobs/conversations/mark_messages_as_read_job_spec.rb @@ -16,7 +16,7 @@ RSpec.describe Conversations::MarkMessagesAsReadJob do context 'when called' do it 'marks all sent messages in a conversation as read' do expect do - described_class.perform_now(conversation) + described_class.perform_now(conversation.id, conversation.contact_last_seen_at) message.reload end.to change(message, :status).from('sent').to('read') end @@ -24,7 +24,7 @@ RSpec.describe Conversations::MarkMessagesAsReadJob do it 'marks all delivered messages in a conversation as read' do message.update!(status: 'delivered') expect do - described_class.perform_now(conversation) + described_class.perform_now(conversation.id, conversation.contact_last_seen_at) message.reload end.to change(message, :status).from('delivered').to('read') end @@ -32,7 +32,7 @@ RSpec.describe Conversations::MarkMessagesAsReadJob do it 'marks all templates messages in a conversation as read' do message.update!(status: 'delivered', message_type: 'template') expect do - described_class.perform_now(conversation) + described_class.perform_now(conversation.id, conversation.contact_last_seen_at) message.reload end.to change(message, :status).from('delivered').to('read') end @@ -40,21 +40,27 @@ RSpec.describe Conversations::MarkMessagesAsReadJob do it 'does not mark failed messages as read' do message.update!(status: 'failed') expect do - described_class.perform_now(conversation) + described_class.perform_now(conversation.id, conversation.contact_last_seen_at) end.not_to change(message.reload, :status) end it 'does not mark incoming messages as read' do message.update!(message_type: 'incoming') expect do - described_class.perform_now(conversation) + described_class.perform_now(conversation.id, conversation.contact_last_seen_at) end.not_to change(message.reload, :status) end it 'does not mark messages created after the contact last seen time as read' do message.update!(created_at: DateTime.now.utc) expect do - described_class.perform_now(conversation) + described_class.perform_now(conversation.id, conversation.contact_last_seen_at) + end.not_to change(message.reload, :status) + end + + it 'does not run the job if the conversation does not exist' do + expect do + described_class.perform_now(1212, conversation.contact_last_seen_at) end.not_to change(message.reload, :status) end end diff --git a/spec/services/instagram/read_status_service_spec.rb b/spec/services/instagram/read_status_service_spec.rb index db5dc73b3..669fcd4f9 100644 --- a/spec/services/instagram/read_status_service_spec.rb +++ b/spec/services/instagram/read_status_service_spec.rb @@ -17,7 +17,11 @@ describe Instagram::ReadStatusService do context 'when messaging_seen callback is fired' do let(:message) { conversation.messages.last } - it 'updates the message status to read if the status is delivered' do + before do + allow(Conversations::MarkMessagesAsReadJob).to receive(:perform_later) + end + + it 'enqueues the MarkMessagesAsReadJob with correct parameters if the message is found' do params = { recipient: { id: 'chatwoot-app-user-id-1' @@ -27,10 +31,10 @@ describe Instagram::ReadStatusService do } } described_class.new(params: params).perform - expect(conversation.reload.messages.last.status).to eq('read') + expect(Conversations::MarkMessagesAsReadJob).to have_received(:perform_later).with(conversation.id, message.created_at) end - it 'does not update the status if message is not found' do + it 'does not enqueue the MarkMessagesAsReadJob if the message is not found' do params = { recipient: { id: 'chatwoot-app-user-id-1' @@ -39,9 +43,8 @@ describe Instagram::ReadStatusService do mid: 'random-message-id' } } - described_class.new(params: params).perform - expect(conversation.reload.messages.last.status).not_to eq('read') + expect(Conversations::MarkMessagesAsReadJob).not_to have_received(:perform_later) end end end