From 970e76ace8c8326108a7d1d3db09c23d11c9eedb Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Tue, 29 Apr 2025 15:33:11 -0700 Subject: [PATCH] feat: API Endpoints to update message status (#11387) - Added an api endpoint for update message status ( available only for api inboxes ) - Moved message status management to a service. - Handles case where read status arrive before delivered fixes: #10314 , #9962 --- .../conversations/messages_controller.rb | 19 +++++- .../update_message_status_job.rb | 2 +- .../facebook/send_on_facebook_service.rb | 8 +-- app/services/instagram/base_send_service.rb | 2 +- app/services/line/send_on_line_service.rb | 4 +- .../messages/status_update_service.rb | 34 ++++++++++ app/services/twilio/send_on_twilio_service.rb | 2 +- .../messages/update.json.jbuilder | 1 + app/workers/email_reply_worker.rb | 2 +- config/routes.rb | 2 +- lib/webhooks/trigger.rb | 2 +- .../conversations/messages_controller_spec.rb | 64 ++++++++++++++++++- .../messages/status_update_service_spec.rb | 46 +++++++++++++ 13 files changed, 173 insertions(+), 15 deletions(-) create mode 100644 app/services/messages/status_update_service.rb create mode 100644 app/views/api/v1/accounts/conversations/messages/update.json.jbuilder create mode 100644 spec/services/messages/status_update_service_spec.rb diff --git a/app/controllers/api/v1/accounts/conversations/messages_controller.rb b/app/controllers/api/v1/accounts/conversations/messages_controller.rb index 63226f342..67381a715 100644 --- a/app/controllers/api/v1/accounts/conversations/messages_controller.rb +++ b/app/controllers/api/v1/accounts/conversations/messages_controller.rb @@ -1,4 +1,6 @@ class Api::V1::Accounts::Conversations::MessagesController < Api::V1::Accounts::Conversations::BaseController + before_action :ensure_api_inbox, only: :update + def index @messages = message_finder.perform end @@ -11,6 +13,11 @@ class Api::V1::Accounts::Conversations::MessagesController < Api::V1::Accounts:: render_could_not_create_error(e.message) end + def update + Messages::StatusUpdateService.new(message, permitted_params[:status], permitted_params[:external_error]).perform + @message = message + end + def destroy ActiveRecord::Base.transaction do message.update!(content: I18n.t('conversations.messages.deleted'), content_type: :text, content_attributes: { deleted: true }) @@ -21,7 +28,9 @@ class Api::V1::Accounts::Conversations::MessagesController < Api::V1::Accounts:: def retry return if message.blank? - message.update!(status: :sent, content_attributes: {}) + service = Messages::StatusUpdateService.new(message, 'sent') + service.perform + message.update!(content_attributes: {}) ::SendReplyJob.perform_later(message.id) rescue StandardError => e render_could_not_create_error(e.message) @@ -56,10 +65,16 @@ class Api::V1::Accounts::Conversations::MessagesController < Api::V1::Accounts:: end def permitted_params - params.permit(:id, :target_language) + params.permit(:id, :target_language, :status, :external_error) end def already_translated_content_available? message.translations.present? && message.translations[permitted_params[:target_language]].present? end + + # API inbox check + def ensure_api_inbox + # Only API inboxes can update messages + render json: { error: 'Message status update is only allowed for API inboxes' }, status: :forbidden unless @conversation.inbox.api? + end end diff --git a/app/jobs/conversations/update_message_status_job.rb b/app/jobs/conversations/update_message_status_job.rb index 1e6333b41..6fcef4361 100644 --- a/app/jobs/conversations/update_message_status_job.rb +++ b/app/jobs/conversations/update_message_status_job.rb @@ -15,7 +15,7 @@ class Conversations::UpdateMessageStatusJob < ApplicationJob conversation.messages.where(status: %w[sent delivered]) .where.not(message_type: 'incoming') .where('messages.created_at <= ?', timestamp).find_each do |message| - message.update!(status: status) + Messages::StatusUpdateService.new(message, status).perform end end end diff --git a/app/services/facebook/send_on_facebook_service.rb b/app/services/facebook/send_on_facebook_service.rb index 5dde19c5a..5e9212edd 100644 --- a/app/services/facebook/send_on_facebook_service.rb +++ b/app/services/facebook/send_on_facebook_service.rb @@ -16,7 +16,7 @@ class Facebook::SendOnFacebookService < Base::SendOnChannelService rescue Facebook::Messenger::FacebookError => e # TODO : handle specific errors or else page will get disconnected handle_facebook_error(e) - message.update!(status: :failed, external_error: e.message) + Messages::StatusUpdateService.new(message, 'failed', e.message).perform end def send_message_to_facebook(delivery_params) @@ -24,7 +24,7 @@ class Facebook::SendOnFacebookService < Base::SendOnChannelService return if parsed_result.nil? if parsed_result['error'].present? - message.update!(status: :failed, external_error: external_error(parsed_result)) + Messages::StatusUpdateService.new(message, 'failed', external_error(parsed_result)).perform Rails.logger.info "Facebook::SendOnFacebookService: Error sending message to Facebook : Page - #{channel.page_id} : #{parsed_result}" end @@ -35,11 +35,11 @@ class Facebook::SendOnFacebookService < Base::SendOnChannelService result = Facebook::Messenger::Bot.deliver(delivery_params, page_id: channel.page_id) JSON.parse(result) rescue JSON::ParserError - message.update!(status: :failed, external_error: 'Facebook was unable to process this request') + Messages::StatusUpdateService.new(message, 'failed', 'Facebook was unable to process this request').perform Rails.logger.error "Facebook::SendOnFacebookService: Error parsing JSON response from Facebook : Page - #{channel.page_id} : #{result}" nil rescue Net::OpenTimeout - message.update!(status: :failed, external_error: 'Request timed out, please try again later') + Messages::StatusUpdateService.new(message, 'failed', 'Request timed out, please try again later').perform Rails.logger.error "Facebook::SendOnFacebookService: Timeout error sending message to Facebook : Page - #{channel.page_id}" nil end diff --git a/app/services/instagram/base_send_service.rb b/app/services/instagram/base_send_service.rb index dee41c23f..ff5f9216e 100644 --- a/app/services/instagram/base_send_service.rb +++ b/app/services/instagram/base_send_service.rb @@ -61,7 +61,7 @@ class Instagram::BaseSendService < Base::SendOnChannelService else external_error = external_error(parsed_response) Rails.logger.error("Instagram response: #{external_error} : #{message_content}") - message.update!(status: :failed, external_error: external_error) + Messages::StatusUpdateService.new(message, 'failed', external_error).perform nil end end diff --git a/app/services/line/send_on_line_service.rb b/app/services/line/send_on_line_service.rb index f8d704128..03ebe0ab7 100644 --- a/app/services/line/send_on_line_service.rb +++ b/app/services/line/send_on_line_service.rb @@ -14,10 +14,10 @@ class Line::SendOnLineService < Base::SendOnChannelService if response.code == '200' # If the request is successful, update the message status to delivered - message.update!(status: :delivered) + Messages::StatusUpdateService.new(message, 'delivered').perform else # If the request is not successful, update the message status to failed and save the external error - message.update!(status: :failed, external_error: external_error(parsed_json)) + Messages::StatusUpdateService.new(message, 'failed', external_error(parsed_json)).perform end end diff --git a/app/services/messages/status_update_service.rb b/app/services/messages/status_update_service.rb new file mode 100644 index 000000000..4868a201e --- /dev/null +++ b/app/services/messages/status_update_service.rb @@ -0,0 +1,34 @@ +class Messages::StatusUpdateService + attr_reader :message, :status, :external_error + + def initialize(message, status, external_error = nil) + @message = message + @status = status + @external_error = external_error + end + + def perform + return false unless valid_status_transition? + + update_message_status + end + + private + + def update_message_status + # Update status and set external_error only when failed + message.update!( + status: status, + external_error: (status == 'failed' ? external_error : nil) + ) + end + + def valid_status_transition? + return false unless Message.statuses.key?(status) + + # Don't allow changing from 'read' to 'delivered' + return false if message.read? && status == 'delivered' + + true + end +end diff --git a/app/services/twilio/send_on_twilio_service.rb b/app/services/twilio/send_on_twilio_service.rb index 3fc420bb2..5bd262759 100644 --- a/app/services/twilio/send_on_twilio_service.rb +++ b/app/services/twilio/send_on_twilio_service.rb @@ -9,7 +9,7 @@ class Twilio::SendOnTwilioService < Base::SendOnChannelService begin twilio_message = channel.send_message(**message_params) rescue Twilio::REST::TwilioError, Twilio::REST::RestError => e - message.update!(status: :failed, external_error: e.message) + Messages::StatusUpdateService.new(message, 'failed', e.message).perform end message.update!(source_id: twilio_message.sid) if twilio_message end diff --git a/app/views/api/v1/accounts/conversations/messages/update.json.jbuilder b/app/views/api/v1/accounts/conversations/messages/update.json.jbuilder new file mode 100644 index 000000000..3798b6c1f --- /dev/null +++ b/app/views/api/v1/accounts/conversations/messages/update.json.jbuilder @@ -0,0 +1 @@ +json.partial! 'api/v1/models/message', message: @message diff --git a/app/workers/email_reply_worker.rb b/app/workers/email_reply_worker.rb index 20cc70d5e..14b668637 100644 --- a/app/workers/email_reply_worker.rb +++ b/app/workers/email_reply_worker.rb @@ -11,6 +11,6 @@ class EmailReplyWorker ConversationReplyMailer.with(account: message.account).email_reply(message).deliver_now rescue StandardError => e ChatwootExceptionTracker.new(e, account: message.account).capture_exception - message.update!(status: :failed, external_error: e.message) + Messages::StatusUpdateService.new(message, 'failed', e.message).perform end end diff --git a/config/routes.rb b/config/routes.rb index 46199db12..6c71a8cc6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -98,7 +98,7 @@ Rails.application.routes.draw do post :filter end scope module: :conversations do - resources :messages, only: [:index, :create, :destroy] do + resources :messages, only: [:index, :create, :destroy, :update] do member do post :translate post :retry diff --git a/lib/webhooks/trigger.rb b/lib/webhooks/trigger.rb index f2e66f997..41b3a415d 100644 --- a/lib/webhooks/trigger.rb +++ b/lib/webhooks/trigger.rb @@ -42,7 +42,7 @@ class Webhooks::Trigger end def update_message_status(error) - message.update!(status: :failed, external_error: error.message) + Messages::StatusUpdateService.new(message, 'failed', error.message).perform end def message diff --git a/spec/controllers/api/v1/accounts/conversations/messages_controller_spec.rb b/spec/controllers/api/v1/accounts/conversations/messages_controller_spec.rb index 94d4cf272..f7ff042e5 100644 --- a/spec/controllers/api/v1/accounts/conversations/messages_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/conversations/messages_controller_spec.rb @@ -84,7 +84,6 @@ RSpec.describe 'Conversation Messages API', type: :request do context 'when api inbox' do let(:api_channel) { create(:channel_api, account: account) } let(:api_inbox) { create(:inbox, channel: api_channel, account: account) } - let(:inbox_member) { create(:inbox_member, user: agent, inbox: api_inbox) } let(:conversation) { create(:conversation, inbox: api_inbox, account: account) } it 'reopens the conversation with new incoming message' do @@ -294,4 +293,67 @@ RSpec.describe 'Conversation Messages API', type: :request do end end end + + describe 'PATCH /api/v1/accounts/{account.id}/conversations/:conversation_id/messages/:id' do + let(:api_channel) { create(:channel_api, account: account) } + let(:api_inbox) { create(:inbox, channel: api_channel, account: account) } + let(:agent) { create(:user, account: account, role: :agent) } + let!(:conversation) { create(:conversation, inbox: api_inbox, account: account) } + let!(:message) { create(:message, conversation: conversation, account: account, status: :sent) } + + context 'when unauthenticated' do + it 'returns unauthorized' do + patch api_v1_account_conversation_message_url(account_id: account.id, conversation_id: conversation.display_id, id: message.id) + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when authenticated agent' do + context 'when agent has non-API inbox' do + let(:inbox) { create(:inbox, account: account) } + let(:agent) { create(:user, account: account, role: :agent) } + let!(:conversation) { create(:conversation, inbox: inbox, account: account) } + + before { create(:inbox_member, inbox: inbox, user: agent) } + + it 'returns forbidden' do + patch api_v1_account_conversation_message_url( + account_id: account.id, + conversation_id: conversation.display_id, + id: message.id + ), params: { status: 'failed', external_error: 'err' }, headers: agent.create_new_auth_token, as: :json + expect(response).to have_http_status(:forbidden) + end + end + + context 'when agent has API inbox' do + before { create(:inbox_member, inbox: api_inbox, user: agent) } + + it 'uses StatusUpdateService to perform status update' do + service = instance_double(Messages::StatusUpdateService) + expect(Messages::StatusUpdateService).to receive(:new) + .with(message, 'failed', 'err123') + .and_return(service) + expect(service).to receive(:perform) + patch api_v1_account_conversation_message_url( + account_id: account.id, + conversation_id: conversation.display_id, + id: message.id + ), params: { status: 'failed', external_error: 'err123' }, headers: agent.create_new_auth_token, as: :json + end + + it 'updates status to failed with external_error' do + patch api_v1_account_conversation_message_url( + account_id: account.id, + conversation_id: conversation.display_id, + id: message.id + ), params: { status: 'failed', external_error: 'err123' }, headers: agent.create_new_auth_token, as: :json + + expect(response).to have_http_status(:success) + expect(message.reload.status).to eq('failed') + expect(message.reload.external_error).to eq('err123') + end + end + end + end end diff --git a/spec/services/messages/status_update_service_spec.rb b/spec/services/messages/status_update_service_spec.rb new file mode 100644 index 000000000..ce8fc2163 --- /dev/null +++ b/spec/services/messages/status_update_service_spec.rb @@ -0,0 +1,46 @@ +require 'rails_helper' + +describe Messages::StatusUpdateService do + let(:account) { create(:account) } + let(:conversation) { create(:conversation, account: account) } + let(:message) { create(:message, conversation: conversation, account: account) } + + describe '#perform' do + context 'when status is valid' do + it 'updates the status of the message' do + service = described_class.new(message, 'delivered') + service.perform + expect(message.reload.status).to eq('delivered') + end + + it 'clears external_error when status is not failed' do + message.update!(status: 'failed', external_error: 'previous error') + service = described_class.new(message, 'delivered') + service.perform + expect(message.reload.status).to eq('delivered') + expect(message.reload.external_error).to be_nil + end + + it 'updates external_error when status is failed' do + service = described_class.new(message, 'failed', 'some error') + service.perform + expect(message.reload.status).to eq('failed') + expect(message.reload.external_error).to eq('some error') + end + end + + context 'when status is invalid' do + it 'returns false for invalid status' do + service = described_class.new(message, 'invalid_status') + expect(service.perform).to be false + end + + it 'prevents transition from read to delivered' do + message.update!(status: 'read') + service = described_class.new(message, 'delivered') + expect(service.perform).to be false + expect(message.reload.status).to eq('read') + end + end + end +end