fix(voice): address codex review feedback on Call-model PR
- Guard against delayed conference-start retries rolling a progressed call back to ringing (Conference::Manager#mark_ringing!). - Scope conference-status webhook Call lookup to the resolved inbox (VoiceController#find_call_for_conference!). - Require explicit call_sid on the agent TwiML leg and on ConferenceController#create/destroy so multi-call conversations can't attach the wrong call (no more most-recent-active fallback). - Scope StatusUpdateService Call lookup to the webhook's account for defense-in-depth. - Update UPDATE_MESSAGE_CALL_STATUS mutation to match the voice_call message by call_sid; helper/voice.js now passes callSid. Prevents an older call's status update from clobbering the newest bubble.
This commit is contained in:
@@ -63,6 +63,7 @@ export function handleVoiceCallUpdated(commit, message, currentUserId) {
|
||||
commit(types.UPDATE_MESSAGE_CALL_STATUS, {
|
||||
conversationId,
|
||||
callStatus: status,
|
||||
callSid,
|
||||
});
|
||||
|
||||
const isNewCall =
|
||||
|
||||
@@ -307,19 +307,23 @@ export const mutations = {
|
||||
}
|
||||
},
|
||||
|
||||
[types.UPDATE_MESSAGE_CALL_STATUS](_state, { conversationId, callStatus }) {
|
||||
[types.UPDATE_MESSAGE_CALL_STATUS](
|
||||
_state,
|
||||
{ conversationId, callStatus, callSid }
|
||||
) {
|
||||
const chat = getConversationById(_state)(conversationId);
|
||||
if (!chat) return;
|
||||
|
||||
const lastCall = (chat.messages || []).findLast(
|
||||
m => m.content_type === CONTENT_TYPES.VOICE_CALL
|
||||
const message = (chat.messages || []).find(
|
||||
m =>
|
||||
m.content_type === CONTENT_TYPES.VOICE_CALL &&
|
||||
m.content_attributes?.data?.call_sid === callSid
|
||||
);
|
||||
if (!message) return;
|
||||
|
||||
if (!lastCall) return;
|
||||
|
||||
lastCall.content_attributes ??= {};
|
||||
lastCall.content_attributes.data = {
|
||||
...lastCall.content_attributes.data,
|
||||
message.content_attributes ??= {};
|
||||
message.content_attributes.data = {
|
||||
...message.content_attributes.data,
|
||||
status: callStatus,
|
||||
};
|
||||
},
|
||||
|
||||
@@ -8,11 +8,12 @@ describe('#mutations', () => {
|
||||
mutations[types.UPDATE_MESSAGE_CALL_STATUS](state, {
|
||||
conversationId: 1,
|
||||
callStatus: 'ringing',
|
||||
callSid: 'CA123',
|
||||
});
|
||||
expect(state.allConversations).toEqual([]);
|
||||
});
|
||||
|
||||
it('does nothing if no voice call message exists', () => {
|
||||
it('does nothing if no matching voice call message exists', () => {
|
||||
const state = {
|
||||
allConversations: [
|
||||
{ id: 1, messages: [{ id: 1, content_type: 'text' }] },
|
||||
@@ -21,6 +22,7 @@ describe('#mutations', () => {
|
||||
mutations[types.UPDATE_MESSAGE_CALL_STATUS](state, {
|
||||
conversationId: 1,
|
||||
callStatus: 'ringing',
|
||||
callSid: 'CA123',
|
||||
});
|
||||
expect(state.allConversations[0].messages[0]).toEqual({
|
||||
id: 1,
|
||||
@@ -28,7 +30,7 @@ describe('#mutations', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('updates the last voice call message status', () => {
|
||||
it('updates only the voice call message matching the given callSid', () => {
|
||||
const state = {
|
||||
allConversations: [
|
||||
{
|
||||
@@ -37,12 +39,16 @@ describe('#mutations', () => {
|
||||
{
|
||||
id: 1,
|
||||
content_type: 'voice_call',
|
||||
content_attributes: { data: { status: 'ringing' } },
|
||||
content_attributes: {
|
||||
data: { call_sid: 'CA111', status: 'ringing' },
|
||||
},
|
||||
},
|
||||
{
|
||||
id: 2,
|
||||
content_type: 'voice_call',
|
||||
content_attributes: { data: { status: 'ringing' } },
|
||||
content_attributes: {
|
||||
data: { call_sid: 'CA222', status: 'ringing' },
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
@@ -51,31 +57,14 @@ describe('#mutations', () => {
|
||||
mutations[types.UPDATE_MESSAGE_CALL_STATUS](state, {
|
||||
conversationId: 1,
|
||||
callStatus: 'in-progress',
|
||||
callSid: 'CA111',
|
||||
});
|
||||
expect(
|
||||
state.allConversations[0].messages[0].content_attributes.data.status
|
||||
).toBe('ringing');
|
||||
).toBe('in-progress');
|
||||
expect(
|
||||
state.allConversations[0].messages[1].content_attributes.data.status
|
||||
).toBe('in-progress');
|
||||
});
|
||||
|
||||
it('creates content_attributes.data if it does not exist', () => {
|
||||
const state = {
|
||||
allConversations: [
|
||||
{
|
||||
id: 1,
|
||||
messages: [{ id: 1, content_type: 'voice_call' }],
|
||||
},
|
||||
],
|
||||
};
|
||||
mutations[types.UPDATE_MESSAGE_CALL_STATUS](state, {
|
||||
conversationId: 1,
|
||||
callStatus: 'completed',
|
||||
});
|
||||
expect(
|
||||
state.allConversations[0].messages[0].content_attributes.data.status
|
||||
).toBe('completed');
|
||||
).toBe('ringing');
|
||||
});
|
||||
|
||||
it('preserves existing data in content_attributes.data', () => {
|
||||
@@ -98,6 +87,7 @@ describe('#mutations', () => {
|
||||
mutations[types.UPDATE_MESSAGE_CALL_STATUS](state, {
|
||||
conversationId: 1,
|
||||
callStatus: 'in-progress',
|
||||
callSid: 'CA123',
|
||||
});
|
||||
expect(
|
||||
state.allConversations[0].messages[0].content_attributes.data
|
||||
@@ -114,6 +104,7 @@ describe('#mutations', () => {
|
||||
mutations[types.UPDATE_MESSAGE_CALL_STATUS](state, {
|
||||
conversationId: 1,
|
||||
callStatus: 'ringing',
|
||||
callSid: 'CA123',
|
||||
});
|
||||
expect(state.allConversations[0].messages).toEqual([]);
|
||||
});
|
||||
|
||||
@@ -33,14 +33,10 @@ class Api::V1::Accounts::ConferenceController < Api::V1::Accounts::BaseControlle
|
||||
private
|
||||
|
||||
def resolve_call!
|
||||
inbox_calls = Call.where(inbox_id: @voice_inbox.id, provider: :twilio)
|
||||
sid = params[:call_sid].presence
|
||||
raise ActionController::ParameterMissing, :call_sid if sid.blank?
|
||||
|
||||
if params[:call_sid].present?
|
||||
inbox_calls.find_by!(provider_call_id: params[:call_sid])
|
||||
else
|
||||
conversation = fetch_conversation_by_display_id
|
||||
inbox_calls.where(conversation_id: conversation.id).active.order(created_at: :desc).first!
|
||||
end
|
||||
Call.where(inbox_id: @voice_inbox.id, provider: :twilio).find_by!(provider_call_id: sid)
|
||||
end
|
||||
|
||||
def set_voice_inbox_for_conference
|
||||
|
||||
@@ -95,14 +95,10 @@ class Twilio::VoiceController < ApplicationController
|
||||
end
|
||||
|
||||
def find_call_for_agent
|
||||
if params[:call_sid].present?
|
||||
Call.find_by!(provider: :twilio, provider_call_id: params[:call_sid])
|
||||
elsif params[:conversation_id].present?
|
||||
conversation = current_account.conversations.find_by!(display_id: params[:conversation_id])
|
||||
Call.where(conversation_id: conversation.id).active.order(created_at: :desc).first!
|
||||
else
|
||||
Call.find_by!(provider: :twilio, provider_call_id: twilio_call_sid)
|
||||
end
|
||||
sid = params[:call_sid].presence
|
||||
raise ArgumentError, 'call_sid is required for agent leg' if sid.blank?
|
||||
|
||||
Call.find_by!(provider: :twilio, provider_call_id: sid)
|
||||
end
|
||||
|
||||
def sync_outbound_leg(call_sid:, direction:)
|
||||
@@ -141,9 +137,10 @@ class Twilio::VoiceController < ApplicationController
|
||||
end
|
||||
|
||||
def find_call_for_conference!(friendly_name:, call_sid:)
|
||||
inbox_calls = Call.where(inbox_id: inbox.id, provider: :twilio)
|
||||
name = friendly_name.to_s
|
||||
call = Call.twilio.by_conference_sid(name).first if name.present?
|
||||
call || Call.find_by!(provider: :twilio, provider_call_id: call_sid)
|
||||
call = inbox_calls.by_conference_sid(name).first if name.present?
|
||||
call || inbox_calls.find_by!(provider_call_id: call_sid)
|
||||
end
|
||||
|
||||
def set_inbox!
|
||||
|
||||
@@ -23,6 +23,9 @@ class Voice::Conference::Manager
|
||||
end
|
||||
|
||||
def mark_ringing!
|
||||
# Guard against delayed conference-start retries rolling a progressed call back to ringing.
|
||||
return unless call.status == 'ringing'
|
||||
|
||||
status_manager.process_status_update('ringing')
|
||||
end
|
||||
|
||||
|
||||
@@ -19,7 +19,7 @@ class Voice::StatusUpdateService
|
||||
normalized_status = normalize_status(call_status)
|
||||
return if normalized_status.blank?
|
||||
|
||||
call = Call.find_by_provider_call_id(:twilio, call_sid)
|
||||
call = Call.where(account_id: account.id).find_by(provider: :twilio, provider_call_id: call_sid)
|
||||
return unless call
|
||||
|
||||
Voice::CallStatus::Manager.new(call: call).process_status_update(
|
||||
|
||||
@@ -91,13 +91,13 @@ RSpec.describe Api::V1::Accounts::ConferenceController, type: :request do
|
||||
expect(conference_service).to have_received(:mark_agent_joined)
|
||||
end
|
||||
|
||||
it 'falls back to the most recent active call on the conversation when call_sid is missing' do
|
||||
it 'rejects the request when call_sid is missing' do
|
||||
post "/api/v1/accounts/#{account.id}/inboxes/#{voice_inbox.id}/conference",
|
||||
headers: agent.create_new_auth_token,
|
||||
params: { conversation_id: conversation.display_id }
|
||||
|
||||
expect(response).to have_http_status(:ok)
|
||||
expect(conference_service).to have_received(:ensure_conference_sid)
|
||||
expect(response).to have_http_status(:unprocessable_content)
|
||||
expect(conference_service).not_to have_received(:ensure_conference_sid)
|
||||
end
|
||||
|
||||
it 'does not allow accessing calls from inboxes without access' do
|
||||
|
||||
Reference in New Issue
Block a user