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

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

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

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

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

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
 

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

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

app/views/api/v1/accounts/conversations/messages/update.json.jbuilder

@@ -0,0 +1 @@
+json.partial! 'api/v1/models/message', message: @message

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

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

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

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

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